-
-
Notifications
You must be signed in to change notification settings - Fork 29
support interfaces for Enum type in DuckDB::LogicalType #930
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
WalkthroughThe changes add support for enum types within the Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Ruby Client
participant LT as LogicalType (Ruby)
participant Ext as C Extension
participant DB as DuckDB Engine
Client->>LT: call internal_type
LT->>Ext: invoke _internal_type method
Ext->>DB: call duckdb_get_type_id (and duckdb_enum_internal_type/duckdb_decimal_internal_type)
DB-->>Ext: return internal type identifier
Ext-->>LT: return type identifier
LT-->>Client: return type as symbol
Client->>LT: call each_dictionary_value (with block)
LT->>Ext: obtain dictionary_size
Ext->>DB: fetch enum dictionary size
DB-->>Ext: return dictionary size
LT->>Ext: iterative call to dictionary_value_at(index)
Ext->>DB: fetch enum dictionary value for index
DB-->>Ext: return dictionary value
Ext-->>LT: yield dictionary value
LT-->>Client: yield each dictionary value
Suggested reviewers
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
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: 0
🧹 Nitpick comments (1)
lib/duckdb/logical_type.rb (1)
22-38: Fix typo in documentation exampleThe implementation of
internal_typelooks good, but there's a small typo in the documentation example.- # ernum_col = users.columns.find { |col| col.name == 'enum_col' } + # enum_col = users.columns.find { |col| col.name == 'enum_col' }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
CHANGELOG.md(1 hunks)ext/duckdb/logical_type.c(3 hunks)lib/duckdb/logical_type.rb(2 hunks)test/duckdb_test/logical_type_test.rb(2 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
test/duckdb_test/logical_type_test.rb (1)
lib/duckdb/logical_type.rb (4)
type(4-149)type(17-20)internal_type(35-38)each_dictionary_value(142-148)
🪛 LanguageTool
CHANGELOG.md
[uncategorized] ~6-~6: Loose punctuation mark.
Context: ... - DuckDB::LogicalType#internal_type, DuckDB::LogicalType#dictionary_size, ...
(UNLIKELY_OPENING_PUNCTUATION)
⏰ Context from checks skipped due to timeout of 90000ms (17)
- GitHub Check: test (mswin, 1.1.3)
- GitHub Check: test (mingw, 1.1.1)
- GitHub Check: test (head, 1.1.1)
- GitHub Check: test (mswin, 1.1.1)
- GitHub Check: test (3.4.1, 1.2.1)
- GitHub Check: test (head, 1.1.1)
- GitHub Check: test (mingw, 1.1.3)
- GitHub Check: test (ucrt, 1.1.1)
- GitHub Check: test (head, 1.2.1)
- GitHub Check: test (head, 1.1.3)
- GitHub Check: test (ucrt, 1.2.1)
- GitHub Check: test (3.3.6, 1.1.1)
- GitHub Check: test (3.2.6, 1.1.1)
- GitHub Check: test (3.3.6, 1.2.1)
- GitHub Check: test (3.3.6, 1.1.3)
- GitHub Check: test (3.1.6, 1.1.1)
- GitHub Check: test (3.1.6, 1.2.1)
🔇 Additional comments (10)
CHANGELOG.md (1)
5-8: Clear documentation of new featuresThe changelog entry properly documents the new Enum type support in the DuckDB::LogicalType class and lists all the newly available methods. This provides good visibility for users about what's changed.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~6-~6: Loose punctuation mark.
Context: ... -DuckDB::LogicalType#internal_type,DuckDB::LogicalType#dictionary_size, ...(UNLIKELY_OPENING_PUNCTUATION)
test/duckdb_test/logical_type_test.rb (4)
121-124: Test for decimal internal type looks goodThis test properly verifies that the internal type of a decimal column is
:integer, which matches the expected behavior.
223-226: Test for enum internal type looks goodThis test correctly verifies that the internal type of an enum column is
:utinyint, which is expected based on the DuckDB implementation.
228-231: Test for enum dictionary size looks goodThis test verifies that the dictionary size of the enum column matches the number of values defined in the CREATE TYPE statement (4 values: 'sad', 'ok', 'happy', '𝘾𝝾օɭ 😎').
233-238: Test for enum dictionary values looks goodThis test properly verifies that all dictionary values can be retrieved and match the expected values from the CREATE TYPE statement. It also tests that Unicode characters and emojis are handled correctly.
lib/duckdb/logical_type.rb (1)
128-148: Well-implemented iterator method for enum dictionary valuesThe
each_dictionary_valuemethod follows Ruby's enumeration patterns correctly:
- Returns an Enumerator when no block is given
- Yields each value when a block is provided
- Properly uses the underlying
dictionary_sizeanddictionary_value_atmethodsThis implementation is consistent with other enumeration methods in the class.
ext/duckdb/logical_type.c (4)
301-321: Good handling of different logical type casesThe implementation of
duckdb_logical_type__internal_typecorrectly handles both Decimal and Enum types by:
- Extracting the logical type from the Ruby object
- Determining the type ID
- Calling the appropriate function based on the type
- Defaulting to DUCKDB_TYPE_INVALID for unsupported types
This approach ensures proper type handling and error cases.
330-334: Simple and effective implementation of dictionary sizeThe method directly leverages the DuckDB C API to retrieve the dictionary size for the enum type.
343-358: Proper memory management for dictionary valuesThe implementation correctly:
- Extracts the logical type from the Ruby object
- Converts the index from Ruby to C
- Gets the dictionary value using the DuckDB C API
- Raises an error if the value is NULL
- Creates a new Ruby string with UTF-8 encoding
- Frees the memory allocated by the DuckDB C API
This approach prevents memory leaks and handles errors appropriately.
392-394: Proper registration of new methodsThe implementation correctly registers the new methods with the Ruby class, making them accessible from Ruby code.
suketa
left a 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.
LGTM
refs: GH-690
In this PR, we support interfaces for Enum type in DuckDB::Column#logical_type about the following.
This is one of the steps for supporting the duckdb_logical_column_type C API.
Summary by CodeRabbit