mihailotim-db commented on code in PR #50285:
URL: https://github.com/apache/spark/pull/50285#discussion_r2051423832
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/FunctionTableSubqueryArgumentExpression.scala:
##########
@@ -86,11 +88,20 @@ case class FunctionTableSubqueryArgumentExpression(
override def hint: Option[HintInfo] = None
override def withNewHint(hint: Option[HintInfo]):
FunctionTableSubqueryArgumentExpression =
copy()
+ override def withNewNestedOuterAttrs(
+ nestedOuterAttrs: Seq[Expression]
+ ): FunctionTableSubqueryArgumentExpression = {
+ assert(nestedOuterAttrs.toSet.subsetOf(outerAttrs.toSet),
+ s"nestedOuterAttrs must be a subset of outerAttrs, " +
+ s"but got ${nestedOuterAttrs.mkString(", ")}")
+ copy(nestedOuterAttrs = nestedOuterAttrs)
+ }
Review Comment:
We can have this as a default implementation and then override for
`DynamicPruningSubquery`
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/FunctionTableSubqueryArgumentExpression.scala:
##########
@@ -86,11 +88,20 @@ case class FunctionTableSubqueryArgumentExpression(
override def hint: Option[HintInfo] = None
override def withNewHint(hint: Option[HintInfo]):
FunctionTableSubqueryArgumentExpression =
copy()
+ override def withNewNestedOuterAttrs(
+ nestedOuterAttrs: Seq[Expression]
+ ): FunctionTableSubqueryArgumentExpression = {
+ assert(nestedOuterAttrs.toSet.subsetOf(outerAttrs.toSet),
+ s"nestedOuterAttrs must be a subset of outerAttrs, " +
+ s"but got ${nestedOuterAttrs.mkString(", ")}")
+ copy(nestedOuterAttrs = nestedOuterAttrs)
+ }
Review Comment:
```suggestion
override def withNewNestedOuterAttrs(
nestedOuterAttrs: Seq[Expression]
): FunctionTableSubqueryArgumentExpression = {
assert(nestedOuterAttrs.toSet.subsetOf(outerAttrs.toSet),
s"nestedOuterAttrs must be a subset of outerAttrs, " +
s"but got ${nestedOuterAttrs.mkString(", ")}")
copy(nestedOuterAttrs = nestedOuterAttrs)
}
```
Please add new lines
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/FunctionTableSubqueryArgumentExpression.scala:
##########
@@ -67,12 +67,14 @@ import org.apache.spark.sql.types.DataType
case class FunctionTableSubqueryArgumentExpression(
plan: LogicalPlan,
outerAttrs: Seq[Expression] = Seq.empty,
+ nestedOuterAttrs: Seq[Expression] = Seq.empty,
Review Comment:
Please update the scaladoc for this case class
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/DynamicPruning.scala:
##########
@@ -67,6 +68,17 @@ case class DynamicPruningSubquery(
copy()
}
+ override def withNewNestedOuterAttrs(
+ nestedOuterAttrs: Seq[Expression]
+ ): DynamicPruningSubquery = {
+ // DynamicPruningSubquery should not have nested outer attrs
Review Comment:
Can we move this comment to the scaladoc?
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/FunctionTableSubqueryArgumentExpression.scala:
##########
@@ -67,12 +67,14 @@ import org.apache.spark.sql.types.DataType
case class FunctionTableSubqueryArgumentExpression(
plan: LogicalPlan,
outerAttrs: Seq[Expression] = Seq.empty,
+ nestedOuterAttrs: Seq[Expression] = Seq.empty,
exprId: ExprId = NamedExpression.newExprId,
partitionByExpressions: Seq[Expression] = Seq.empty,
withSinglePartition: Boolean = false,
orderByExpressions: Seq[SortOrder] = Seq.empty,
selectedInputExpressions: Seq[PythonUDTFSelectedExpression] = Seq.empty)
- extends SubqueryExpression(plan, outerAttrs, exprId, Seq.empty, None) with
Unevaluable {
+ extends SubqueryExpression(
+ plan, outerAttrs, nestedOuterAttrs, exprId, Seq.empty, None) with
Unevaluable {
Review Comment:
Can you check the formatting here?
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/subquery.scala:
##########
@@ -67,6 +67,8 @@ abstract class PlanExpression[T <: QueryPlan[_]] extends
Expression {
*
* @param plan: the subquery plan
* @param outerAttrs: the outer references in the subquery plan
+ * @param nestedOuterAttrs: the outer references in the subquery plan that
cannot be resolved
Review Comment:
Not sure about the name here. Nested implies it's below the current node in
the plan, not outside of it
--
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-unsubscr...@spark.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
