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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
