You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "haohuaijin (via GitHub)" <gi...@apache.org> on 2026/09/12 14:06:01 UTC
[PR] Fix/integer interval propagation [datafusion]
haohuaijin opened a new pull request, #25234:
URL: https://github.com/apache/datafusion/pull/25234
## Which issue does this PR close?
<!--
We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. For example `Closes #123` indicates that this PR will close issue #123.
-->
- Closes #.
## Rationale for this change
<!--
Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed.
Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes.
Please explain the problem you are trying to solve in terms of the user-visible
behavior, rather than the implementation.
For example, "The code in `foo.rs` doesn't handle nulls" is a symptom of the
implementation. "COUNT(DISTINCT) returns wrong results when the column contains
nulls" is the user-visible problem.
-->
## What changes are included in this PR?
<!--
There is no need to duplicate the description in the issue here, but it is sometimes worth providing a summary of the individual changes in this PR.
-->
## What is the testing strategy for this PR?
<!--
We typically require tests for all PRs in order to:
1. Prevent the code from being accidentally broken by subsequent changes
2. Serve as another way to document the expected behavior of the code
Briefly describe how this PR is tested, and point to the specific tests you added. For example: 'This new feature is covered by the `sqllogictest` cases added in `foo.slt`'.
If this PR does not add tests, explain why. For example, if the change is already covered by existing tests, please mention it.
You should also check the `codecov` bot reply on this PR to confirm the changed code is exercised.
-->
## Are there any user-facing changes?
<!--
If there are user-facing changes then we may require documentation to be updated before approving the PR.
If there are any breaking changes to public APIs, please add the `api change` label.
-->
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: prevent unsafe integer interval propagation [datafusion]
Posted by "haohuaijin (via GitHub)" <gi...@apache.org>.
haohuaijin commented on PR #25234:
URL: https://github.com/apache/datafusion/pull/25234#issuecomment-5675491827
Thanks @kosiew , i apply suggestion in [99eaf75](https://github.com/apache/datafusion/pull/25234/commits/99eaf75bd5592dcd839d3d881e375702ef7b6506)
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: prevent unsafe integer interval propagation [datafusion]
Posted by "haohuaijin (via GitHub)" <gi...@apache.org>.
haohuaijin commented on code in PR #25234:
URL: https://github.com/apache/datafusion/pull/25234#discussion_r4012424326
##########
datafusion/expr-common/src/interval_arithmetic.rs:
##########
@@ -2358,6 +2382,85 @@ mod tests {
Ok(())
}
+ #[test]
+ fn test_widening_integer_cast_bounds() {
+ use DataType::{Int8, Int16, Int32, Int64, UInt8, UInt16, UInt32, UInt64};
+ use arrow::compute::CastOptions;
+
+ let types = [
Review Comment:
added in [99eaf75](https://github.com/apache/datafusion/pull/25234/commits/99eaf75bd5592dcd839d3d881e375702ef7b6506)
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: prevent unsafe integer interval propagation [datafusion]
Posted by "kosiew (via GitHub)" <gi...@apache.org>.
kosiew commented on PR #25234:
URL: https://github.com/apache/datafusion/pull/25234#issuecomment-5694399293
🚀
@haohuaijin
Thank you for your contribution.
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: prevent unsafe integer interval propagation [datafusion]
Posted by "haohuaijin (via GitHub)" <gi...@apache.org>.
haohuaijin commented on PR #25234:
URL: https://github.com/apache/datafusion/pull/25234#issuecomment-5664779910
hi @kosiew would you mind reviewing this fix?
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: prevent unsafe integer interval propagation [datafusion]
Posted by "haohuaijin (via GitHub)" <gi...@apache.org>.
haohuaijin commented on code in PR #25234:
URL: https://github.com/apache/datafusion/pull/25234#discussion_r3999298250
##########
datafusion/expr-common/src/interval_arithmetic.rs:
##########
@@ -422,9 +422,43 @@ impl Interval {
data_type: &DataType,
cast_options: &CastOptions,
) -> Result<Self> {
+ let source_type = self.data_type();
+ // An unbounded integer endpoint still has a finite limit imposed by its
+ // type. Preserve that limit when widening so subsequent arithmetic can
+ // prove that it does not overflow the destination type.
+ use DataType::{Int8, Int16, Int32, Int64, UInt8, UInt16, UInt32, UInt64};
+ let widening_integer_cast = matches!(
+ (&source_type, data_type),
+ (Int8, Int16 | Int32 | Int64)
+ | (Int16, Int32 | Int64)
+ | (Int32, Int64)
+ | (UInt8, UInt16 | UInt32 | UInt64 | Int16 | Int32 | Int64)
+ | (UInt16, UInt32 | UInt64 | Int32 | Int64)
+ | (UInt32, UInt64 | Int64)
+ );
+ let lower = if widening_integer_cast && self.lower.is_null() {
+ get_extreme_value!(
+ MIN,
+ MIN_DECIMAL128_FOR_EACH_PRECISION,
+ MIN_DECIMAL256_FOR_EACH_PRECISION,
+ &source_type
+ )
+ } else {
+ self.lower.clone()
+ };
+ let upper = if widening_integer_cast && self.upper.is_null() {
+ get_extreme_value!(
+ MAX,
+ MAX_DECIMAL128_FOR_EACH_PRECISION,
+ MAX_DECIMAL256_FOR_EACH_PRECISION,
+ &source_type
+ )
+ } else {
+ self.upper.clone()
+ };
Review Comment:
Preserve source type bounds during integer widening casts (e.g. `UInt32` to `Int64`), allowing overflow checks to recognize safe arithmetic and retain valid optimizations. like the test case in https://github.com/apache/datafusion/pull/25234/changes#diff-4e8dde04785dd86931aa4ccbab316d31d7a66eb6f1126721fcf15906919a8f98R240-R286
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: prevent unsafe integer interval propagation [datafusion]
Posted by "haohuaijin (via GitHub)" <gi...@apache.org>.
haohuaijin commented on PR #25234:
URL: https://github.com/apache/datafusion/pull/25234#issuecomment-5713193661
Hi @kosiew , i apply the option 1 and raise the follow issue https://github.com/apache/datafusion/issues/25407
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: prevent unsafe integer interval propagation [datafusion]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #25234:
URL: https://github.com/apache/datafusion/pull/25234#issuecomment-5652589689
## [Codecov](https://app.codecov.io/gh/apache/datafusion/pull/25234?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `76.52174%` with `54 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 81.88%. Comparing base ([`bb21f51`](https://app.codecov.io/gh/apache/datafusion/commit/bb21f51013a3f476c5e510de745c77e194fcb584?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`3ccde05`](https://app.codecov.io/gh/apache/datafusion/commit/3ccde0598f6f6ef565cbde4ffd403ede78606430?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 11 commits behind head on main.
| [Files with missing lines](https://app.codecov.io/gh/apache/datafusion/pull/25234?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [datafusion/physical-expr/src/expressions/binary.rs](https://app.codecov.io/gh/apache/datafusion/pull/25234?src=pr&el=tree&filepath=datafusion%2Fphysical-expr%2Fsrc%2Fexpressions%2Fbinary.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZGF0YWZ1c2lvbi9waHlzaWNhbC1leHByL3NyYy9leHByZXNzaW9ucy9iaW5hcnkucnM=) | 79.14% | [5 Missing and 29 partials :warning: ](https://app.codecov.io/gh/apache/datafusion/pull/25234?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [datafusion/expr-common/src/interval\_arithmetic.rs](https://app.codecov.io/gh/apache/datafusion/pull/25234?src=pr&el=tree&filepath=datafusion%2Fexpr-common%2Fsrc%2Finterval_arithmetic.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZGF0YWZ1c2lvbi9leHByLWNvbW1vbi9zcmMvaW50ZXJ2YWxfYXJpdGhtZXRpYy5ycw==) | 68.96% | [5 Missing and 13 partials :warning: ](https://app.codecov.io/gh/apache/datafusion/pull/25234?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [datafusion/physical-plan/src/filter.rs](https://app.codecov.io/gh/apache/datafusion/pull/25234?src=pr&el=tree&filepath=datafusion%2Fphysical-plan%2Fsrc%2Ffilter.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZGF0YWZ1c2lvbi9waHlzaWNhbC1wbGFuL3NyYy9maWx0ZXIucnM=) | 77.77% | [1 Missing and 1 partial :warning: ](https://app.codecov.io/gh/apache/datafusion/pull/25234?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## main #25234 +/- ##
==========================================
- Coverage 81.93% 81.88% -0.06%
==========================================
Files 1133 1133
Lines 423529 424746 +1217
Branches 423529 424746 +1217
==========================================
+ Hits 347032 347791 +759
- Misses 55907 56303 +396
- Partials 20590 20652 +62
```
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/datafusion/pull/25234?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: prevent unsafe integer interval propagation [datafusion]
Posted by "kosiew (via GitHub)" <gi...@apache.org>.
kosiew merged PR #25234:
URL: https://github.com/apache/datafusion/pull/25234
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: prevent unsafe integer interval propagation [datafusion]
Posted by "kosiew (via GitHub)" <gi...@apache.org>.
kosiew commented on code in PR #25234:
URL: https://github.com/apache/datafusion/pull/25234#discussion_r4011884794
##########
datafusion/expr-common/src/interval_arithmetic.rs:
##########
@@ -2358,6 +2382,85 @@ mod tests {
Ok(())
}
+ #[test]
+ fn test_widening_integer_cast_bounds() {
+ use DataType::{Int8, Int16, Int32, Int64, UInt8, UInt16, UInt32, UInt64};
+ use arrow::compute::CastOptions;
+
+ let types = [
Review Comment:
Nice coverage here. One additional test that might be useful is exercising this through a mixed signed/unsigned coercion path, since `coerce_operands` and `coerce_for_comparison` also invoke this cast when unifying operand types. For example, we could check that an unbounded `UInt8` is coerced to the bounded `0..255` range before a widening operation with `Int16`, perhaps through `mul`/`div` or `contains`/`intersect`. Not a blocker since the direct cast matrix already covers the underlying behavior.
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: prevent unsafe integer interval propagation [datafusion]
Posted by "kosiew (via GitHub)" <gi...@apache.org>.
kosiew commented on PR #25234:
URL: https://github.com/apache/datafusion/pull/25234#issuecomment-5712148157
@haohuaijin
I agree with option 1. The widening optimization exposes pre-existing interval-bound failures in negation and narrowing casts, and fixing both correctly needs broader boundary coverage than this sort-correctness fix. Please remove the widening change, keep the overflow/division fixes, and update the FIFO and sort expectations. We can track widening separately with targeted tests for those bound transitions.
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: prevent unsafe integer interval propagation [datafusion]
Posted by "haohuaijin (via GitHub)" <gi...@apache.org>.
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: prevent unsafe integer interval propagation [datafusion]
Posted by "haohuaijin (via GitHub)" <gi...@apache.org>.
haohuaijin commented on PR #25234:
URL: https://github.com/apache/datafusion/pull/25234#issuecomment-5724273493
thanks again @kosiew
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: prevent unsafe integer interval propagation [datafusion]
Posted by "haohuaijin (via GitHub)" <gi...@apache.org>.
haohuaijin commented on code in PR #25234:
URL: https://github.com/apache/datafusion/pull/25234#discussion_r3999302739
##########
datafusion/expr-common/src/interval_arithmetic.rs:
##########
@@ -422,9 +422,43 @@ impl Interval {
data_type: &DataType,
cast_options: &CastOptions,
) -> Result<Self> {
+ let source_type = self.data_type();
+ // An unbounded integer endpoint still has a finite limit imposed by its
+ // type. Preserve that limit when widening so subsequent arithmetic can
+ // prove that it does not overflow the destination type.
+ use DataType::{Int8, Int16, Int32, Int64, UInt8, UInt16, UInt32, UInt64};
+ let widening_integer_cast = matches!(
+ (&source_type, data_type),
+ (Int8, Int16 | Int32 | Int64)
+ | (Int16, Int32 | Int64)
+ | (Int32, Int64)
+ | (UInt8, UInt16 | UInt32 | UInt64 | Int16 | Int32 | Int64)
+ | (UInt16, UInt32 | UInt64 | Int32 | Int64)
+ | (UInt32, UInt64 | Int64)
+ );
+ let lower = if widening_integer_cast && self.lower.is_null() {
+ get_extreme_value!(
+ MIN,
+ MIN_DECIMAL128_FOR_EACH_PRECISION,
+ MIN_DECIMAL256_FOR_EACH_PRECISION,
+ &source_type
+ )
+ } else {
+ self.lower.clone()
+ };
+ let upper = if widening_integer_cast && self.upper.is_null() {
+ get_extreme_value!(
+ MAX,
+ MAX_DECIMAL128_FOR_EACH_PRECISION,
+ MAX_DECIMAL256_FOR_EACH_PRECISION,
+ &source_type
+ )
+ } else {
+ self.upper.clone()
+ };
Review Comment:
This addresses the [CI timeout](https://github.com/apache/datafusion/actions/runs/34702765587/job/103578173073): lost bounds blocked join pruning, leaving the FIFO test waiting for output.
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org