add DuckDB::LogicalType.boolean DuckDB::LogicalType::BOOLEAN etc. #980
Conversation
WalkthroughAdds many new public LogicalType accessors and constants, a class-level LogicalType cache, a LogicalType.new(type_id) constructor, new ValueImpl and ScalarFunction classes with ScalarFunction#set_name, and extends int-to-symbol mappings; includes tests verifying singleton type methods. Changes
Sequence Diagram(s)sequenceDiagram
actor Client
participant LTMethod as DuckDB::LogicalType.<br/>type_method()
participant Cache as @logical_types
participant LTNew as DuckDB::LogicalType.new(type_id)
Client->>LTMethod: call (e.g., .boolean)
activate LTMethod
LTMethod->>Cache: check for key "boolean"
alt found
Cache-->>LTMethod: return cached instance
else not found
LTMethod->>LTNew: call new(type_id)
activate LTNew
LTNew-->>LTMethod: return instance
deactivate LTNew
LTMethod->>Cache: store instance under "boolean"
end
LTMethod-->>Client: return LogicalType instance
deactivate LTMethod
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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)
lib/duckdb/logical_type.rb (1)
51-58: Consider simplifying the caching logic (optional).The conditional logic could be simplified using Ruby's
||=operator for a more idiomatic approach:define_singleton_method(method_name) do - if @logical_types[type_id].nil? - logical_type = DuckDB::LogicalType.new(type_id) - @logical_types[type_id] = logical_type - else - @logical_types[type_id] - end + @logical_types[type_id] ||= DuckDB::LogicalType.new(type_id) end
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
CHANGELOG.md(1 hunks)lib/duckdb/converter/int_to_sym.rb(1 hunks)lib/duckdb/logical_type.rb(1 hunks)test/duckdb_test/logical_type_test.rb(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: suketa
Repo: suketa/ruby-duckdb PR: 0
File: :0-0
Timestamp: 2025-01-24T22:37:32.786Z
Learning: In Ruby-DuckDB C extensions, when initializing resources like prepared statements, store them directly in the context structure instead of using temporary variables to prevent memory leaks, especially when exceptions might be thrown.
📚 Learning: 2025-01-26T00:37:44.001Z
Learnt from: suketa
Repo: suketa/ruby-duckdb PR: 871
File: ext/duckdb/appender.c:170-178
Timestamp: 2025-01-26T00:37:44.001Z
Learning: In ruby-duckdb's DuckDB::Appender#append_uint8, values larger than uint8 max (255) are silently truncated to maintain backward compatibility, following C's implicit truncation behavior. Range checking is intentionally omitted to avoid breaking changes.
Applied to files:
lib/duckdb/converter/int_to_sym.rbCHANGELOG.md
🧬 Code graph analysis (1)
test/duckdb_test/logical_type_test.rb (1)
lib/duckdb/logical_type.rb (1)
type(74-77)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: test (3.5.0-preview1, 1.4.1)
- GitHub Check: test (head, 1.4.1)
- GitHub Check: test (3.4.7, 1.4.1)
- GitHub Check: test (3.3.9, 1.4.1)
- GitHub Check: test (3.3.9, 1.3.2)
- GitHub Check: test (3.5.0-preview1, 1.3.2)
- GitHub Check: test (3.4.7, 1.3.2)
- GitHub Check: test (3.5.0-preview1, 1.4.1)
- GitHub Check: test (head, 1.3.2)
- GitHub Check: test (head, 1.4.1)
- GitHub Check: test (3.4.7, 1.4.1)
- GitHub Check: test (3.2.9, 1.3.2)
- GitHub Check: test (3.2.9, 1.4.1)
- GitHub Check: test (3.3.9, 1.3.2)
- GitHub Check: test (3.3.9, 1.4.1)
- GitHub Check: test (ucrt, 1.3.2)
- GitHub Check: test (3.2.6, 1.3.2)
- GitHub Check: test (3.3.8, 1.4.1)
- GitHub Check: test (3.3.8, 1.3.2)
- GitHub Check: test (3.4.1, 1.4.1)
🔇 Additional comments (3)
lib/duckdb/converter/int_to_sym.rb (1)
71-76: LGTM!The new type mappings for
bignum,sqlnull,string_literal, andinteger_literalare correctly added and align with the corresponding type IDs defined inlib/duckdb/logical_type.rb.lib/duckdb/logical_type.rb (1)
8-60: Implementation of lazy-loaded singleton methods looks good.The pattern of using a class instance variable
@logical_typesto cache singleton instances is correct. Note that line 59'sconst_seteagerly invokes each singleton method at class load time, so allLogicalTypeinstances are created upfront rather than truly lazily. This is an acceptable design choice that ensures singleton behavior and minimal overhead.CHANGELOG.md (1)
6-29: LGTM!The changelog comprehensively documents all the new singleton methods, constants, and public API additions introduced in this PR. The documentation is clear and well-structured.
| def test_singleton_method_defined | ||
| %i[ | ||
| boolean | ||
| tinyint | ||
| smallint | ||
| integer | ||
| bigint | ||
| utinyint | ||
| usmallint | ||
| uinteger | ||
| ubigint | ||
| float | ||
| double | ||
| timestamp | ||
| date | ||
| time | ||
| interval | ||
| hugeint | ||
| uhugeint | ||
| varchar | ||
| blob | ||
| timestamp_s | ||
| timestamp_ms | ||
| timestamp_ns | ||
| bit | ||
| time_tz | ||
| timestamp_tz | ||
| bignum | ||
| sqlnull | ||
| string_literal | ||
| integer_literal | ||
| ].each do |method_name| | ||
| logical_type = DuckDB::LogicalType.send(method_name) | ||
| assert_instance_of(DuckDB::LogicalType, logical_type) | ||
| assert_equal(method_name, logical_type.type) | ||
| end | ||
| end |
There was a problem hiding this comment.
Add test coverage for the any type.
The test is missing coverage for the any type, which is defined in lib/duckdb/logical_type.rb at line 44 with type_id 34. Please add :any to the list of method names being tested.
Apply this diff to add the missing test:
def test_singleton_method_defined
%i[
boolean
tinyint
smallint
integer
bigint
utinyint
usmallint
uinteger
ubigint
float
double
timestamp
date
time
interval
hugeint
uhugeint
varchar
blob
timestamp_s
timestamp_ms
timestamp_ns
bit
time_tz
timestamp_tz
+ any
bignum
sqlnull
string_literal
integer_literal
].each do |method_name|📝 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.
| def test_singleton_method_defined | |
| %i[ | |
| boolean | |
| tinyint | |
| smallint | |
| integer | |
| bigint | |
| utinyint | |
| usmallint | |
| uinteger | |
| ubigint | |
| float | |
| double | |
| timestamp | |
| date | |
| time | |
| interval | |
| hugeint | |
| uhugeint | |
| varchar | |
| blob | |
| timestamp_s | |
| timestamp_ms | |
| timestamp_ns | |
| bit | |
| time_tz | |
| timestamp_tz | |
| bignum | |
| sqlnull | |
| string_literal | |
| integer_literal | |
| ].each do |method_name| | |
| logical_type = DuckDB::LogicalType.send(method_name) | |
| assert_instance_of(DuckDB::LogicalType, logical_type) | |
| assert_equal(method_name, logical_type.type) | |
| end | |
| end | |
| def test_singleton_method_defined | |
| %i[ | |
| boolean | |
| tinyint | |
| smallint | |
| integer | |
| bigint | |
| utinyint | |
| usmallint | |
| uinteger | |
| ubigint | |
| float | |
| double | |
| timestamp | |
| date | |
| time | |
| interval | |
| hugeint | |
| uhugeint | |
| varchar | |
| blob | |
| timestamp_s | |
| timestamp_ms | |
| timestamp_ns | |
| bit | |
| time_tz | |
| timestamp_tz | |
| any | |
| bignum | |
| sqlnull | |
| string_literal | |
| integer_literal | |
| ].each do |method_name| | |
| logical_type = DuckDB::LogicalType.send(method_name) | |
| assert_instance_of(DuckDB::LogicalType, logical_type) | |
| assert_equal(method_name, logical_type.type) | |
| end | |
| end |
🤖 Prompt for AI Agents
In test/duckdb_test/logical_type_test.rb around lines 116 to 152, the test array
of singleton method names omits :any; update the array to include :any so the
loop calls DuckDB::LogicalType.send(:any) and asserts its instance and type,
ensuring test coverage for the `any` logical type defined in
lib/duckdb/logical_type.rb.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
lib/duckdb/logical_type.rb (1)
8-8: Cache initialized but populated eagerly at load time.The
@logical_typescache is initialized here, but since line 54 callssend(method_name)for each constant, all instances are created immediately when the class loads rather than lazily on first access. The caching still works correctly (methods and constants share the same instances), but the lazy initialization benefit is minimal.Consider documenting this eager initialization behavior, or if true lazy loading is desired, populate constants on first access using a different pattern.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
lib/duckdb/logical_type.rb(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: test (3.5.0-preview1, 1.3.2)
- GitHub Check: test (head, 1.4.1)
- GitHub Check: test (3.3.9, 1.4.1)
- GitHub Check: test (mingw, 1.3.2)
- GitHub Check: test (3.4.1, 1.3.2)
🔇 Additional comments (1)
lib/duckdb/logical_type.rb (1)
50-55: No issues found withLogicalType.new(type_id)constructor implementation.The C extension implementation at
ext/duckdb/logical_type.c:53-76properly:
- Accepts an integer type_id parameter and converts it with
NUM2INT(type_id_arg)- Validates each type_id using
duckdb_create_logical_type()andduckdb_get_type_id()- Raises
ArgumentErrorwith a descriptive message for invalid type IDs- Manages resources correctly with proper cleanup on error
The Ruby code uses only specific, valid type IDs (20, 21, 22, 29-31, 34-38), all of which are supported by the underlying DuckDB C library. The
||=memoization pattern is safe since the class is initialized once at load time.
| { | ||
| boolean: 1, | ||
| tinyint: 2, | ||
| smallint: 3, | ||
| integer: 4, | ||
| bigint: 5, | ||
| utinyint: 6, | ||
| usmallint: 7, | ||
| uinteger: 8, | ||
| ubigint: 9, | ||
| float: 10, | ||
| double: 11, | ||
| timestamp: 12, | ||
| date: 13, | ||
| time: 14, | ||
| interval: 15, | ||
| hugeint: 16, | ||
| uhugeint: 32, | ||
| varchar: 17, | ||
| blob: 18, | ||
| # decimal: 19, | ||
| timestamp_s: 20, | ||
| timestamp_ms: 21, | ||
| timestamp_ns: 22, | ||
| # enum: 23, | ||
| # list: 24, | ||
| # struct: 25, | ||
| # map: 26, | ||
| # array: 33, | ||
| # uuid: 27, | ||
| # union: 28, | ||
| bit: 29, | ||
| time_tz: 30, | ||
| timestamp_tz: 31, | ||
| any: 34, | ||
| bignum: 35, | ||
| sqlnull: 36, | ||
| string_literal: 37, | ||
| integer_literal: 38 | ||
| # time_ns: 39 |
There was a problem hiding this comment.
🧩 Analysis chain
Verify type ID mappings match DuckDB's internal constants.
The integer type IDs (1-38) must exactly match DuckDB's internal type enumeration. Please verify these mappings are correct and consider documenting the source of these values (e.g., referencing the DuckDB header file or documentation where these constants are defined).
Additionally, several types are commented out (decimal, enum, list, struct, map, array, uuid, union, time_ns). If these are intentionally excluded, consider adding a brief comment explaining why (e.g., "complex types require additional implementation" or "not yet supported").
Run the following script to check if there are references to DuckDB type constants that could be used instead of magic numbers:
🏁 Script executed:
#!/bin/bash
# Search for DuckDB type constant definitions in C extension or documentation
rg -n -C3 'DUCKDB_TYPE_|duckdb_type' --type=c --type=h --type=rbLength of output: 26
---
🏁 Script executed:
```shell
#!/bin/bash
# View the actual file to understand context
cat lib/duckdb/logical_type.rb
Length of output: 6097
🏁 Script executed:
#!/bin/bash
# Search for DuckDB type constant references (without specifying unsupported type)
rg -n 'DUCKDB_TYPE|duckdb_type|TYPE_ID|LogicalType' -C2Length of output: 40309
🏁 Script executed:
#!/bin/bash
# Check if there are comments or documentation about these type mappings
rg -n 'type.*id|type.*constant|DuckDB.*version' -i -C2Length of output: 35333
🏁 Script executed:
#!/bin/bash
# Search for references to the type mapping constant/method
rg -n 'logical_type|TYPE.*MAP' -iLength of output: 26105
🏁 Script executed:
#!/bin/bash
# Check for DuckDB gem or library version specifications
fd -e gemfile -e gemspec -e Rakefile | head -20Length of output: 14
🏁 Script executed:
#!/bin/bash
# Look for any DuckDB documentation or comments about type IDs
find . -type f \( -name "*.md" -o -name "*.txt" -o -name "*.rb" \) -exec grep -l "type.*id\|duckdb.*type" -i {} \; | head -20Length of output: 383
🏁 Script executed:
#!/bin/bash
# View the converter to understand type ID validation/conversion
cat lib/duckdb/converter/int_to_sym.rbLength of output: 1899
🏁 Script executed:
#!/bin/bash
# Extract and compare type ID comments from tests with the hash
grep -n "DUCKDB_TYPE_" test/duckdb_test/logical_type_test.rbLength of output: 225
🏁 Script executed:
#!/bin/bash
# Check if there are version constraints on commented-out types
rg -n "decimal|enum|list|struct|map|array|uuid|union|time_ns" test/duckdb_test/logical_type_test.rb -C2Length of output: 6396
🏁 Script executed:
#!/bin/bash
# Cross-check that type IDs in logical_type.rb singleton hash match converter exactly
cat lib/duckdb/logical_type.rb | grep -A 40 "^\s*{$" | head -50Length of output: 793
🏁 Script executed:
#!/bin/bash
# Extract just the type IDs from both files to compare
echo "=== logical_type.rb type mappings ===" && \
rg -o '(\w+):\s*(\d+)' lib/duckdb/logical_type.rb | sort -t: -k2 -n && \
echo "" && \
echo "=== converter HASH_TYPES mappings ===" && \
rg -o '(\d+)\s*=>\s*:(\w+)' lib/duckdb/converter/int_to_sym.rb | sort -t: -k1 -nLength of output: 1105
🏁 Script executed:
[skip_cloning]Length of output: 52
Fix the type ID mapping inconsistencies in the converter.
The type ID mappings in lib/duckdb/logical_type.rb are correct and match DuckDB's internal constants, as confirmed by test comments and passing tests. However, there are inconsistencies between this file and lib/duckdb/converter/int_to_sym.rb:
-
Type 34 (:any) is missing from converter: It's defined as a singleton method in logical_type.rb but absent from
HASH_TYPESin the converter. CallingDuckDB::LogicalType.any.typewill fail with "Unknown type: 34". -
Type 39 (time_ns) missing from converter: It's commented out in both places, but should be handled consistently.
-
Commented-out types are intentionally excluded from singleton methods: Types like decimal, enum, list, struct, map, array, uuid, and union are complex and require additional parameters (e.g., decimal needs width/scale). They are present in the converter and fully functional when created via
LogicalType.new(type_id), as the tests demonstrate.
Actions required:
- Add type 34 (:any) and type 39 (time_ns) to converter's
HASH_TYPES - Add brief comments in logical_type.rb explaining why certain types are commented out (e.g., "# decimal: 19 — requires width and scale parameters")
Summary by CodeRabbit