Conversation
📝 WalkthroughWalkthroughA new casting feature is added to DuckDB, allowing users to convert values between types (integer, float, varchar, timestamp, date). The implementation delegates to existing converter utilities with comprehensive test coverage and minor exception-handling refactoring in converter methods. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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 (2)
lib/duckdb/casting.rb (1)
17-18: Minor: Consider usingvalue.inspectfor consistency.The error message uses
#{value}directly, butConvertermethods use#{value.inspect}for better debugging output (e.g., distinguishingnilfrom"nil", showing string quotes).Suggested change
- raise ArgumentError, "Unsupported type: #{type} for value: #{value}" + raise ArgumentError, "Unsupported type: #{type} for value: #{value.inspect}"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/duckdb/casting.rb` around lines 17 - 18, The error raise in lib/duckdb/casting.rb uses #{value} which is inconsistent with other Converter methods; update the else branch that raises ArgumentError to interpolate the inspected representation of the value (#{value.inspect}) so the message becomes: raise ArgumentError, "Unsupported type: #{type} for value: #{value.inspect}"; ensure this change is applied in the same method/class that handles type conversion (Converter or the casting method) to keep debug output consistent.test/duckdb_test/casting_test.rb (1)
58-61: Consider adding a test for invalid date input.For consistency with
test_cast_timestamp_invalid, consider adding a test case verifying that invalid date strings raiseArgumentError.Suggested test
def test_cast_date_invalid assert_raises(ArgumentError) { DuckDB.cast('invalid date', :date) } end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/duckdb_test/casting_test.rb` around lines 58 - 61, Add a new unit test that mirrors test_cast_timestamp_invalid by asserting that DuckDB.cast raises ArgumentError for invalid date input; specifically, create a test method (e.g., test_cast_date_invalid) that calls DuckDB.cast with a bad string like 'invalid date' and asserts it raises ArgumentError so test_cast_date and DuckDB.cast's date handling are covered for failure cases.
🤖 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/casting_test.rb`:
- Around line 48-52: The assertion in test_cast_timestamp can fail in non-UTC
environments because DateTime.new(...).to_time uses the local TZ; update the
test to normalize the DateTime conversion to UTC before casting and comparison:
convert the DateTime to a Time in UTC (or construct the expected Time in UTC) so
that DuckDB.cast(DateTime.new(...)) is compared against a UTC Time; modify the
call references in test_cast_timestamp and the DuckDB.cast assertion to use the
UTC-normalized Time to avoid timezone-dependent failures.
---
Nitpick comments:
In `@lib/duckdb/casting.rb`:
- Around line 17-18: The error raise in lib/duckdb/casting.rb uses #{value}
which is inconsistent with other Converter methods; update the else branch that
raises ArgumentError to interpolate the inspected representation of the value
(#{value.inspect}) so the message becomes: raise ArgumentError, "Unsupported
type: #{type} for value: #{value.inspect}"; ensure this change is applied in the
same method/class that handles type conversion (Converter or the casting method)
to keep debug output consistent.
In `@test/duckdb_test/casting_test.rb`:
- Around line 58-61: Add a new unit test that mirrors
test_cast_timestamp_invalid by asserting that DuckDB.cast raises ArgumentError
for invalid date input; specifically, create a test method (e.g.,
test_cast_date_invalid) that calls DuckDB.cast with a bad string like 'invalid
date' and asserts it raises ArgumentError so test_cast_date and DuckDB.cast's
date handling are covered for failure cases.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
CHANGELOG.mdlib/duckdb.rblib/duckdb/casting.rblib/duckdb/converter.rbtest/duckdb_test/casting_test.rb
Comment on lines
+48
to
+52
| def test_cast_timestamp | ||
| assert_equal(Time.new(2024, 1, 2, 3, 4, 5), DuckDB.cast('2024-01-02 03:04:05', :timestamp)) | ||
| assert_equal(Time.new(2024, 1, 2, 3, 4, 5), DuckDB.cast(Time.new(2024, 1, 2, 3, 4, 5), :timestamp)) | ||
| assert_equal(Time.new(2024, 1, 2, 3, 4, 5, '+0000'), DuckDB.cast(DateTime.new(2024, 1, 2, 3, 4, 5), :timestamp)) | ||
| end |
There was a problem hiding this comment.
Potential timezone-dependent test failure.
Line 51 asserts equality with Time.new(2024, 1, 2, 3, 4, 5, '+0000'), but DateTime.new(2024, 1, 2, 3, 4, 5).to_time converts to the local timezone, which may differ from UTC. This test could fail in non-UTC environments.
Proposed fix
- assert_equal(Time.new(2024, 1, 2, 3, 4, 5, '+0000'), DuckDB.cast(DateTime.new(2024, 1, 2, 3, 4, 5), :timestamp))
+ dt = DateTime.new(2024, 1, 2, 3, 4, 5)
+ assert_equal(dt.to_time, DuckDB.cast(dt, :timestamp))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/duckdb_test/casting_test.rb` around lines 48 - 52, The assertion in
test_cast_timestamp can fail in non-UTC environments because
DateTime.new(...).to_time uses the local TZ; update the test to normalize the
DateTime conversion to UTC before casting and comparison: convert the DateTime
to a Time in UTC (or construct the expected Time in UTC) so that
DuckDB.cast(DateTime.new(...)) is compared against a UTC Time; modify the call
references in test_cast_timestamp and the DuckDB.cast assertion to use the
UTC-normalized Time to avoid timezone-dependent failures.
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 by CodeRabbit
New Features
Tests