Repository navigation
Conversation
Materialize MATCH_CONDITION into asOfCondition and sort keys, expand USING into equi-join predicates, and validate operand references, types, and determinism. Gate SQL plans on sort-merge conf at analysis time.
cloud-fan
left a comment
There was a problem hiding this comment.
0 blocking, 2 non-blocking, 1 nit.
Clean analysis layer; the main issue is a validation/planner-capability gap surfaced as a question, behind off-by-default confs.
Correctness (1)
- ResolveAsOfJoin.scala:149: analysis accepts operand types whose
orderExpressionthe planner'sfindFromOrdercan't parse (STRING/struct/array) → crypticIllegalStateExceptionat planning — see inline
Suggestions (1)
- AsOfJoinSQLSuite.scala:41: no positive analyze-only test for a valid MATCH_CONDITION — see inline
Nits: 1 minor item (see inline comments).
Verification
Traced the SQL MATCH_CONDITION path end-to-end: validateMatchConditionOperands admits any orderable, type-compatible operands, but AsOfJoinSelection.findFromOrder (SparkStrategies.scala:514-528) recognizes only Subtract/If(_,Subtract,_). STRING operands yield If(EqualTo,0,...) and struct/array operands yield CreateStruct/ZipWith, none of which findFromOrder matches — so with both confs on these queries pass analysis, skip the rewrite, and throw IllegalStateException at planning. Subtractable-scalar operands (the DataFrame path's only shape) are unaffected.
…rands Reject STRING/struct/array MATCH_CONDITION operands at analysis with AS_OF_JOIN.UNSUPPORTED_MATCH_CONDITION_OPERAND until multi-column sort-merge execution lands in SPARK-58124. Run operand normalization before the planner-capability check so table-reference errors still win. Add positive analyze-only TIMESTAMP test and drop redundant leftSortExprs resolved checks.
Wrap materializeMatchComparison return type for the 100-char limit, fix import order in ResolveAsOfJoin, and move UNSUPPORTED_MATCH_CONDITION_OPERAND after UNSUPPORTED_DIRECTION in error-conditions.json for SparkThrowableSuite.
cloud-fan
left a comment
There was a problem hiding this comment.
5 addressed, 0 remaining, 2 new. (2 = 2 newly introduced, 0 late catches.)
0 blocking, 2 non-blocking, 0 nits.
The planner-capability gap and both follow-ups are correctly resolved, and the two CI-gate breakages I was about to flag (the mis-ordered error-condition key and the over-length line) were fixed by 7d9bc48, which you pushed while I was reviewing. The fix is sound. What remains are two non-blocking cleanups left by the refactor.
Design / architecture (2)
- basicLogicalOperators.scala:2631:
resolveMatchComparisonis now dead code after the split — see inline - ResolveAsOfJoin.scala:200:
supportsSubtractForPlannerduplicatesAsOfJoin.supportsSubtract— see inline
Verification
Traced the fix against the accepted (scalar) path: it inserts a type gate and splits resolveMatchComparison into normalizeMatchOperands + materializeMatchComparison, whose composition is byte-identical to the old body — no expression construction changed, so rows are unaffected. The gated operand domain (supportsSubtractForPlanner: numeric/datetime/interval) is exactly what the planner's findFromOrder consumes (Subtract / If(_, Subtract, _), SparkStrategies.scala:517-528) and what the DataFrame joinAsOf analogue produces, so the previously planner-unsupported STRING/struct/array operands now fail at analysis with a clear error instead of IllegalStateException at planning.
…ubtract Remove unused resolveMatchComparison and call AsOfJoin.supportsSubtract from the planner-support gate instead of duplicating the type predicate.
cloud-fan
left a comment
There was a problem hiding this comment.
2 addressed, 0 remaining, 0 new. (0 = 0 newly introduced, 0 late catches.)
0 blocking, 0 non-blocking, 0 nits.
Both prior non-blocking findings — the dead resolveMatchComparison and the duplicated supportsSubtractForPlanner — are correctly and completely resolved in e44becab19e. The removals leave no dangling callers or orphaned imports, and reusing AsOfJoin.supportsSubtract is byte-identical to the old predicate, so operand-gating behavior is unchanged. LGTM.
Verification
Traced the delta against the accepted path: it touches no expression construction — materializeMatchComparison / normalizeMatchOperands bodies are unchanged — and the reused AsOfJoin.supportsSubtract has the same 7-type arm (numeric / datetime / interval) as the removed local copy, so the operand-support gate's boolean output is identical for every dataType. The reuse keeps the analysis gate coupled to exactly what buildLeafOrderExpression routes down the Subtract path (basicLogicalOperators.scala:2762) and what the planner's findFromOrder can consume (Subtract / If(_, Subtract, _), SparkStrategies.scala:517-524), removing the silent-drift hazard the prior review flagged.
### What changes were proposed in this pull request? Second stacked PR for [SPARK-58092](https://issues.apache.org/jira/browse/SPARK-58092) / [SPARK-58123](https://issues.apache.org/jira/browse/SPARK-58123). **Follows #57251** (parser, merged). Rebased onto current `master`; this PR is a **2-commit** analysis-only delta. **Analysis rule** - Add `ResolveAsOfJoin` to materialize `MATCH_CONDITION` into `asOfCondition`, `orderExpression`, and per-side sort expressions - Expand `USING (...)` into equi-join predicates via `NaturalAndUsingJoinResolution` - Validate MATCH_CONDITION operands: cross-side references, determinism, and type compatibility (including struct/array) **Logical plan** - Extend `AsOfJoin` with `leftSortExprs`, `rightSortExprs`, and `requiresSortMergeAsOfJoin` - Add `resolveMatchComparison` / `matchSortExpressions` helpers shared with the DataFrame API path - Resolve `matchLeftOperand` / `matchRightOperand` via the standard analyzer expression walk before materialization **Gating** - `CheckAnalysis` rejects SQL ASOF plans when `spark.sql.join.sortMergeAsOfJoin.enabled=false` (`AS_OF_JOIN.SORT_MERGE_REQUIRED`) **Out of scope (follow-up PR)** - Sort-merge physical operator wiring for multi-column sort keys and SQL execution tests — **#57277** ### Why are the changes needed? Parser (#57251) produces unresolved `AsOfJoin` nodes with parsed `MATCH_CONDITION` operands. Analysis must resolve operands, validate semantics, and produce the same logical shape the existing DataFrame `joinAsOf` path uses before physical planning can run. ### Does this PR introduce _any_ user-facing change? Yes (behind `spark.sql.join.asofJoin.enabled`, default off). With the parser conf enabled, SQL ASOF JOIN now progresses through analysis: valid queries become analyzable logical plans; invalid MATCH_CONDITION expressions fail with structured analysis errors instead of reaching execution unresolved. ### How was this patch tested? Added analysis negative tests to `AsOfJoinSQLSuite`: - `AS_OF_JOIN.SORT_MERGE_REQUIRED` - `ASOF_JOIN_MATCH_CONDITION_TABLE_REFERENCE` (cross-side and invalid refs) - `ASOF_JOIN_MATCH_CONDITION_INVALID_EXPRESSION` - `ASOF_JOIN_MATCH_CONDITION_INVALID_TYPE` Locally: ``` build/sbt "sql/testOnly org.apache.spark.sql.AsOfJoinSQLSuite" \ "catalyst/testOnly org.apache.spark.sql.catalyst.optimizer.RewriteAsOfJoinSuite" ``` ### Was this patch authored or co-authored using generative AI tooling? Co-authored with Cursor. Closes #57264 from srielau/SPARK-58123. Authored-by: Serge Rielau <serge@rielau.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com> (cherry picked from commit ea9d6a7) Signed-off-by: Wenchen Fan <wenchen@databricks.com>
### What changes were proposed in this pull request? Wires sort-merge physical execution for SQL ASOF JOIN on top of the parser (#57251) and analysis (#57264) work. **`SortMergeAsOfJoinExec`** - Sort by multi-column `leftSortExprs` / `rightSortExprs` from analysis (replacing single `leftAsOfExpr` / `rightAsOfExpr`) - Detect backward scan direction from `asOfCondition` for composite `MATCH_CONDITION` operands **Planner / optimizer** - `AsOfJoinSelection` and `RewriteAsOfJoin` use sort-merge when `requiresSortMergeAsOfJoin` is set (SQL path) or `spark.sql.join.sortMergeAsOfJoin.enabled` is on (DataFrame path) - Typed `case j: AsOfJoin` in `AsOfJoinSelection` (avoids brittle positional match) **Analysis / errors** - Remove the temporary `AS_OF_JOIN.UNSUPPORTED_MATCH_CONDITION_OPERAND` gate and its error condition - Remove `AS_OF_JOIN.SORT_MERGE_REQUIRED` — SQL ASOF JOIN no longer requires the DataFrame sort-merge conf - Stop populating DataFrame sort expressions in `AsOfJoin.apply()` while operands may still be unresolved **Tests** - Add `AsOfJoinSortMergeSQLSuite` for SQL execution only (scalar, struct, array, forward/strict, `USING`, left outer, etc.) - Keep analysis-negative `MATCH_CONDITION` cases in `AsOfJoinSQLSuite` only (removed duplicates from the execution suite) - Add parser tests for invalid `<>` / `<=>` operator text; rename `sqlAsofJoinDisabled` → `sqlAsOfJoinDisabled` ### Why are the changes needed? Analysis materializes per-side sort expressions for struct/tuple/array `MATCH_CONDITION` operands. The physical operator and planner must use those expressions for correct sort-merge ASOF semantics. ### Does this PR introduce _any_ user-facing change? Yes. With `spark.sql.join.asofJoin.enabled` (default off), valid SQL ASOF JOIN queries execute via `SortMergeAsOfJoinExec` — no second conf is required for SQL. `spark.sql.join.sortMergeAsOfJoin.enabled` remains the opt-in for DataFrame `joinAsOf`. ### How was this patch tested? ```bash build/sbt -Dscalastyle.skip=true \ "sql/testOnly org.apache.spark.sql.AsOfJoinSQLSuite \ org.apache.spark.sql.AsOfJoinSortMergeSQLSuite \ org.apache.spark.sql.SortMergeAsOfJoinSuite \ org.apache.spark.sql.DataFrameAsOfJoinSuite" build/sbt -Dscalastyle.skip=true "core/testOnly org.apache.spark.SparkThrowableSuite" ``` 32/32 in `AsOfJoinSQLSuite` + `AsOfJoinSortMergeSQLSuite`; `SparkThrowableSuite` clean. ### Was this patch authored or co-authored using generative AI tooling? Co-authored with Cursor. Closes #57277 from srielau/SPARK-58124. Authored-by: Serge Rielau <serge@rielau.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com>
### What changes were proposed in this pull request? Wires sort-merge physical execution for SQL ASOF JOIN on top of the parser (#57251) and analysis (#57264) work. **`SortMergeAsOfJoinExec`** - Sort by multi-column `leftSortExprs` / `rightSortExprs` from analysis (replacing single `leftAsOfExpr` / `rightAsOfExpr`) - Detect backward scan direction from `asOfCondition` for composite `MATCH_CONDITION` operands **Planner / optimizer** - `AsOfJoinSelection` and `RewriteAsOfJoin` use sort-merge when `requiresSortMergeAsOfJoin` is set (SQL path) or `spark.sql.join.sortMergeAsOfJoin.enabled` is on (DataFrame path) - Typed `case j: AsOfJoin` in `AsOfJoinSelection` (avoids brittle positional match) **Analysis / errors** - Remove the temporary `AS_OF_JOIN.UNSUPPORTED_MATCH_CONDITION_OPERAND` gate and its error condition - Remove `AS_OF_JOIN.SORT_MERGE_REQUIRED` — SQL ASOF JOIN no longer requires the DataFrame sort-merge conf - Stop populating DataFrame sort expressions in `AsOfJoin.apply()` while operands may still be unresolved **Tests** - Add `AsOfJoinSortMergeSQLSuite` for SQL execution only (scalar, struct, array, forward/strict, `USING`, left outer, etc.) - Keep analysis-negative `MATCH_CONDITION` cases in `AsOfJoinSQLSuite` only (removed duplicates from the execution suite) - Add parser tests for invalid `<>` / `<=>` operator text; rename `sqlAsofJoinDisabled` → `sqlAsOfJoinDisabled` ### Why are the changes needed? Analysis materializes per-side sort expressions for struct/tuple/array `MATCH_CONDITION` operands. The physical operator and planner must use those expressions for correct sort-merge ASOF semantics. ### Does this PR introduce _any_ user-facing change? Yes. With `spark.sql.join.asofJoin.enabled` (default off), valid SQL ASOF JOIN queries execute via `SortMergeAsOfJoinExec` — no second conf is required for SQL. `spark.sql.join.sortMergeAsOfJoin.enabled` remains the opt-in for DataFrame `joinAsOf`. ### How was this patch tested? ```bash build/sbt -Dscalastyle.skip=true \ "sql/testOnly org.apache.spark.sql.AsOfJoinSQLSuite \ org.apache.spark.sql.AsOfJoinSortMergeSQLSuite \ org.apache.spark.sql.SortMergeAsOfJoinSuite \ org.apache.spark.sql.DataFrameAsOfJoinSuite" build/sbt -Dscalastyle.skip=true "core/testOnly org.apache.spark.SparkThrowableSuite" ``` 32/32 in `AsOfJoinSQLSuite` + `AsOfJoinSortMergeSQLSuite`; `SparkThrowableSuite` clean. ### Was this patch authored or co-authored using generative AI tooling? Co-authored with Cursor. Closes #57277 from srielau/SPARK-58124. Authored-by: Serge Rielau <serge@rielau.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com> (cherry picked from commit bd511b5) Signed-off-by: Wenchen Fan <wenchen@databricks.com>
What changes were proposed in this pull request?
Second stacked PR for SPARK-58092 / SPARK-58123.
Follows #57251 (parser, merged). Rebased onto current
master; this PR is a 2-commit analysis-only delta.Analysis rule
ResolveAsOfJointo materializeMATCH_CONDITIONintoasOfCondition,orderExpression, and per-side sort expressionsUSING (...)into equi-join predicates viaNaturalAndUsingJoinResolutionLogical plan
AsOfJoinwithleftSortExprs,rightSortExprs, andrequiresSortMergeAsOfJoinresolveMatchComparison/matchSortExpressionshelpers shared with the DataFrame API pathmatchLeftOperand/matchRightOperandvia the standard analyzer expression walk before materializationGating
CheckAnalysisrejects SQL ASOF plans whenspark.sql.join.sortMergeAsOfJoin.enabled=false(AS_OF_JOIN.SORT_MERGE_REQUIRED)Out of scope (follow-up PR)
Why are the changes needed?
Parser (#57251) produces unresolved
AsOfJoinnodes with parsedMATCH_CONDITIONoperands. Analysis must resolve operands, validate semantics, and produce the same logical shape the existing DataFramejoinAsOfpath uses before physical planning can run.Does this PR introduce any user-facing change?
Yes (behind
spark.sql.join.asofJoin.enabled, default off). With the parser conf enabled, SQL ASOF JOIN now progresses through analysis: valid queries become analyzable logical plans; invalid MATCH_CONDITION expressions fail with structured analysis errors instead of reaching execution unresolved.How was this patch tested?
Added analysis negative tests to
AsOfJoinSQLSuite:AS_OF_JOIN.SORT_MERGE_REQUIREDASOF_JOIN_MATCH_CONDITION_TABLE_REFERENCE(cross-side and invalid refs)ASOF_JOIN_MATCH_CONDITION_INVALID_EXPRESSIONASOF_JOIN_MATCH_CONDITION_INVALID_TYPELocally:
Was this patch authored or co-authored using generative AI tooling?
Co-authored with Cursor.