Add additional testing for `unwrap_cast_in_comparison` by alamb · Pull Request #4147 · apache/datafusion

alamb · 2022-11-08T18:34:43Z

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

I want to be able to better test adding new data type support to unwrap cast rule
I want to make sure the coerce rule has the same behavior as the arrow cast kernels

What changes are included in this PR?

add new tests for unwrap_cast_in_comparison

Are these changes tested?

All tests!

Are there any user-facing changes?

No

alamb · 2022-11-08T18:36:01Z

datafusion/optimizer/src/unwrap_cast_in_comparison.rs

+                // 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);
+                }
+            }


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)

This is super cool, double verification 💯

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

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 :)

isidentical · 2022-11-08T23:49:44Z

datafusion/optimizer/src/unwrap_cast_in_comparison.rs

+
+    #[test]
+    fn test_try_cast_to_type_nulls() {
+        // test values that can be cast to/from all integer types


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.

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.

This is a copy/pasted comment 🤦 -- I will fix it in the next PR. Sorry for the confusion and nice spot

fixed in #4148

isidentical · 2022-11-08T23:53:38Z

datafusion/optimizer/src/unwrap_cast_in_comparison.rs

+
+    #[test]
+    fn test_try_cast_to_type_int_out_of_range() {
+        let max_i64 = ScalarValue::Int64(Some(i64::MAX));


~~@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.

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.

maybe we should support unsigned data type in the rule

yeah- as @liukun4515 mentions I have a PR queued up to support the unsigned case :)

waynexia

Like it! This is very concrete 👍

liukun4515 · 2022-11-09T11:56:02Z

datafusion/optimizer/src/unwrap_cast_in_comparison.rs

+    }
+
+    #[test]
+    fn test_try_decimal_cast_out_of_range() {


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.

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

liukun4515 · 2022-11-09T11:57:50Z

datafusion/optimizer/src/unwrap_cast_in_comparison.rs

+                // 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(


great test for the cast

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.

alamb · 2022-11-09T18:28:55Z

Thank you all for the comments

ursabot · 2022-11-09T18:32:48Z

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.
Conbench compare runs links:
[Skipped ⚠️ Benchmarking of arrow-datafusion-commits is not supported on ec2-t3-xlarge-us-east-2] ec2-t3-xlarge-us-east-2
[Skipped ⚠️ Benchmarking of arrow-datafusion-commits is not supported on test-mac-arm] test-mac-arm
[Skipped ⚠️ Benchmarking of arrow-datafusion-commits is not supported on ursa-i9-9960x] ursa-i9-9960x
[Skipped ⚠️ Benchmarking of arrow-datafusion-commits is not supported on ursa-thinkcentre-m75q] ursa-thinkcentre-m75q
Buildkite builds:
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

Add additional testing for unwrap_cast_in_comparison

0a3bf55

github-actions bot added the optimizer Optimizer rules label Nov 8, 2022

alamb commented Nov 8, 2022

View reviewed changes

alamb requested a review from liukun4515 November 8, 2022 18:38

This was referenced Nov 8, 2022

Add support for DataType::Timestamp casts in unwrap_cast_in_comparison optimizer pass #4148

Merged

Support unsigned integers in unwrap_cast_in_comparison Optimizer rule #4149

Merged

isidentical reviewed Nov 8, 2022

View reviewed changes

waynexia approved these changes Nov 9, 2022

View reviewed changes

liukun4515 reviewed Nov 9, 2022

View reviewed changes

alamb merged commit 8f67d05 into apache:master Nov 9, 2022

alamb deleted the alamb/unwrap_casts_tests branch November 9, 2022 18:29

Comments

Conversation

alamb commented Nov 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

isidentical Nov 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

waynexia left a comment

Choose a reason for hiding this comment

Uh oh!

liukun4515 Nov 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alamb commented Nov 9, 2022

Uh oh!

ursabot commented Nov 9, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

alamb commented Nov 8, 2022 •

edited

Loading

isidentical Nov 8, 2022 •

edited

Loading

liukun4515 Nov 9, 2022 •

edited

Loading