HeartSaVioR commented on code in PR #59910: URL: https://github.com/apache/spark/pull/59910#discussion_r4422630818
########## sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveExternalLookupJoin.scala: ########## @@ -0,0 +1,100 @@ +/* + * 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.catalyst.analysis + +import org.apache.spark.sql.catalyst.optimizer.EliminateResolvedHint +import org.apache.spark.sql.catalyst.plans.{Inner, LeftOuter} +import org.apache.spark.sql.catalyst.plans.logical.{ExternalLookupJoin, Join, JoinHint, + JoinStrategyHint, LogicalPlan} +import org.apache.spark.sql.catalyst.rules.Rule +import org.apache.spark.sql.catalyst.trees.TreePattern.JOIN +import org.apache.spark.sql.errors.{QueryCompilationErrors, QueryErrorsBase} +import org.apache.spark.sql.execution.datasources.v2.ExternalLookupRelations + +/** + * Replaces an eligible join with a lookup join holder. + * + * A join is planned as an external lookup join when its right side reads a table that supports + * external lookup but not batch read. It must be a stream-static join: the left side is a + * streaming query, whose micro-batches tolerate lookup results that change between requests, and + * the lookup-only table is on the right side. + */ +object ResolveExternalLookupJoin extends Rule[LogicalPlan] with QueryErrorsBase { + private val SupportedJoinTypes = Seq(Inner, LeftOuter) + + override def apply(plan: LogicalPlan): LogicalPlan = + plan.resolveOperatorsUpWithPruning(_.containsPattern(JOIN), ruleId) { + case join: Join if join.left.resolved && join.right.resolved => resolveLookupJoin(join) + } + + private def resolveLookupJoin(join: Join): LogicalPlan = { + val Join(left, right, joinType, condition, hint) = join + // The lookup-only table must be on the right side; the sides are never swapped. + ExternalLookupRelations.lookupOnlyTable(left).foreach { table => + throw QueryCompilationErrors.unsupportedExternalLookupJoinError( + "LOOKUP_TABLE_ON_LEFT", Map("relation" -> toSQLId(table.name()))) + } + // TODO: look through projections, filters and aliases above the lookup relation. + val lookupTable = ExternalLookupRelations.table(right) + // TODO: allow the EXTERNAL_LOOKUP hint once it is introduced, and apply these checks to the + // joins it opts in. + if (!lookupTable.exists(ExternalLookupRelations.isLookupOnly)) return join + val relation = lookupTable.map(table => toSQLId(table.name())).getOrElse("the lookup side") + if (!left.isStreaming) { + throw QueryCompilationErrors.unsupportedExternalLookupJoinError( + "STATIC_LEFT_SIDE", Map("relation" -> relation)) + } + // A join hint the user wrote is never silently dropped: an external lookup join cannot run a + // physical join strategy, and a lookup-only table has no batch scan for one. + joinStrategyHint(hint, left, right).foreach { strategy => + throw QueryCompilationErrors.unsupportedExternalLookupJoinError( + "CONFLICTING_JOIN_STRATEGY", + Map("strategy" -> toSQLStmt(strategy.displayName), "relation" -> relation)) + } + // A missing condition is reported by PrepareExternalLookupJoin, on the final plan. + if (condition.isEmpty) return join + // The join reads a lookup-only table that no other join can read, so a join that cannot be a + // lookup join fails here with the reason. + if (!SupportedJoinTypes.contains(joinType)) { + throw QueryCompilationErrors.unsupportedExternalLookupJoinError( + "JOIN_TYPE", + Map( + "joinType" -> toSQLStmt(joinType.sql), + "relation" -> relation, + "supportedJoinTypes" -> SupportedJoinTypes.map(t => toSQLStmt(t.sql)).mkString(", "))) + } + ExternalLookupJoin( Review Comment: > [!NOTE] > **DISCLAIMER:** This comment was written by an LLM, Claude Opus 5.5 (`claude-opus-5-5`, 1M context), posting on behalf of @HeartSaVioR. A follow-up to this thread. The risk you named in the last paragraph, that subqueries in the join condition reach the conversion before `RewritePredicateSubquery`, was missed in the earlier round. With the analyzed plan kept a `Join`, IN and EXISTS subqueries in the condition pass analysis. The optimizer rewrites them, and scalar subqueries in join conditions, only in a `Join`, but by then the join has become an external lookup join, so the subquery would reach the residual unevaluated. A reference-free scalar subquery could even be offered as a lookup parameter. In [ed83fc0f2fd](https://github.com/apache/spark/commit/ed83fc0f2fdc0595ae7cdf23b32159f841872816), a lookup join whose condition contains any subquery is rejected with the new `EXTERNAL_LOOKUP_JOIN_UNSUPPORTED.SUBQUERY_IN_JOIN_CONDITION` error when the DataFrame is analyzed. The same classification also rejects it during the conversion. New tests: an IN subquery, an EXISTS subquery and a scalar subquery in the condition each fail with this error when the DataFrame is built. ########## sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveExternalLookupJoin.scala: ########## @@ -0,0 +1,100 @@ +/* + * 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.catalyst.analysis + +import org.apache.spark.sql.catalyst.optimizer.EliminateResolvedHint +import org.apache.spark.sql.catalyst.plans.{Inner, LeftOuter} +import org.apache.spark.sql.catalyst.plans.logical.{ExternalLookupJoin, Join, JoinHint, + JoinStrategyHint, LogicalPlan} +import org.apache.spark.sql.catalyst.rules.Rule +import org.apache.spark.sql.catalyst.trees.TreePattern.JOIN +import org.apache.spark.sql.errors.{QueryCompilationErrors, QueryErrorsBase} +import org.apache.spark.sql.execution.datasources.v2.ExternalLookupRelations + +/** + * Replaces an eligible join with a lookup join holder. + * + * A join is planned as an external lookup join when its right side reads a table that supports + * external lookup but not batch read. It must be a stream-static join: the left side is a + * streaming query, whose micro-batches tolerate lookup results that change between requests, and + * the lookup-only table is on the right side. + */ +object ResolveExternalLookupJoin extends Rule[LogicalPlan] with QueryErrorsBase { + private val SupportedJoinTypes = Seq(Inner, LeftOuter) + + override def apply(plan: LogicalPlan): LogicalPlan = + plan.resolveOperatorsUpWithPruning(_.containsPattern(JOIN), ruleId) { + case join: Join if join.left.resolved && join.right.resolved => resolveLookupJoin(join) + } + + private def resolveLookupJoin(join: Join): LogicalPlan = { + val Join(left, right, joinType, condition, hint) = join + // The lookup-only table must be on the right side; the sides are never swapped. + ExternalLookupRelations.lookupOnlyTable(left).foreach { table => + throw QueryCompilationErrors.unsupportedExternalLookupJoinError( + "LOOKUP_TABLE_ON_LEFT", Map("relation" -> toSQLId(table.name()))) + } + // TODO: look through projections, filters and aliases above the lookup relation. + val lookupTable = ExternalLookupRelations.table(right) + // TODO: allow the EXTERNAL_LOOKUP hint once it is introduced, and apply these checks to the + // joins it opts in. + if (!lookupTable.exists(ExternalLookupRelations.isLookupOnly)) return join + val relation = lookupTable.map(table => toSQLId(table.name())).getOrElse("the lookup side") + if (!left.isStreaming) { + throw QueryCompilationErrors.unsupportedExternalLookupJoinError( + "STATIC_LEFT_SIDE", Map("relation" -> relation)) + } + // A join hint the user wrote is never silently dropped: an external lookup join cannot run a + // physical join strategy, and a lookup-only table has no batch scan for one. + joinStrategyHint(hint, left, right).foreach { strategy => + throw QueryCompilationErrors.unsupportedExternalLookupJoinError( + "CONFLICTING_JOIN_STRATEGY", + Map("strategy" -> toSQLStmt(strategy.displayName), "relation" -> relation)) + } + // A missing condition is reported by PrepareExternalLookupJoin, on the final plan. + if (condition.isEmpty) return join + // The join reads a lookup-only table that no other join can read, so a join that cannot be a + // lookup join fails here with the reason. + if (!SupportedJoinTypes.contains(joinType)) { + throw QueryCompilationErrors.unsupportedExternalLookupJoinError( + "JOIN_TYPE", + Map( + "joinType" -> toSQLStmt(joinType.sql), + "relation" -> relation, + "supportedJoinTypes" -> SupportedJoinTypes.map(t => toSQLStmt(t.sql)).mkString(", "))) + } + ExternalLookupJoin( Review Comment: > [!NOTE] > **DISCLAIMER:** This comment was written by an LLM, Claude Opus 5.5 (`claude-opus-5-5`, 1M context), posting on behalf of @HeartSaVioR. One more follow-up on the same theme: a plan-shape dependence in the condition. `RewriteWithExpression` runs after the conversion, and for a common expression that is not cheap, such as the input of `upper(t.department) BETWEEN 'A' AND 'Z'`, it evaluates the expression in a projection below the join. On the lookup side, that projection leaves no lookup relation for the join to read, so the query passed analysis and then failed with `LOOKUP_PLAN` when the micro-batch was planned. A cheap input, such as a bare column, was inlined and ran. In [ed83fc0f2fd](https://github.com/apache/spark/commit/ed83fc0f2fdc0595ae7cdf23b32159f841872816), `FormExternalLookupJoin` inlines the common expressions of every `With` in the condition when it forms the join, so no projection is ever placed below it. A definition may be evaluated more than once, which is safe because analysis allows only deterministic join conditions. `RewriteWithExpression` itself is unchanged. New test: a non-cheap `BETWEEN` over a lookup column and one over a stream column both run as lookup joins with the expected rows. I also checked the rules that run between the conversion and `PrepareExternalLookupJoin` for any other that puts an operator below the lookup side. `ConvertToCatalyst` widens only single-child operators, and the remaining join rules match `Join` only. ########## sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/PrepareExternalLookupJoin.scala: ########## @@ -0,0 +1,233 @@ +/* + * 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.execution.datasources.v2 + +import java.util.{Collections, IdentityHashMap} + +import scala.collection.mutable + +import org.apache.spark.sql.catalyst.expressions.{Alias, And, Attribute, EqualTo, Expression, + Literal, PredicateHelper} +import org.apache.spark.sql.catalyst.plans.LeftOuter +import org.apache.spark.sql.catalyst.plans.logical.{ExternalLookupJoin, Filter, Join, LocalRelation, + LogicalPlan, PreparedExternalLookupJoin, Project} +import org.apache.spark.sql.catalyst.rules.Rule +import org.apache.spark.sql.catalyst.trees.TreePattern.EXTERNAL_LOOKUP_JOIN +import org.apache.spark.sql.catalyst.util.CharVarcharUtils +import org.apache.spark.sql.connector.catalog.{SupportsRead, TableCapability} +import org.apache.spark.sql.connector.expressions.{Expression => V2Expression, FieldReference} +import org.apache.spark.sql.connector.expressions.filter.{Predicate => V2Predicate} +import org.apache.spark.sql.connector.read.{LookupParameter, SupportsExternalLookup} +import org.apache.spark.sql.errors.{QueryCompilationErrors, QueryErrorsBase} +import org.apache.spark.sql.internal.SQLConf +import org.apache.spark.sql.types.{BooleanType, DataType} + +/** Finalizes an optimized external lookup side immediately before ordinary V2 scan preparation. */ +object PrepareExternalLookupJoin extends Rule[LogicalPlan] with PredicateHelper + with QueryErrorsBase { + override def apply(plan: LogicalPlan): LogicalPlan = { + val prepared = plan.transformUpWithPruning(_.containsPattern(EXTERNAL_LOOKUP_JOIN)) { + case join: ExternalLookupJoin if provablyEmpty(join) => emptyJoinResult(join) + case join: ExternalLookupJoin => prepare(join) + } + // Prepared lookup joins no longer contain their lookup relation, so a lookup-only table that + // is still read here cannot run. Pre-order visits a join before the relation it reads. + prepared.foreach { + // Classic Dataset.join(right, usingColumns) first analyzes Join(left, right, joinType, None), + // including CheckAnalysis, to disambiguate a self-join, and only then builds the USING join. + // That plan cannot be told apart from a join without a condition, so analysis keeps it, and + // a missing condition is reported here, on the final plan. + case Join(_, right, _, None, _) if readsLookupOnlyTable(right) => + throw QueryCompilationErrors.unsupportedExternalLookupJoinError("MISSING_JOIN_CONDITION") + case node => + ExternalLookupRelations.lookupOnlyTable(node).foreach { table => + throw QueryCompilationErrors.unsupportedBatchReadError(table) + } + } + prepared + } + + private def readsLookupOnlyTable(plan: LogicalPlan): Boolean = plan match { + case node @ (_: Project | _: Filter) => readsLookupOnlyTable(node.children.head) + case node => ExternalLookupRelations.lookupOnlyTable(node).isDefined + } + + // The optimizer folds `WHERE false` on the lookup side into an empty relation, and a condition + // such as `s.id = r.id AND false` into `false`. Such a join needs no connector call. + private def provablyEmpty(join: ExternalLookupJoin): Boolean = + join.right.maxRows.contains(0L) || (join.condition match { + case Some(Literal(value, BooleanType)) => value == null || value == false + case _ => false + }) + + private def emptyJoinResult(join: ExternalLookupJoin): LogicalPlan = join.joinType match { + case LeftOuter => + val nullLookupColumns = join.right.output.map { attr => + Alias(Literal(null, attr.dataType), attr.name)(attr.exprId) + } + Project(join.left.output ++ nullLookupColumns, join.left) + case _ => LocalRelation(join.output, Seq.empty, join.isStreaming) + } + + private def prepare(join: ExternalLookupJoin): PreparedExternalLookupJoin = { + val relation = extractLookupPlan(join.right).getOrElse { + throw QueryCompilationErrors.unsupportedExternalLookupJoinError("LOOKUP_PLAN") + } + val tableName = relation.table.name() + val supportsRead = relation.table match { + case table: SupportsRead + if table.capabilities().contains(TableCapability.EXTERNAL_LOOKUP) => table + case _ => + throw QueryCompilationErrors.unsupportedExternalLookupJoinError( + "TABLE", Map("table" -> toSQLId(tableName))) + } + val scanBuilder = supportsRead.newScanBuilder(relation.options) + val lookupBuilder = scanBuilder match { + case builder: SupportsExternalLookup => builder + case _ => + throw QueryCompilationErrors.unsupportedExternalLookupJoinError( + "SCAN_BUILDER", Map("table" -> toSQLId(tableName))) + } + + val conjuncts = join.condition.toSeq.flatMap(splitConjunctivePredicates) + val offeredCandidates = mutable.ArrayBuffer.empty[OfferedLookupPredicate] + val residual = mutable.ArrayBuffer.empty[Expression] + conjuncts.foreach { + case equality @ EqualTo(leftExpr, rightAttr: Attribute) + if leftExpr.references.subsetOf(join.left.outputSet) && + join.right.outputSet.contains(rightAttr) => + offeredCandidates += OfferedLookupPredicate(leftExpr, rightAttr, equality, "=", false) + case equality @ EqualTo(rightAttr: Attribute, leftExpr) + if leftExpr.references.subsetOf(join.left.outputSet) && + join.right.outputSet.contains(rightAttr) => + offeredCandidates += OfferedLookupPredicate(leftExpr, rightAttr, equality, "=", false) + case other => residual += other Review Comment: > [!NOTE] > **DISCLAIMER:** This comment was written by an LLM, Claude Opus 5.5 (`claude-opus-5-5`, 1M context), posting on behalf of @HeartSaVioR. A follow-up: external UDFs had the same gap. `PlanExternalUDFs` extracts them from a join condition only for a `Join`. In [ed83fc0f2fd](https://github.com/apache/spark/commit/ed83fc0f2fdc0595ae7cdf23b32159f841872816), an external UDF over lookup columns gets the same treatment as a Python UDF. In an inner join it is evaluated in a `Filter` above the prepared join, where `PlanExternalUDFs` plans it, and a left outer join fails during planning with `UNSUPPORTED_FEATURE.EXTERNAL_UDF_IN_ON_CLAUSE`. External UDFs need a static conf, so the new test is plan-level: it checks both join types and that `PlanExternalUDFs` plans the filter. -- 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]
