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 12:07:11 UTC
[PR] fix: only propagate cast statistics through safe conversions [datafusion]
haohuaijin opened a new pull request, #25227:
URL: https://github.com/apache/datafusion/pull/25227
### Which issue does this PR close?
- closes #25221
### Rationale for this change
MIN/MAX over numeric casts of strings can return incorrect results without scanning the values.
### What changes are included in this PR?
Preserve statistics for supported widening casts and integer narrowing casts with safe exact bounds. Return unknown statistics for unsupported or unsafe conversions.
### What is the testing strategy for this PR?
Unit tests cover non-monotonic casts and narrowing bounds. `cast_statistics.slt` reproduces incorrect INT and BIGINT aggregates and checks the corrected results.
### Are there any user-facing changes?
Correct MIN/MAX results for affected casts. Some casts no longer use statistics-based aggregate optimization.
--
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: only propagate cast statistics through safe conversions [datafusion]
Posted by "haohuaijin (via GitHub)" <gi...@apache.org>.
haohuaijin commented on code in PR #25227:
URL: https://github.com/apache/datafusion/pull/25227#discussion_r3999343833
##########
datafusion/physical-expr/src/expressions/cast.rs:
##########
@@ -249,8 +249,11 @@ impl CastExpr {
})
}
- /// Check if casting from the specified source type to the target type is a
- /// widening cast (e.g. from `Int8` to `Int16`).
+ /// Check if casting from the source type to the target type is known to be
+ /// lossless and strictly order-preserving for all source values, preserving nulls.
+ /// This includes widening casts (e.g. `Int8` to `Int16`) and representation
+ /// conversions such as `Int32` to `Date32`, which interprets the same integer
+ /// as days since the epoch.
pub fn check_bigger_cast(cast_type: &DataType, src: &DataType) -> bool {
if cast_type.eq(src) {
return true;
Review Comment:
We can extend this helper in a follow-up to recognize more safe conversions, such as `UInt32` → `Int64`, allowing statistics to be preserved without requiring exact bounds on both ends.
--
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: only propagate cast statistics through safe conversions [datafusion]
Posted by "haohuaijin (via GitHub)" <gi...@apache.org>.
haohuaijin commented on code in PR #25227:
URL: https://github.com/apache/datafusion/pull/25227#discussion_r4052709163
##########
datafusion/physical-expr/src/expressions/cast.rs:
##########
@@ -260,6 +263,8 @@ impl CastExpr {
(Int8, Int16 | Int32 | Int64)
| (Int16, Int32 | Int64)
| (Int32, Int64)
+ | (Int32, Date32)
Review Comment:
> these are lossless because they have the same underlying representation?
yes
> Should we also have entries for Date64 and Int64?
added, we can also extend this helper in a follow-up to recognize more safe conversions, such as `UInt32` → `Int64` that i mention in https://github.com/apache/datafusion/pull/25227#discussion_r3999343833
--
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: only propagate cast statistics through safe conversions [datafusion]
Posted by "haohuaijin (via GitHub)" <gi...@apache.org>.
haohuaijin commented on code in PR #25227:
URL: https://github.com/apache/datafusion/pull/25227#discussion_r3996341542
##########
datafusion/physical-expr/src/expressions/cast.rs:
##########
@@ -249,8 +249,11 @@ impl CastExpr {
})
}
- /// Check if casting from the specified source type to the target type is a
- /// widening cast (e.g. from `Int8` to `Int16`).
+ /// Check if casting from the source type to the target type is known to be
+ /// lossless and strictly order-preserving for all source values, preserving nulls.
+ /// This includes widening casts (e.g. `Int8` to `Int16`) and representation
+ /// conversions such as `Int32` to `Date32`, which interprets the same integer
+ /// as days since the epoch.
pub fn check_bigger_cast(cast_type: &DataType, src: &DataType) -> bool {
if cast_type.eq(src) {
return true;
Review Comment:
`check_bigger_cast` is already used for [ordering inference](https://github.com/apache/datafusion/blob/b94929a87f74dbe194db8ccd528592491d96606f/datafusion/physical-expr/src/expressions/cast.rs#L302). I added `Int32 ↔ Date32` because Arrow reinterprets the same underlying values, preserving ordering and nulls. This also keeps ClickBench's `MIN`/`MAX` statistics optimization working after this PR restricts cast statistics propagation. Should we rename the helper to reflect its broader contract than widening?
--
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: only propagate cast statistics through safe conversions [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #25227:
URL: https://github.com/apache/datafusion/pull/25227
--
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: only propagate cast statistics through safe conversions [datafusion]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #25227:
URL: https://github.com/apache/datafusion/pull/25227#issuecomment-5646250237
## [Codecov](https://app.codecov.io/gh/apache/datafusion/pull/25227?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 `91.22807%` with `10 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 81.94%. 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 ([`b94929a`](https://app.codecov.io/gh/apache/datafusion/commit/b94929a87f74dbe194db8ccd528592491d96606f?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 3 commits behind head on main.
| [Files with missing lines](https://app.codecov.io/gh/apache/datafusion/pull/25227?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/cast.rs](https://app.codecov.io/gh/apache/datafusion/pull/25227?src=pr&el=tree&filepath=datafusion%2Fphysical-expr%2Fsrc%2Fexpressions%2Fcast.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZGF0YWZ1c2lvbi9waHlzaWNhbC1leHByL3NyYy9leHByZXNzaW9ucy9jYXN0LnJz) | 80.76% | [0 Missing and 5 partials :warning: ](https://app.codecov.io/gh/apache/datafusion/pull/25227?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [datafusion/physical-expr/src/projection.rs](https://app.codecov.io/gh/apache/datafusion/pull/25227?src=pr&el=tree&filepath=datafusion%2Fphysical-expr%2Fsrc%2Fprojection.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZGF0YWZ1c2lvbi9waHlzaWNhbC1leHByL3NyYy9wcm9qZWN0aW9uLnJz) | 94.31% | [1 Missing and 4 partials :warning: ](https://app.codecov.io/gh/apache/datafusion/pull/25227?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 #25227 +/- ##
==========================================
+ Coverage 81.93% 81.94% +0.01%
==========================================
Files 1133 1133
Lines 423529 423910 +381
Branches 423529 423910 +381
==========================================
+ Hits 347032 347388 +356
- Misses 55907 55919 +12
- Partials 20590 20603 +13
```
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/datafusion/pull/25227?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: only propagate cast statistics through safe conversions [datafusion]
Posted by "haohuaijin (via GitHub)" <gi...@apache.org>.
haohuaijin commented on PR #25227:
URL: https://github.com/apache/datafusion/pull/25227#issuecomment-5740597083
Thanks @alamb , i applied all suggestioin in [e70d444](https://github.com/apache/datafusion/pull/25227/commits/e70d4443dbb1889b44edbd25576d71ea2da3c42a)
--
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: only propagate cast statistics through safe conversions [datafusion]
Posted by "haohuaijin (via GitHub)" <gi...@apache.org>.
haohuaijin commented on PR #25227:
URL: https://github.com/apache/datafusion/pull/25227#issuecomment-5664921888
Hi @alamb @Dandandan could you help review this when you get a chance?
--
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: only propagate cast statistics through safe conversions [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #25227:
URL: https://github.com/apache/datafusion/pull/25227#discussion_r4050838674
##########
datafusion/physical-expr/src/expressions/cast.rs:
##########
@@ -260,6 +263,8 @@ impl CastExpr {
(Int8, Int16 | Int32 | Int64)
| (Int16, Int32 | Int64)
| (Int32, Int64)
+ | (Int32, Date32)
Review Comment:
these are lossless because they have the same underlying representation?
Should we also have entries for Date64 and Int64?
##########
datafusion/physical-expr/src/projection.rs:
##########
@@ -872,15 +872,39 @@ fn project_column_statistics_through_expr(
return inner_stats;
}
+ let min_value = inner_stats
+ .min_value
+ .cast_to(target_type)
+ .unwrap_or(Precision::Absent);
+ let max_value = inner_stats
+ .max_value
+ .cast_to(target_type)
+ .unwrap_or(Precision::Absent);
+ let source_type = inner_stats
+ .min_value
+ .get_value()
+ .or_else(|| inner_stats.max_value.get_value())
+ .map(ScalarValue::data_type);
+ // Copy extrema only for casts that preserve order and cannot discard values
+ // or fail within the input domain. Copying string endpoints into a numeric
+ // domain, for example, does not bound the converted column. Merely casting
+ // a failing endpoint to NULL also cannot establish the remaining extrema.
+ let preserves_values = source_type.is_some_and(|source_type| {
+ CastExpr::check_bigger_cast(target_type, &source_type)
+ || (source_type.is_integer() && target_type.is_integer()
Review Comment:
I think this would be easier to read if we also factored this specific integer check into its own function too so the logic isn't quite as complex
Something like
```rust
CastExpr::check_bigger_cast(target_type, &source_type)
|| is_within_extrema(&inner_stats.min_value, &inner_stats.max_value, &min_value, &max_value)
```
Then you could also add some examples / unit tests for the extrema check
##########
datafusion/sqllogictest/test_files/parquet_statistics.slt:
##########
@@ -186,6 +186,39 @@ physical_plan
statement ok
DROP TABLE typed_table;
+######
Review Comment:
can we also write a test to check the extrema condition (like casting an int64 in parquet into an int8)?
--
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