feat(sql): add iso-8601 format support for intervals #4291
Conversation
WalkthroughThe changes introduce new capabilities for handling intervals within the SQL module, notably adding support for ISO 8601 format strings. Functions have been renamed, new ones added, and the entire interval transformation logic refactored to handle both interval abbreviations and ISO 8601 formats effectively. These enhancements improve the clarity and functionality of interval processing in SQL operations. 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
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
Files selected for processing (4)
- src/sql/Cargo.toml (1 hunks)
- src/sql/src/statements/transform/expand_interval.rs (8 hunks)
- tests/cases/standalone/common/types/interval/interval.result (1 hunks)
- tests/cases/standalone/common/types/interval/interval.sql (1 hunks)
Files skipped from review due to trivial changes (2)
- src/sql/Cargo.toml
- tests/cases/standalone/common/types/interval/interval.sql
Additional comments not posted (12)
src/sql/src/statements/transform/expand_interval.rs (7)
81-83: Ensure correct handling of binary operations.The
normalize_interval_namefunction is used to handle the left operand of binary operations. Ensure that it correctly processes both abbreviated and ISO 8601 format strings.Verification successful
Verified: The
normalize_interval_namefunction correctly handles binary operations.The function processes both abbreviated and ISO 8601 format strings, as evidenced by the comprehensive test cases covering these scenarios.
src/sql/src/statements/transform/expand_interval.rs: Implementation and tests fornormalize_interval_name.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the normalize_interval_name function correctly processes the left operand of binary operations. # Test: Search for the function usage. Expect: Correct handling of both formats. rg --type rust $'normalize_interval_name'Length of output: 1524
Script:
#!/bin/bash # Extract the implementation of the normalize_interval_name function ast-grep --lang rust --pattern 'fn normalize_interval_name($_) { $$$ }' # List the test cases related to normalize_interval_name rg 'normalize_interval_name' --type rust --context 5Length of output: 9737
104-104: Ensure correct handling of cast expressions.The
normalize_interval_namefunction is used to handle cast expressions. Ensure that it correctly processes both abbreviated and ISO 8601 format strings.Verification successful
Ensure correct handling of cast expressions.
The
normalize_interval_namefunction is thoroughly tested and correctly processes both abbreviated and ISO 8601 format strings.
- The test cases in
src/sql/src/statements/transform/expand_interval.rscover various scenarios including valid and invalid inputs for both formats.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the normalize_interval_name function correctly processes cast expressions. # Test: Search for the function usage. Expect: Correct handling of both formats. rg --type rust $'normalize_interval_name'Length of output: 1524
Script:
#!/bin/bash # Extract test cases for the normalize_interval_name function to verify if they cover both abbreviated and ISO 8601 format strings. # Search for test functions that call normalize_interval_name rg --type rust 'normalize_interval_name' --context 10Length of output: 14870
71-74: Ensure correct handling of normalized interval names.The
normalize_interval_namefunction is used to handle the interval value. Ensure that it correctly processes both abbreviated and ISO 8601 format strings.Verification successful
Ensure correct handling of normalized interval names.
The
normalize_interval_namefunction correctly processes both abbreviated and ISO 8601 format strings, as confirmed by the implementation and extensive test cases.
normalize_interval_namefunction handles:
- Abbreviated interval strings (e.g., "1y2mo3d")
- ISO 8601 format strings (e.g., "P1Y2M3D")
- Test cases verify both valid and invalid inputs.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the normalize_interval_name function correctly processes both abbreviated and ISO 8601 format strings. # Test: Search for the function usage. Expect: Correct handling of both formats. rg --type rust $'normalize_interval_name'Length of output: 1524
Script:
#!/bin/bash # Extract the implementation of the `normalize_interval_name` function ast-grep --lang rust --pattern $'fn normalize_interval_name($_) { $$$ }' --json # Extract the test cases related to `normalize_interval_name` rg --type rust 'normalize_interval_name' --context 10 --jsonLength of output: 36402
161-176: Ensure correct parsing of ISO 8601 format strings.The
parse_iso8601_intervalfunction is a new addition to handle ISO 8601 format strings. Ensure that it correctly parses these strings and converts them to milliseconds.Verification successful
The
parse_iso8601_intervalfunction is correctly tested.The
normalize_interval_namefunction, which utilizesparse_iso8601_interval, is comprehensively tested, including cases with ISO 8601 format strings. This ensures the correctness ofparse_iso8601_interval.
src/sql/src/statements/transform/expand_interval.rs: Tests fornormalize_interval_namecover various ISO 8601 format strings.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the parse_iso8601_interval function correctly parses ISO 8601 format strings. # Test: Search for the function usage. Expect: Correct parsing of ISO 8601 format strings. rg --type rust $'parse_iso8601_interval'Length of output: 260
Script:
#!/bin/bash # Extracting the context around the function call to understand its usage and testing. # Display lines around the function call to understand its context rg --type rust -C 10 'parse_iso8601_interval' src/sql/src/statements/transform/expand_interval.rsLength of output: 1140
Script:
#!/bin/bash # Searching for tests related to `normalize_interval_name` to verify if it covers ISO 8601 format strings. # Find test files and look for `normalize_interval_name` usage rg --type rust 'normalize_interval_name'Length of output: 1524
192-196: Ensure correct identification of ISO 8601 format strings.The
is_iso8601_formatfunction is a new addition to check if a given interval string is in ISO 8601 format. Ensure that it correctly identifies these strings.Verification successful
Ensure correct identification of ISO 8601 format strings.
The
is_iso8601_formatfunction correctly identifies ISO 8601 format strings, and theparse_iso8601_intervalfunction processes these strings accurately.
- The
is_iso8601_formatfunction checks for strings starting with 'P', 'p', '-P', or '-p', which are valid indicators of ISO 8601 intervals.- The
parse_iso8601_intervalfunction correctly parses these strings and converts them to milliseconds.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the is_iso8601_format function correctly identifies ISO 8601 format strings. # Test: Search for the function usage. Expect: Correct identification of ISO 8601 format strings. rg --type rust $'is_iso8601_format'Length of output: 230
Script:
#!/bin/bash # Extract the surrounding lines of the usage of is_iso8601_format to understand its context and usage. rg --type rust -A 10 -B 10 'is_iso8601_format\(interval_str\)' src/sql/src/statements/transform/expand_interval.rsLength of output: 1033
Script:
#!/bin/bash # Extract the full implementation of parse_iso8601_interval to verify its correctness. rg --type rust -A 20 -B 0 'fn parse_iso8601_interval' src/sql/src/statements/transform/expand_interval.rsLength of output: 810
141-159: Ensure correct handling of ISO 8601 format strings.The
normalize_interval_namefunction has been updated to handle ISO 8601 format strings. Ensure that it correctly processes these strings and maintains existing functionality.Verification successful
Verified handling of ISO 8601 format strings.
The
normalize_interval_namefunction correctly processes ISO 8601 format strings, as evidenced by the comprehensive test cases and their expected outcomes.
- Valid ISO 8601 strings are correctly converted to milliseconds.
- Negative interval strings are correctly processed.
- Invalid ISO 8601 strings return
None.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the normalize_interval_name function correctly processes ISO 8601 format strings. # Test: Search for the function usage. Expect: Correct handling of ISO 8601 format strings. rg --type rust $'normalize_interval_name'Length of output: 1524
Script:
#!/bin/bash # Description: Extract and review test cases for the normalize_interval_name function to verify ISO 8601 format handling. # Search for test cases in the expand_interval.rs file. rg --type rust --pcre2 'normalize_interval_name\("P\d+Y\d+M\d+D.*\)' src/sql/src/statements/transform/expand_interval.rsLength of output: 177
Script:
#!/bin/bash # Description: Extract all test cases for the normalize_interval_name function to verify ISO 8601 format handling. # Search for all test cases in the expand_interval.rs file. rg --type rust --pcre2 'normalize_interval_name\(".*"\)' src/sql/src/statements/transform/expand_interval.rs -A 3Length of output: 629
178-190: Ensure correct expansion of interval abbreviation strings.The
expand_interval_abbreviationfunction is a new addition to handle interval abbreviation strings. Ensure that it correctly expands these strings to full interval names.tests/cases/standalone/common/types/interval/interval.result (5)
341-347: Ensure correct conversion of ISO 8601 format interval.The test case demonstrates the conversion of an ISO 8601 format interval to the internal representation. Ensure that it correctly covers the new functionality and maintains existing functionality.
349-355: Ensure correct conversion of negative ISO 8601 format interval.The test case demonstrates the conversion of a negative ISO 8601 format interval to the internal representation. Ensure that it correctly covers the new functionality and maintains existing functionality.
357-363: Ensure correct casting of ISO 8601 format interval.The test case demonstrates casting an ISO 8601 format interval to the internal representation. Ensure that it correctly covers the new functionality and maintains existing functionality.
365-371: Ensure correct addition of ISO 8601 format interval.The test case demonstrates the addition of an ISO 8601 format interval to another interval. Ensure that it correctly covers the new functionality and maintains existing functionality.
Line range hint
1-372:
Ensure correct interval calculations and conversions.The remaining test cases demonstrate various interval calculations and conversions. Ensure that they correctly cover the new functionality and maintain existing functionality.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #4291 +/- ##
==========================================
- Coverage 85.22% 84.96% -0.27%
==========================================
Files 1059 1059
Lines 187054 187111 +57
==========================================
- Hits 159412 158973 -439
- Misses 27642 28138 +496 |
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
Files selected for processing (1)
- src/sql/src/statements/transform/expand_interval.rs (8 hunks)
Additional comments not posted (5)
src/sql/src/statements/transform/expand_interval.rs (5)
149-159: LGTM! But verify the function usage in the codebase.The code changes are approved.
However, ensure that all function calls to
normalize_interval_nameare updated from the old nameexpand_interval_name.Verification successful
Function usage verification complete.
All references to the old function name
expand_interval_namehave been successfully updated tonormalize_interval_name.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `normalize_interval_name` are updated from the old name `expand_interval_name`. # Test: Search for the function usage. Expect: Only occurrences of the new function name. rg --type rust $'normalize_interval_name'Length of output: 1524
Script:
#!/bin/bash # Description: Verify that there are no remaining calls to the old function name `expand_interval_name`. # Test: Search for the old function name. Expect: No occurrences of the old function name. rg --type rust $'expand_interval_name'Length of output: 37
192-196: LGTM! But verify the function usage in the codebase.The code changes are approved.
However, ensure that all function calls to
is_iso8601_formatare correct.Verification successful
Let's inspect the surrounding code where
is_iso8601_formatis used to ensure it is being used correctly.
The function usage is correct.
The function
is_iso8601_formatis used appropriately within thenormalize_interval_namefunction to check for ISO 8601 formatted interval strings.
src/sql/src/statements/transform/expand_interval.rs: Lines 192-196Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `is_iso8601_format` are correct. # Test: Search for the function usage. Expect: Correct usage of the function. rg --type rust $'is_iso8601_format'Length of output: 230
Script:
#!/bin/bash # Extract lines around the usage of `is_iso8601_format` to verify the context. rg --type rust -A 10 -B 10 'is_iso8601_format' src/sql/src/statements/transform/expand_interval.rsLength of output: 1527
178-190: LGTM! But verify the function usage in the codebase.The code changes are approved.
However, ensure that all function calls to
expand_interval_abbreviationare correct.Verification successful
Function usage verified successfully.
The function
expand_interval_abbreviationis used correctly within the same file as a fallback when the interval string is not in ISO8601 format.
src/sql/src/statements/transform/expand_interval.rs: Verified usage in the conditional block.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `expand_interval_abbreviation` are correct. # Test: Search for the function usage. Expect: Correct usage of the function. rg --type rust $'expand_interval_abbreviation'Length of output: 268
Script:
#!/bin/bash # Extract and review the context around the function call to ensure it is used correctly. rg -A 5 -B 5 'expand_interval_abbreviation' src/sql/src/statements/transform/expand_interval.rsLength of output: 701
Line range hint
71-104:
LGTM! But verify the struct usage in the codebase.The code changes are approved.
However, ensure that all struct usages of
ExpandIntervalTransformRuleare correct.Verification successful
Verified struct usage of
ExpandIntervalTransformRule.The usage of
ExpandIntervalTransformRulein the codebase is correct. The struct is properly integrated and tested for various interval expressions, ensuring the correctness of the changes made.
src/sql/src/statements/transform.rssrc/sql/src/statements/transform/expand_interval.rsScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all struct usages of `ExpandIntervalTransformRule` are correct. # Test: Search for the struct usage. Expect: Correct usage of the struct. rg --type rust $'ExpandIntervalTransformRule'Length of output: 1016
Script:
#!/bin/bash # Extract lines around usages of ExpandIntervalTransformRule to verify correctness. # Extract lines around usage in transform.rs rg --type rust --context 5 'ExpandIntervalTransformRule' src/sql/src/statements/transform.rs # Extract lines around usage in expand_interval.rs rg --type rust --context 5 'ExpandIntervalTransformRule' src/sql/src/statements/transform/expand_interval.rsLength of output: 3438
161-176: LGTM! But verify the function usage in the codebase.The code changes are approved.
However, ensure that all function calls to
parse_iso8601_intervalare correct.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
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
I hereby agree to the terms of the GreptimeDB CLA.
Refer to a related PR or issue link
#4215
What's changed and what's your intention?
Add support for the format of ISO 8601 for the interval type.
Checklist
Summary by CodeRabbit
New Features
Improvements
Tests