You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "srielau (via GitHub)" <gi...@apache.org> on 2026/07/14 16:05:39 UTC

[PR] [SPARK-58122][SQL] Add SQL ASOF JOIN parser and error conditions [spark]

srielau opened a new pull request, #57251:
URL: https://github.com/apache/spark/pull/57251

   ## What changes were proposed in this pull request?
   
   First stacked PR for [SPARK-58092](https://issues.apache.org/jira/browse/SPARK-58092) / [SPARK-58122](https://issues.apache.org/jira/browse/SPARK-58122): SQL surface area for ASOF JOIN only (parser, keywords, errors, feature flag, minimal logical plan).
   
   ### In scope
   - ANTLR grammar: `ASOF JOIN ... MATCH_CONDITION (left_expr op right_expr) [ON ... | USING ...]`
   - Keywords `ASOF` and `MATCH_CONDITION` (lexer, docs, golden files, JDBC metadata test)
   - `spark.sql.join.asofJoin.enabled` conf (default off)
   - Parse-time errors for disabled feature and invalid MATCH_CONDITION operators (`=`, `AND`, `OR`)
   - Minimal `AsOfJoin` extensions (`usingColumns`, `matchComparison`, `fromMatchCondition`) — plan stays unresolved until analysis lands
   - Tests: `PlanParserSuite` (asof join), `AsOfJoinSQLSuite` (parse error)
   
   ### Out of scope (follow-up PRs)
   - `ResolveAsOfJoin` / analysis validation
   - Sort-merge execution (`SortMergeAsOfJoinExec`, planner)
   - `spark.sql.join.sortMergeAsOfJoin.enabled`
   - Integration tests (`AsOfJoinSortMergeSQLSuite`)
   
   ## Why is this needed?
   
   Splits the large ASOF JOIN change into reviewable pieces. This PR is parser-only and should be zero user-visible behavior when the conf is off (default).
   
   ## Does this PR introduce any user-facing change?
   
   No, when `spark.sql.join.asofJoin.enabled` is false (default). When enabled, SQL parses to an unresolved `AsOfJoin` logical node; execution is not wired yet.
   
   ## How was this patch tested?
   
   ```
   build/sbt "catalyst/testOnly org.apache.spark.sql.catalyst.parser.PlanParserSuite -- -z \"asof join\"" "sql/testOnly org.apache.spark.sql.AsOfJoinSQLSuite"
   ```
   
   ## Was this patch authored or co-authored using generative AI tooling?
   
   Co-authored with Cursor.


-- 
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: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

Re: [PR] [SPARK-58122][SQL] Add SQL ASOF JOIN parser and error conditions [spark]

Posted by "srielau (via GitHub)" <gi...@apache.org>.
srielau commented on PR #57251:
URL: https://github.com/apache/spark/pull/57251#issuecomment-4981074845

   Thanks @cloud-fan for the review — addressed in d6e12fa09d4:
   
   - **Blocking**: replaced `AsOfMatchCondition` with `matchLeftOperand` / `matchOperator` / `matchRightOperand` on `AsOfJoin` (no custom `mapExpressions` / `treePatternBits` overrides). Stacked analysis PR #57264 is rebased on top.
   - **Suggestions**: `AstBuilder` uses `sqlAsOfJoinEnabled`; unused error conditions are wired in #57264.
   - **Nits**: forward-reference scaladoc links fixed/removed.
   - **PR description**: added SQL vs DataFrame API asymmetry (tolerance / `Nearest`).


-- 
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: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

Re: [PR] [SPARK-58122][SQL] Add SQL ASOF JOIN parser and error conditions [spark]

Posted by "dtenedor (via GitHub)" <gi...@apache.org>.
dtenedor closed pull request #57251: [SPARK-58122][SQL] Add SQL ASOF JOIN parser and error conditions
URL: https://github.com/apache/spark/pull/57251


-- 
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: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

Re: [PR] [SPARK-58122][SQL] Add SQL ASOF JOIN parser and error conditions [spark]

Posted by "srielau (via GitHub)" <gi...@apache.org>.
srielau commented on code in PR #57251:
URL: https://github.com/apache/spark/pull/57251#discussion_r3587599464


##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/basicLogicalOperators.scala:
##########
@@ -2533,7 +2536,10 @@ case class AsOfJoin(
     condition: Option[Expression],
     joinType: JoinType,
     orderExpression: Expression,
-    toleranceAssertion: Option[Expression]) extends BinaryNode {
+    toleranceAssertion: Option[Expression],
+    usingColumns: Option[Seq[String]] = None,
+    matchComparison: Option[AsOfMatchCondition] = None)

Review Comment:
   Agreed — switched to plain `Expression` operand fields plus `matchOperator` on `AsOfJoin` in d6e12fa09d4. That drops the hand-rolled `mapExpressions` / `treePatternBits` overrides and lets `references` / `expressions` see the parsed operands. Follow-up analysis PR #57264 is rebased on top.



##########
sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala:
##########
@@ -8466,6 +8475,8 @@ class SQLConf extends Serializable with Logging with SqlApiConf {
 
   def sortMergeAsOfJoinEnabled: Boolean = getConf(SORT_MERGE_AS_OF_JOIN_ENABLED)
 
+  def sqlAsOfJoinEnabled: Boolean = getConf(SQL_ASOF_JOIN_ENABLED)

Review Comment:
   Done in d6e12fa09d4: `AstBuilder.withAsOfJoin` now uses `conf.sqlAsOfJoinEnabled`.



##########
common/utils/src/main/resources/error/error-conditions.json:
##########
@@ -156,6 +156,30 @@
     ],
     "sqlState" : "42713"
   },
