haohuaijin commented on PR #25234: URL: https://github.com/apache/datafusion/pull/25234#issuecomment-5699295433
Hi @kosiew, I looked into the [merge queue failures](https://github.com/apache/datafusion/actions/runs/35077165383/job/104732215991). The [widening change](https://github.com/apache/datafusion/pull/25234#discussion_r3999298250) exposed two existing gaps in interval handling: [negation](https://github.com/apache/datafusion/blob/2d0f3c3f0ce671e5606544e0f3725174c98b3518/datafusion/physical-expr/src/expressions/negative.rs#L126) doesn’t handle `i32::MIN`, and [narrowing casts](https://github.com/apache/datafusion/blob/2d0f3c3f0ce671e5606544e0f3725174c98b3518/datafusion/physical-expr/src/expressions/cast.rs#L371) can fail on estimated bounds even when the actual data fits. Previously, those endpoints stayed unbounded, so these paths didn’t encounter the concrete limits. Preserving the source type’s bounds is valid, but it brought these cases into play, so we need to address them or leave widening out of this PR. I think we have two options: 1. **Remove the widening change**, use `Float64` for the [FIFO test’s ordered column](https://github.com/apache/datafusion/blob/2d0f3c3f0ce671e5606544e0f3725174c98b3518/datafusion/core/tests/fifo/mod.rs#L242), and update the [sort expectation](https://github.com/apache/datafusion/blob/2d0f3c3f0ce671e5606544e0f3725174c98b3518/datafusion/sqllogictest/test_files/filter_without_sort_exec.slt#L251). I tested this, and all 154 previously failing SQLite files pass. 2. **Keep widening** and fix how [negation](https://github.com/apache/datafusion/blob/2d0f3c3f0ce671e5606544e0f3725174c98b3518/datafusion/physical-expr/src/expressions/negative.rs#L126) and [narrowing casts](https://github.com/apache/datafusion/blob/2d0f3c3f0ce671e5606544e0f3725174c98b3518/datafusion/physical-expr/src/expressions/cast.rs#L371) handle those bounds. I’m perfer 1 to keep this PR focused, then opening a separate issue for the widening optimization. What do you think? I’d like to hear your thoughts before I update the PR. -- 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]
