You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "mihailoale-db (via GitHub)" <gi...@apache.org> on 2025/05/06 15:22:33 UTC
[PR] [SPARK-52019] [SQL] Strip outer reference before creating a name in toPrettySQL [spark]
mihailoale-db opened a new pull request, #50804:
URL: https://github.com/apache/spark/pull/50804
### What changes were proposed in this pull request?
In the following query we would have `min(outer(t2.t2a))` as a name for `min(t2a)` expression.
```
SELECT t1a
FROM t1
WHERE t1a IN (SELECT t2a
FROM t2
WHERE EXISTS (SELECT min(t2a)
FROM t3))
```
This is a problem in compatibility between single-pass resolver and fixed-point analyzer because names in single-pass are generated after we finish resolution of aggregate expression `min(t2a)` (bottom-up manner) and at that point we have `OuterReference` wrapped around aggregate expression (name looks like `outer(min(t2a))`).
I propose that we fix it in fixed point so we can compute the name in single-pass more easily.
### Why are the changes needed?
To ease development of single-pass analyzer.
### Does this PR introduce _any_ user-facing change?
`Explain extended` of affected plans would be different.
### How was this patch tested?
Existing tests (regenerated golden files).
### Was this patch authored or co-authored using generative AI tooling?
No.
--
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-52019] [SQL] Strip outer reference before creating a name in toPrettySQL [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #50804:
URL: https://github.com/apache/spark/pull/50804#discussion_r2078186222
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/package.scala:
##########
@@ -122,7 +144,8 @@ package object util extends Logging {
QuotingUtils.quoteIfNeeded(part)
}
- def toPrettySQL(e: Expression): String = usePrettyExpression(e).sql
+ def toPrettySQL(e: Expression, stripOuterReference: Boolean = true): String =
Review Comment:
where do we set `stripOuterReference` to false?
--
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-52019] [SQL] Strip outer reference before creating a name in toPrettySQL [spark]
Posted by "vladimirg-db (via GitHub)" <gi...@apache.org>.
vladimirg-db commented on code in PR #50804:
URL: https://github.com/apache/spark/pull/50804#discussion_r2077114892
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/package.scala:
##########
@@ -93,22 +94,37 @@ package object util extends Logging {
// Replaces attributes, string literals, complex type extractors with their pretty form so that
// generated column names don't contain back-ticks or double-quotes.
- def usePrettyExpression(e: Expression): Expression = e transform {
- case a: Attribute => new PrettyAttribute(a)
- case Literal(s: UTF8String, StringType) => PrettyAttribute(s.toString, StringType)
- case Literal(v, t: NumericType) if v != null => PrettyAttribute(v.toString, t)
- case Literal(null, dataType) => PrettyAttribute("NULL", dataType)
- case e: GetStructField =>
- val name = e.name.getOrElse(e.childSchema(e.ordinal).name)
- PrettyAttribute(usePrettyExpression(e.child).sql + "." + name, e.dataType)
- case e: GetArrayStructFields =>
- PrettyAttribute(s"${usePrettyExpression(e.child)}.${e.field.name}", e.dataType)
- case r: InheritAnalysisRules =>
- PrettyAttribute(r.makeSQLString(r.parameters.map(toPrettySQL)), r.dataType)
- case c: Cast if c.getTagValue(Cast.USER_SPECIFIED_CAST).isEmpty =>
- PrettyAttribute(usePrettyExpression(c.child).sql, c.dataType)
- case p: PythonFuncExpression => PrettyPythonUDF(p.name, p.dataType, p.children)
- }
+ private def usePrettyExpression(e: Expression, stripOuterReference: Boolean = true): Expression =
+ e transform {
+ case aggregateExpression: AggregateExpression
Review Comment:
Let's improve the scaladoc by explaining this specific logic. Also, let's make the scaladoc like this: `/** ... */`
--
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-52019] [SQL] Strip outer reference before creating a name in toPrettySQL [spark]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on PR #50804:
URL: https://github.com/apache/spark/pull/50804#issuecomment-2858762745
+1, LGTM. Merging to master.
Thank you, @mihailoale-db and @vladimirg-db 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-52019] [SQL] Strip outer reference before creating a name in toPrettySQL [spark]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk closed pull request #50804: [SPARK-52019] [SQL] Strip outer reference before creating a name in toPrettySQL
URL: https://github.com/apache/spark/pull/50804
--
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-52019] [SQL] Strip outer reference before creating a name in toPrettySQL [spark]
Posted by "mihailoale-db (via GitHub)" <gi...@apache.org>.
mihailoale-db commented on PR #50804:
URL: https://github.com/apache/spark/pull/50804#issuecomment-2858104016
@cloud-fan PTAL when you have time (all tests passed). Thanks!
--
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-52019] [SQL] Strip outer reference before creating a name in toPrettySQL [spark]
Posted by "mihailoale-db (via GitHub)" <gi...@apache.org>.
mihailoale-db commented on code in PR #50804:
URL: https://github.com/apache/spark/pull/50804#discussion_r2077357505
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/package.scala:
##########
@@ -93,22 +94,37 @@ package object util extends Logging {
// Replaces attributes, string literals, complex type extractors with their pretty form so that
// generated column names don't contain back-ticks or double-quotes.
- def usePrettyExpression(e: Expression): Expression = e transform {
- case a: Attribute => new PrettyAttribute(a)
- case Literal(s: UTF8String, StringType) => PrettyAttribute(s.toString, StringType)
- case Literal(v, t: NumericType) if v != null => PrettyAttribute(v.toString, t)
- case Literal(null, dataType) => PrettyAttribute("NULL", dataType)
- case e: GetStructField =>
- val name = e.name.getOrElse(e.childSchema(e.ordinal).name)
- PrettyAttribute(usePrettyExpression(e.child).sql + "." + name, e.dataType)
- case e: GetArrayStructFields =>
- PrettyAttribute(s"${usePrettyExpression(e.child)}.${e.field.name}", e.dataType)
- case r: InheritAnalysisRules =>
- PrettyAttribute(r.makeSQLString(r.parameters.map(toPrettySQL)), r.dataType)
- case c: Cast if c.getTagValue(Cast.USER_SPECIFIED_CAST).isEmpty =>
- PrettyAttribute(usePrettyExpression(c.child).sql, c.dataType)
- case p: PythonFuncExpression => PrettyPythonUDF(p.name, p.dataType, p.children)
- }
+ private def usePrettyExpression(e: Expression, stripOuterReference: Boolean = true): Expression =
+ e transform {
+ case aggregateExpression: AggregateExpression
Review Comment:
done!
--
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-52019] [SQL] Strip outer reference before creating a name in toPrettySQL [spark]
Posted by "mihailoale-db (via GitHub)" <gi...@apache.org>.
mihailoale-db commented on PR #50804:
URL: https://github.com/apache/spark/pull/50804#issuecomment-2858447101
@MaxGekk It impacts only resolved expressions trees that have `OuterReference` under `AggregateExpression`. Before the change name would be *agg(outer(...))* and after it it's *outer(agg(...))*. The only intention is to make single-pass development easier as this change is basically noop for fixed-point but follows bottom up semantics that single-pass enforces.
--
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