fix(spark): keep to_date/to_timestamp data skipping working after ReplaceExpressions - #19474
fix(spark): keep to_date/to_timestamp data skipping working after ReplaceExpressions#19474voonhous wants to merge 3 commits into
Conversation
…real optimizer The existing to_timestamp case in TestDataSkippingUtils only runs OptimizeIn, so the RuntimeReplaceable ParseToDate/ParseToTimestamp nodes survive into the translation. On a real read path the FinishAnalysis batch (ReplaceExpressions) rewrites them into Cast/GetTimestamp trees before filters reach file pruning. Add an entry point that runs the full session optimizer over the resolved filter (via a HoodieDummyExpressionHolder fork, see SPARK-44219) and asserts on the translated pruning behavior. The no-format variants still prune via the Cast whitelist arm; the with-format variants currently translate to TrueLiteral and prune nothing, which is the gap tracked by apache#19446.
…ceExpressions The order-preserving whitelist only matched the analysis-time ParseToDate/ParseToTimestamp shapes via the per-version hook. Since SPARK-38240 those nodes are RuntimeReplaceable, and the optimizer rewrites them into GetTimestamp trees before filters reach file pruning, so with an explicit format the translation fell through to TrueLiteral and pruned nothing. Match GetTimestamp in the shared whitelist with a typed pattern (its constructor arity differs across Spark versions but left() is stable), flip the new optimizer-realistic test expectations to pruning, and pin the replacement shapes directly in TestHoodieCatalystExpressionUtils. The per-version hook arms stay: the expression-index and partition-stats paths still inspect un-replaced shapes. Fixes apache#19446
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! The PR restores data skipping for to_date/to_timestamp predicates with explicit formats by matching the post-ReplaceExpressions GetTimestamp shape in the order-preserving whitelist, and backs it with optimizer-realistic tests. The core recursion (GetTimestamp → gt.left, and to_date's Cast(GetTimestamp(...) as date) composing through the existing Cast arm) looks correct, and the typed match is a sensible way to handle the cross-version arity differences. One edge case around non-order-preserving formats is worth double-checking in the inline comment. Please take a look, and this should be ready for a Hudi committer or PMC member to take it from here. A few naming and simplification suggestions below — code is generally clean and the new comments are helpful.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #19474 +/- ##
=========================================
Coverage 76.96% 76.96%
+ Complexity 33850 33840 -10
=========================================
Files 2575 2575
Lines 143372 143383 +11
Branches 17572 17569 -3
=========================================
+ Hits 110349 110360 +11
- Misses 24758 24763 +5
+ Partials 8265 8260 -5
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
… the dead date parsing hook Self-review follow-up on the GetTimestamp whitelist arm: - Match GetTimestamp only when the format is a literal fixed-width year-first pattern: the translation re-applies the parse over string min/max stats, which is only sound when lexicographic order of parseable strings agrees with chronological order. Also require failOnError off so ANSI-mode index lookups can never throw on an unparseable stat value. - Wrap transformed index-lookup bounds in Coalesce(bound, true): partial transformations (string parsing) return null on unparseable stats, in which case f(max(col)) != max(f(col)) and a null bound must keep the file, not prune it. - Delete unapplyOrderPreservingDateParsing and its four per-version overrides: the whitelist's only production consumer receives post-optimizer dataFilters (the pruning rule is injected after the FinishAnalysis batch), so the analysis-time ParseToDate/ ParseToTimestamp shapes never reach it. The expression-index matcher's copy of the same problem is tracked in apache#19480. - Move HoodieDummyExpressionHolder to testutils and reuse it from TestBucketIndexSupport and TestDataSkippingUtils. - Tests: negative-format rows (MM/dd/yyyy, yyyy-M-d), unparseable-stat row, Cast-retaining date_add row, ANSI-mode lookup test, and gate pins for the replacement shapes; ANSI is pinned off in the positive fixtures because Spark 4 defaults it on. Follow-ups filed during self-review: apache#19479 (dead left arm), apache#19480 (expression-index optimizer boundary), apache#19481 (whitelist tightening).
github-actions
Bot
added
size:L
hudi-agent
left a comment
hudi-agent
left a comment
There was a problem hiding this comment.
Thanks for the thorough revisions here — both prior findings are addressed. The missing-period nit is moot since that Scaladoc block was fully rewritten (and now ends with a period), and the order-preservation question is substantively resolved: the new ORDER_PRESERVING_DATE_FORMATS whitelist, the !gt.failOnError gate, and the null-tolerant Coalesce(bound, TrueLiteral) wrapping together restrict pruning to sound formats and safely keep files when a stat value is unparseable or ANSI would throw. I traced the three-valued-logic soundness of wrapTransformedBoundNullSafe and it holds (null→keep is always safe; the surviving null AND false→prune case is independently valid), and the added tests for divergent formats, variable-width, ANSI, and unparseable-max are good coverage. One behavior change worth double-checking is noted inline (dropping the analysis-time ParseToDate/ParseToTimestamp matching that master had). Please take a look, and this should be ready for a Hudi committer or PMC member to take it from here.
| case Some(child) => unapply(child) | ||
| case None => None | ||
| } | ||
| case _ => None |
There was a problem hiding this comment.
🤖 Removing unapplyOrderPreservingDateParsing means raw ParseToDate/ParseToTimestamp (analysis-time shapes) are no longer matched here — origin/master did match them, and the earlier version of this PR documented them as "still inspected by the expression-index and partition-stats paths." I traced that the expression-index path unwraps ParseTo* via attributeFetcher before the matcher sees it, and the read path sees post-ReplaceExpressions shapes, so this looks safe to me. Could you confirm you verified no path feeds a raw ParseTo* into translateIntoColumnStatsIndexFilterExpr and thereby silently loses pruning that worked on master?
4 participants
Describe the issue this Pull Request addresses
Closes #19446
Since SPARK-38240, ParseToDate/ParseToTimestamp are RuntimeReplaceable, so the optimizer rewrites them into Cast/GetTimestamp trees before any filter reaches file pruning. The order-preserving whitelist only matched the pre-replacement shapes, so to_date/to_timestamp with an explicit format translated to TrueLiteral and data skipping silently pruned nothing. The existing green to_timestamp case in TestDataSkippingUtils only runs OptimizeIn, which is why it never caught this.
Summary and Changelog
Impact
Data skipping works again for to_date(col, fmt)/to_timestamp(col, fmt) predicates with order-preserving format patterns, without introducing wrong-results pruning for non-order-preserving formats, unparseable stat values, or ANSI mode. Note that under Spark 4 default config (ANSI on) these predicates intentionally do not prune: safe fallback, not wrong results. No API changes.
Risk Level
low. The arm only fires for allowlisted literal formats with failOnError off; everything else falls back to the pre-existing no-pruning behavior. The null-tolerant bound wrapping only ever keeps more files than before. Covered by the new optimizer-realistic tests, negative-format tests, and the ANSI test.
Follow-ups filed during review: #19479 (left(col, n) whitelist arm is dead for the same RuntimeReplaceable reason), #19480 (expression-index matcher has the same optimizer-boundary bug), #19481 (unix_timestamp parity and format-monotonicity holes in the live arms).
Documentation Update
none
Contributor's checklist