[Minor] Extract interval parsing logic, add unit tests #2984
Conversation
|
|
||
| /// Parses a string with an interval like `'0.5 MONTH'` to an | ||
| /// appropriately typed [`ScalarValue`] | ||
| pub(crate) fn parse_interval(leading_field: &str, value: &str) -> Result<ScalarValue> { |
There was a problem hiding this comment.
This code was moved as closely as possible from parser.rs
| let leading_field = leading_field | ||
| .as_ref() | ||
| .map(|dt| dt.to_string()) | ||
| .unwrap_or_else(|| "second".to_string()); |
There was a problem hiding this comment.
This is the same code as used previously, but it has been raised up and passed in https://github.com/apache/arrow-datafusion/pull/2984/files#diff-c01a34949db6258aa1593f011ecf90f62cbde406acd5cdbf8b9b60b970ace1cfL2294-L2297
| use super::*; | ||
|
|
||
| #[test] | ||
| fn test_parse_ym() { |
There was a problem hiding this comment.
And thank you for them! Comparing the results of differing implementations is an extremely useful test.
Codecov Report
@@ Coverage Diff @@
## master #2984 +/- ##
==========================================
+ Coverage 85.76% 85.81% +0.05%
==========================================
Files 281 282 +1
Lines 51515 51531 +16
==========================================
+ Hits 44182 44222 +40
+ Misses 7333 7309 -24
Help us with your feedback. Take ten seconds to tell us how you rate us. |
| @@ -618,6 +619,28 @@ impl ScalarValue { | |||
| precision, scale | |||
| ))) | |||
| } | |||
|
|
|||
| /// Returns a [`ScalarValue::IntervalYearMonth`] representing | |||
| /// `years` years and `months` months | |||
There was a problem hiding this comment.
These seem very nice to have. I think more convenience functions like this are the key to terser and more readable code.
| use super::*; | ||
|
|
||
| #[test] | ||
| fn test_parse_ym() { |
There was a problem hiding this comment.
And thank you for them! Comparing the results of differing implementations is an extremely useful test.
|
Benchmark runs are scheduled for baseline = 0fa6a93 and contender = c7fa789. c7fa789 is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
Which issue does this PR close?
N/A
Rationale for this change
Something I noticed while working on https://github.com/apache/arrow-datafusion/pull/2981/files was that the parsing of strings like
3 daysto interval constants was in the sql parser logic directly which made it somewhat hard to followSince apache/datafusion-sqlparser-rs#517 added support for arbitrary
Exprs as interval arguments (rather than just strings), I thought refactoring this code into a function would be a step towards supporting generic argumentsIt also made it it easier to write tests
I was thinking eventually we could use some of the code from @avantgardnerio from apache/arrow-rs#2031 in Arrow but before I did that I wanted to make sure the existing code had reasonable test coverage (to make sure we didn't break things).
What changes are included in this PR?
ScalarValue::Interval*Are there any user-facing changes?