dtenedor commented on code in PR #54729:
URL: https://github.com/apache/spark/pull/54729#discussion_r2941952659
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/resolver/ExpressionIdAssigner.scala:
##########
@@ -597,11 +700,126 @@ class ExpressionIdAssigner {
}
}
+ /**
+ * Insert the subquery expression id to the current mapping.
+ * Some optimization rules rely on subquery expression ids such as
[[mergeScalarSubqueries]].
+ * And some subquery expression id may be referenced in other operators.
(e.g.: [[Exists]]
+ * subquery expression id may be referenced as an attribute reference in
[[Filter]] condition).
+ * It is important to avoid duplicated subquery expression ids and add the
(oldId -> newId) into
+ * the current mapping to make sure later references is correct if the
subquery expression id is
+ * reassigned.
+ *
+ * There are also cases where the same subquery expression can appear again
in the same operator
+ * instead of being referenced as an [[AttributeReference]] in the latter
usage. For example,
+ * consider the following query:
+ * {{{
+ * select country,
+ * (select count(*) from data) as cnt,
+ * count(id) as cnt_id from data group by all
+ * }}}
+ * The logical plan for it is:
+ * {{{
+ * Aggregate [country#30, scalar-subquery#27 []],
+ * [country#30, scalar-subquery#27 [] AS cnt#28L, count(1) AS cnt_id#29L]
+ * :- Aggregate [count(1) AS count(1)#52L]
+ * : +- Project
+ * : +- LocalRelation [country#40, city#41, name#42, id#43, power#44]
+ * +- Aggregate [count(1) AS count(1)#52L]
+ * +- Project
+ * +- LocalRelation [country#53, city#54, name#55, id#56, power#57]
+ * +- Project [country#30]
+ * +- LocalRelation [country#30, city#31, name#32, id#33, power#34]
+ * }}}
+ * In this case, we reassign all the attributes within the subquery plan
together with the
+ * subquery expression itself to avoid duplicate expression ids.
+ */
+ def mapSubqueryExpression[SubqueryExpressionType <: SubqueryExpression](
+ subquery: SubqueryExpression,
+ alwaysUpdateExpressionId: Boolean = false): SubqueryExpressionType = {
+ if (mappingStack.peek().mapping.isEmpty) {
+ throw SparkException.internalError(
+ "Expression ID mapping doesn't exist. Please first call " +
Review Comment:
can we use a Scala multi-line string here to clean it up? Same for L787
below.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/resolver/AliasKind.scala:
##########
@@ -0,0 +1,35 @@
+/*
+ * 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.resolver
+
+/**
+ * Represents the kind of alias being processed.
Review Comment:
This comment pretty much paraphrases the object name -- can we put an
explanation of how it's used?
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/resolver/NameResolutionParameters.scala:
##########
@@ -0,0 +1,51 @@
+/*
+ * 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.resolver
+
+import org.apache.spark.sql.catalyst.expressions.Expression
+
+/**
+ * Parameters for name resolution in [[NameScope]] and [[NameScopeStack]].
Review Comment:
Thanks for describing the parameters explicitly -- let's also mention here
where this object is expected to be used?
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/resolver/NonDeterministicExpressionCheck.scala:
##########
@@ -0,0 +1,70 @@
+/*
+ * 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.resolver
+
+import org.apache.spark.sql.catalyst.analysis.AnalysisErrorAt
+import org.apache.spark.sql.catalyst.expressions.{Expression, ExprUtils}
+import org.apache.spark.sql.catalyst.plans.logical._
+
+/**
+ * A rule to validate that non-deterministic expressions only appear in
operators that allow them.
+ */
+object NonDeterministicExpressionCheck extends (LogicalPlan => Unit) {
+ def apply(plan: LogicalPlan): Unit = {
+ plan.foreachUp { operator =>
+ if (nonDeterministic(operator.expressions) &&
+ !operatorAllowsNonDeterministicExpressions(operator) &&
+ !operator.isInstanceOf[Project] &&
Review Comment:
Can we clean up all these `isInstanceOf` checks with a pattern match check
instead?
--
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]
