Skip to content

Conversation

@suketa
Copy link
Owner

@suketa suketa commented Oct 13, 2025

Summary by CodeRabbit

  • New Features

    • Added compatibility with DuckDB 1.4.x.
  • Chores

    • CI now tests against DuckDB 1.3.2 and 1.4.1 across macOS, Ubuntu, and Windows.
    • Build/configuration updated to detect DuckDB 1.4 runtime capabilities.
  • Tests

    • Test suite adjusted to handle behavioral differences in DuckDB 1.4.0–1.4.1.
  • Documentation

    • Changelog updated to reflect CI coverage of DuckDB 1.3.2 and 1.4.1.

@coderabbitai
Copy link

coderabbitai bot commented Oct 13, 2025

Walkthrough

CI matrices on macOS, Ubuntu, and Windows update DuckDB versions to 1.4.1 and 1.3.2. Build-time detection in ext/duckdb/extconf.rb now checks for duckdb_appender_create_query (DuckDB ≥1.4.0). ext/duckdb/ruby-duckdb.h defines HAVE_DUCKDB_H_GE_V1_4_0 when present. appender.c gains API-version‑gated error handling. A test skips for library versions 1.4.0/1.4.1. CHANGELOG notes CI updates.

Changes

Cohort / File(s) Summary
CI workflows (DuckDB matrix update)
.github/workflows/test_on_macos.yml, .github/workflows/test_on_ubuntu.yml, .github/workflows/test_on_windows.yml
Update DuckDB version matrix from ['1.2.2','1.3.2'] to ['1.4.1','1.3.2']; no other workflow logic changed.
Build configuration (feature detection)
ext/duckdb/extconf.rb
Add have_func check for duckdb_appender_create_query to detect DuckDB ≥1.4.0 during build and export the corresponding HAVE macro before generating the Makefile.
Header macros (version guard)
ext/duckdb/ruby-duckdb.h
If HAVE_DUCKDB_APPENDER_CREATE_QUERY is defined, define HAVE_DUCKDB_H_GE_V1_4_0.
Native implementation (appender error path)
ext/duckdb/appender.c
Add conditional handling for two DuckDB appender APIs: when HAVE_DUCKDB_H_GE_V1_4_0, call duckdb_appender_error_data, extract and destroy error_data and return its message; otherwise use legacy duckdb_appender_error and return nil when no message.
Tests (version-conditional skip)
test/duckdb_test/database_test.rb
In test_s_open_with_config, read DuckDB::LIBRARY_VERSION and skip the test for library versions 1.4.0 and 1.4.1.
Docs
CHANGELOG.md
Update Unreleased notes to mention CI tests DuckDB 1.3.2 and 1.4.1.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  actor CI as CI Runner
  participant Extconf as ext/duckdb/extconf.rb
  participant Compiler as C Compiler
  participant Header as duckdb.h
  note right of Extconf #DDEEFF: Build-time detection for duckdb_appender_create_query

  CI->>Extconf: run extconf.rb
  Extconf->>Compiler: compile small test referencing duckdb_appender_create_query
  Compiler->>Header: resolve symbol in duckdb.h
  alt symbol present (DuckDB ≥ 1.4.0)
    Compiler-->>Extconf: success -> define HAVE_DUCKDB_APPENDER_CREATE_QUERY
    Extconf->>CI: emit Makefile with HAVE_DUCKDB_H_GE_V1_4_0
  else symbol absent
    Compiler-->>Extconf: failure -> no macro
    Extconf->>CI: emit Makefile without 1.4.0 guard
  end
