Add additional testing for unwrap_cast_in_comparison
#4147
Conversation
| // Verify that calling the arrow | ||
| // cast kernel yields the same results | ||
| // input array | ||
| let literal_array = literal.to_array_of_size(1); | ||
| let expected_array = expected_value.to_array_of_size(1); | ||
| let cast_array = cast_with_options( | ||
| &literal_array, | ||
| &target_type, | ||
| &CastOptions { safe: true }, | ||
| ) | ||
| .expect("Expected to be cast array with arrow cast kernel"); | ||
|
|
||
| assert_eq!( | ||
| &expected_array, &cast_array, | ||
| "Result of casing {:?} with arrow was\n {:#?}\nbut expected\n{:#?}", | ||
| literal, cast_array, expected_array | ||
| ); | ||
|
|
||
| // Verify that for timestamp types the timezones are the same | ||
| // (ScalarValue::cmp doesn't account for timezones); | ||
| if let ( | ||
| DataType::Timestamp(left_unit, left_tz), | ||
| DataType::Timestamp(right_unit, right_tz), | ||
| ) = (actual_value.get_datatype(), expected_value.get_datatype()) | ||
| { | ||
| assert_eq!(left_unit, right_unit); | ||
| assert_eq!(left_tz, right_tz); | ||
| } | ||
| } |
There was a problem hiding this comment.
I want this coverage in particular to ensure that the casting we implement in unwrap cast is consistent with the arrow cast kernel -- this is especially important for timestamps (see #3938)
There was a problem hiding this comment.
This is super cool, double verification 💯
There was a problem hiding this comment.
I want this coverage in particular to ensure that the casting we implement in unwrap cast is consistent with the arrow cast kernel -- this is especially important for timestamps (see #3938)
thanks @alamb
I know the timestamp is most important type in the time series database.
Optimization about the type of time will benefit the InfluxIO
There was a problem hiding this comment.
I know the timestamp is most important type in the time series database.
Optimization about the type of time will benefit the InfluxIO
Yes, indeed this is why I care so much about timestamps :)
|
|
||
| #[test] | ||
| fn test_try_cast_to_type_nulls() { | ||
| // test values that can be cast to/from all integer types |
There was a problem hiding this comment.
It is a bit confusing that this description is the same as the one below, so might make sense to actually emphasize this is only testing null values for integer types.
There was a problem hiding this comment.
It is a bit confusing that this description is the same as the one below, so might make sense to actually emphasize this is only testing null values for integer types.
agree, from the test case i got your mean.
You want to test that the from value is Null.
There was a problem hiding this comment.
This is a copy/pasted comment 🤦 -- I will fix it in the next PR. Sorry for the confusion and nice spot
|
|
||
| #[test] | ||
| fn test_try_cast_to_type_int_out_of_range() { | ||
| let max_i64 = ScalarValue::Int64(Some(i64::MAX)); |
There was a problem hiding this comment.
@alamb if I didn't miss any, I guess it makes sense to also include some signed -> unsigned conversions here as well. Seems like unsigned types are not supported yet. Can be ignored.
There was a problem hiding this comment.
If want to cover this case maybe adding the test then notate it with #[should_panic] is a choice. Then we won't miss this coverage when "signed <-> unsigned" conversions get implemented in the future.
There was a problem hiding this comment.
maybe we should support unsigned data type in the rule
There was a problem hiding this comment.
yeah- as @liukun4515 mentions I have a PR queued up to support the unsigned case :)
| } | ||
|
|
||
| #[test] | ||
| fn test_try_decimal_cast_out_of_range() { |
There was a problem hiding this comment.
maybe I have tested unsupported cases in test_not_unwrap_cast_with_decimal_comparison, but it's good to simplify the test and add more test.
There was a problem hiding this comment.
I agree you have covered most (if not all ) of these cases already in test_not_unwrap_cast_with_decimal_comparison
The reason I want to add redundant coverage was while write tests for unwrapping timestamp casts I also wanted to verify they were consistent with the cast kernel which I couldn't figure out how to do easily.
Also, the tests as written tested both IN list cast removal as well as comparison cast removal, but the code that handles those structures is the same. Thus adding tests for unwrapping timestamps for both of those forms seemed like it didn't add extra coverage and was overly verbose and got even worse as I contemplated testing all the other types.
So since the only code that is different for different data types was try_cast_to_type I figured I would write a test for that function to get enough coverage
| // input array | ||
| let literal_array = literal.to_array_of_size(1); | ||
| let expected_array = expected_value.to_array_of_size(1); | ||
| let cast_array = cast_with_options( |
There was a problem hiding this comment.
I am glad I had it too 😅 as I rediscovered that the arrow cast kernel doesn't support decimal <--> unsigned and so my initial implementation to support unsigned would have been inconsistent.
|
Thank you all for the comments |
|
Benchmark runs are scheduled for baseline = 9692fb0 and contender = 8f67d05. 8f67d05 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?
re #3938 and #3702
(I am breaking up the work in several PRs to make it easier to review)
Rationale for this change
What changes are included in this PR?
unwrap_cast_in_comparisonAre these changes tested?
All tests!
Are there any user-facing changes?
No