Add TIME_NS column support to DuckDB::Result#1237
Merged
Conversation
- Add `_to_time_from_duckdb_time_ns` in converter.rb to decompose nanoseconds-since-midnight into hour/min/sec/microsecond (sub-microsecond precision is truncated to match Ruby's Time precision) - Add `rbduckdb_time_ns_to_ruby` C function and `id__to_time_from_duckdb_time_ns` ID wiring in conveter.c / converter.h - Handle `DUCKDB_TYPE_TIME_NS` (type 39) in result.c vector dispatch and value_impl.c duckdb_value dispatch - Add focused test file result_time_ns_test.rb - Add TIME_NS entry to result_each_test.rb TEST_TABLES Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds DuckDB TIME_NS support across the Ruby extension: new C converter ID/function, vector/value dispatch for Changes
Sequence Diagram(s)sequenceDiagram
participant RubyApp as Ruby (caller)
participant Ext as C extension
participant DuckDB as DuckDB C API
participant Converter as Ruby Converter
RubyApp->>DuckDB: execute query with TIME_NS column
DuckDB-->>Ext: returns vector with DUCKDB_TYPE_TIME_NS elements
Ext->>Ext: rbduckdb_vector_value_at detects DUCKDB_TYPE_TIME_NS
Ext->>Converter: call rbduckdb_time_ns_to_ruby(ts.nanos)
Converter-->>RubyApp: returns Ruby Time (nanos truncated to micros)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/duckdb_test/result_each_test.rb`:
- Line 110: The TIME_NS test row is inserting the literal "'12:34:56.789123456'"
without an explicit type cast, causing DuckDB to return NULL and later a
NoMethodError when Time methods are called; update the test data row where the
column/type is 'TIME_NS' to cast the literal to TIME_NS (change the literal to
include ::TIME_NS) so the INSERT parses the nanosecond-precision time
correctly—look for the array entry containing 'TIME_NS' in result_each_test.rb
and modify the third value in that tuple to "'12:34:56.789123456'::TIME_NS".
🪄 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: 158af264-fd15-41b9-88c4-4e7c8e00e1b6
📒 Files selected for processing (7)
CHANGELOG.mdext/duckdb/converter.hext/duckdb/conveter.cext/duckdb/result.cext/duckdb/value_impl.clib/duckdb/converter.rbtest/duckdb_test/result_each_test.rb
| # set TIMEZONE to Asia/Kabul to test TIMETZ and TIMESTAMPTZ | ||
| [:ok, 'TIMETZ', 'TIMETZ', "'2019-11-03 12:34:56.123456789'", Time, timetz_expected ], | ||
| [:ok, 'TIMESTAMPTZ', 'TIMESTAMPTZ', "'2019-11-03 12:34:56.123456789'", Time, Time.parse('2019-11-03 08:04:56.123456+0000') ], | ||
| [:ok, 'TIME_NS', 'TIME_NS', "'12:34:56.789123456'", Time, Time.parse('12:34:56.789123') ], |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if TIME_NS is a recognized DuckDB type and how it should be used
rg -n 'TIME_NS' --type=c ext/duckdb/
# Check similar test patterns for other time types
rg -n "TIMESTAMP_NS|TIME_TZ" test/duckdb_test/result_each_test.rb -A2 -B2
# Look for how TIME_NS is documented/used elsewhere
rg -n 'time_ns|TIME_NS' lib/ ext/Repository: suketa/ruby-duckdb
Length of output: 2652
🏁 Script executed:
sed -n '70,90p' lib/duckdb/converter.rbRepository: suketa/ruby-duckdb
Length of output: 777
🏁 Script executed:
sed -n '130,145p' ext/duckdb/conveter.cRepository: suketa/ruby-duckdb
Length of output: 676
🏁 Script executed:
grep -n "time_ns\|TIME_NS" lib/duckdb/converter.rb -B2 -A5Repository: suketa/ruby-duckdb
Length of output: 309
🏁 Script executed:
grep -n "_to_time_from_duckdb_time" lib/duckdb/converter.rb -A3Repository: suketa/ruby-duckdb
Length of output: 1621
🏁 Script executed:
sed -n '140,150p' test/duckdb_test/result_each_test.rbRepository: suketa/ruby-duckdb
Length of output: 476
🏁 Script executed:
grep -n "TIME_NS\|time_ns" ext/extconf.rb ext/duckdb.h 2>/dev/null || grep -rn "DuckDB.*1\." ext/extconf.rb 2>/dev/null | head -20Repository: suketa/ruby-duckdb
Length of output: 44
🏁 Script executed:
# Check if there are any version checks or conditionals around TIME_NS
grep -rn "TIME_NS" ext/duckdb/ --type=c -B3 -A3 | head -40
# Check DuckDB C library version constants
fd -e h -e c ext/duckdb | xargs grep -l "DUCKDB_VERSION\|DUCKDB_TYPE_TIME"Repository: suketa/ruby-duckdb
Length of output: 529
🏁 Script executed:
# Check if there's a .duckdb.so or compiled extension
find ext -name "*.so" -o -name "*.h" | grep -E "(version|duckdb\.h)" | head -5
# Search for any version-gated code or TIME_NS conditionals
rg "VERSION|TIME_NS" ext/ --type=c --type=h -i | grep -E "(if|define)" | head -20Repository: suketa/ruby-duckdb
Length of output: 195
🏁 Script executed:
# Check DuckDB header for type definitions
cat ext/duckdb/ruby-duckdb.h | grep -i "time_ns\|version" | head -20
# Look for the actual duckdb version being used
rg "duckdb.*version" ext/ lib/ -i -A2 -B2
# Check when TIME_NS was introduced in DuckDB - look at comments or documentation
rg "TIME_NS|type.*39" ext/duckdb/ -A2 -B2Repository: suketa/ruby-duckdb
Length of output: 3946
🏁 Script executed:
# Find the query generation logic in the test
sed -n '1,50p' test/duckdb_test/result_each_test.rb
# Look for the query building logic
rg -n "insert into\|CREATE TABLE\|def.*query" test/duckdb_test/result_each_test.rb -i -B2 -A2 | head -40Repository: suketa/ruby-duckdb
Length of output: 4720
🏁 Script executed:
# Check git log for the TIME_NS addition
git log -p --all -- test/duckdb_test/result_each_test.rb | grep -A5 -B5 "TIME_NS" | head -50Repository: suketa/ruby-duckdb
Length of output: 44
🏁 Script executed:
# Find the exact TIME_NS entry and surrounding context
sed -n '100,125p' test/duckdb_test/result_each_test.rb
# Find where queries are executed
rg -n "def test_\|execute\|TEST_TABLES\[" test/duckdb_test/result_each_test.rb | head -20
# Look at how the test iterates through TEST_TABLES
sed -n '150,200p' test/duckdb_test/result_each_test.rbRepository: suketa/ruby-duckdb
Length of output: 4189
🏁 Script executed:
# Check what happens with BIT which uses explicit casting
sed -n '115,120p' test/duckdb_test/result_each_test.rb
# Search for other types that use explicit casting in the test data
rg "::BIT|::TIME|CAST" test/duckdb_test/result_each_test.rbRepository: suketa/ruby-duckdb
Length of output: 922
🏁 Script executed:
# Check if there are any recent commits that added TIME_NS
git log --oneline --all -- test/duckdb_test/result_each_test.rb | head -10
# Look for any TODOs or FIXMEs related to TIME_NS
rg "TODO|FIXME|TIME_NS" test/ lib/ ext/ -iRepository: suketa/ruby-duckdb
Length of output: 2393
🏁 Script executed:
# Show the full commit that added TIME_NS support
git show 1903178 --stat
# Also check if there are any other TIME_NS tests or examples in the codebase
rg "TIME_NS" . --type=rb --type=c -B3 -A3 | grep -E "(test_|def |TIME_NS)" | head -30Repository: suketa/ruby-duckdb
Length of output: 11518
🏁 Script executed:
# Check if there's any documentation or comments about TIME_NS syntax in DuckDB
rg "TIME_NS|time_ns" ext/duckdb/ -B5 -A5 | grep -E "(comment|TODO|//|/\*)" | head -20
# Check if there's a way to cast to TIME_NS
rg "CAST.*TIME" test/ lib/ -i | head -10Repository: suketa/ruby-duckdb
Length of output: 835
🏁 Script executed:
# Look for the result_time_ns_test.rb file
find test -name "*time_ns*" -o -name "*TIME_NS*"
# If it exists, show its contents
cat test/duckdb_test/result_time_ns_test.rb 2>/dev/null || echo "File not found"Repository: suketa/ruby-duckdb
Length of output: 76
🏁 Script executed:
# Search for TIME test patterns to see if there's a difference
rg -n "result_time.*test|TIME.*test" test/duckdb_test/ | grep -v result_each_test
# Look for the actual TIME entry in TEST_TABLES to compare with TIME_NS
sed -n '50,60p' test/duckdb_test/result_each_test.rbRepository: suketa/ruby-duckdb
Length of output: 2045
🏁 Script executed:
# Check if result_time_ns_test.rb exists
ls -la test/duckdb_test/result_time_ns_test.rb 2>&1
# List all test files to see what exists
ls test/duckdb_test/*time* 2>/dev/null || echo "No time-related test files found"
# Check git status to see if the file was supposed to be there
git log --name-status --oneline 1903178 | head -30Repository: suketa/ruby-duckdb
Length of output: 1036
🏁 Script executed:
# Check for recent commits after TIME_NS support was added
git log --oneline --all | head -20
# Check if there are any skipped tests related to TIME_NS
rg "skip.*TIME_NS|TIME_NS.*skip" test/Repository: suketa/ruby-duckdb
Length of output: 1382
🏁 Script executed:
# Check if there are any comments or documentation about TIME_NS syntax
rg "TIME_NS|time_ns" . --type=md --type=txt 2>/dev/null | head -20
# Check if BIT uses explicit casting and if TIME_NS should too
rg "::BIT|::TIME|CAST.*TIME" test/duckdb_test/ -B2 -A2 | head -40
# Look at how result_time_tz_test.rb handles TIMETZ to see if TIME_NS needs similar setup
cat test/duckdb_test/result_time_tz_test.rbRepository: suketa/ruby-duckdb
Length of output: 3743
🏁 Script executed:
# Check if TIME_NS needs special setup like ICU like TIMETZ does
rg "INSTALL|LOAD" test/duckdb_test/result_time_tz_test.rb
# Check if the prepare_test_table_and_data function has any special handling we missed
sed -n '117,135p' test/duckdb_test/result_each_test.rb
# Check if TIME_NS might need explicit casting like BIT does
rg -A30 "def prepare_test_table_and_data" test/duckdb_test/result_each_test.rb | head -40Repository: suketa/ruby-duckdb
Length of output: 2276
🏁 Script executed:
# Compare how TIME and TIME_NS values are being parsed in the INSERT statement
rg "TIME.*'[0-9:]" test/duckdb_test/result_each_test.rb | grep -E "(TIME_NS|'TIME'|TIME[^_Z])"
# Check if we should be using CAST for TIME_NS
git show 1903178:test/duckdb_test/result_each_test.rb | grep -A2 -B2 "TIME_NS"Repository: suketa/ruby-duckdb
Length of output: 2727
🏁 Script executed:
# Count how many TIME_NS entries exist in the current file
grep -n "TIME_NS" test/duckdb_test/result_each_test.rb | wc -l
# Show all TIME_NS entries with context
grep -n "TIME_NS" test/duckdb_test/result_each_test.rb -B1 -A1
# Check if this might be a syntax issue - does TIME_NS need explicit casting like BIT?
# Let me check if DuckDB documentation says TIME_NS needs casting
rg "::TIME|CAST.*TIME" test/ -i | head -10Repository: suketa/ruby-duckdb
Length of output: 1609
🏁 Script executed:
# Check if there's any example of TIME_NS being used with CAST
git log --all -p | grep -A5 -B5 "TIME_NS.*CAST\|CAST.*TIME_NS" | head -20
# Search for any workarounds or issues related to TIME_NS literal parsing
rg "TIME_NS" . -i | grep -E "(TODO|FIXME|BUG|issue|workaround)"Repository: suketa/ruby-duckdb
Length of output: 148
🏁 Script executed:
# Check if adding explicit casting would work by looking at similar patterns
# First, let's see what happens if we look at how the INSERT query would look
echo "Current TIME_NS test:"
echo "INSERT INTO tests VALUES ( '12:34:56.789123456' )"
echo ""
echo "Likely fix - with explicit casting:"
echo "INSERT INTO tests VALUES ( '12:34:56.789123456'::TIME_NS )"
echo ""
# Verify that BIT uses explicit casting and TIME doesn't
grep -E "BIT|'TIME'" test/duckdb_test/result_each_test.rb | grep -E "(::|\[')" | head -5Repository: suketa/ruby-duckdb
Length of output: 950
Add explicit casting to TIME_NS literal in test data.
The TIME_NS test fails with NoMethodError: undefined method 'hour' for nil because the INSERT statement cannot parse the TIME_NS literal. Precision-dependent types like BIT require explicit casting (e.g., '010110'::BIT), and TIME_NS follows the same pattern.
Update line 110 in test/duckdb_test/result_each_test.rb to add explicit casting:
From:
[:ok, 'TIME_NS', 'TIME_NS', "'12:34:56.789123456'", Time, Time.parse('12:34:56.789123') ],
To:
[:ok, 'TIME_NS', 'TIME_NS', "'12:34:56.789123456'::TIME_NS", Time, Time.parse('12:34:56.789123') ],
Without the casting, DuckDB returns NULL for the TIME_NS column, causing the nil error when the test tries to access time properties.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/duckdb_test/result_each_test.rb` at line 110, The TIME_NS test row is
inserting the literal "'12:34:56.789123456'" without an explicit type cast,
causing DuckDB to return NULL and later a NoMethodError when Time methods are
called; update the test data row where the column/type is 'TIME_NS' to cast the
literal to TIME_NS (change the literal to include ::TIME_NS) so the INSERT
parses the nanosecond-precision time correctly—look for the array entry
containing 'TIME_NS' in result_each_test.rb and modify the third value in that
tuple to "'12:34:56.789123456'::TIME_NS".
TIME_NS vector support was added in DuckDB v1.5.0. On 1.4.x the type constant (39) is defined in the header but the runtime returns nil for TIME_NS column values. Add MINIMUM_DUCKDB_VERSION map and skip guard in the dynamically generated test methods in result_each_test.rb. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/duckdb_test/result_each_test.rb`:
- Around line 166-169: The test currently calls
DuckDBTest.duckdb_library_version inside a dynamically defined method which can
resolve to the nested class DuckDBTest::DuckDBTest; change those calls to use
the top-level namespace by replacing DuckDBTest.duckdb_library_version with
::DuckDBTest.duckdb_library_version so the module method is always invoked
(apply this update in the dynamic test methods surrounding the
MINIMUM_DUCKDB_VERSION check and the other occurrence noted).
🪄 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: 0ada2dc0-3ed0-4b99-b7e1-2aae56fe811b
📒 Files selected for processing (1)
test/duckdb_test/result_each_test.rb
Inside `module DuckDBTest`, bare `DuckDBTest` resolves to `DuckDBTest::DuckDBTest` (the test base class), not the top-level module where `duckdb_library_version` is defined. Prefix with `::` to force top-level lookup. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
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
Add support for
TIME_NS(DUCKDB_TYPE_TIME_NS, type 39) columns inDuckDB::Result.TIME_NSvalues are returned asTimeobjects. Nanoseconds are truncated to microseconds, consistent with howTIMESTAMP_NSis handled and with Ruby'sTimeprecision limit.Changes
lib/duckdb/converter.rb— add_to_time_from_duckdb_time_ns(nanos)that decomposes nanoseconds-since-midnight into hour/min/sec/microsecondext/duckdb/conveter.c/converter.h— addrbduckdb_time_ns_to_rubyC function andid__to_time_from_duckdb_time_nsIDext/duckdb/result.c— addvector_time_nsandDUCKDB_TYPE_TIME_NScase inrbduckdb_vector_value_atext/duckdb/value_impl.c— addDUCKDB_TYPE_TIME_NScase usingduckdb_get_time_nstest/duckdb_test/result_each_test.rb— add TIME_NS entry toTEST_TABLES(covers both sync and async query paths)CHANGELOG.md— document the additionSummary by CodeRabbit