Loading
sequenceDiagram
  autonumber
  actor Ruby as Ruby VM
  participant Appender as appender.c
  participant DuckDB as DuckDB runtime
  note right of Appender #FFF0D9: Runtime error-message path (API-gated)

  Ruby->>Appender: appender_error_message(appender)
  alt HAVE_DUCKDB_H_GE_V1_4_0
    Appender->>DuckDB: duckdb_appender_error_data(appender)
    DuckDB-->>Appender: error_data (or NULL)
    alt error_data present
      Appender->>DuckDB: duckdb_error_destroy(error_data)
      Appender-->>Ruby: return error_data->message as Ruby string
    else no error
      Appender-->>Ruby: return nil
    end
  else legacy API
    Appender->>DuckDB: duckdb_appender_error(appender)
    DuckDB-->>Appender: C string (or NULL)
    alt msg != NULL
      Appender-->>Ruby: return msg as Ruby string
    else
      Appender-->>Ruby: return nil
    end
  end
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

Poem

Thump-thump, I hop through CI’s stack,
New DuckDB burrows, I check each track.
Macros wink on, tests skip with grace,
Native calls align in their place.
Carrot cheers — builds pass, I clap my paws! 🥕🐇

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Title Check ⚠️ Warning The title contains a typo (“dump” instead of “bump”) and does not clearly and accurately convey that the primary change is updating the DuckDB dependency to version 1.4.1, so it fails to summarize the main change. Please revise the title to “Bump DuckDB to v1.4.1” (or similar) so it clearly and correctly reflects the main version-update change.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch bump_duckdb_1.4.1

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between bf17e9f and bfc02fd.

