feat(sql): add casting support for shortened intervals #4220
Conversation
WalkthroughThe recent changes enhance handling and transformation of SQL Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (2)
src/sql/src/statements/transform/expand_interval.rs (2)
Line range hint
68-115: Refined logic for handling interval expressions.The modifications to
visit_exprmethod enhance the handling of both simple and complex interval expressions. The use of cloning and matching patterns is appropriate for the transformations required. However, there's a potential area of improvement in error handling when the expected data is not found.Consider adding error handling or logging when
expand_interval_namereturnsNone, to aid in debugging and ensure robustness.
Line range hint
161-343: Comprehensive unit tests for interval transformations.The tests cover a wide range of scenarios, including basic and compound conversions, and transformations within different types of expressions. This thorough testing is crucial for ensuring the reliability of the new interval handling features.
However, it would be beneficial to add negative test cases to verify the behavior when invalid inputs are provided.
Would you like assistance in writing these negative test cases?
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- src/sql/src/statements/transform/expand_interval.rs (10 hunks)
- tests/cases/standalone/common/types/interval/interval.result (2 hunks)
- tests/cases/standalone/common/types/interval/interval.sql (2 hunks)
Files skipped from review due to trivial changes (2)
- tests/cases/standalone/common/types/interval/interval.result
- tests/cases/standalone/common/types/interval/interval.sql
Additional comments not posted (3)
src/sql/src/statements/transform/expand_interval.rs (3)
21-21: Updated import statement to includeDataType.This change is necessary for the new features related to interval type casting and is consistent with the PR objectives.
39-39: Addition of new interval abbreviations for better granularity.Adding 'ms' for milliseconds, 'us' for microseconds, and 'ns' for nanoseconds enhances the functionality for handling more precise time intervals. This change aligns with the PR's goal to improve interval handling.
55-55: Documentation update for new interval abbreviations.The documentation here is updated to reflect the new abbreviations, ensuring clarity and maintainability. It's important for future developers to understand these mappings quickly.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4220 +/- ##
==========================================
- Coverage 84.84% 84.57% -0.28%
==========================================
Files 1041 1041
Lines 183028 183074 +46
==========================================
- Hits 155291 154830 -461
- Misses 27737 28244 +507 |
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- src/sql/src/statements/transform/expand_interval.rs (10 hunks)
Files skipped from review as they are similar to previous changes (1)
- src/sql/src/statements/transform/expand_interval.rs
There was a problem hiding this comment.
Thanks for your follow-up @etolbakov!
There can be also a follow-up to GreptimeTeam/docs#1021 (remove millis). Would you take a look at that? Or I can do it today.
|
Thanks @tisonkun! |
…4220) * feat(sql): add casting support for shortened intervals * chore(sql): apply CR suggestion, minor renamings
…4220) * feat(sql): add casting support for shortened intervals * chore(sql): apply CR suggestion, minor renamings
I hereby agree to the terms of the GreptimeDB CLA.
Refer to a related PR or issue link
#4168
What's changed and what's your intention?
Checklist
Summary by CodeRabbit
New Features
Bug Fixes