andygrove commented on PR #5982: URL: https://github.com/apache/datafusion-comet/pull/5982#issuecomment-5876153959
This is a light fully automated review since there are so many PRs open. `get_properties` falls back to an unbounded range when `child.range.arithmetic_negate()` errors (`native/spark-expr/src/math_funcs/negative.rs:308`), and the comment above it says that only happens at the minimum of a signed integer or on a null decimal or interval bound. For `Duration` it happens for every range. `ScalarValue::arithmetic_negate` in datafusion-common 55.1.0 has no arm for any of the `Duration*` variants, so every `Duration` bound falls through to the catch-all internal error. Widening is still sound, so this is not a correctness problem. But `test_get_properties_reverses_ordering_for_the_non_wrapping_types` includes `Duration(Second)` without ever asserting `props.range`, so nothing records that `Duration` always widens. Could the comment mention `Duration`, and could the test assert the range for a comfortably bounded `Duration` input such as `[5, 10]` seconds? -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
