voonhous opened a new issue, #20527:
URL: https://github.com/apache/hudi/issues/20527
## Summary
Refactor Spark data skipping (`DataSkippingUtils` and the order-preserving
matcher) into four stages, with no behavior change, and land it before the
remaining fixes under #20449. Each stage answers one question:
| Stage | Question | Lives in |
| --- | --- | --- |
| Shape | How do AND / OR / NOT combine? | `Compiler` |
| Leaf | Which side is the column? (canonical `term OP constant`) | `Leaf` |
| Range | Is the function order-preserving, and what are its bounds? |
`Range`, `RangeRule`, one `rules/*Rule` per function family |
| Verdict | Which bound check does the operator need? | `Verdict` |
`DataSkippingUtils.translateIntoColumnStatsIndexFilterExpr` stays as the
entry point, so callers (column stats, partition stats, expression index) do
not change. Prototype: #20524.
## Problem today
- `tryComposeIndexFilterExpr` is one recursive match of 26 cases. 20 are
leaf cases, and 12 of those are the 6 comparisons written twice, once per
operand side.
- Every leaf case makes all four decisions at once: it matches the side,
calls the matcher, rebuilds the bound with `swapAttributeRefInExpr`, and picks
min or max.
- The Range decision is split across 3 files: which functions
(`OrderPreservingTransformation`, 27 cases), which casts
(`isCastPreservingOrdering`, 13 arms), and how the bound is rebuilt
(`swapAttributeRefInExpr` plus each leaf case).
- The matcher returns only the column and throws the function chain away.
The chain is then re-walked to rebuild the bound, and there is no slot to say
"this function reverses order" or "this bound can be NULL".
- Cross-cutting fixes are repeated per case. #19474 adds
`wrapTransformedBoundNullSafe` at 17 call sites, and a leaf case added later
that forgets the wrap brings the bug back and still compiles.
- The safety rule is implicit. Functions were added with no conditions, and
each fix since has carved out one bad shape: #19475, #20228, #20230, #20457,
#20458. Casts are still a blocklist (`case _ => true`), see #20513.
## Benefits
- **Each decision is made once.** A comparison is written once: `Leaf.parse`
puts the column on the left, and `flip(op)` is the only code that knows `10 <
f` means `f > 10`. `Verdict` has one line per operator.
- **One place per fix.** Every mechanism in the #20449 plan maps to one slot:
| Mechanism | Lands in |
| --- | --- |
| Order-breaking and collation casts (#19475, #20458, #20513) | `CastRule`
allowlist |
| Negative factors, datediff start (#20457, #20230) |
`ConstantOperandRule`, `Decreasing` direction |
| Integral overflow (#20460) | `ConstantOperandRule`, per-file guard |
| `LIKE 'prefix%'` (#20430) | `Verdict`, the StartsWith line |
| Missing stats, NULL bounds (#20296, #19474) | `Compiler.keepOnNull`,
once for every case |
| Date/time formats (#19481) | `DateTimeFormatRule` guard |
| `BitwiseOr` not monotonic | delete one case in `ConstantOperandRule` |
- **The function chain is kept.** Each rule takes one hop: it names its
child operand in a `Step`, and `Step.apply` rebuilds that layer on the way back
up. Order-reversing functions (`Decreasing`) and per-file guards become fields,
not new code paths.
- **The safety rule is explicit.** `Step(term, child, direction, fileGuard,
partial)` has no default values, so a new case must state its direction, its
per-file guard, and whether it can return NULL. The scaladoc states that
emitting a Step claims order preservation, and that returning nothing is always
safe.
- **Less code in the entry point, and dead code removed.**
`DataSkippingUtils` goes from 566 to 157 lines. The matcher
(`OrderPreservingTransformation`,
`tryMatchAttributeOrderingPreservingTransformation`,
`AllowedTransformationExpression`), `swapAttributeRefInExpr` and the `gen*`
helpers are deleted.
- **Testable per stage.** Leaf parsing, each verdict, and each rule row can
be unit tested on their own. The #20439 oracle's bug families map onto rule
rows.
- **Standard design.** Iceberg's `InclusiveMetricsEvaluator` uses the same
operator set and removes NOT first. DataFusion's `PruningPredicate` treats
unknown as true, and its `SortProperties` is the same increasing / decreasing
notion. ClickHouse's `getMonotonicityForRange` is the same split between static
and per-range guards.
## Cons
- **More files.** 11 new files, +864 / -567 lines overall, about 190 of
which are license headers. Tracing one filter now means hopping across files
instead of scrolling one match.
- **Known bugs are preserved, not fixed.** For example, the STARTS WITH
verdict (#20430), any date format (#19481), and `BitwiseOr`. Each is marked in
place and fixed by its own follow-up PR.
- **One parity workaround.** `Range.rebindOffChainReferences` reproduces
today's re-binding of column references outside the function chain (for ex,
`date_add(d, dayofmonth(d))`). It goes away once constant operands are required.
- **Interface removal.**
`HoodieCatalystExpressionUtils.tryMatchAttributeOrderingPreservingTransformation`
and some `ColumnStatsExpressionUtils` helpers are removed. They have no
in-repo caller.
- **Open PRs need a rebase.** #20513 becomes a one-word change in
`CastRule`; #19474 shrinks to `keepOnNull` plus its format guard.
- **The Spark-version hook stays.** `ParseToDate` / `ParseToTimestamp`
shapes differ across Spark versions, so they are still unwrapped per version
behind the adapter.
## Verification
- [x] Old and new translators on 21,340 generated filters, column stats and
expression index modes: identical output, except one unreachable case (a cast
over an unresolved child, expression index only).
- [x] Spark 3.5: `TestDataSkippingUtils` (70),
`TestHoodieCatalystExpressionUtils` (14), `TestColumnStatsIndexWithSQL` (82)
pass. Only the matcher test helper changed; no case or assertion did.
- [ ] Spark 4.x compile and the same suites
- [ ] Expression index and partition stats functional suites
## Plan
1. This refactor, behavior-preserving.
2. Rebase #20513 (casts deny-by-default) and #19474 (NULL bounds,
`GetTimestamp` gate).
3. Follow-ups, one PR each: order-reversing ranges for negative factors and
datediff, constant operands for `DateAdd` / `DateSub` / `ShiftLeft` /
`ShiftRight` / `DateDiff`, delete `BitwiseOr`, #19481 format guards, #20460
overflow guard, #20430 verdict.
--
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]