+  "ASOF_JOIN_MATCH_CONDITION_INVALID_EXPRESSION" : {

Review Comment:
   Those conditions are exercised in the stacked analysis PR #57264 (`ResolveAsOfJoin`, `CheckAnalysis` for `AS_OF_JOIN.SORT_MERGE_REQUIRED`). I kept them here so the parser/analysis error surface lands together; happy to split if you prefer one condition per PR.



##########
sql/core/src/test/scala/org/apache/spark/sql/AsOfJoinSQLSuite.scala:
##########
@@ -0,0 +1,72 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *    http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark.sql
+
+import org.apache.spark.sql.catalyst.parser.ParseException
+import org.apache.spark.sql.internal.SQLConf
+import org.apache.spark.sql.test.SharedSparkSession
+
+/**
+ * SQL ASOF JOIN surface tests (parser and feature gating).
+ * Execution semantics and complex MATCH_CONDITION types are covered by
+ * [[AsOfJoinSortMergeSQLSuite]], which requires sort-merge ASOF join.

Review Comment:
   Fixed in d6e12fa09d4 — forward reference is now backticks.



##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/basicLogicalOperators.scala:
##########
@@ -2564,6 +2584,27 @@ case class AsOfJoin(
 
   final override val nodePatterns: Seq[TreePattern] = Seq(AS_OF_JOIN)
 
+  /**
+   * [[AsOfMatchCondition]] is not an [[Expression]], so the default [[mapExpressions]] path does
+   * not rewrite its operand trees. Analysis rules must still reach those operands once
+   * [[org.apache.spark.sql.catalyst.analysis.ResolveAsOfJoin]] is introduced.

Review Comment:
   Resolved by removing the forward-reference scaladoc block entirely once the operands became plain `Expression` fields (no special `mapExpressions` note needed).



-- 
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: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

Re: [PR] [SPARK-58122][SQL] Add SQL ASOF JOIN parser and error conditions [spark]

Posted by "dtenedor (via GitHub)" <gi...@apache.org>.
dtenedor commented on PR #57251:
URL: https://github.com/apache/spark/pull/57251#issuecomment-4982619401

   **Merge Summary:**
   - merged into master https://github.com/apache/spark/commit/84bb88952f33955deb1aa6bbd4dff729973452f2
   - merged into branch-4.x https://github.com/apache/spark/commit/7fbcdbc3d93ba7a9c38cb86eba89146c59e481b0
   
   *Posted by `merge_spark_pr.py`*


-- 
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: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

Re: [PR] [SPARK-58122][SQL] Add SQL ASOF JOIN parser and error conditions [spark]

Posted by "srielau (via GitHub)" <gi...@apache.org>.
srielau commented on PR #57251:
URL: https://github.com/apache/spark/pull/57251#issuecomment-4972788258

   @sarutak First drop of ASOF join syntax for review


-- 
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: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

Re: [PR] [SPARK-58122][SQL] Add SQL ASOF JOIN parser and error conditions [spark]

Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #57251:
URL: https://github.com/apache/spark/pull/57251#discussion_r3584142349


##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/basicLogicalOperators.scala:
##########
@@ -2533,7 +2536,10 @@ case class AsOfJoin(
     condition: Option[Expression],
     joinType: JoinType,
     orderExpression: Expression,
-    toleranceAssertion: Option[Expression]) extends BinaryNode {
+    toleranceAssertion: Option[Expression],
+    usingColumns: Option[Seq[String]] = None,
+    matchComparison: Option[AsOfMatchCondition] = None)

Review Comment:
   The match operands are wrapped in `AsOfMatchCondition`, a case class that is **not** an `Expression`. Because of that, `QueryPlan.expressions` skips it, so the two operands are absent from `AsOfJoin.references` — any rule keying on `references`/`expressions` (column pruning, constraint propagation, `missingInput` accounting) silently under-counts the attributes the match condition actually reads. It also forces the hand-rolled `mapExpressions` (below) and `treePatternBits` overrides, which have to be kept in sync by hand.
   
   The sibling join `NearestByJoin` stores its per-pair `rankingExpression` as a plain `Expression` field and needs zero such overrides, and this class's own `asOfCondition`/`orderExpression` are already plain `Expression` fields that survive rewriting fine. The in-code rationale ("must be a case class ... so expression rewriting does not destroy the shape", `joinTypes.scala`) doesn't actually require a non-`Expression` type — plain `Expression` operand fields plus a plain `MatchComparisonOperator` field would give correct `references`, drop both overrides, and match the peer.
   
   Is there a reason the operands can't be plain `Expression` fields here? If not, that seems like the simpler representation, and worth settling now while this drop is exactly what reviewers are signing off on.



##########
sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala:
##########
@@ -8466,6 +8475,8 @@ class SQLConf extends Serializable with Logging with SqlApiConf {
 
   def sortMergeAsOfJoinEnabled: Boolean = getConf(SORT_MERGE_AS_OF_JOIN_ENABLED)
 
+  def sqlAsOfJoinEnabled: Boolean = getConf(SQL_ASOF_JOIN_ENABLED)

Review Comment:
   This accessor has no callers — `AstBuilder` reads the conf via `conf.getConf(SQLConf.SQL_ASOF_JOIN_ENABLED)` directly. Either use it there for consistency with the other `def`-style accessors, or defer it to the PR that needs it.



##########
sql/core/src/test/scala/org/apache/spark/sql/AsOfJoinSQLSuite.scala:
##########
@@ -0,0 +1,72 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *    http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark.sql
+
+import org.apache.spark.sql.catalyst.parser.ParseException
+import org.apache.spark.sql.internal.SQLConf
+import org.apache.spark.sql.test.SharedSparkSession
+
+/**
+ * SQL ASOF JOIN surface tests (parser and feature gating).
+ * Execution semantics and complex MATCH_CONDITION types are covered by
+ * [[AsOfJoinSortMergeSQLSuite]], which requires sort-merge ASOF join.

Review Comment:
   `[[AsOfJoinSortMergeSQLSuite]]` doesn't resolve — no such class exists yet (it's follow-up work; the existing sort-merge suite is `SortMergeAsOfJoinSuite`). Scaladoc `[[..]]` links are expected to resolve, so this will surface as a broken-link warning. Use backticks for the forward reference:
   ```suggestion
    * `AsOfJoinSortMergeSQLSuite`, which requires sort-merge ASOF join.
   ```



##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/basicLogicalOperators.scala:
##########
@@ -2564,6 +2584,27 @@ case class AsOfJoin(
 
   final override val nodePatterns: Seq[TreePattern] = Seq(AS_OF_JOIN)
 
+  /**
+   * [[AsOfMatchCondition]] is not an [[Expression]], so the default [[mapExpressions]] path does
+   * not rewrite its operand trees. Analysis rules must still reach those operands once
+   * [[org.apache.spark.sql.catalyst.analysis.ResolveAsOfJoin]] is introduced.

Review Comment:
   `[[org.apache.spark.sql.catalyst.analysis.ResolveAsOfJoin]]` doesn't resolve — the rule doesn't exist yet (follow-up). Same broken-`[[..]]`-link issue as in `AsOfJoinSQLSuite`; drop it to backticks for the forward reference:
   ```suggestion
      * `ResolveAsOfJoin` is introduced.
   ```



##########
common/utils/src/main/resources/error/error-conditions.json:
##########
@@ -156,6 +156,30 @@
     ],
     "sqlState" : "42713"
   },
+  "ASOF_JOIN_MATCH_CONDITION_INVALID_EXPRESSION" : {

Review Comment:
   `ASOF_JOIN_MATCH_CONDITION_INVALID_EXPRESSION`, `ASOF_JOIN_MATCH_CONDITION_INVALID_TYPE`, `ASOF_JOIN_MATCH_CONDITION_TABLE_REFERENCE`, and `AS_OF_JOIN.SORT_MERGE_REQUIRED` are defined here but unreferenced anywhere in this PR — they map to the follow-up analysis/execution work. Nothing in CI fails on an orphaned error class, so this isn't a break, but consider landing each condition with its first user so the error catalog stays self-contained per PR.



-- 
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: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org