-
-
Notifications
You must be signed in to change notification settings - Fork 29
dump duckdb to v1.4.1 #978
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
WalkthroughCI matrices on macOS, Ubuntu, and Windows update DuckDB versions to 1.4.1 and 1.3.2. Build-time detection in Changes
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
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
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (2)📚 Learning: 2025-01-24T22:18:24.630ZApplied to files:
📚 Learning: 2025-01-25T05:11:42.406ZApplied to files:
⏰ 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)
🔇 Additional comments (1)
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. Comment |
There was a problem hiding this 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.rbstill 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
📒 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.rbstill declaresDUCKDB_REQUIRED_VERSION = '1.2.0'(Line 5), please confirm:
- Is support for DuckDB 1.2.x being dropped?
- Should
DUCKDB_REQUIRED_VERSIONbe 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_queryfunction to enable version-specific features.
| library_version = Gem::Version.new(DuckDB::LIBRARY_VERSION) | ||
| skip 'config test' if %w[1.4.0 1.4.1].include?(library_version.to_s) |
There was a problem hiding this comment.
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.rbLength 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.
There was a problem hiding this 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
📒 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)
Summary by CodeRabbit
New Features
Chores
Tests
Documentation