fix: only propagate cast statistics through safe conversions - #25227
haohuaijin wants to merge 5 commits into
Conversation
| /// 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; |
There was a problem hiding this comment.
check_bigger_cast is already used for ordering inference. 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?
There was a problem hiding this 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.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #25227 +/- ##
========================================
Coverage 82.28% 82.28%
========================================
Files 1137 1137
Lines 430211 430339 +128
Branches 430211 430339 +128
========================================
+ Hits 354018 354125 +107
- Misses 54771 54788 +17
- Partials 21422 21426 +4 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Hi @alamb @Dandandan could you help review this when you get a chance? |
Which issue does this PR close?
Rationale for this change
MIN/MAXover a cast can return incorrect results when aggregate optimization replaces the scan with converted column statistics. Casting the original endpoints is only valid if they remain extrema in the target domain.For example, a Parquet string column containing
('1', '100', '2')has string extrema'1'and'2'. Previously,MIN(CAST(a AS INT)), MAX(CAST(a AS INT))could return1, 2from those statistics instead of the correct1, 100. The same issue affectsBIGINT.What changes are included in this PR?
CastExpr::check_bigger_castto recognizeInt32 ↔ Date32, and clarify that its contract includes lossless, strictly order-preserving conversions that preserve nulls. Arrow reinterprets the samei32values as days since the epoch, so these conversions preserve both statistics and ordering properties. This also retains the statistics-basedMIN/MAXoptimization for ClickBench'sUInt16 → Int32 → Date32projection.What is the testing strategy for this PR?
parquet_statistics.sltforINTandBIGINT, both expecting1, 100.Int32 ↔ Date32test coveringi32::MIN,i32::MAX, negative values, zero, NULL, and strict ordering properties.Local formatting, Clippy, license, spelling, workflow, and documentation checks also pass.
Are there any user-facing changes?
Affected casts now produce correct
MIN/MAXresults. Conversions whose safety is not established may require a scan instead of using statistics-based aggregate optimization. Supported safe integer conversions andInt32 ↔ Date32retain that optimization.