You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2022/11/08 18:34:46 UTC
[GitHub] [arrow-datafusion] alamb opened a new pull request, #4147: Add additional testing for `unwrap_cast_in_comparison`
alamb opened a new pull request, #4147:
URL: https://github.com/apache/arrow-datafusion/pull/4147
# Which issue does this PR close?
re https://github.com/apache/arrow-datafusion/issues/3938 and https://github.com/apache/arrow-datafusion/issues/3702
# Rationale for this change
1. I want to be able to better test adding new data type support to unwrap cast rule
2. I want to make sure the coerce rule has the same behavior as the arrow cast kernels
# What changes are included in this PR?
1. add new tests for `unwrap_cast_in_comparison`
# Are these changes tested?
All tests!
# Are there any user-facing changes?
No
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] liukun4515 commented on a diff in pull request #4147: Add additional testing for `unwrap_cast_in_comparison`
Posted by GitBox <gi...@apache.org>.
liukun4515 commented on code in PR #4147:
URL: https://github.com/apache/arrow-datafusion/pull/4147#discussion_r1017858525
##########
datafusion/optimizer/src/unwrap_cast_in_comparison.rs:
##########
@@ -653,4 +655,196 @@ mod tests {
fn null_decimal(precision: u8, scale: u8) -> Expr {
lit(ScalarValue::Decimal128(None, precision, scale))
}
+
+ #[test]
+ fn test_try_cast_to_type_nulls() {
+ // test values that can be cast to/from all integer types
+ let scalars = vec![
+ ScalarValue::Int8(None),
+ ScalarValue::Int16(None),
+ ScalarValue::Int32(None),
+ ScalarValue::Int64(None),
+ ScalarValue::Decimal128(None, 3, 0),
+ ScalarValue::Decimal128(None, 8, 2),
+ ];
+
+ for s1 in &scalars {
+ for s2 in &scalars {
+ expect_cast(
+ s1.clone(),
+ s2.get_datatype(),
+ ExpectedCast::Value(s2.clone()),
+ );
+ }
+ }
+ }
+
+ #[test]
+ fn test_try_cast_to_type_int_in_range() {
+ // test values that can be cast to/from all integer types
+ let scalars = vec![
+ ScalarValue::Int8(Some(123)),
+ ScalarValue::Int16(Some(123)),
+ ScalarValue::Int32(Some(123)),
+ ScalarValue::Int64(Some(123)),
+ ScalarValue::Decimal128(Some(123), 3, 0),
+ ScalarValue::Decimal128(Some(12300), 8, 2),
+ ];
+
+ for s1 in &scalars {
+ for s2 in &scalars {
+ expect_cast(
+ s1.clone(),
+ s2.get_datatype(),
+ ExpectedCast::Value(s2.clone()),
+ );
+ }
+ }
+ }
+
+ #[test]
+ fn test_try_cast_to_type_int_out_of_range() {
+ let max_i64 = ScalarValue::Int64(Some(i64::MAX));
Review Comment:
https://github.com/apache/arrow-datafusion/pull/4149
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] isidentical commented on a diff in pull request #4147: Add additional testing for `unwrap_cast_in_comparison`
Posted by GitBox <gi...@apache.org>.
isidentical commented on code in PR #4147:
URL: https://github.com/apache/arrow-datafusion/pull/4147#discussion_r1017235828
##########
datafusion/optimizer/src/unwrap_cast_in_comparison.rs:
##########
@@ -653,4 +655,196 @@ mod tests {
fn null_decimal(precision: u8, scale: u8) -> Expr {
lit(ScalarValue::Decimal128(None, precision, scale))
}
+
+ #[test]
+ fn test_try_cast_to_type_nulls() {
+ // test values that can be cast to/from all integer types
+ let scalars = vec![
+ ScalarValue::Int8(None),
+ ScalarValue::Int16(None),
+ ScalarValue::Int32(None),
+ ScalarValue::Int64(None),
+ ScalarValue::Decimal128(None, 3, 0),
+ ScalarValue::Decimal128(None, 8, 2),
+ ];
+
+ for s1 in &scalars {
+ for s2 in &scalars {
+ expect_cast(
+ s1.clone(),
+ s2.get_datatype(),
+ ExpectedCast::Value(s2.clone()),
+ );
+ }
+ }
+ }
+
+ #[test]
+ fn test_try_cast_to_type_int_in_range() {
+ // test values that can be cast to/from all integer types
+ let scalars = vec![
+ ScalarValue::Int8(Some(123)),
+ ScalarValue::Int16(Some(123)),
+ ScalarValue::Int32(Some(123)),
+ ScalarValue::Int64(Some(123)),
+ ScalarValue::Decimal128(Some(123), 3, 0),
+ ScalarValue::Decimal128(Some(12300), 8, 2),
+ ];
+
+ for s1 in &scalars {
+ for s2 in &scalars {
+ expect_cast(
+ s1.clone(),
+ s2.get_datatype(),
+ ExpectedCast::Value(s2.clone()),
+ );
+ }
+ }
+ }
+
+ #[test]
+ fn test_try_cast_to_type_int_out_of_range() {
+ let max_i64 = ScalarValue::Int64(Some(i64::MAX));
+ let max_u64 = ScalarValue::UInt64(Some(u64::MAX));
+ expect_cast(max_i64.clone(), DataType::Int8, ExpectedCast::NoValue);
+
+ expect_cast(max_i64.clone(), DataType::Int16, ExpectedCast::NoValue);
+
+ expect_cast(max_i64, DataType::Int32, ExpectedCast::NoValue);
+
+ expect_cast(max_u64, DataType::Int64, ExpectedCast::NoValue);
+
+ // decimal out of range
+ expect_cast(
+ ScalarValue::Decimal128(Some(99999999999999999999999999999999999900), 38, 0),
+ DataType::Int64,
+ ExpectedCast::NoValue,
+ );
+
+ expect_cast(
+ ScalarValue::Decimal128(Some(-9999999999999999999999999999999999), 37, 1),
+ DataType::Int64,
+ ExpectedCast::NoValue,
+ );
+ }
+
+ #[test]
+ fn test_try_decimal_cast_in_range() {
+ expect_cast(
+ ScalarValue::Decimal128(Some(12300), 5, 2),
+ DataType::Decimal128(3, 0),
+ ExpectedCast::Value(ScalarValue::Decimal128(Some(123), 3, 0)),
+ );
+
+ expect_cast(
+ ScalarValue::Decimal128(Some(12300), 5, 2),
+ DataType::Decimal128(8, 0),
+ ExpectedCast::Value(ScalarValue::Decimal128(Some(123), 8, 0)),
+ );
+
+ expect_cast(
+ ScalarValue::Decimal128(Some(12300), 5, 2),
+ DataType::Decimal128(8, 5),
+ ExpectedCast::Value(ScalarValue::Decimal128(Some(12300000), 8, 5)),
+ );
+ }
+
+ #[test]
+ fn test_try_decimal_cast_out_of_range() {
+ // decimal would lose precision
+ expect_cast(
+ ScalarValue::Decimal128(Some(12345), 5, 2),
+ DataType::Decimal128(3, 0),
+ ExpectedCast::NoValue,
+ );
+
+ // decimal would lose precision
+ expect_cast(
+ ScalarValue::Decimal128(Some(12300), 5, 2),
+ DataType::Decimal128(2, 0),
+ ExpectedCast::NoValue,
+ );
+ }
+
+ #[test]
+ fn test_try_cast_to_type_unsupported() {
+ // int64 to list
+ expect_cast(
+ ScalarValue::Int64(Some(12345)),
+ DataType::List(Box::new(Field::new("f", DataType::Int32, true))),
+ ExpectedCast::NoValue,
+ );
+ }
+
+ #[derive(Debug, Clone)]
+ enum ExpectedCast {
+ /// test successfully cast value and it is as specified
+ Value(ScalarValue),
+ /// test returned OK, but could not cast the value
+ NoValue,
+ }
+
+ /// Runs try_cast_literal_to_type with the specified inputs and
+ /// ensure it computes the expected output, and ensures the
+ /// casting is consistent with the Arrow kernels
+ fn expect_cast(
+ literal: ScalarValue,
+ target_type: DataType,
+ expected_result: ExpectedCast,
+ ) {
+ let actual_result = try_cast_literal_to_type(&literal, &target_type);
+
+ println!("expect_cast: ");
+ println!(" {:?} --> {:?}", literal, target_type);
+ println!(" expected_result: {:?}", expected_result);
+ println!(" actual_result: {:?}", actual_result);
+
+ match expected_result {
+ ExpectedCast::Value(expected_value) => {
+ let actual_value = actual_result
+ .expect("Expected success but got error")
+ .expect("Expected cast value but got None");
+
+ assert_eq!(actual_value, expected_value);
+
+ // 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);
+ }
+ }
Review Comment:
This is super cool, double verification 💯
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] isidentical commented on a diff in pull request #4147: Add additional testing for `unwrap_cast_in_comparison`
Posted by GitBox <gi...@apache.org>.
isidentical commented on code in PR #4147:
URL: https://github.com/apache/arrow-datafusion/pull/4147#discussion_r1017234261
##########
datafusion/optimizer/src/unwrap_cast_in_comparison.rs:
##########
@@ -653,4 +655,196 @@ mod tests {
fn null_decimal(precision: u8, scale: u8) -> Expr {
lit(ScalarValue::Decimal128(None, precision, scale))
}
+
+ #[test]
+ fn test_try_cast_to_type_nulls() {
+ // test values that can be cast to/from all integer types
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on pull request #4147: Add additional testing for `unwrap_cast_in_comparison`
Posted by GitBox <gi...@apache.org>.
alamb commented on PR #4147:
URL: https://github.com/apache/arrow-datafusion/pull/4147#issuecomment-1309192060
Thank you all for the comments
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] liukun4515 commented on a diff in pull request #4147: Add additional testing for `unwrap_cast_in_comparison`
Posted by GitBox <gi...@apache.org>.
liukun4515 commented on code in PR #4147:
URL: https://github.com/apache/arrow-datafusion/pull/4147#discussion_r1017842586
##########
datafusion/optimizer/src/unwrap_cast_in_comparison.rs:
##########
@@ -653,4 +655,196 @@ mod tests {
fn null_decimal(precision: u8, scale: u8) -> Expr {
lit(ScalarValue::Decimal128(None, precision, scale))
}
+
+ #[test]
+ fn test_try_cast_to_type_nulls() {
+ // test values that can be cast to/from all integer types
+ let scalars = vec![
+ ScalarValue::Int8(None),
+ ScalarValue::Int16(None),
+ ScalarValue::Int32(None),
+ ScalarValue::Int64(None),
+ ScalarValue::Decimal128(None, 3, 0),
+ ScalarValue::Decimal128(None, 8, 2),
+ ];
+
+ for s1 in &scalars {
+ for s2 in &scalars {
+ expect_cast(
+ s1.clone(),
+ s2.get_datatype(),
+ ExpectedCast::Value(s2.clone()),
+ );
+ }
+ }
+ }
+
+ #[test]
+ fn test_try_cast_to_type_int_in_range() {
+ // test values that can be cast to/from all integer types
+ let scalars = vec![
+ ScalarValue::Int8(Some(123)),
+ ScalarValue::Int16(Some(123)),
+ ScalarValue::Int32(Some(123)),
+ ScalarValue::Int64(Some(123)),
+ ScalarValue::Decimal128(Some(123), 3, 0),
+ ScalarValue::Decimal128(Some(12300), 8, 2),
+ ];
+
+ for s1 in &scalars {
+ for s2 in &scalars {
+ expect_cast(
+ s1.clone(),
+ s2.get_datatype(),
+ ExpectedCast::Value(s2.clone()),
+ );
+ }
+ }
+ }
+
+ #[test]
+ fn test_try_cast_to_type_int_out_of_range() {
+ let max_i64 = ScalarValue::Int64(Some(i64::MAX));
+ let max_u64 = ScalarValue::UInt64(Some(u64::MAX));
+ expect_cast(max_i64.clone(), DataType::Int8, ExpectedCast::NoValue);
+
+ expect_cast(max_i64.clone(), DataType::Int16, ExpectedCast::NoValue);
+
+ expect_cast(max_i64, DataType::Int32, ExpectedCast::NoValue);
+
+ expect_cast(max_u64, DataType::Int64, ExpectedCast::NoValue);
+
+ // decimal out of range
+ expect_cast(
+ ScalarValue::Decimal128(Some(99999999999999999999999999999999999900), 38, 0),
+ DataType::Int64,
+ ExpectedCast::NoValue,
+ );
+
+ expect_cast(
+ ScalarValue::Decimal128(Some(-9999999999999999999999999999999999), 37, 1),
+ DataType::Int64,
+ ExpectedCast::NoValue,
+ );
+ }
+
+ #[test]
+ fn test_try_decimal_cast_in_range() {
+ expect_cast(
+ ScalarValue::Decimal128(Some(12300), 5, 2),
+ DataType::Decimal128(3, 0),
+ ExpectedCast::Value(ScalarValue::Decimal128(Some(123), 3, 0)),
+ );
+
+ expect_cast(
+ ScalarValue::Decimal128(Some(12300), 5, 2),
+ DataType::Decimal128(8, 0),
+ ExpectedCast::Value(ScalarValue::Decimal128(Some(123), 8, 0)),
+ );
+
+ expect_cast(
+ ScalarValue::Decimal128(Some(12300), 5, 2),
+ DataType::Decimal128(8, 5),
+ ExpectedCast::Value(ScalarValue::Decimal128(Some(12300000), 8, 5)),
+ );
+ }
+
+ #[test]
+ fn test_try_decimal_cast_out_of_range() {
+ // decimal would lose precision
+ expect_cast(
+ ScalarValue::Decimal128(Some(12345), 5, 2),
+ DataType::Decimal128(3, 0),
+ ExpectedCast::NoValue,
+ );
+
+ // decimal would lose precision
+ expect_cast(
+ ScalarValue::Decimal128(Some(12300), 5, 2),
+ DataType::Decimal128(2, 0),
+ ExpectedCast::NoValue,
+ );
+ }
+
+ #[test]
+ fn test_try_cast_to_type_unsupported() {
+ // int64 to list
+ expect_cast(
+ ScalarValue::Int64(Some(12345)),
+ DataType::List(Box::new(Field::new("f", DataType::Int32, true))),
+ ExpectedCast::NoValue,
+ );
+ }
+
+ #[derive(Debug, Clone)]
+ enum ExpectedCast {
+ /// test successfully cast value and it is as specified
+ Value(ScalarValue),
+ /// test returned OK, but could not cast the value
+ NoValue,
+ }
+
+ /// Runs try_cast_literal_to_type with the specified inputs and
+ /// ensure it computes the expected output, and ensures the
+ /// casting is consistent with the Arrow kernels
+ fn expect_cast(
+ literal: ScalarValue,
+ target_type: DataType,
+ expected_result: ExpectedCast,
+ ) {
+ let actual_result = try_cast_literal_to_type(&literal, &target_type);
+
+ println!("expect_cast: ");
+ println!(" {:?} --> {:?}", literal, target_type);
+ println!(" expected_result: {:?}", expected_result);
+ println!(" actual_result: {:?}", actual_result);
+
+ match expected_result {
+ ExpectedCast::Value(expected_value) => {
+ let actual_value = actual_result
+ .expect("Expected success but got error")
+ .expect("Expected cast value but got None");
+
+ assert_eq!(actual_value, expected_value);
+
+ // 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(
Review Comment:
great test for the `cast`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] isidentical commented on a diff in pull request #4147: Add additional testing for `unwrap_cast_in_comparison`
Posted by GitBox <gi...@apache.org>.
isidentical commented on code in PR #4147:
URL: https://github.com/apache/arrow-datafusion/pull/4147#discussion_r1017236065
##########
datafusion/optimizer/src/unwrap_cast_in_comparison.rs:
##########
@@ -653,4 +655,196 @@ mod tests {
fn null_decimal(precision: u8, scale: u8) -> Expr {
lit(ScalarValue::Decimal128(None, precision, scale))
}
+
+ #[test]
+ fn test_try_cast_to_type_nulls() {
+ // test values that can be cast to/from all integer types
+ let scalars = vec![
+ ScalarValue::Int8(None),
+ ScalarValue::Int16(None),
+ ScalarValue::Int32(None),
+ ScalarValue::Int64(None),
+ ScalarValue::Decimal128(None, 3, 0),
+ ScalarValue::Decimal128(None, 8, 2),
+ ];
+
+ for s1 in &scalars {
+ for s2 in &scalars {
+ expect_cast(
+ s1.clone(),
+ s2.get_datatype(),
+ ExpectedCast::Value(s2.clone()),
+ );
+ }
+ }
+ }
+
+ #[test]
+ fn test_try_cast_to_type_int_in_range() {
+ // test values that can be cast to/from all integer types
+ let scalars = vec![
+ ScalarValue::Int8(Some(123)),
+ ScalarValue::Int16(Some(123)),
+ ScalarValue::Int32(Some(123)),
+ ScalarValue::Int64(Some(123)),
+ ScalarValue::Decimal128(Some(123), 3, 0),
+ ScalarValue::Decimal128(Some(12300), 8, 2),
+ ];
+
+ for s1 in &scalars {
+ for s2 in &scalars {
+ expect_cast(
+ s1.clone(),
+ s2.get_datatype(),
+ ExpectedCast::Value(s2.clone()),
+ );
+ }
+ }
+ }
+
+ #[test]
+ fn test_try_cast_to_type_int_out_of_range() {
+ let max_i64 = ScalarValue::Int64(Some(i64::MAX));
Review Comment:
@alamb if I didn't miss any, I guess it makes sense to also include some signed -> unsigned conversions here as well.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] ursabot commented on pull request #4147: Add additional testing for `unwrap_cast_in_comparison`
Posted by GitBox <gi...@apache.org>.
ursabot commented on PR #4147:
URL: https://github.com/apache/arrow-datafusion/pull/4147#issuecomment-1309196076
Benchmark runs are scheduled for baseline = 9692fb01604cb2c25a05e49d6a4e6c30c1d32c75 and contender = 8f67d055be23493cc449b7b75c7e6beac6b85aef. 8f67d055be23493cc449b7b75c7e6beac6b85aef is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Skipped :warning: Benchmarking of arrow-datafusion-commits is not supported on ec2-t3-xlarge-us-east-2] [ec2-t3-xlarge-us-east-2](https://conbench.ursa.dev/compare/runs/a4b423553a314a6a872b406d191c5d25...48e80e1b849e4c7fbddc33c0682c5c7f/)
[Skipped :warning: Benchmarking of arrow-datafusion-commits is not supported on test-mac-arm] [test-mac-arm](https://conbench.ursa.dev/compare/runs/afb1582a7b304a0b9c982067bffcca4c...2a19171c47b645d9afcee3e7e930c8a2/)
[Skipped :warning: Benchmarking of arrow-datafusion-commits is not supported on ursa-i9-9960x] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/8bdcf4cd9e934f68bb514991e01f7300...29375de016aa435bb2b1c830ec706ae6/)
[Skipped :warning: Benchmarking of arrow-datafusion-commits is not supported on ursa-thinkcentre-m75q] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/7b1f94fe0ca54a7894cb11fd9775ddf2...45eb62d87a544c96872057d9144a01a0/)
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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb merged pull request #4147: Add additional testing for `unwrap_cast_in_comparison`
Posted by GitBox <gi...@apache.org>.
alamb merged PR #4147:
URL: https://github.com/apache/arrow-datafusion/pull/4147
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] isidentical commented on a diff in pull request #4147: Add additional testing for `unwrap_cast_in_comparison`
Posted by GitBox <gi...@apache.org>.
isidentical commented on code in PR #4147:
URL: https://github.com/apache/arrow-datafusion/pull/4147#discussion_r1017236065
##########
datafusion/optimizer/src/unwrap_cast_in_comparison.rs:
##########
@@ -653,4 +655,196 @@ mod tests {
fn null_decimal(precision: u8, scale: u8) -> Expr {
lit(ScalarValue::Decimal128(None, precision, scale))
}
+
+ #[test]
+ fn test_try_cast_to_type_nulls() {
+ // test values that can be cast to/from all integer types
+ let scalars = vec![
+ ScalarValue::Int8(None),
+ ScalarValue::Int16(None),
+ ScalarValue::Int32(None),
+ ScalarValue::Int64(None),
+ ScalarValue::Decimal128(None, 3, 0),
+ ScalarValue::Decimal128(None, 8, 2),
+ ];
+
+ for s1 in &scalars {
+ for s2 in &scalars {
+ expect_cast(
+ s1.clone(),
+ s2.get_datatype(),
+ ExpectedCast::Value(s2.clone()),
+ );
+ }
+ }
+ }
+
+ #[test]
+ fn test_try_cast_to_type_int_in_range() {
+ // test values that can be cast to/from all integer types
+ let scalars = vec![
+ ScalarValue::Int8(Some(123)),
+ ScalarValue::Int16(Some(123)),
+ ScalarValue::Int32(Some(123)),
+ ScalarValue::Int64(Some(123)),
+ ScalarValue::Decimal128(Some(123), 3, 0),
+ ScalarValue::Decimal128(Some(12300), 8, 2),
+ ];
+
+ for s1 in &scalars {
+ for s2 in &scalars {
+ expect_cast(
+ s1.clone(),
+ s2.get_datatype(),
+ ExpectedCast::Value(s2.clone()),
+ );
+ }
+ }
+ }
+
+ #[test]
+ fn test_try_cast_to_type_int_out_of_range() {
+ let max_i64 = ScalarValue::Int64(Some(i64::MAX));
Review 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.
##########
datafusion/optimizer/src/unwrap_cast_in_comparison.rs:
##########
@@ -653,4 +655,196 @@ mod tests {
fn null_decimal(precision: u8, scale: u8) -> Expr {
lit(ScalarValue::Decimal128(None, precision, scale))
}
+
+ #[test]
+ fn test_try_cast_to_type_nulls() {
+ // test values that can be cast to/from all integer types
+ let scalars = vec![
+ ScalarValue::Int8(None),
+ ScalarValue::Int16(None),
+ ScalarValue::Int32(None),
+ ScalarValue::Int64(None),
+ ScalarValue::Decimal128(None, 3, 0),
+ ScalarValue::Decimal128(None, 8, 2),
+ ];
+
+ for s1 in &scalars {
+ for s2 in &scalars {
+ expect_cast(
+ s1.clone(),
+ s2.get_datatype(),
+ ExpectedCast::Value(s2.clone()),
+ );
+ }
+ }
+ }
+
+ #[test]
+ fn test_try_cast_to_type_int_in_range() {
+ // test values that can be cast to/from all integer types
+ let scalars = vec![
+ ScalarValue::Int8(Some(123)),
+ ScalarValue::Int16(Some(123)),
+ ScalarValue::Int32(Some(123)),
+ ScalarValue::Int64(Some(123)),
+ ScalarValue::Decimal128(Some(123), 3, 0),
+ ScalarValue::Decimal128(Some(12300), 8, 2),
+ ];
+
+ for s1 in &scalars {
+ for s2 in &scalars {
+ expect_cast(
+ s1.clone(),
+ s2.get_datatype(),
+ ExpectedCast::Value(s2.clone()),
+ );
+ }
+ }
+ }
+
+ #[test]
+ fn test_try_cast_to_type_int_out_of_range() {
+ let max_i64 = ScalarValue::Int64(Some(i64::MAX));
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] liukun4515 commented on a diff in pull request #4147: Add additional testing for `unwrap_cast_in_comparison`
Posted by GitBox <gi...@apache.org>.
liukun4515 commented on code in PR #4147:
URL: https://github.com/apache/arrow-datafusion/pull/4147#discussion_r1017856918
##########
datafusion/optimizer/src/unwrap_cast_in_comparison.rs:
##########
@@ -653,4 +655,196 @@ mod tests {
fn null_decimal(precision: u8, scale: u8) -> Expr {
lit(ScalarValue::Decimal128(None, precision, scale))
}
+
+ #[test]
+ fn test_try_cast_to_type_nulls() {
+ // test values that can be cast to/from all integer types
+ let scalars = vec![
+ ScalarValue::Int8(None),
+ ScalarValue::Int16(None),
+ ScalarValue::Int32(None),
+ ScalarValue::Int64(None),
+ ScalarValue::Decimal128(None, 3, 0),
+ ScalarValue::Decimal128(None, 8, 2),
+ ];
+
+ for s1 in &scalars {
+ for s2 in &scalars {
+ expect_cast(
+ s1.clone(),
+ s2.get_datatype(),
+ ExpectedCast::Value(s2.clone()),
+ );
+ }
+ }
+ }
+
+ #[test]
+ fn test_try_cast_to_type_int_in_range() {
+ // test values that can be cast to/from all integer types
+ let scalars = vec![
+ ScalarValue::Int8(Some(123)),
+ ScalarValue::Int16(Some(123)),
+ ScalarValue::Int32(Some(123)),
+ ScalarValue::Int64(Some(123)),
+ ScalarValue::Decimal128(Some(123), 3, 0),
+ ScalarValue::Decimal128(Some(12300), 8, 2),
+ ];
+
+ for s1 in &scalars {
+ for s2 in &scalars {
+ expect_cast(
+ s1.clone(),
+ s2.get_datatype(),
+ ExpectedCast::Value(s2.clone()),
+ );
+ }
+ }
+ }
+
+ #[test]
+ fn test_try_cast_to_type_int_out_of_range() {
+ let max_i64 = ScalarValue::Int64(Some(i64::MAX));
Review Comment:
maybe we should support unsigned data type in the rule
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on a diff in pull request #4147: Add additional testing for `unwrap_cast_in_comparison`
Posted by GitBox <gi...@apache.org>.
alamb commented on code in PR #4147:
URL: https://github.com/apache/arrow-datafusion/pull/4147#discussion_r1018281512
##########
datafusion/optimizer/src/unwrap_cast_in_comparison.rs:
##########
@@ -653,4 +655,196 @@ mod tests {
fn null_decimal(precision: u8, scale: u8) -> Expr {
lit(ScalarValue::Decimal128(None, precision, scale))
}
+
+ #[test]
+ fn test_try_cast_to_type_nulls() {
+ // test values that can be cast to/from all integer types
+ let scalars = vec![
+ ScalarValue::Int8(None),
+ ScalarValue::Int16(None),
+ ScalarValue::Int32(None),
+ ScalarValue::Int64(None),
+ ScalarValue::Decimal128(None, 3, 0),
+ ScalarValue::Decimal128(None, 8, 2),
+ ];
+
+ for s1 in &scalars {
+ for s2 in &scalars {
+ expect_cast(
+ s1.clone(),
+ s2.get_datatype(),
+ ExpectedCast::Value(s2.clone()),
+ );
+ }
+ }
+ }
+
+ #[test]
+ fn test_try_cast_to_type_int_in_range() {
+ // test values that can be cast to/from all integer types
+ let scalars = vec![
+ ScalarValue::Int8(Some(123)),
+ ScalarValue::Int16(Some(123)),
+ ScalarValue::Int32(Some(123)),
+ ScalarValue::Int64(Some(123)),
+ ScalarValue::Decimal128(Some(123), 3, 0),
+ ScalarValue::Decimal128(Some(12300), 8, 2),
+ ];
+
+ for s1 in &scalars {
+ for s2 in &scalars {
+ expect_cast(
+ s1.clone(),
+ s2.get_datatype(),
+ ExpectedCast::Value(s2.clone()),
+ );
+ }
+ }
+ }
+
+ #[test]
+ fn test_try_cast_to_type_int_out_of_range() {
+ let max_i64 = ScalarValue::Int64(Some(i64::MAX));
+ let max_u64 = ScalarValue::UInt64(Some(u64::MAX));
+ expect_cast(max_i64.clone(), DataType::Int8, ExpectedCast::NoValue);
+
+ expect_cast(max_i64.clone(), DataType::Int16, ExpectedCast::NoValue);
+
+ expect_cast(max_i64, DataType::Int32, ExpectedCast::NoValue);
+
+ expect_cast(max_u64, DataType::Int64, ExpectedCast::NoValue);
+
+ // decimal out of range
+ expect_cast(
+ ScalarValue::Decimal128(Some(99999999999999999999999999999999999900), 38, 0),
+ DataType::Int64,
+ ExpectedCast::NoValue,
+ );
+
+ expect_cast(
+ ScalarValue::Decimal128(Some(-9999999999999999999999999999999999), 37, 1),
+ DataType::Int64,
+ ExpectedCast::NoValue,
+ );
+ }
+
+ #[test]
+ fn test_try_decimal_cast_in_range() {
+ expect_cast(
+ ScalarValue::Decimal128(Some(12300), 5, 2),
+ DataType::Decimal128(3, 0),
+ ExpectedCast::Value(ScalarValue::Decimal128(Some(123), 3, 0)),
+ );
+
+ expect_cast(
+ ScalarValue::Decimal128(Some(12300), 5, 2),
+ DataType::Decimal128(8, 0),
+ ExpectedCast::Value(ScalarValue::Decimal128(Some(123), 8, 0)),
+ );
+
+ expect_cast(
+ ScalarValue::Decimal128(Some(12300), 5, 2),
+ DataType::Decimal128(8, 5),
+ ExpectedCast::Value(ScalarValue::Decimal128(Some(12300000), 8, 5)),
+ );
+ }
+
+ #[test]
+ fn test_try_decimal_cast_out_of_range() {
+ // decimal would lose precision
+ expect_cast(
+ ScalarValue::Decimal128(Some(12345), 5, 2),
+ DataType::Decimal128(3, 0),
+ ExpectedCast::NoValue,
+ );
+
+ // decimal would lose precision
+ expect_cast(
+ ScalarValue::Decimal128(Some(12300), 5, 2),
+ DataType::Decimal128(2, 0),
+ ExpectedCast::NoValue,
+ );
+ }
+
+ #[test]
+ fn test_try_cast_to_type_unsupported() {
+ // int64 to list
+ expect_cast(
+ ScalarValue::Int64(Some(12345)),
+ DataType::List(Box::new(Field::new("f", DataType::Int32, true))),
+ ExpectedCast::NoValue,
+ );
+ }
+
+ #[derive(Debug, Clone)]
+ enum ExpectedCast {
+ /// test successfully cast value and it is as specified
+ Value(ScalarValue),
+ /// test returned OK, but could not cast the value
+ NoValue,
+ }
+
+ /// Runs try_cast_literal_to_type with the specified inputs and
+ /// ensure it computes the expected output, and ensures the
+ /// casting is consistent with the Arrow kernels
+ fn expect_cast(
+ literal: ScalarValue,
+ target_type: DataType,
+ expected_result: ExpectedCast,
+ ) {
+ let actual_result = try_cast_literal_to_type(&literal, &target_type);
+
+ println!("expect_cast: ");
+ println!(" {:?} --> {:?}", literal, target_type);
+ println!(" expected_result: {:?}", expected_result);
+ println!(" actual_result: {:?}", actual_result);
+
+ match expected_result {
+ ExpectedCast::Value(expected_value) => {
+ let actual_value = actual_result
+ .expect("Expected success but got error")
+ .expect("Expected cast value but got None");
+
+ assert_eq!(actual_value, expected_value);
+
+ // 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);
+ }
+ }
Review 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 :)
##########
datafusion/optimizer/src/unwrap_cast_in_comparison.rs:
##########
@@ -653,4 +655,196 @@ mod tests {
fn null_decimal(precision: u8, scale: u8) -> Expr {
lit(ScalarValue::Decimal128(None, precision, scale))
}
+
+ #[test]
+ fn test_try_cast_to_type_nulls() {
+ // test values that can be cast to/from all integer types
+ let scalars = vec![
+ ScalarValue::Int8(None),
+ ScalarValue::Int16(None),
+ ScalarValue::Int32(None),
+ ScalarValue::Int64(None),
+ ScalarValue::Decimal128(None, 3, 0),
+ ScalarValue::Decimal128(None, 8, 2),
+ ];
+
+ for s1 in &scalars {
+ for s2 in &scalars {
+ expect_cast(
+ s1.clone(),
+ s2.get_datatype(),
+ ExpectedCast::Value(s2.clone()),
+ );
+ }
+ }
+ }
+
+ #[test]
+ fn test_try_cast_to_type_int_in_range() {
+ // test values that can be cast to/from all integer types
+ let scalars = vec![
+ ScalarValue::Int8(Some(123)),
+ ScalarValue::Int16(Some(123)),
+ ScalarValue::Int32(Some(123)),
+ ScalarValue::Int64(Some(123)),
+ ScalarValue::Decimal128(Some(123), 3, 0),
+ ScalarValue::Decimal128(Some(12300), 8, 2),
+ ];
+
+ for s1 in &scalars {
+ for s2 in &scalars {
+ expect_cast(
+ s1.clone(),
+ s2.get_datatype(),
+ ExpectedCast::Value(s2.clone()),
+ );
+ }
+ }
+ }
+
+ #[test]
+ fn test_try_cast_to_type_int_out_of_range() {
+ let max_i64 = ScalarValue::Int64(Some(i64::MAX));
+ let max_u64 = ScalarValue::UInt64(Some(u64::MAX));
+ expect_cast(max_i64.clone(), DataType::Int8, ExpectedCast::NoValue);
+
+ expect_cast(max_i64.clone(), DataType::Int16, ExpectedCast::NoValue);
+
+ expect_cast(max_i64, DataType::Int32, ExpectedCast::NoValue);
+
+ expect_cast(max_u64, DataType::Int64, ExpectedCast::NoValue);
+
+ // decimal out of range
+ expect_cast(
+ ScalarValue::Decimal128(Some(99999999999999999999999999999999999900), 38, 0),
+ DataType::Int64,
+ ExpectedCast::NoValue,
+ );
+
+ expect_cast(
+ ScalarValue::Decimal128(Some(-9999999999999999999999999999999999), 37, 1),
+ DataType::Int64,
+ ExpectedCast::NoValue,
+ );
+ }
+
+ #[test]
+ fn test_try_decimal_cast_in_range() {
+ expect_cast(
+ ScalarValue::Decimal128(Some(12300), 5, 2),
+ DataType::Decimal128(3, 0),
+ ExpectedCast::Value(ScalarValue::Decimal128(Some(123), 3, 0)),
+ );
+
+ expect_cast(
+ ScalarValue::Decimal128(Some(12300), 5, 2),
+ DataType::Decimal128(8, 0),
+ ExpectedCast::Value(ScalarValue::Decimal128(Some(123), 8, 0)),
+ );
+
+ expect_cast(
+ ScalarValue::Decimal128(Some(12300), 5, 2),
+ DataType::Decimal128(8, 5),
+ ExpectedCast::Value(ScalarValue::Decimal128(Some(12300000), 8, 5)),
+ );
+ }
+
+ #[test]
+ fn test_try_decimal_cast_out_of_range() {
+ // decimal would lose precision
+ expect_cast(
+ ScalarValue::Decimal128(Some(12345), 5, 2),
+ DataType::Decimal128(3, 0),
+ ExpectedCast::NoValue,
+ );
+
+ // decimal would lose precision
+ expect_cast(
+ ScalarValue::Decimal128(Some(12300), 5, 2),
+ DataType::Decimal128(2, 0),
+ ExpectedCast::NoValue,
+ );
+ }
+
+ #[test]
+ fn test_try_cast_to_type_unsupported() {
+ // int64 to list
+ expect_cast(
+ ScalarValue::Int64(Some(12345)),
+ DataType::List(Box::new(Field::new("f", DataType::Int32, true))),
+ ExpectedCast::NoValue,
+ );
+ }
+
+ #[derive(Debug, Clone)]
+ enum ExpectedCast {
+ /// test successfully cast value and it is as specified
+ Value(ScalarValue),
+ /// test returned OK, but could not cast the value
+ NoValue,
+ }
+
+ /// Runs try_cast_literal_to_type with the specified inputs and
+ /// ensure it computes the expected output, and ensures the
+ /// casting is consistent with the Arrow kernels
+ fn expect_cast(
+ literal: ScalarValue,
+ target_type: DataType,
+ expected_result: ExpectedCast,
+ ) {
+ let actual_result = try_cast_literal_to_type(&literal, &target_type);
+
+ println!("expect_cast: ");
+ println!(" {:?} --> {:?}", literal, target_type);
+ println!(" expected_result: {:?}", expected_result);
+ println!(" actual_result: {:?}", actual_result);
+
+ match expected_result {
+ ExpectedCast::Value(expected_value) => {
+ let actual_value = actual_result
+ .expect("Expected success but got error")
+ .expect("Expected cast value but got None");
+
+ assert_eq!(actual_value, expected_value);
+
+ // 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(
Review 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.
##########
datafusion/optimizer/src/unwrap_cast_in_comparison.rs:
##########
@@ -653,4 +655,196 @@ mod tests {
fn null_decimal(precision: u8, scale: u8) -> Expr {
lit(ScalarValue::Decimal128(None, precision, scale))
}
+
+ #[test]
+ fn test_try_cast_to_type_nulls() {
+ // test values that can be cast to/from all integer types
+ let scalars = vec![
+ ScalarValue::Int8(None),
+ ScalarValue::Int16(None),
+ ScalarValue::Int32(None),
+ ScalarValue::Int64(None),
+ ScalarValue::Decimal128(None, 3, 0),
+ ScalarValue::Decimal128(None, 8, 2),
+ ];
+
+ for s1 in &scalars {
+ for s2 in &scalars {
+ expect_cast(
+ s1.clone(),
+ s2.get_datatype(),
+ ExpectedCast::Value(s2.clone()),
+ );
+ }
+ }
+ }
+
+ #[test]
+ fn test_try_cast_to_type_int_in_range() {
+ // test values that can be cast to/from all integer types
+ let scalars = vec![
+ ScalarValue::Int8(Some(123)),
+ ScalarValue::Int16(Some(123)),
+ ScalarValue::Int32(Some(123)),
+ ScalarValue::Int64(Some(123)),
+ ScalarValue::Decimal128(Some(123), 3, 0),
+ ScalarValue::Decimal128(Some(12300), 8, 2),
+ ];
+
+ for s1 in &scalars {
+ for s2 in &scalars {
+ expect_cast(
+ s1.clone(),
+ s2.get_datatype(),
+ ExpectedCast::Value(s2.clone()),
+ );
+ }
+ }
+ }
+
+ #[test]
+ fn test_try_cast_to_type_int_out_of_range() {
+ let max_i64 = ScalarValue::Int64(Some(i64::MAX));
+ let max_u64 = ScalarValue::UInt64(Some(u64::MAX));
+ expect_cast(max_i64.clone(), DataType::Int8, ExpectedCast::NoValue);
+
+ expect_cast(max_i64.clone(), DataType::Int16, ExpectedCast::NoValue);
+
+ expect_cast(max_i64, DataType::Int32, ExpectedCast::NoValue);
+
+ expect_cast(max_u64, DataType::Int64, ExpectedCast::NoValue);
+
+ // decimal out of range
+ expect_cast(
+ ScalarValue::Decimal128(Some(99999999999999999999999999999999999900), 38, 0),
+ DataType::Int64,
+ ExpectedCast::NoValue,
+ );
+
+ expect_cast(
+ ScalarValue::Decimal128(Some(-9999999999999999999999999999999999), 37, 1),
+ DataType::Int64,
+ ExpectedCast::NoValue,
+ );
+ }
+
+ #[test]
+ fn test_try_decimal_cast_in_range() {
+ expect_cast(
+ ScalarValue::Decimal128(Some(12300), 5, 2),
+ DataType::Decimal128(3, 0),
+ ExpectedCast::Value(ScalarValue::Decimal128(Some(123), 3, 0)),
+ );
+
+ expect_cast(
+ ScalarValue::Decimal128(Some(12300), 5, 2),
+ DataType::Decimal128(8, 0),
+ ExpectedCast::Value(ScalarValue::Decimal128(Some(123), 8, 0)),
+ );
+
+ expect_cast(
+ ScalarValue::Decimal128(Some(12300), 5, 2),
+ DataType::Decimal128(8, 5),
+ ExpectedCast::Value(ScalarValue::Decimal128(Some(12300000), 8, 5)),
+ );
+ }
+
+ #[test]
+ fn test_try_decimal_cast_out_of_range() {
Review 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
##########
datafusion/optimizer/src/unwrap_cast_in_comparison.rs:
##########
@@ -653,4 +655,196 @@ mod tests {
fn null_decimal(precision: u8, scale: u8) -> Expr {
lit(ScalarValue::Decimal128(None, precision, scale))
}
+
+ #[test]
+ fn test_try_cast_to_type_nulls() {
+ // test values that can be cast to/from all integer types
Review Comment:
This is a copy/pasted comment 🤦 -- I will fix it in the next PR. Sorry for the confusion and nice spot
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] liukun4515 commented on a diff in pull request #4147: Add additional testing for `unwrap_cast_in_comparison`
Posted by GitBox <gi...@apache.org>.
liukun4515 commented on code in PR #4147:
URL: https://github.com/apache/arrow-datafusion/pull/4147#discussion_r1017844991
##########
datafusion/optimizer/src/unwrap_cast_in_comparison.rs:
##########
@@ -653,4 +655,196 @@ mod tests {
fn null_decimal(precision: u8, scale: u8) -> Expr {
lit(ScalarValue::Decimal128(None, precision, scale))
}
+
+ #[test]
+ fn test_try_cast_to_type_nulls() {
+ // test values that can be cast to/from all integer types
+ let scalars = vec![
+ ScalarValue::Int8(None),
+ ScalarValue::Int16(None),
+ ScalarValue::Int32(None),
+ ScalarValue::Int64(None),
+ ScalarValue::Decimal128(None, 3, 0),
+ ScalarValue::Decimal128(None, 8, 2),
+ ];
+
+ for s1 in &scalars {
+ for s2 in &scalars {
+ expect_cast(
+ s1.clone(),
+ s2.get_datatype(),
+ ExpectedCast::Value(s2.clone()),
+ );
+ }
+ }
+ }
+
+ #[test]
+ fn test_try_cast_to_type_int_in_range() {
+ // test values that can be cast to/from all integer types
+ let scalars = vec![
+ ScalarValue::Int8(Some(123)),
+ ScalarValue::Int16(Some(123)),
+ ScalarValue::Int32(Some(123)),
+ ScalarValue::Int64(Some(123)),
+ ScalarValue::Decimal128(Some(123), 3, 0),
+ ScalarValue::Decimal128(Some(12300), 8, 2),
+ ];
+
+ for s1 in &scalars {
+ for s2 in &scalars {
+ expect_cast(
+ s1.clone(),
+ s2.get_datatype(),
+ ExpectedCast::Value(s2.clone()),
+ );
+ }
+ }
+ }
+
+ #[test]
+ fn test_try_cast_to_type_int_out_of_range() {
+ let max_i64 = ScalarValue::Int64(Some(i64::MAX));
+ let max_u64 = ScalarValue::UInt64(Some(u64::MAX));
+ expect_cast(max_i64.clone(), DataType::Int8, ExpectedCast::NoValue);
+
+ expect_cast(max_i64.clone(), DataType::Int16, ExpectedCast::NoValue);
+
+ expect_cast(max_i64, DataType::Int32, ExpectedCast::NoValue);
+
+ expect_cast(max_u64, DataType::Int64, ExpectedCast::NoValue);
+
+ // decimal out of range
+ expect_cast(
+ ScalarValue::Decimal128(Some(99999999999999999999999999999999999900), 38, 0),
+ DataType::Int64,
+ ExpectedCast::NoValue,
+ );
+
+ expect_cast(
+ ScalarValue::Decimal128(Some(-9999999999999999999999999999999999), 37, 1),
+ DataType::Int64,
+ ExpectedCast::NoValue,
+ );
+ }
+
+ #[test]
+ fn test_try_decimal_cast_in_range() {
+ expect_cast(
+ ScalarValue::Decimal128(Some(12300), 5, 2),
+ DataType::Decimal128(3, 0),
+ ExpectedCast::Value(ScalarValue::Decimal128(Some(123), 3, 0)),
+ );
+
+ expect_cast(
+ ScalarValue::Decimal128(Some(12300), 5, 2),
+ DataType::Decimal128(8, 0),
+ ExpectedCast::Value(ScalarValue::Decimal128(Some(123), 8, 0)),
+ );
+
+ expect_cast(
+ ScalarValue::Decimal128(Some(12300), 5, 2),
+ DataType::Decimal128(8, 5),
+ ExpectedCast::Value(ScalarValue::Decimal128(Some(12300000), 8, 5)),
+ );
+ }
+
+ #[test]
+ fn test_try_decimal_cast_out_of_range() {
+ // decimal would lose precision
+ expect_cast(
+ ScalarValue::Decimal128(Some(12345), 5, 2),
+ DataType::Decimal128(3, 0),
+ ExpectedCast::NoValue,
+ );
+
+ // decimal would lose precision
+ expect_cast(
+ ScalarValue::Decimal128(Some(12300), 5, 2),
+ DataType::Decimal128(2, 0),
+ ExpectedCast::NoValue,
+ );
+ }
+
+ #[test]
+ fn test_try_cast_to_type_unsupported() {
+ // int64 to list
+ expect_cast(
+ ScalarValue::Int64(Some(12345)),
+ DataType::List(Box::new(Field::new("f", DataType::Int32, true))),
+ ExpectedCast::NoValue,
+ );
+ }
+
+ #[derive(Debug, Clone)]
+ enum ExpectedCast {
+ /// test successfully cast value and it is as specified
+ Value(ScalarValue),
+ /// test returned OK, but could not cast the value
+ NoValue,
+ }
+
+ /// Runs try_cast_literal_to_type with the specified inputs and
+ /// ensure it computes the expected output, and ensures the
+ /// casting is consistent with the Arrow kernels
+ fn expect_cast(
+ literal: ScalarValue,
+ target_type: DataType,
+ expected_result: ExpectedCast,
+ ) {
+ let actual_result = try_cast_literal_to_type(&literal, &target_type);
+
+ println!("expect_cast: ");
+ println!(" {:?} --> {:?}", literal, target_type);
+ println!(" expected_result: {:?}", expected_result);
+ println!(" actual_result: {:?}", actual_result);
+
+ match expected_result {
+ ExpectedCast::Value(expected_value) => {
+ let actual_value = actual_result
+ .expect("Expected success but got error")
+ .expect("Expected cast value but got None");
+
+ assert_eq!(actual_value, expected_value);
+
+ // 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);
+ }
+ }
Review 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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on a diff in pull request #4147: Add additional testing for `unwrap_cast_in_comparison`
Posted by GitBox <gi...@apache.org>.
alamb commented on code in PR #4147:
URL: https://github.com/apache/arrow-datafusion/pull/4147#discussion_r1018284789
##########
datafusion/optimizer/src/unwrap_cast_in_comparison.rs:
##########
@@ -653,4 +655,196 @@ mod tests {
fn null_decimal(precision: u8, scale: u8) -> Expr {
lit(ScalarValue::Decimal128(None, precision, scale))
}
+
+ #[test]
+ fn test_try_cast_to_type_nulls() {
+ // test values that can be cast to/from all integer types
Review Comment:
fixed in https://github.com/apache/arrow-datafusion/pull/4148
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] liukun4515 commented on a diff in pull request #4147: Add additional testing for `unwrap_cast_in_comparison`
Posted by GitBox <gi...@apache.org>.
liukun4515 commented on code in PR #4147:
URL: https://github.com/apache/arrow-datafusion/pull/4147#discussion_r1017840904
##########
datafusion/optimizer/src/unwrap_cast_in_comparison.rs:
##########
@@ -653,4 +655,196 @@ mod tests {
fn null_decimal(precision: u8, scale: u8) -> Expr {
lit(ScalarValue::Decimal128(None, precision, scale))
}
+
+ #[test]
+ fn test_try_cast_to_type_nulls() {
+ // test values that can be cast to/from all integer types
+ let scalars = vec![
+ ScalarValue::Int8(None),
+ ScalarValue::Int16(None),
+ ScalarValue::Int32(None),
+ ScalarValue::Int64(None),
+ ScalarValue::Decimal128(None, 3, 0),
+ ScalarValue::Decimal128(None, 8, 2),
+ ];
+
+ for s1 in &scalars {
+ for s2 in &scalars {
+ expect_cast(
+ s1.clone(),
+ s2.get_datatype(),
+ ExpectedCast::Value(s2.clone()),
+ );
+ }
+ }
+ }
+
+ #[test]
+ fn test_try_cast_to_type_int_in_range() {
+ // test values that can be cast to/from all integer types
+ let scalars = vec![
+ ScalarValue::Int8(Some(123)),
+ ScalarValue::Int16(Some(123)),
+ ScalarValue::Int32(Some(123)),
+ ScalarValue::Int64(Some(123)),
+ ScalarValue::Decimal128(Some(123), 3, 0),
+ ScalarValue::Decimal128(Some(12300), 8, 2),
+ ];
+
+ for s1 in &scalars {
+ for s2 in &scalars {
+ expect_cast(
+ s1.clone(),
+ s2.get_datatype(),
+ ExpectedCast::Value(s2.clone()),
+ );
+ }
+ }
+ }
+
+ #[test]
+ fn test_try_cast_to_type_int_out_of_range() {
+ let max_i64 = ScalarValue::Int64(Some(i64::MAX));
+ let max_u64 = ScalarValue::UInt64(Some(u64::MAX));
+ expect_cast(max_i64.clone(), DataType::Int8, ExpectedCast::NoValue);
+
+ expect_cast(max_i64.clone(), DataType::Int16, ExpectedCast::NoValue);
+
+ expect_cast(max_i64, DataType::Int32, ExpectedCast::NoValue);
+
+ expect_cast(max_u64, DataType::Int64, ExpectedCast::NoValue);
+
+ // decimal out of range
+ expect_cast(
+ ScalarValue::Decimal128(Some(99999999999999999999999999999999999900), 38, 0),
+ DataType::Int64,
+ ExpectedCast::NoValue,
+ );
+
+ expect_cast(
+ ScalarValue::Decimal128(Some(-9999999999999999999999999999999999), 37, 1),
+ DataType::Int64,
+ ExpectedCast::NoValue,
+ );
+ }
+
+ #[test]
+ fn test_try_decimal_cast_in_range() {
+ expect_cast(
+ ScalarValue::Decimal128(Some(12300), 5, 2),
+ DataType::Decimal128(3, 0),
+ ExpectedCast::Value(ScalarValue::Decimal128(Some(123), 3, 0)),
+ );
+
+ expect_cast(
+ ScalarValue::Decimal128(Some(12300), 5, 2),
+ DataType::Decimal128(8, 0),
+ ExpectedCast::Value(ScalarValue::Decimal128(Some(123), 8, 0)),
+ );
+
+ expect_cast(
+ ScalarValue::Decimal128(Some(12300), 5, 2),
+ DataType::Decimal128(8, 5),
+ ExpectedCast::Value(ScalarValue::Decimal128(Some(12300000), 8, 5)),
+ );
+ }
+
+ #[test]
+ fn test_try_decimal_cast_out_of_range() {
Review Comment:
maybe I have test unsupported cases in `test_not_unwrap_cast_with_decimal_comparison`, but it's good to simplify the test and add more test.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] liukun4515 commented on a diff in pull request #4147: Add additional testing for `unwrap_cast_in_comparison`
Posted by GitBox <gi...@apache.org>.
liukun4515 commented on code in PR #4147:
URL: https://github.com/apache/arrow-datafusion/pull/4147#discussion_r1017849643
##########
datafusion/optimizer/src/unwrap_cast_in_comparison.rs:
##########
@@ -653,4 +655,196 @@ mod tests {
fn null_decimal(precision: u8, scale: u8) -> Expr {
lit(ScalarValue::Decimal128(None, precision, scale))
}
+
+ #[test]
+ fn test_try_cast_to_type_nulls() {
+ // test values that can be cast to/from all integer types
Review 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`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on a diff in pull request #4147: Add additional testing for `unwrap_cast_in_comparison`
Posted by GitBox <gi...@apache.org>.
alamb commented on code in PR #4147:
URL: https://github.com/apache/arrow-datafusion/pull/4147#discussion_r1018054641
##########
datafusion/optimizer/src/unwrap_cast_in_comparison.rs:
##########
@@ -653,4 +655,196 @@ mod tests {
fn null_decimal(precision: u8, scale: u8) -> Expr {
lit(ScalarValue::Decimal128(None, precision, scale))
}
+
+ #[test]
+ fn test_try_cast_to_type_nulls() {
+ // test values that can be cast to/from all integer types
+ let scalars = vec![
+ ScalarValue::Int8(None),
+ ScalarValue::Int16(None),
+ ScalarValue::Int32(None),
+ ScalarValue::Int64(None),
+ ScalarValue::Decimal128(None, 3, 0),
+ ScalarValue::Decimal128(None, 8, 2),
+ ];
+
+ for s1 in &scalars {
+ for s2 in &scalars {
+ expect_cast(
+ s1.clone(),
+ s2.get_datatype(),
+ ExpectedCast::Value(s2.clone()),
+ );
+ }
+ }
+ }
+
+ #[test]
+ fn test_try_cast_to_type_int_in_range() {
+ // test values that can be cast to/from all integer types
+ let scalars = vec![
+ ScalarValue::Int8(Some(123)),
+ ScalarValue::Int16(Some(123)),
+ ScalarValue::Int32(Some(123)),
+ ScalarValue::Int64(Some(123)),
+ ScalarValue::Decimal128(Some(123), 3, 0),
+ ScalarValue::Decimal128(Some(12300), 8, 2),
+ ];
+
+ for s1 in &scalars {
+ for s2 in &scalars {
+ expect_cast(
+ s1.clone(),
+ s2.get_datatype(),
+ ExpectedCast::Value(s2.clone()),
+ );
+ }
+ }
+ }
+
+ #[test]
+ fn test_try_cast_to_type_int_out_of_range() {
+ let max_i64 = ScalarValue::Int64(Some(i64::MAX));
Review Comment:
yeah- as @liukun4515 mentions I have a PR queued up to support the unsigned case :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] liukun4515 commented on a diff in pull request #4147: Add additional testing for `unwrap_cast_in_comparison`
Posted by GitBox <gi...@apache.org>.
liukun4515 commented on code in PR #4147:
URL: https://github.com/apache/arrow-datafusion/pull/4147#discussion_r1017840904
##########
datafusion/optimizer/src/unwrap_cast_in_comparison.rs:
##########
@@ -653,4 +655,196 @@ mod tests {
fn null_decimal(precision: u8, scale: u8) -> Expr {
lit(ScalarValue::Decimal128(None, precision, scale))
}
+
+ #[test]
+ fn test_try_cast_to_type_nulls() {
+ // test values that can be cast to/from all integer types
+ let scalars = vec![
+ ScalarValue::Int8(None),
+ ScalarValue::Int16(None),
+ ScalarValue::Int32(None),
+ ScalarValue::Int64(None),
+ ScalarValue::Decimal128(None, 3, 0),
+ ScalarValue::Decimal128(None, 8, 2),
+ ];
+
+ for s1 in &scalars {
+ for s2 in &scalars {
+ expect_cast(
+ s1.clone(),
+ s2.get_datatype(),
+ ExpectedCast::Value(s2.clone()),
+ );
+ }
+ }
+ }
+
+ #[test]
+ fn test_try_cast_to_type_int_in_range() {
+ // test values that can be cast to/from all integer types
+ let scalars = vec![
+ ScalarValue::Int8(Some(123)),
+ ScalarValue::Int16(Some(123)),
+ ScalarValue::Int32(Some(123)),
+ ScalarValue::Int64(Some(123)),
+ ScalarValue::Decimal128(Some(123), 3, 0),
+ ScalarValue::Decimal128(Some(12300), 8, 2),
+ ];
+
+ for s1 in &scalars {
+ for s2 in &scalars {
+ expect_cast(
+ s1.clone(),
+ s2.get_datatype(),
+ ExpectedCast::Value(s2.clone()),
+ );
+ }
+ }
+ }
+
+ #[test]
+ fn test_try_cast_to_type_int_out_of_range() {
+ let max_i64 = ScalarValue::Int64(Some(i64::MAX));
+ let max_u64 = ScalarValue::UInt64(Some(u64::MAX));
+ expect_cast(max_i64.clone(), DataType::Int8, ExpectedCast::NoValue);
+
+ expect_cast(max_i64.clone(), DataType::Int16, ExpectedCast::NoValue);
+
+ expect_cast(max_i64, DataType::Int32, ExpectedCast::NoValue);
+
+ expect_cast(max_u64, DataType::Int64, ExpectedCast::NoValue);
+
+ // decimal out of range
+ expect_cast(
+ ScalarValue::Decimal128(Some(99999999999999999999999999999999999900), 38, 0),
+ DataType::Int64,
+ ExpectedCast::NoValue,
+ );
+
+ expect_cast(
+ ScalarValue::Decimal128(Some(-9999999999999999999999999999999999), 37, 1),
+ DataType::Int64,
+ ExpectedCast::NoValue,
+ );
+ }
+
+ #[test]
+ fn test_try_decimal_cast_in_range() {
+ expect_cast(
+ ScalarValue::Decimal128(Some(12300), 5, 2),
+ DataType::Decimal128(3, 0),
+ ExpectedCast::Value(ScalarValue::Decimal128(Some(123), 3, 0)),
+ );
+
+ expect_cast(
+ ScalarValue::Decimal128(Some(12300), 5, 2),
+ DataType::Decimal128(8, 0),
+ ExpectedCast::Value(ScalarValue::Decimal128(Some(123), 8, 0)),
+ );
+
+ expect_cast(
+ ScalarValue::Decimal128(Some(12300), 5, 2),
+ DataType::Decimal128(8, 5),
+ ExpectedCast::Value(ScalarValue::Decimal128(Some(12300000), 8, 5)),
+ );
+ }
+
+ #[test]
+ fn test_try_decimal_cast_out_of_range() {
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on a diff in pull request #4147: Add additional testing for `unwrap_cast_in_comparison`
Posted by GitBox <gi...@apache.org>.
alamb commented on code in PR #4147:
URL: https://github.com/apache/arrow-datafusion/pull/4147#discussion_r1016989407
##########
datafusion/optimizer/src/unwrap_cast_in_comparison.rs:
##########
@@ -653,4 +655,196 @@ mod tests {
fn null_decimal(precision: u8, scale: u8) -> Expr {
lit(ScalarValue::Decimal128(None, precision, scale))
}
+
+ #[test]
+ fn test_try_cast_to_type_nulls() {
+ // test values that can be cast to/from all integer types
+ let scalars = vec![
+ ScalarValue::Int8(None),
+ ScalarValue::Int16(None),
+ ScalarValue::Int32(None),
+ ScalarValue::Int64(None),
+ ScalarValue::Decimal128(None, 3, 0),
+ ScalarValue::Decimal128(None, 8, 2),
+ ];
+
+ for s1 in &scalars {
+ for s2 in &scalars {
+ expect_cast(
+ s1.clone(),
+ s2.get_datatype(),
+ ExpectedCast::Value(s2.clone()),
+ );
+ }
+ }
+ }
+
+ #[test]
+ fn test_try_cast_to_type_int_in_range() {
+ // test values that can be cast to/from all integer types
+ let scalars = vec![
+ ScalarValue::Int8(Some(123)),
+ ScalarValue::Int16(Some(123)),
+ ScalarValue::Int32(Some(123)),
+ ScalarValue::Int64(Some(123)),
+ ScalarValue::Decimal128(Some(123), 3, 0),
+ ScalarValue::Decimal128(Some(12300), 8, 2),
+ ];
+
+ for s1 in &scalars {
+ for s2 in &scalars {
+ expect_cast(
+ s1.clone(),
+ s2.get_datatype(),
+ ExpectedCast::Value(s2.clone()),
+ );
+ }
+ }
+ }
+
+ #[test]
+ fn test_try_cast_to_type_int_out_of_range() {
+ let max_i64 = ScalarValue::Int64(Some(i64::MAX));
+ let max_u64 = ScalarValue::UInt64(Some(u64::MAX));
+ expect_cast(max_i64.clone(), DataType::Int8, ExpectedCast::NoValue);
+
+ expect_cast(max_i64.clone(), DataType::Int16, ExpectedCast::NoValue);
+
+ expect_cast(max_i64, DataType::Int32, ExpectedCast::NoValue);
+
+ expect_cast(max_u64, DataType::Int64, ExpectedCast::NoValue);
+
+ // decimal out of range
+ expect_cast(
+ ScalarValue::Decimal128(Some(99999999999999999999999999999999999900), 38, 0),
+ DataType::Int64,
+ ExpectedCast::NoValue,
+ );
+
+ expect_cast(
+ ScalarValue::Decimal128(Some(-9999999999999999999999999999999999), 37, 1),
+ DataType::Int64,
+ ExpectedCast::NoValue,
+ );
+ }
+
+ #[test]
+ fn test_try_decimal_cast_in_range() {
+ expect_cast(
+ ScalarValue::Decimal128(Some(12300), 5, 2),
+ DataType::Decimal128(3, 0),
+ ExpectedCast::Value(ScalarValue::Decimal128(Some(123), 3, 0)),
+ );
+
+ expect_cast(
+ ScalarValue::Decimal128(Some(12300), 5, 2),
+ DataType::Decimal128(8, 0),
+ ExpectedCast::Value(ScalarValue::Decimal128(Some(123), 8, 0)),
+ );
+
+ expect_cast(
+ ScalarValue::Decimal128(Some(12300), 5, 2),
+ DataType::Decimal128(8, 5),
+ ExpectedCast::Value(ScalarValue::Decimal128(Some(12300000), 8, 5)),
+ );
+ }
+
+ #[test]
+ fn test_try_decimal_cast_out_of_range() {
+ // decimal would lose precision
+ expect_cast(
+ ScalarValue::Decimal128(Some(12345), 5, 2),
+ DataType::Decimal128(3, 0),
+ ExpectedCast::NoValue,
+ );
+
+ // decimal would lose precision
+ expect_cast(
+ ScalarValue::Decimal128(Some(12300), 5, 2),
+ DataType::Decimal128(2, 0),
+ ExpectedCast::NoValue,
+ );
+ }
+
+ #[test]
+ fn test_try_cast_to_type_unsupported() {
+ // int64 to list
+ expect_cast(
+ ScalarValue::Int64(Some(12345)),
+ DataType::List(Box::new(Field::new("f", DataType::Int32, true))),
+ ExpectedCast::NoValue,
+ );
+ }
+
+ #[derive(Debug, Clone)]
+ enum ExpectedCast {
+ /// test successfully cast value and it is as specified
+ Value(ScalarValue),
+ /// test returned OK, but could not cast the value
+ NoValue,
+ }
+
+ /// Runs try_cast_literal_to_type with the specified inputs and
+ /// ensure it computes the expected output, and ensures the
+ /// casting is consistent with the Arrow kernels
+ fn expect_cast(
+ literal: ScalarValue,
+ target_type: DataType,
+ expected_result: ExpectedCast,
+ ) {
+ let actual_result = try_cast_literal_to_type(&literal, &target_type);
+
+ println!("expect_cast: ");
+ println!(" {:?} --> {:?}", literal, target_type);
+ println!(" expected_result: {:?}", expected_result);
+ println!(" actual_result: {:?}", actual_result);
+
+ match expected_result {
+ ExpectedCast::Value(expected_value) => {
+ let actual_value = actual_result
+ .expect("Expected success but got error")
+ .expect("Expected cast value but got None");
+
+ assert_eq!(actual_value, expected_value);
+
+ // 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);
+ }
+ }
Review 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 https://github.com/apache/arrow-datafusion/issues/3938)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] waynexia commented on a diff in pull request #4147: Add additional testing for `unwrap_cast_in_comparison`
Posted by GitBox <gi...@apache.org>.
waynexia commented on code in PR #4147:
URL: https://github.com/apache/arrow-datafusion/pull/4147#discussion_r1017390763
##########
datafusion/optimizer/src/unwrap_cast_in_comparison.rs:
##########
@@ -653,4 +655,196 @@ mod tests {
fn null_decimal(precision: u8, scale: u8) -> Expr {
lit(ScalarValue::Decimal128(None, precision, scale))
}
+
+ #[test]
+ fn test_try_cast_to_type_nulls() {
+ // test values that can be cast to/from all integer types
+ let scalars = vec![
+ ScalarValue::Int8(None),
+ ScalarValue::Int16(None),
+ ScalarValue::Int32(None),
+ ScalarValue::Int64(None),
+ ScalarValue::Decimal128(None, 3, 0),
+ ScalarValue::Decimal128(None, 8, 2),
+ ];
+
+ for s1 in &scalars {
+ for s2 in &scalars {
+ expect_cast(
+ s1.clone(),
+ s2.get_datatype(),
+ ExpectedCast::Value(s2.clone()),
+ );
+ }
+ }
+ }
+
+ #[test]
+ fn test_try_cast_to_type_int_in_range() {
+ // test values that can be cast to/from all integer types
+ let scalars = vec![
+ ScalarValue::Int8(Some(123)),
+ ScalarValue::Int16(Some(123)),
+ ScalarValue::Int32(Some(123)),
+ ScalarValue::Int64(Some(123)),
+ ScalarValue::Decimal128(Some(123), 3, 0),
+ ScalarValue::Decimal128(Some(12300), 8, 2),
+ ];
+
+ for s1 in &scalars {
+ for s2 in &scalars {
+ expect_cast(
+ s1.clone(),
+ s2.get_datatype(),
+ ExpectedCast::Value(s2.clone()),
+ );
+ }
+ }
+ }
+
+ #[test]
+ fn test_try_cast_to_type_int_out_of_range() {
+ let max_i64 = ScalarValue::Int64(Some(i64::MAX));
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org