Support TIMESTAMP column writing Ruby Time object in DataChunk#set_value#1124
Support TIMESTAMP column writing Ruby Time object in DataChunk#set_value#1124
Conversation
- Add MemoryHelper.write_timestamp C method that converts a Ruby Time object to duckdb_timestamp using local time components (matching the read convention of Converter#_to_time which uses Time.local) - Add :timestamp case in DataChunk#set_value to call write_timestamp - Add test_data_chunk_set_value_timestamp to verify round-trip Co-authored-by: Copilot <[email protected]>
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds TIMESTAMP write support to the Ruby bindings: a C-level Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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.
🧹 Nitpick comments (1)
test/duckdb_test/data_chunk_test.rb (1)
376-397: Add one non-UTC timestamp case to harden timezone coverage.This test is good; adding a non-UTC
Timeinput will better guard the conversion path.Suggested test strengthening
time1 = Time.new(2024, 3, 15, 10, 30, 45, '+00:00') time2 = Time.new(2000, 1, 1, 0, 0, 0, '+00:00') + time3 = Time.new(2024, 3, 15, 10, 30, 45, '+09:00') @@ output.set_value(0, 0, time1) output.set_value(0, 1, time2) - output.size = 2 + output.set_value(0, 2, time3) + output.size = 3 done = true end end @@ - assert_equal 2, rows.length + assert_equal 3, rows.length assert_equal time1.utc, rows[0].first.utc assert_equal time2.utc, rows[1].first.utc + assert_equal time3.utc, rows[2].first.utc🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/duckdb_test/data_chunk_test.rb` around lines 376 - 397, Add a third timestamp with a non-UTC offset (e.g., Time.new(..., '+05:30')) to the test to exercise timezone conversion: create a new variable (e.g., time3) alongside time1/time2, set it into the table_function output in table_function.execute (use output.set_value for the appropriate row/column index and increment output.size), register the table function and query test_set_value_timestamp(), then assert the returned row for that new entry matches time3.utc (use the same pattern as the existing assertions). Ensure you reference the existing symbols time1, time2, table_function.execute, output.set_value, `@conn.register_table_function`, and test_set_value_timestamp to locate where to insert the new case.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/duckdb_test/data_chunk_test.rb`:
- Around line 376-397: Add a third timestamp with a non-UTC offset (e.g.,
Time.new(..., '+05:30')) to the test to exercise timezone conversion: create a
new variable (e.g., time3) alongside time1/time2, set it into the table_function
output in table_function.execute (use output.set_value for the appropriate
row/column index and increment output.size), register the table function and
query test_set_value_timestamp(), then assert the returned row for that new
entry matches time3.utc (use the same pattern as the existing assertions).
Ensure you reference the existing symbols time1, time2, table_function.execute,
output.set_value, `@conn.register_table_function`, and test_set_value_timestamp to
locate where to insert the new case.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
CHANGELOG.mdext/duckdb/memory_helper.clib/duckdb/data_chunk.rbtest/duckdb_test/data_chunk_test.rb
Co-authored-by: Copilot <[email protected]>
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 `@sample/issue922.rb`:
- Line 38: The code calls the nonexistent symbol polars.dataframe.new which will
raise NameError; replace that call with the correct constant/class constructor
Polars::DataFrame.new(...) (capitalize Polars and use :: to reference the
DataFrame class) so the code invokes the Polars::DataFrame constructor instead
of trying to call undefined methods like polars or dataframe.
1 participant
Summary
Support writing Ruby
Timeobjects to TIMESTAMP columns viaDuckDB::DataChunk#set_value.Changes
ext/duckdb/memory_helper.c: AddMemoryHelper.write_timestampC method that converts a RubyTimeobject to aduckdb_timestamp. It calls.getlocalon the input first to extract local-time components, matching the read convention ofConverter#_to_timewhich usesTime.local.lib/duckdb/data_chunk.rb: Add:timestampcase inDataChunk#set_valueto callMemoryHelper.write_timestamp.test/duckdb_test/data_chunk_test.rb: Addtest_data_chunk_set_value_timestampto verify round-trip of twoTimevalues through a table function.CHANGELOG.md: Document the new feature.Example
Summary by CodeRabbit
New Features
Tests