[SPARK-59409][SQL][TESTS] Expand RewriteAsOfJoin optimizer rule test coverage - #58701
Draft
LukaZdravic wants to merge 1 commit into
Draft
[SPARK-59409][SQL][TESTS] Expand RewriteAsOfJoin optimizer rule test coverage#58701LukaZdravic wants to merge 1 commit into
LukaZdravic wants to merge 1 commit into
Conversation
…coverage Refactor RewriteAsOfJoinSuite onto a shared `expectedRewrite` helper and add coverage for previously-untested parts of the RewriteAsOfJoin optimizer rule: - the missing nearest + allowExactMatches=false + no-tolerance combination (as-of condition NOT(left = right)); - a real LeftOuter case (the old "left outer" test built an Inner join); - the no-op guard, for both the requiresSortMergeAsOfJoin node flag and the sort-merge as-of config; - the attribute remapping exercised by an operator above the join. Test-only; no production changes.
There was a problem hiding this comment.
🟡 Changes recommended
A newly introduced Scala import line exceeds Spark’s 100-character style limit and may fail CI style checks.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR refactors and expands RewriteAsOfJoinSuite to improve coverage of the RewriteAsOfJoin optimizer rule and reduce duplicated expected-plan construction logic.
Changes:
- Introduces a shared
expectedRewrite(filter, orderExpression, joinType)helper to centralize expected rewritten plan construction. - Fixes the
"left outer"test to actually useLeftOuter(instead of accidentally duplicating theInnercase). - Adds new tests for missing nearest/allowExactMatches combinations, no-op guards (sort-merge operator flag/config), and attribute remapping above the rewritten join.
File summaries
| File | Description |
|---|---|
| sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/optimizer/RewriteAsOfJoinSuite.scala | Refactors expected-plan construction and expands RewriteAsOfJoin rule test coverage, including guard/no-op and attribute remapping cases. |
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Draft — depends on SPARK-59326. This PR builds on the
RewriteAsOfJoinSuitechanges from SPARK-59326 (the forward/nearest tolerance tests) and refactors the whole suite onto a shared helper.What changes were proposed in this pull request?
Test-only change that expands and refactors
RewriteAsOfJoinSuite, the unit test suite for theRewriteAsOfJoinoptimizer rule:expectedRewrite(filter, orderExpression, joinType)helper so each test spells out only the parts that vary (the as-of condition and the ordering distance), removing ~15 lines of duplicated plan-building per test."left outer"test, which built anInnerjoin (a silent duplicate of"simple"); it now builds a realLeftOuterjoin, isolating the one behavior LeftOuter changes (omitting theIS NOT NULLfilter).(direction, tolerance, allowExactMatches)combination:nearest+allowExactMatches = false+ no tolerance, whose as-of condition isNOT(left = right).!conf.useSortMergeAsOfJoinOperator(requiresSortMergeAsOfJoin), covering both triggers: therequiresSortMergeAsOfJoinnode flag and the sort-merge as-of config.Projectabove theAsOfJoinso the attribute remapping performed bytransformUpWithNewOutputis exercised.Why are the changes needed?
The suite covered only positive rewrites and left several parts of the rule untested: one direction/tolerance/exact-match combination was missing, the
"left outer"test did not actually testLeftOuter, the no-op guard had no coverage, and the parent-operator attribute remapping was never exercised. The sibling rule's suite (RewriteNearestByJoinSuite) already covers these categories; this bringsRewriteAsOfJoinSuiteup to the same bar.Does this PR introduce any user-facing change?
No.
How was this patch tested?
Unit tests in
RewriteAsOfJoinSuite:build/sbt 'catalyst/testOnly *RewriteAsOfJoinSuite'(17 tests).Was this patch authored or co-authored using generative AI tooling?
Generated-by: Claude Code (Claude Opus 4.8)