fix(spark): reject order-breaking casts and negative factors in data ... - #19475
fix(spark): reject order-breaking casts and negative factors in data ...#19475voonhous wants to merge 1 commit into
Conversation
…skipping Data skipping translated filters over cast/multiply/divide transformations by re-applying them to column min/max stats, assuming they preserve ordering. Narrowing numeric casts wrap around in non-ANSI mode, collation-changing string casts re-sort on Spark 4, and negative multiply/divide factors reverse ordering, so the translated bounds could silently prune files containing matching rows. - isCastPreservingOrdering now requires Cast.canUpCast for numeric pairs and identical string types (collation-aware on Spark 4) for string pairs - Multiply/Divide transformations now require a strictly positive numeric literal factor Non-order-preserving shapes fall back to no pruning for that predicate. Fixes apache#19445
voonhous
changed the title
voonhous
changed the title
hudi-agent
left a comment
hudi-agent
left a comment
There was a problem hiding this comment.
Thanks for working on this! This PR tightens the data-skipping order-preservation matcher so order-breaking transforms (narrowing numeric casts, collation-changing string casts on Spark 4, and negative/zero multiply/divide factors) fall back to no pruning instead of silently dropping matching files. The logic traces cleanly: the cast guard uses Cast.canUpCast and keeps the String↔Numeric arms ahead of the numeric arm, isPositiveNumericLiteral correctly rejects negative/zero/null/non-literal and NaN factors, and Divide only matches the numerator position. Every change moves in the conservative direction (less pruning, never incorrect pruning), and the added tests reproduce the original bug. No correctness issues found. A few style/readability suggestions in the inline comments. Please take a look, and this should be ready for a Hudi committer or PMC member to take it from here. One minor over-commenting nit on isPositiveNumericLiteral; the rest of the diff is clean and readable.
cc @yihua
| // statically (the optimizer folds constant factors to literals before data skipping runs). | ||
| // Typed null literals carry a null value and fail the value match | ||
| private def isPositiveNumericLiteral(expr: Expression): Boolean = expr match { | ||
| case Literal(value, _) => value match { |
There was a problem hiding this comment.
🤖 nit: the block comment above could be trimmed — the method name already says "positive numeric literal", and the case _ => false arm makes the null/non-literal behavior self-evident. The most non-obvious part (why non-literals are excluded) could stand on its own as a one-liner: // non-literal operands can't be evaluated statically; the optimizer folds constant expressions before data skipping runs.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #19475 +/- ##
============================================
- Coverage 77.00% 76.84% -0.17%
+ Complexity 33873 33756 -117
============================================
Files 2576 2575 -1
Lines 143463 143385 -78
Branches 17589 17633 +44
============================================
- Hits 110476 110181 -295
- Misses 24716 24935 +219
+ Partials 8271 8269 -2
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
| case Multiply(OrderPreservingTransformation(attrRef), _, _) => Some(attrRef) | ||
| case Multiply(_, OrderPreservingTransformation(attrRef), _) => Some(attrRef) | ||
| case Divide(OrderPreservingTransformation(attrRef), _, _) => Some(attrRef) | ||
| case Multiply(OrderPreservingTransformation(attrRef), factor, _) if isPositiveNumericLiteral(factor) => Some(attrRef) |
There was a problem hiding this comment.
A strictly positive factor is not sufficient to make multiplication order-preserving because integral arithmetic wraps in non-ANSI mode. For a bigint file containing {51, Long.MaxValue}, A * 2L > 100 is accepted here and translated using the transformed max. Spark evaluates Long.MaxValue * 2L as -2, so the file is pruned, although 51 * 2L = 102 matches the original predicate.
Please restrict multiplication to analyzed types/factors for which overflow cannot break monotonicity, or conservatively reject the unsafe integral/decimal cases. This exact file-stats scenario should be added as a regression test.
5 participants
...skipping
Describe the issue this Pull Request addresses
Closes #19445
Data skipping translates a filter over a transformed column (e.g.
cast(a as int) > 100) into a filter over the column's min/max stats by re-applying the transformation toa_minValue/a_maxValue. This is only sound when the transformation preserves ordering, but the matcher accepted several order-breaking shapes:isCastPreservingOrderingonly rejectedString <-> Numericand returnedtruefor everything else.cast(4294967297L as int)wraps to1in non-ANSI mode, so[cast(min), cast(max)]is not a valid bound and files containing matching rows are pruned -- silently missing rows.StringTypeis collation-parameterized on Spark 4, so a collated string fell through the stable-identifiercase (StringType, ...)arms tocase _ => true, even though a different collation sorts differently.Multiply/Dividearms ofOrderPreservingTransformationmatched any operand, including negative literals, which reverse ordering (a * -1swaps min and max).Summary and Changelog
Order-breaking cast/multiply/divide shapes no longer translate into column-stats filters; they fall back to
TrueLiteral(no pruning for that predicate), so queries return correct results at the cost of scanning more files.HoodieSparkTypeUtils.isCastPreservingOrdering:String <-> Numericrejections first (now type patterns, so Spark 4 collated strings are also rejected); they must stay ahead of the numeric arm becauseCast.canUpCasttreats atomic-to-string casts as legal up-castsstring -> stringrequires the identicalStringType(collation-aware equality on Spark 4; trivially true on Spark 3)numeric -> numericrequiresCast.canUpCast(from, to), rejecting narrowing casts; conservative for weakly monotonic casts likedouble -> float, which only costs pruning opportunityBaseHoodieCatalystExpressionUtils.OrderPreservingTransformation: theMultiply/Dividearms now require a strictly positive numeric literal factor (isPositiveNumericLiteral). Operand positions are unchanged (attr * lit,lit * attr,attr / lit). The optimizer folds constant factors to literals before data skipping runs, so requiring a plainLiteralcosts nothing in practice.TestHoodieCatalystExpressionUtils: flipped theTODO(#19445)pinned assertion (narrowing cast now returnsNone); new tests for thecanUpCastboundaries, the multiply/divide guard, and a reflection-based collation test (assumption-skipped on Spark 3 profiles, since collation APIs only exist on Spark 4)TestDataSkippingUtils: new parameterized source with the issue repro (CAST(A AS INT) > 100over a file withA_maxValue = 4294967297Lmust not be pruned) plus widening-cast and positive-factor controls proving legitimate pruning still worksImpact
No public API change. Correctness fix for data skipping: queries whose filters contain narrowing casts, collation-changing casts, or negative constant factors previously could silently miss rows; they now skip nothing for those predicates. Queries that relied on the unsound pruning will scan more files.
Risk Level
low. Every newly rejected shape falls back to
TrueLiteral(no pruning), so the failure direction is only lost pruning opportunity, never lost rows. The touched files have a single shared copy compiled under all supported Spark profiles (3.3 - 4.2), andCast.canUpCastexists on all of them.Documentation Update
none
Contributor's checklist