Rename DuckDB::TableFunction bind data accessors to Ruby style - #1439
Open
otegami wants to merge 1 commit into
Open
Rename DuckDB::TableFunction bind data accessors to Ruby style#1439otegami wants to merge 1 commit into
otegami wants to merge 1 commit into
Conversation
GitHub: suketaGH-1123 #set_bind_data / #get_bind_data mirrored the C API names, but storing and reading bind data is attribute access, so spell it that way: BindInfo#bind_data= and FunctionInfo#bind_data / InitInfo#bind_data. Both old names are unreleased (suketaGH-1436, suketaGH-1438), so nothing that ever shipped changes, and the API keeps one name per concept instead of two.
📝 WalkthroughWalkthroughThe Ruby table-function bind-data APIs were renamed from ChangesBind-data API
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/duckdb_test/table_function/init_info_test.rb (1)
151-157: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winKeep an identity check for bind data.
This test now checks only hash equality. Use a named bind-data object and
assert_samefor the retrieved object.assert_equaldoes not detect a copied hash.Proposed test adjustment
+ 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.bind_data = { token: 'init-round-trip', n: 7 } + bind_info.bind_data = bind_data end ... assert_equal({ token: 'init-round-trip', n: 7 }, observed_bind_data) + assert_same(bind_data, observed_bind_data)🤖 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 151 - 157, Update the table_function.init test around observed_bind_data to assign a named bind-data object before setting bind_info.bind_data, then retrieve it through init_info.bind_data and assert_same that object rather than relying only on assert_equal, preserving the existing field-value checks.
🤖 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.
Inline comments:
In `@ext/duckdb/table_function_function_info.c`:
- Line 46: The renamed C helpers still lack the required rbduckdb_ prefix. In
ext/duckdb/table_function_function_info.c at lines 8-8, 46-46, and 85-85, rename
the declaration, implementation, and Ruby registration to
rbduckdb_table_function_function_info_bind_data; in
ext/duckdb/table_function_init_info.c at lines 12-12, 128-128, and 148-148,
apply the corresponding rbduckdb_table_function_init_info_bind_data name
consistently.
---
Nitpick comments:
In `@test/duckdb_test/table_function/init_info_test.rb`:
- Around line 151-157: Update the table_function.init test around
observed_bind_data to assign a named bind-data object before setting
bind_info.bind_data, then retrieve it through init_info.bind_data and
assert_same that object rather than relying only on assert_equal, preserving the
existing field-value checks.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 08b192b4-f420-4798-bcb6-42b64c2a940b
📒 Files selected for processing (6)
CHANGELOG.mdext/duckdb/table_function_bind_info.cext/duckdb/table_function_function_info.cext/duckdb/table_function_init_info.ctest/duckdb_test/table_function/function_info_test.rbtest/duckdb_test/table_function/init_info_test.rb
| * data = function_info.bind_data | ||
| */ | ||
| static VALUE table_function_function_info_get_bind_data(VALUE self) { | ||
| static VALUE table_function_function_info_bind_data(VALUE self) { |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Prefix both renamed C helpers with rbduckdb_.
The Ruby API rename is consistent. The renamed C symbols still use table_function_*. Apply the required prefix to each declaration, definition, and registration.
ext/duckdb/table_function_function_info.c#L46-L46: rename the implementation torbduckdb_table_function_function_info_bind_data.ext/duckdb/table_function_function_info.c#L8-L8: update the forward declaration.ext/duckdb/table_function_function_info.c#L85-L85: update the Ruby method registration.ext/duckdb/table_function_init_info.c#L128-L128: rename the implementation torbduckdb_table_function_init_info_bind_data.ext/duckdb/table_function_init_info.c#L12-L12: update the forward declaration.ext/duckdb/table_function_init_info.c#L148-L148: update the Ruby method registration.
As per coding guidelines, all C symbols in ext/duckdb/**/*.c must be prefixed with rbduckdb_.
📍 Affects 2 files
ext/duckdb/table_function_function_info.c#L46-L46(this comment)ext/duckdb/table_function_function_info.c#L8-L8ext/duckdb/table_function_function_info.c#L85-L85ext/duckdb/table_function_init_info.c#L128-L128ext/duckdb/table_function_init_info.c#L12-L12ext/duckdb/table_function_init_info.c#L148-L148
🤖 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_function_info.c` at line 46, The renamed C helpers
still lack the required rbduckdb_ prefix. In
ext/duckdb/table_function_function_info.c at lines 8-8, 46-46, and 85-85, rename
the declaration, implementation, and Ruby registration to
rbduckdb_table_function_function_info_bind_data; in
ext/duckdb/table_function_init_info.c at lines 12-12, 128-128, and 148-148,
apply the corresponding rbduckdb_table_function_init_info_bind_data name
consistently.
Source: Coding guidelines
suketa
requested changes
Aug 8, 2026
suketa
left a comment
suketa
left a comment
Owner
There was a problem hiding this comment.
Thank you, Please change CHANGELOG.md
| - add `DuckDB::TableFunction::InitInfo#bind_data` (and `#get_bind_data`) to retrieve, during the init phase, the object stored by `BindInfo#set_bind_data`. | ||
| - add `DuckDB::TableFunction::BindInfo#bind_data=` to store an arbitrary Ruby object as a custom table function's bind data. | ||
| - add `DuckDB::TableFunction::FunctionInfo#bind_data` to retrieve, during execution, the object stored by `BindInfo#bind_data=`. | ||
| - add `DuckDB::TableFunction::InitInfo#bind_data` to retrieve, during the init phase, the object stored by `BindInfo#bind_data=`. |
Owner
There was a problem hiding this comment.
Could you change these entries into Braking changes section?
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
Storing and reading bind data is attribute access, so name it that way:
BindInfo#set_bind_data→#bind_data=FunctionInfo#get_bind_data→#bind_dataInitInfo#get_bind_data→#bind_dataRename rather than alias, because all three are unreleased.
Nothing that shipped changes, and the API keeps one name per concept.
DataChunk#size=is the same shape: it wrapsduckdb_data_chunk_set_sizeand noset_sizeexists.Summary by CodeRabbit
bind_data = valueto set bind data.bind_datato retrieve bind data.