[SPARK-59299][SQL][TESTS][FOLLOWUP] Split ASOF JOIN parser test cases into separate tests - #58630
Closed
LukaZdravic wants to merge 3 commits into
Closed
[SPARK-59299][SQL][TESTS][FOLLOWUP] Split ASOF JOIN parser test cases into separate tests#58630LukaZdravic wants to merge 3 commits into
LukaZdravic wants to merge 3 commits into
Conversation
vranes
approved these changes
Sep 8, 2026
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Pull request overview
Splits the bundled ASOF JOIN parser assertions in PlanParserSuite into individually named tests so failures are isolated and self-identifying.
Changes:
- Closes the original
test("asof join")block after the canonical case. - Adds separate
test("asof join - ...")blocks for each previously bundled parsing scenario. - Repeats the
SQL_ASOF_JOIN_ENABLEDconfiguration enablement per test to keep cases independent.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
uros-b
approved these changes
Sep 8, 2026
Member
|
Thank you @LukaZdravic and @vranes! |
Member
|
@LukaZdravic Please try to make the CI green. |
Contributor
Author
|
… into separate tests
The `test("asof join")` block in PlanParserSuite bundled nine positive parsing cases under a
single test name. On a failure the report would only name `asof join`, not which case broke,
and because the first failing assertion aborts the block, the later cases would not run. Split
the block so each scenario is its own named test:
- basic MATCH_CONDITION (kept as `asof join`);
- LEFT ASOF with an ON condition;
- USING single / multiple join columns;
- the `<=`, `>` and `<` match operators;
- explicit INNER / LEFT OUTER join types.
No assertions were changed; this only redistributes the existing cases into named tests.
Follow-up to the review on SPARK-59299 (apache#58578).
Test-only; no production code changes.
…review Address review nits on the split tests: - `left asof with on condition` -> `explicit left join type with on condition`, to match the explicit inner / left outer join type tests below; - `using single join column` / `using multiple join columns` -> `using with a single join column` / `using with multiple join columns`. Test-only; test names only, no assertion changes.
LukaZdravic
force-pushed
the
SPARK-59299-asof-parser-tests-followup
branch
from
September 8, 2026 20:35
83a805f to
dc4ea22
Compare
uros-b
pushed a commit
that referenced
this pull request
Sep 9, 2026
…into separate tests ### What changes were proposed in this pull request? Follow-up to SPARK-59299 (#58578) addressing a review comment. `PlanParserSuite`'s `test("asof join")` bundled nine positive `ASOF JOIN` parsing cases under one test name. This splits them so each scenario is its own named test: - `asof join` - basic `MATCH_CONDITION` (`t.a >= u.a`), retained as the canonical case; - `asof join - left asof with on condition`; - `asof join - using single join column` / `... multiple join columns`; - `asof join - less than or equal / greater than / less than match operator`; - `asof join - explicit inner / left outer join type`. No assertions were changed - the existing cases are only redistributed into separate tests (the diff is purely additive). ### Why are the changes needed? With all cases in one `test(...)` block, a failure is reported only as `asof join` without indicating which case broke, and the first failing assertion aborts the block so the remaining cases never run. One scenario per test makes failures self-identifying and independent, matching the surrounding `asof join - ...` tests in the same suite. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Test-only change: the existing `test("asof join")` assertions are only redistributed into separately-named tests, with no assertion modified (the diff is purely additive). Coverage is verified by this PR's GitHub Actions CI, which compiles `sql/catalyst` and runs `PlanParserSuite`. ### Was this patch authored or co-authored using generative AI tooling? Generated-by: Claude Code (Claude Opus 4.8) Closes #58630 from LukaZdravic/SPARK-59299-asof-parser-tests-followup. Authored-by: Luka Zdravić <lukazdravic10@gmail.com> Signed-off-by: Uros Bojanic <221401595+uros-b@users.noreply.github.com> (cherry picked from commit ebfe65f) Signed-off-by: Uros Bojanic <221401595+uros-b@users.noreply.github.com>
Member
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.
What changes were proposed in this pull request?
Follow-up to SPARK-59299 (#58578) addressing a review comment.
PlanParserSuite'stest("asof join")bundled nine positiveASOF JOINparsing cases under one test name. Thissplits them so each scenario is its own named test:
asof join- basicMATCH_CONDITION(t.a >= u.a), retained as the canonical case;asof join - left asof with on condition;asof join - using single join column/... multiple join columns;asof join - less than or equal / greater than / less than match operator;asof join - explicit inner / left outer join type.No assertions were changed - the existing cases are only redistributed into separate tests
(the diff is purely additive).
Why are the changes needed?
With all cases in one
test(...)block, a failure is reported only asasof joinwithoutindicating which case broke, and the first failing assertion aborts the block so the remaining
cases never run. One scenario per test makes failures self-identifying and independent,
matching the surrounding
asof join - ...tests in the same suite.Does this PR introduce any user-facing change?
No.
How was this patch tested?
Test-only change: the existing
test("asof join")assertions are only redistributed intoseparately-named tests, with no assertion modified (the diff is purely additive). Coverage is
verified by this PR's GitHub Actions CI, which compiles
sql/catalystand runsPlanParserSuite.Was this patch authored or co-authored using generative AI tooling?
Generated-by: Claude Code (Claude Opus 4.8)