📒 Files selected for processing (1)
  • ext/duckdb/appender.c (1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-01-24T22:18:24.630Z
Learnt from: suketa
PR: suketa/ruby-duckdb#0
File: :0-0
Timestamp: 2025-01-24T22:18:24.630Z
Learning: When using ruby-asan (AddressSanitizer), avoid letting DuckDB throw C++ exceptions by checking conditions (like table existence) beforehand and raising Ruby exceptions directly to prevent ASAN from intercepting the C++ exceptions.

Applied to files:

  • ext/duckdb/appender.c
📚 Learning: 2025-01-25T05:11:42.406Z
Learnt from: suketa
PR: suketa/ruby-duckdb#0
File: :0-0
Timestamp: 2025-01-25T05:11:42.406Z
Learning: In Ruby C extensions, when raising exceptions with dynamically allocated error messages, ensure to free the memory after rb_exc_raise as the code continues execution if the exception is caught with rescue.

Applied to files:

  • ext/duckdb/appender.c
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
  • GitHub Check: All tests passed on macos
  • GitHub Check: test (mingw, 1.4.1)
  • GitHub Check: test (head, 1.3.2)
  • GitHub Check: test (ucrt, 1.4.1)
  • GitHub Check: test (3.3.8, 1.4.1)
  • GitHub Check: test (mswin, 1.3.2)
  • GitHub Check: test (mswin, 1.4.1)
  • GitHub Check: test (ucrt, 1.3.2)
  • GitHub Check: test (3.4.1, 1.4.1)
  • GitHub Check: test (3.2.6, 1.4.1)
  • GitHub Check: test (3.4.1, 1.3.2)
  • GitHub Check: test (mingw, 1.3.2)
  • GitHub Check: test (3.2.6, 1.3.2)
  • GitHub Check: test (3.3.8, 1.3.2)
🔇 Additional comments (1)
ext/duckdb/appender.c (1)

96-116: LGTM! Memory leak fixed.

The conditional compilation properly handles the DuckDB API change between versions. The critical memory leak issue from the previous review has been correctly addressed:

  • Line 109 now calls duckdb_destroy_error_data(&error_data) after copying the message to a Ruby string
  • The cleanup occurs in all code paths (both when an error exists and when it doesn't)
  • The message is copied before the error_data is destroyed, which is correct since the message pointer is owned by the error_data object

Both the 1.4.0+ and legacy paths correctly return Qnil when no error exists and a Ruby string when an error is present.

Based on learnings.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

♻️ Duplicate comments (2)
.github/workflows/test_on_macos.yml (1)

19-19: Verify dropping 1.2.2 from the test matrix is intentional.

Same concern as raised for the Ubuntu workflow: the CI matrix removes 1.2.2 while ext/duckdb/extconf.rb still requires DuckDB >= 1.2.0. Please confirm whether 1.2.x support is being dropped and update the required version accordingly.

.github/workflows/test_on_windows.yml (1)

19-19: Verify dropping 1.2.2 from the test matrix is intentional.

Same concern as raised for the Ubuntu and macOS workflows: removing 1.2.2 from the test matrix while maintaining DUCKDB_REQUIRED_VERSION = '1.2.0' may indicate incomplete version requirement updates.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 46dbb3e and b61c89f.

📒 Files selected for processing (7)
  • .github/workflows/test_on_macos.yml (1 hunks)
  • .github/workflows/test_on_ubuntu.yml (1 hunks)
  • .github/workflows/test_on_windows.yml (1 hunks)
  • CHANGELOG.md (1 hunks)
  • ext/duckdb/extconf.rb (1 hunks)
  • ext/duckdb/ruby-duckdb.h (1 hunks)
  • test/duckdb_test/database_test.rb (1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (19)
  • GitHub Check: test (mingw, 1.3.2)
  • GitHub Check: test (head, 1.4.1)
  • GitHub Check: test (head, 1.3.2)
  • GitHub Check: test (ucrt, 1.3.2)
  • GitHub Check: test (3.3.8, 1.3.2)
  • GitHub Check: test (3.4.1, 1.4.1)
  • GitHub Check: test (3.4.1, 1.3.2)
  • GitHub Check: test (3.2.6, 1.4.1)
  • GitHub Check: test (3.5.0-preview1, 1.4.1)
  • GitHub Check: test (3.5.0-preview1, 1.4.1)
  • GitHub Check: test (3.3.9, 1.4.1)
  • GitHub Check: test (head, 1.3.2)
  • GitHub Check: test (3.3.9, 1.4.1)
  • GitHub Check: test (3.5.0-preview1, 1.3.2)
  • GitHub Check: test (head, 1.4.1)
  • GitHub Check: test (head, 1.4.1)
  • GitHub Check: test (3.4.6, 1.4.1)
  • GitHub Check: test (3.2.9, 1.4.1)
  • GitHub Check: test (3.2.9, 1.4.1)
🔇 Additional comments (4)
CHANGELOG.md (1)

7-7: LGTM!

The changelog accurately reflects the CI dependency updates to test against both DuckDB 1.3.2 and 1.4.1.

.github/workflows/test_on_ubuntu.yml (1)

19-19: Verify dropping 1.2.2 from the test matrix is intentional.

The CI matrix now tests against ['1.4.1', '1.3.2'], removing 1.2.2. Since ext/duckdb/extconf.rb still declares DUCKDB_REQUIRED_VERSION = '1.2.0' (Line 5), please confirm:

  • Is support for DuckDB 1.2.x being dropped?
  • Should DUCKDB_REQUIRED_VERSION be updated to '1.3.0' or '1.3.2'?
  • Or is 1.2.x still supported but no longer tested in CI?

If 1.2.x support is being dropped, consider updating the required version constant and documenting this as a breaking change in CHANGELOG.md.

ext/duckdb/ruby-duckdb.h (1)

15-17: LGTM!

The version detection macro follows the established pattern (lines 11-13 for v1.3.0) and correctly gates DuckDB 1.4.0+ features based on the presence of duckdb_appender_create_query.

ext/duckdb/extconf.rb (1)

67-68: LGTM!

The feature detection for DuckDB 1.4.0 follows the established pattern (lines 62, 65) and correctly checks for the duckdb_appender_create_query function to enable version-specific features.

Comment on lines +41 to +42
library_version = Gem::Version.new(DuckDB::LIBRARY_VERSION)
skip 'config test' if %w[1.4.0 1.4.1].include?(library_version.to_s)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion | 🟠 Major

🧩 Analysis chain

Document why 1.4.0 and 1.4.1 require skipping.

The test skips versions 1.4.0 and 1.4.1 without explanation, yet this PR bumps to 1.4.1. This suggests a known compatibility issue with DuckDB::Config in these versions. Please add a comment explaining:

  • What specific issue exists in 1.4.0/1.4.1
  • Whether this is a DuckDB bug or a ruby-duckdb limitation
  • If there's an upstream issue tracker link
  • Whether this will be fixed in a future version

Additionally, consider using Gem::Version comparison (library_version >= Gem::Version.new('1.4.0') && library_version <= Gem::Version.new('1.4.1')) to handle pre-release versions correctly.


🏁 Script executed:

#!/bin/bash
rg -n "LIBRARY_VERSION" -g "*.rb"

Length of output: 422


🏁 Script executed:

#!/bin/bash
# Locate any changelog or release notes in markdown files
rg -l "CHANGELOG" -g "*.md"
# Search for references to versions 1.4.0 or 1.4.1 in markdown files
rg -n "1\\.4\\.0|1\\.4\\.1" -g "*.md"

Length of output: 51


🏁 Script executed:

#!/bin/bash
# Search for DuckDB::Config usage
rg -n "DuckDB::Config" -g "*.rb"

Length of output: 1625


🏁 Script executed:

#!/bin/bash
sed -n '1,200p' lib/duckdb/config.rb

Length of output: 2048


Document skip rationale and update skip logic

Add an inline comment above the skip in test/duckdb_test/database_test.rb (lines 41–42) explaining that DuckDB v1.4.0 and v1.4.1 lack proper Config support (link to the upstream DuckDB issue/PR), and note when it’s expected to be fixed. Then replace:

if %w[1.4.0 1.4.1].include?(library_version.to_s)

with:

if library_version >= Gem::Version.new('1.4.0') && library_version <= Gem::Version.new('1.4.1')
🤖 Prompt for AI Agents
In test/duckdb_test/database_test.rb around lines 41–42, add an inline comment
above the skip explaining that DuckDB v1.4.0 and v1.4.1 lack proper Config
support with a link to the upstream DuckDB issue/PR and note the expected
fix/target version; then replace the current string-inclusion check with a range
comparison using Gem::Version (i.e., check if library_version >=
Gem::Version.new('1.4.0') && library_version <= Gem::Version.new('1.4.1')) so
the intent and version comparison are explicit and robust.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between b61c89f and bf17e9f.

📒 Files selected for processing (1)
  • ext/duckdb/appender.c (1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (18)
  • GitHub Check: test (ucrt, 1.3.2)
  • GitHub Check: test (head, 1.4.1)
  • GitHub Check: test (head, 1.3.2)
  • GitHub Check: test (mingw, 1.4.1)
  • GitHub Check: test (ucrt, 1.4.1)
  • GitHub Check: test (3.2.6, 1.4.1)
  • GitHub Check: test (3.4.1, 1.3.2)
  • GitHub Check: test (mingw, 1.3.2)
  • GitHub Check: test (mswin, 1.4.1)
  • GitHub Check: test (3.3.8, 1.3.2)
  • GitHub Check: test (3.2.6, 1.3.2)
  • GitHub Check: test (3.4.1, 1.4.1)
  • GitHub Check: test (3.3.8, 1.4.1)
  • GitHub Check: test (3.4.7, 1.4.1)
  • GitHub Check: test (head, 1.4.1)
  • GitHub Check: test (3.5.0-preview1, 1.4.1)
  • GitHub Check: test (3.3.9, 1.4.1)
  • GitHub Check: test (3.2.9, 1.4.1)

@suketa suketa merged commit 4322b83 into main Oct 13, 2025
38 checks passed
@suketa suketa deleted the bump_duckdb_1.4.1 branch October 13, 2025 09:53
@coderabbitai coderabbitai bot mentioned this pull request Dec 26, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants