Add DuckDB::Connection#register_logical_type#1241
Merged
suketa merged 3 commits intosuketa:mainfrom Apr 7, 2026
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
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 (4)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThis pull request adds support for registering custom logical types with DuckDB connections. The implementation includes a public Ruby method with type validation, a private C wrapper that calls the DuckDB C API, lifecycle management to keep logical type objects alive, and comprehensive test coverage for enum and struct type registration scenarios. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Ruby Client
participant RubyAPI as Connection<br/>(Ruby)
participant CExtension as _register_logical_type<br/>(C)
participant DuckDBAPI as DuckDB C API
Client->>RubyAPI: register_logical_type(logical_type)
RubyAPI->>RubyAPI: Validate LogicalType argument
RubyAPI->>CExtension: _register_logical_type(logical_type)
CExtension->>CExtension: Extract connection & payload
CExtension->>DuckDBAPI: duckdb_register_logical_type(...)
alt Registration succeeds
DuckDBAPI-->>CExtension: Success
CExtension->>CExtension: Append to registered_functions<br/>(keep object alive)
CExtension-->>RubyAPI: Return
RubyAPI-->>Client: Success
else Registration fails
DuckDBAPI-->>CExtension: Error
CExtension-->>RubyAPI: Raise eDuckDBError
RubyAPI-->>Client: Raise DuckDB::Error
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
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 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)
lib/duckdb/connection.rb (1)
154-158: Pre-validatelogical_type.aliasbefore native registration.The method documents alias as required, but currently relies on native failure. A Ruby-side precheck gives a clearer, immediate contract error.
💡 Suggested refinement
def register_logical_type(logical_type) raise TypeError, "#{logical_type.class} is not a DuckDB::LogicalType" unless logical_type.is_a?(LogicalType) + if logical_type.alias.nil? || logical_type.alias.empty? + raise ArgumentError, 'logical_type.alias must be set before registration' + end _register_logical_type(logical_type) end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/duckdb/connection.rb` around lines 154 - 158, The register_logical_type method should validate that logical_type.alias exists and is a proper String before calling _register_logical_type; add a pre-check in register_logical_type that raises ArgumentError (or TypeError if you prefer consistency) when logical_type.alias is nil, empty, or not a String, referencing the LogicalType instance and its alias attribute, then call _register_logical_type only after the alias validation passes to provide a clear Ruby-side contract error instead of relying on native failure.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CHANGELOG.md`:
- Line 21: Move the changelog entry that adds
"DuckDB::Connection#register_logical_type" out of the released 1.5.1.1 section
and place it under the Unreleased section header; specifically locate the line
containing "add `DuckDB::Connection#register_logical_type` to register a custom
logical type with a connection." and cut/paste it into the Unreleased section so
it remains listed as not-yet-released.
---
Nitpick comments:
In `@lib/duckdb/connection.rb`:
- Around line 154-158: The register_logical_type method should validate that
logical_type.alias exists and is a proper String before calling
_register_logical_type; add a pre-check in register_logical_type that raises
ArgumentError (or TypeError if you prefer consistency) when logical_type.alias
is nil, empty, or not a String, referencing the LogicalType instance and its
alias attribute, then call _register_logical_type only after the alias
validation passes to provide a clear Ruby-side contract error instead of relying
on native failure.
🪄 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: 76330f8c-1249-46e5-a446-c514e7f62b32
📒 Files selected for processing (4)
CHANGELOG.mdext/duckdb/connection.clib/duckdb/connection.rbtest/duckdb_test/connection_test.rb
otegami
force-pushed
the
feature/register-logical-type
branch
from
24e0784 to
7ff0c76
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
lib/duckdb/connection.rb (1)
154-158: Pre-validate logical type alias before calling native registration.On Line 157, missing alias currently fails in C with a generic registration error. A Ruby-side alias check gives a clearer, immediate failure reason.
♻️ Proposed refinement
def register_logical_type(logical_type) raise TypeError, "#{logical_type.class} is not a DuckDB::LogicalType" unless logical_type.is_a?(LogicalType) + alias_name = logical_type.alias + if alias_name.nil? || alias_name.empty? + raise DuckDB::Error, 'LogicalType alias must be set before registration' + end _register_logical_type(logical_type) end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/duckdb/connection.rb` around lines 154 - 158, The register_logical_type method currently calls _register_logical_type without verifying that logical_type has a valid alias, which causes a generic native error; before calling _register_logical_type, add a Ruby-side prevalidation that the passed logical_type (instance of LogicalType) responds to and returns a non-empty alias (e.g., check logical_type.alias or a suitable accessor) and raise a clear TypeError/ArgumentError if missing or blank, referencing register_logical_type, LogicalType, and _register_logical_type to locate the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@lib/duckdb/connection.rb`:
- Around line 154-158: The register_logical_type method currently calls
_register_logical_type without verifying that logical_type has a valid alias,
which causes a generic native error; before calling _register_logical_type, add
a Ruby-side prevalidation that the passed logical_type (instance of LogicalType)
responds to and returns a non-empty alias (e.g., check logical_type.alias or a
suitable accessor) and raise a clear TypeError/ArgumentError if missing or
blank, referencing register_logical_type, LogicalType, and
_register_logical_type to locate the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7dfe1d50-ff1f-4ca2-ae07-d81076d53968
📒 Files selected for processing (4)
CHANGELOG.mdext/duckdb/connection.clib/duckdb/connection.rbtest/duckdb_test/connection_test.rb
✅ Files skipped from review due to trivial changes (1)
- CHANGELOG.md
🚧 Files skipped from review as they are similar to previous changes (2)
- ext/duckdb/connection.c
- test/duckdb_test/connection_test.rb
otegami
force-pushed
the
feature/register-logical-type
branch
from
7ff0c76 to
f9dd52c
Compare
otegami
changed the title
suketa
requested changes
Apr 7, 2026
Owner
suketa
left a comment
suketa
left a comment
There was a problem hiding this comment.
Thanks for your great work!
I commented.
| * mood.alias = 'mood' | ||
| * con.register_logical_type(mood) | ||
| * con.query('CREATE TABLE t (m mood)') | ||
| */ |
Owner
There was a problem hiding this comment.
This is private method implementation. so use /* nodoc */ instead documents
otegami
force-pushed
the
feature/register-logical-type
branch
from
f9dd52c to
31af81b
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
lib/duckdb/connection.rb (1)
140-158: Implementation is correct and follows existing patterns.The type validation using
TypeErroris consistent withregister_scalar_function_setat line 226. The documentation is well-structured with YARD tags and accurately reflects the C API behavior—testing confirms thatDuckDB::Erroris properly raised when the logical type has no alias set.No changes required.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/duckdb/connection.rb` around lines 140 - 158, No changes required: the register_logical_type method correctly validates the argument type using LogicalType (raising TypeError) and delegates to _register_logical_type, and the documentation/YARD tags and error behavior match existing patterns (see register_logical_type, LogicalType, and _register_logical_type).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@lib/duckdb/connection.rb`:
- Around line 140-158: No changes required: the register_logical_type method
correctly validates the argument type using LogicalType (raising TypeError) and
delegates to _register_logical_type, and the documentation/YARD tags and error
behavior match existing patterns (see register_logical_type, LogicalType, and
_register_logical_type).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9511f228-2704-4d28-bf5d-38387fa26e0a
📒 Files selected for processing (5)
.rubocop.ymlCHANGELOG.mdext/duckdb/connection.clib/duckdb/connection.rbtest/duckdb_test/connection_test.rb
✅ Files skipped from review due to trivial changes (3)
- .rubocop.yml
- CHANGELOG.md
- test/duckdb_test/connection_test.rb
🚧 Files skipped from review as they are similar to previous changes (1)
- ext/duckdb/connection.c
Contributor
Author
|
Done: |
refs: suketaGH-940 In this PR, we add `DuckDB::Connection#register_logical_type` to register a custom logical type with a connection by wrapping the following C API: - [duckdb_state duckdb_register_logical_type(duckdb_connection con, duckdb_logical_type type, duckdb_create_type_info info);](https://duckdb.org/docs/current/clients/c/api#duckdb_register_logical_type) The method accepts a `DuckDB::LogicalType` instance that must have an alias set via `LogicalType#alias=` before registration. The alias becomes the SQL type name. The third parameter `duckdb_create_type_info` is reserved for future use in the DuckDB C API and is passed as `NULL`.
otegami
force-pushed
the
feature/register-logical-type
branch
from
31af81b to
adc6928
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
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: fix GH-940
In this PR, we add
DuckDB::Connection#register_logical_typeto register a custom logical type with a connection by wrapping the following C API:The method accepts a
DuckDB::LogicalTypeinstance that must have an alias set viaLogicalType#alias=before registration. The alias becomes the SQL type name.The third parameter
duckdb_create_type_infois reserved for future use in the DuckDB C API and is passed asNULL.Summary by CodeRabbit
New Features