Add DuckDB::TableFunction::InitInfo#bind_data - #1438
Merged
Conversation
GitHub: suketaGH-1123 Wrap duckdb_init_get_bind_data() so the init callback can read the object the bind callback stashed with BindInfo#set_bind_data. Init runs before execute and is where per-scan state (thread count, projected columns) is decided, so it needs the bind-phase data too. #bind_data is the Ruby-style reader and #get_bind_data mirrors the C API name, following #max_threads= / #set_max_threads. ref: https://duckdb.org/docs/stable/clients/c/api.html#duckdb_init_get_bind_data
📝 WalkthroughWalkthroughThe change adds ChangesTable-function bind-data retrieval
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant BindInfo
participant DuckDB
participant InitInfo
BindInfo->>DuckDB: Store bind data with set_bind_data
DuckDB->>InitInfo: Pass bind data during initialization
InitInfo->>DuckDB: Retrieve bind data
InitInfo->>InitInfo: Convert function data to a Ruby object
InitInfo-->>BindInfo: Return the bind-data object or nil
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
test/duckdb_test/table_function/init_info_test.rb (2)
149-152: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert identity with the object passed to
set_bind_data.The current assertion compares only the two accessor results. Capture the hash in an outer local and assert that both results are the same object as that local.
This finding is based on the changed test assertions and the stated object-identity behavior.
Proposed assertion change
+ bind_data = { token: 'init-round-trip', n: 7 } table_function.bind do |bind_info| bind_info.add_result_column('value', DuckDB::LogicalType::BIGINT) - bind_info.set_bind_data({ token: 'init-round-trip', n: 7 }) + bind_info.set_bind_data(bind_data) end ... assert_equal({ token: 'init-round-trip', n: 7 }, observed_bind_data) - assert_same observed_bind_data, observed_via_alias + assert_same bind_data, observed_bind_data + assert_same bind_data, observed_via_aliasAlso applies to: 167-168
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/duckdb_test/table_function/init_info_test.rb` around lines 149 - 152, Update the table_function bind setup around bind_info.set_bind_data to store the hash in an outer local before passing it in. Change both accessor assertions to verify object identity against that captured local, not merely equality with each other.
143-145: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep the bind-data round-trip test active on Windows.
Line 144 skips before the bind, init, query, and assertions run. If
GC.compactis the only unsafe operation, guard only that call and keep the accessor assertions enabled.This finding is based on the changed test flow and the stated integration-test objective.
Proposed test change
- skip 'GC.compact hangs on Windows in parallel test execution' if Gem.win_platform? - ... - GC.compact + GC.compact unless Gem.win_platform?Also applies to: 156-159
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/duckdb_test/table_function/init_info_test.rb` around lines 143 - 145, Remove the Windows skip from test_init_info_bind_data_round_trip and keep the bind, init, query, and accessor assertions running on all platforms. Guard only the GC.compact invocation that hangs during parallel Windows execution, while preserving the existing round-trip verification.ext/duckdb/table_function_init_info.c (1)
12-12: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPrefix the new C symbol with
rbduckdb_.The declaration, definition, and method registrations use
table_function_init_info_get_bind_data. Rename all three references to onerbduckdb_-prefixed name.As per coding guidelines, all C symbols in
ext/duckdb/**/*.cmust be prefixed withrbduckdb_.Proposed rename
-static VALUE table_function_init_info_get_bind_data(VALUE self); +static VALUE rbduckdb_table_function_init_info_get_bind_data(VALUE self); -static VALUE table_function_init_info_get_bind_data(VALUE self) { +static VALUE rbduckdb_table_function_init_info_get_bind_data(VALUE self) { - rb_define_method(cDuckDBTableFunctionInitInfo, "get_bind_data", table_function_init_info_get_bind_data, 0); - rb_define_method(cDuckDBTableFunctionInitInfo, "bind_data", table_function_init_info_get_bind_data, 0); + rb_define_method(cDuckDBTableFunctionInitInfo, "get_bind_data", rbduckdb_table_function_init_info_get_bind_data, 0); + rb_define_method(cDuckDBTableFunctionInitInfo, "bind_data", rbduckdb_table_function_init_info_get_bind_data, 0);Also applies to: 129-135, 149-150
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ext/duckdb/table_function_init_info.c` at line 12, Rename the table_function_init_info_get_bind_data declaration, definition, and method registration references to a consistent rbduckdb_-prefixed symbol, preserving the existing behavior and signatures.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@ext/duckdb/table_function_init_info.c`:
- Line 12: Rename the table_function_init_info_get_bind_data declaration,
definition, and method registration references to a consistent
rbduckdb_-prefixed symbol, preserving the existing behavior and signatures.
In `@test/duckdb_test/table_function/init_info_test.rb`:
- Around line 149-152: Update the table_function bind setup around
bind_info.set_bind_data to store the hash in an outer local before passing it
in. Change both accessor assertions to verify object identity against that
captured local, not merely equality with each other.
- Around line 143-145: Remove the Windows skip from
test_init_info_bind_data_round_trip and keep the bind, init, query, and accessor
assertions running on all platforms. Guard only the GC.compact invocation that
hangs during parallel Windows execution, while preserving the existing
round-trip verification.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 0122cec4-86c1-41ae-bd5c-67fe12e1f00a
📒 Files selected for processing (3)
CHANGELOG.mdext/duckdb/table_function_init_info.ctest/duckdb_test/table_function/init_info_test.rb
Owner
|
Thank you I merged it |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
GitHub: GH-1123
Wrap duckdb_init_get_bind_data() so the init callback can read the object the bind callback stashed with BindInfo#set_bind_data. Init runs before execute and is where per-scan state (thread count, projected columns) is decided, so it needs the bind-phase data too. #bind_data is the Ruby-style reader and #get_bind_data mirrors the C API name, following #max_threads= / #set_max_threads. ref: https://duckdb.org/docs/stable/clients/c/api.html#duckdb_init_get_bind_data
Summary by CodeRabbit
New Features
TableFunction::InitInfo.nilwhen unset.Bug Fixes
Documentation