Merged
Conversation
- Add _add C method wrapping duckdb_add_scalar_function_to_set - Add ScalarFunctionSet#add Ruby method with TypeError guard - Make name: optional (default nil) in ScalarFunction.create so functions can be created without a name for use in a set - Add tests for multiple overloads (different types, counts, varargs) Co-authored-by: Copilot <[email protected]>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a public Changes
Sequence Diagram(s)sequenceDiagram
participant Ruby as Ruby caller
participant CExt as C extension (_add)
participant DuckDB as DuckDB C API
Ruby->>CExt: ScalarFunctionSet#add(scalar_function)
CExt->>CExt: extract rubyDuckDBScalarFunction
CExt->>DuckDB: duckdb_add_scalar_function_to_set(set, scalar_function)
DuckDB-->>CExt: success / DuckDBError
alt success
CExt-->>Ruby: return self
else error
CExt-->>Ruby: raise eDuckDBError (duplicate overload)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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: 2
🧹 Nitpick comments (1)
ext/duckdb/scalar_function_set.c (1)
47-48: Add acall-seq:doc block for the new C-exposed method.The new function is documented as
:nodoc:only; this file’s C API style expectscall-seq:blocks.As per coding guidelines `ext/duckdb/**/*.c`: “All C functions should use comment blocks with `call-seq:` for documentation”.Proposed patch
-/* :nodoc: */ +/* + * call-seq: + * _add(scalar_function) -> self + * + * Internal API: adds a DuckDB::ScalarFunction to the set. + */ static VALUE rbduckdb_scalar_function_set__add(VALUE self, VALUE scalar_function) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ext/duckdb/scalar_function_set.c` around lines 47 - 48, Add a C-style documentation comment block with a call-seq: above the new function rbduckdb_scalar_function_set__add; specifically, insert a comment that includes "call-seq: scalar_function_set.add(scalar_function)" (or similar Ruby-facing signature), a short one-line description of what the method does, and notes about the parameter (scalar_function) and return value, matching the file's existing C API comment style used elsewhere for functions like rbduckdb_scalar_function_set__add; ensure the block appears immediately above the static VALUE rbduckdb_scalar_function_set__add(VALUE self, VALUE scalar_function) declaration and follows the same formatting conventions as other ext/duckdb/*.c comment blocks.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ext/duckdb/scalar_function_set.c`:
- Around line 55-57: The error message raised when
duckdb_add_scalar_function_to_set(p->scalar_function_set, sf->scalar_function)
returns DuckDBError is overly specific ("duplicate overload?") though the branch
can represent any failure; change the rb_raise call to use a neutral,
non-speculative message (e.g., "failed to add scalar function to set") while
preserving the same exception type eDuckDBError and keeping the existing call
site (the block that checks the return of duckdb_add_scalar_function_to_set for
DuckDBError and calls rb_raise).
In `@lib/duckdb/scalar_function.rb`:
- Around line 66-67: Update the YARD documentation for the scalar function
constructor (the method with signature including return_type:, name: nil,
parameter_type:, parameter_types:, varargs_type:, null_handling:) to mark the
name: parameter as optional and adjust any examples to show calling the
constructor without providing name; update the `@param` description for name to
reflect it can be omitted and clarify behavior when omitted (e.g., autogenerated
or inferred), and make the corresponding change in the second doc block that
documents the same constructor/overload.
---
Nitpick comments:
In `@ext/duckdb/scalar_function_set.c`:
- Around line 47-48: Add a C-style documentation comment block with a call-seq:
above the new function rbduckdb_scalar_function_set__add; specifically, insert a
comment that includes "call-seq: scalar_function_set.add(scalar_function)" (or
similar Ruby-facing signature), a short one-line description of what the method
does, and notes about the parameter (scalar_function) and return value, matching
the file's existing C API comment style used elsewhere for functions like
rbduckdb_scalar_function_set__add; ensure the block appears immediately above
the static VALUE rbduckdb_scalar_function_set__add(VALUE self, VALUE
scalar_function) declaration and follows the same formatting conventions as
other ext/duckdb/*.c comment blocks.
🪄 Autofix (Beta)
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
Run ID: f322c948-2cd9-41bb-98ab-05a1a2c3c4ce
📒 Files selected for processing (5)
CHANGELOG.mdext/duckdb/scalar_function_set.clib/duckdb/scalar_function.rblib/duckdb/scalar_function_set.rbtest/duckdb_test/scalar_function_set_test.rb
Comment on lines
+55
to
+57
| if (duckdb_add_scalar_function_to_set(p->scalar_function_set, sf->scalar_function) == DuckDBError) { | ||
| rb_raise(eDuckDBError, "failed to add scalar function to set (duplicate overload?)"); | ||
| } |
There was a problem hiding this comment.
Avoid narrowing the failure reason in the raised error.
Line 56 suggests duplicate overload as the cause, but this branch handles any DuckDBError. A neutral message is safer and less misleading.
Proposed patch
- if (duckdb_add_scalar_function_to_set(p->scalar_function_set, sf->scalar_function) == DuckDBError) {
- rb_raise(eDuckDBError, "failed to add scalar function to set (duplicate overload?)");
- }
+ if (duckdb_add_scalar_function_to_set(p->scalar_function_set, sf->scalar_function) == DuckDBError) {
+ rb_raise(eDuckDBError, "failed to add scalar function to set");
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (duckdb_add_scalar_function_to_set(p->scalar_function_set, sf->scalar_function) == DuckDBError) { | |
| rb_raise(eDuckDBError, "failed to add scalar function to set (duplicate overload?)"); | |
| } | |
| if (duckdb_add_scalar_function_to_set(p->scalar_function_set, sf->scalar_function) == DuckDBError) { | |
| rb_raise(eDuckDBError, "failed to add scalar function to set"); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ext/duckdb/scalar_function_set.c` around lines 55 - 57, The error message
raised when duckdb_add_scalar_function_to_set(p->scalar_function_set,
sf->scalar_function) returns DuckDBError is overly specific ("duplicate
overload?") though the branch can represent any failure; change the rb_raise
call to use a neutral, non-speculative message (e.g., "failed to add scalar
function to set") while preserving the same exception type eDuckDBError and
keeping the existing call site (the block that checks the return of
duckdb_add_scalar_function_to_set for DuckDBError and calls rb_raise).
Co-authored-by: Copilot <[email protected]>
suketa
force-pushed
the
implement-scalar-function-set-add
branch
from
cff9a04 to
4159814
Compare
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
1 participant
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.
Summary
Partial implementation of #1221 — adds the
addmethod toScalarFunctionSet.Changes
ScalarFunctionSet#add(scalar_function): adds aScalarFunctionoverload to the set. RaisesTypeErrorfor non-ScalarFunctionarguments. Wrapsduckdb_add_scalar_function_to_set.ScalarFunction.create:name:is now optional (defaults tonil), so functions can be created without a name for use inside a set (the set provides the name).What's next
Connection#register_scalar_function_set(set)Summary by CodeRabbit
New Features
addmethod to scalar function sets to register additional overloads, validating inputs and returning the same set for chaining.Tests