You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "cloud-fan (via GitHub)" <gi...@apache.org> on 2024/11/12 05:34:10 UTC
[PR] [SPARK-50130][SQL][FOLLOWUP] Simplify the resolution of LazyOuterReference [spark]
cloud-fan opened a new pull request, #48820:
URL: https://github.com/apache/spark/pull/48820
<!--
Thanks for sending a pull request! Here are some tips for you:
1. If this is your first time, please read our contributor guidelines: https://spark.apache.org/contributing.html
2. Ensure you have added or run the appropriate tests for your PR: https://spark.apache.org/developer-tools.html
3. If the PR is unfinished, add '[WIP]' in your PR title, e.g., '[WIP][SPARK-XXXX] Your PR title ...'.
4. Be sure to keep the PR description updated to reflect all changes.
5. Please write your PR title to summarize what this PR proposes.
6. If possible, provide a concise example to reproduce the issue for a faster review.
7. If you want to add a new configuration, please read the guideline first for naming configurations in
'core/src/main/scala/org/apache/spark/internal/config/ConfigEntry.scala'.
8. If you want to add or modify an error type or message, please read the guideline first in
'common/utils/src/main/resources/error/README.md'.
-->
### What changes were proposed in this pull request?
<!--
Please clarify what changes you are proposing. The purpose of this section is to outline the changes and how this PR fixes the issue.
If possible, please consider writing useful notes for better and faster reviews in your PR. See the examples below.
1. If you refactor some codes with changing classes, showing the class hierarchy will help reviewers.
2. If you fix some SQL features, you can provide some references of other DBMSes.
3. If there is design documentation, please add the link.
4. If there is a discussion in the mailing list, please add the link.
-->
This is a followup of https://github.com/apache/spark/pull/48664 to simplify the code. The new workflow is:
1. The Clolumn API creates `LazyOuterReference`
2. `QueryExecution` does lazy analysis if its main query contains `LazyOuterReference`. Eager analysis is still performed if only subquery expressions contain `LazyOuterReference`.
3. The column resolution framework is updated to resolve `LazyOuterReference`
After this simplification, we no longer need the special logic to strip `LazyOuterReference` in the DataFrame side. We no longer need the extra flag in the subquery expressions.
### Why are the changes needed?
<!--
Please clarify why the changes are needed. For instance,
1. If you propose a new API, clarify the use case for a new API.
5. If you fix a bug, you can clarify why it is a bug.
-->
cleanup
### Does this PR introduce _any_ user-facing change?
<!--
Note that it means *any* user-facing change including all aspects such as the documentation fix.
If yes, please clarify the previous behavior and the change this PR proposes - provide the console output, description and/or an example to show the behavior difference if possible.
If possible, please also clarify if this is a user-facing change compared to the released Spark versions or within the unreleased branches such as master.
If no, write 'No'.
-->
no
### How was this patch tested?
<!--
If tests were added, say they were added here. Please make sure to add some test cases that check the changes thoroughly including negative and positive cases if possible.
If it was tested in a way different from regular unit tests, please clarify how you tested step by step, ideally copy and paste-able, so that other reviewers can test and check, and descendants can verify in the future.
If tests were not added, please describe why they were not added and/or why it was difficult to add.
If benchmark tests were added, please run the benchmarks in GitHub Actions for the consistent environment, and the instructions could accord to: https://spark.apache.org/developer-tools.html#github-workflow-benchmarks.
-->
existing tests
### Was this patch authored or co-authored using generative AI tooling?
<!--
If generative AI tooling has been used in the process of authoring this patch, please include the
phrase: 'Generated-by: ' followed by the name of the tool and its version.
If no, write 'No'.
Please refer to the [ASF Generative Tooling Guidance](https://www.apache.org/legal/generative-tooling.html) for details.
-->
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-50130][SQL][FOLLOWUP] Simplify the resolution of LazyOuterReference [spark]
Posted by "ueshin (via GitHub)" <gi...@apache.org>.
ueshin commented on PR #48820:
URL: https://github.com/apache/spark/pull/48820#issuecomment-2474723006
@cloud-fan
> In SQL, users can just write un-qualified column names to reference the outer plan. We should allow the same in DataFrame API.
In that case, `sf.col("a")` should also be allowed instead of `sf.col("a").outer()` in the above example?
```py
l.select(
"a",
(
r
.where(sf.col("b") == sf.col("a"))
.select(sf.sum("d"))
.scalar()
),
).show()
```
otherwise, users may not know why it's necessary for `a`, but not for `b`.
So far we do need at least one `.outer()` to make the analysis lazy, it should have a consistent meaning; otherwise we need another way to make the analysis lazy.
cc @allisonwang-db who suggested the current `outer()` behavior.
--
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-50130][SQL][FOLLOWUP] Simplify the resolution of LazyOuterReference [spark]
Posted by "ueshin (via GitHub)" <gi...@apache.org>.
ueshin commented on PR #48820:
URL: https://github.com/apache/spark/pull/48820#issuecomment-2475386387
@cloud-fan Yes, Spark Connect is easy. It doesn't even need `LazyOuterReference` and `UnresolvedOuterReference`. I prototyped with Spark Connect.
I'm just wondering if we can introduce this difference between classic and connect. I think they should have the same behavior.
--
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-50130][SQL][FOLLOWUP] Simplify the implementation of col.outer() [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #48820:
URL: https://github.com/apache/spark/pull/48820#discussion_r1860542552
##########
sql/api/src/main/scala/org/apache/spark/sql/Column.scala:
##########
@@ -1383,20 +1383,27 @@ class Column(val node: ColumnNode) extends Logging {
def over(): Column = over(Window.spec)
/**
- * Marks this column reference as an outer reference for subqueries.
+ * Mark this column as an outer column if its expression refers to columns from an outer query. This
+ * is used to trigger lazy analysis of Spark Classic DataFrame, so that we can use it to build
+ * subquery expressions. Spark Connect DataFrame is always lazily analyzed and does not need to
+ * use this function.
Review Comment:
```suggestion
* Mark this column as an outer column if its expression refers to columns from an outer query.
* This is used to trigger lazy analysis of Spark Classic DataFrame, so that we can use it to
* build subquery expressions. Spark Connect DataFrame is always lazily analyzed and does not
* need to use this function.
```
--
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-50130][SQL][FOLLOWUP] Simplify the resolution of LazyOuterReference [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on PR #48820:
URL: https://github.com/apache/spark/pull/48820#issuecomment-2475481711
I think it's fine for Classic Spark to have more limitations. We are moving users from Classic Spark to Spark Connect, not the other direction. This also makes DataFrame more consistent with SQL.
--
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-50130][SQL][FOLLOWUP] Simplify the implementation of col.outer() [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #48820:
URL: https://github.com/apache/spark/pull/48820#discussion_r1858660380
##########
python/pyspark/sql/column.py:
##########
@@ -1524,7 +1524,11 @@ def over(self, window: "WindowSpec") -> "Column":
@dispatch_col_method
def outer(self) -> "Column":
"""
- Mark this column reference as an outer reference for subqueries.
+ Mark this as an outer Column if its expression may reference columns in the outer plan.
Review Comment:
do you mean `outer query plan`?
--
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-50130][SQL][FOLLOWUP] Simplify the resolution of LazyOuterReference [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on PR #48820:
URL: https://github.com/apache/spark/pull/48820#issuecomment-2473143992
@ueshin After thinking more about it, I think the new test case should pass instead of fail. My opinion is:
1. In SQL, users can just write un-qualified column names to reference the outer plan. We should allow the same in DataFrame API.
2. Eventually, Spark Connect is the main API and DataFrame is always lazy. Then `.outer()` is only needed to avoid ambiguity when there are column name conflicts, to explicitly reference the outer plan only.
--
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-50130][SQL][FOLLOWUP] Simplify the resolution of LazyOuterReference [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on PR #48820:
URL: https://github.com/apache/spark/pull/48820#issuecomment-2475379635
@ueshin I think `.where(sf.col("b") == sf.col("a"))` should be allowed in Spark Connect. For now we need at lease one `.outer()` to trigger lazy analysis for Classic Spark, but it's not really a problem for Spark Connect.
--
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-50130][SQL][FOLLOWUP] Simplify the resolution of LazyOuterReference [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #48820:
URL: https://github.com/apache/spark/pull/48820#discussion_r1853170102
##########
python/pyspark/sql/tests/test_subquery.py:
##########
@@ -189,7 +181,7 @@ def test_scalar_subquery_against_local_relations(self):
"c1",
(
self.spark.table("t2")
- .where(sf.col("c2").outer() == sf.col("c2"))
+ .where(sf.outer(sf.col("t1.c2")) == sf.col("t2.c2"))
Review Comment:
This one can be `sf.outer("t1.c2")`
--
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-50130][SQL][FOLLOWUP] Simplify the implementation of col.outer() [spark]
Posted by "allisonwang-db (via GitHub)" <gi...@apache.org>.
allisonwang-db commented on code in PR #48820:
URL: https://github.com/apache/spark/pull/48820#discussion_r1859960681
##########
python/pyspark/sql/column.py:
##########
@@ -1524,7 +1524,11 @@ def over(self, window: "WindowSpec") -> "Column":
@dispatch_col_method
def outer(self) -> "Column":
"""
- Mark this column reference as an outer reference for subqueries.
+ Mark this as an outer Column if its expression may reference columns in the outer plan.
Review Comment:
Yea, or just `outer query`. This is going to be in the API doc, but we can always refine it later.
--
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-50130][SQL][FOLLOWUP] Simplify the resolution of LazyOuterReference [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #48820:
URL: https://github.com/apache/spark/pull/48820#discussion_r1853171330
##########
python/pyspark/sql/tests/test_subquery.py:
##########
@@ -263,7 +284,7 @@ def test_correlated_scalar_subquery(self):
"a",
(
self.spark.table("r")
- .where(sf.col("a").outer() == sf.col("c"))
+ .where(sf.outer(sf.col("a")) == sf.col("c"))
Review Comment:
ditto
--
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-50130][SQL][FOLLOWUP] Simplify the implementation of col.outer() [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on PR #48820:
URL: https://github.com/apache/spark/pull/48820#issuecomment-2505180383
thanks for the review, merging to master!
--
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-50130][SQL][FOLLOWUP] Simplify the resolution of LazyOuterReference [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #48820:
URL: https://github.com/apache/spark/pull/48820#discussion_r1839964432
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/ColumnResolutionHelper.scala:
##########
@@ -221,35 +221,39 @@ trait ColumnResolutionHelper extends Logging with DataTypeErrorsBase {
val outerPlan = AnalysisContext.get.outerPlan
if (outerPlan.isEmpty) return e
- e.transformWithPruning(_.containsAnyPattern(UNRESOLVED_ATTRIBUTE, TEMP_RESOLVED_COLUMN)) {
+ def resolve(nameParts: Seq[String]): Option[Expression] = try {
+ outerPlan.get match {
+ // Subqueries in UnresolvedHaving can host grouping expressions and aggregate functions.
+ // We should resolve columns with `agg.output` and the rule `ResolveAggregateFunctions` will
+ // push them down to Aggregate later. This is similar to what we do in `resolveColumns`.
+ case u @ UnresolvedHaving(_, agg: Aggregate) =>
+ agg.resolveChildren(nameParts, conf.resolver)
+ .orElse(u.resolveChildren(nameParts, conf.resolver))
+ .map(wrapOuterReference)
+ case other =>
+ other.resolveChildren(nameParts, conf.resolver).map(wrapOuterReference)
+ }
+ } catch {
+ case ae: AnalysisException =>
+ logDebug(ae.getMessage)
+ None
+ }
+
+ e.transformWithPruning(
+ _.containsAnyPattern(UNRESOLVED_ATTRIBUTE, TEMP_RESOLVED_COLUMN, LAZY_OUTER_REFERENCE)) {
case u: UnresolvedAttribute =>
- resolveOuterReference(u.nameParts, outerPlan.get).getOrElse(u)
+ resolve(u.nameParts).getOrElse(u)
+ case u: LazyOuterReference =>
Review Comment:
This is the new change. Other changes are just reverting back the previous changes.
--
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-50130][SQL][FOLLOWUP] Simplify the implementation of col.outer() [spark]
Posted by "allisonwang-db (via GitHub)" <gi...@apache.org>.
allisonwang-db commented on code in PR #48820:
URL: https://github.com/apache/spark/pull/48820#discussion_r1858198370
##########
python/pyspark/sql/column.py:
##########
@@ -1524,7 +1524,11 @@ def over(self, window: "WindowSpec") -> "Column":
@dispatch_col_method
def outer(self) -> "Column":
"""
- Mark this column reference as an outer reference for subqueries.
+ Mark this as an outer Column if its expression may reference columns in the outer plan.
+
+ This is used to trigger lazy analysis of Spark Classic DataFrame, so that we can use it
+ to build subquery expressions. Spark Connect DataFrame is always lazily analyzed and
+ does not need to use this function.
Review Comment:
I think we need more examples here (but can added in later PRs)
##########
python/pyspark/sql/column.py:
##########
@@ -1524,7 +1524,11 @@ def over(self, window: "WindowSpec") -> "Column":
@dispatch_col_method
def outer(self) -> "Column":
"""
- Mark this column reference as an outer reference for subqueries.
+ Mark this as an outer Column if its expression may reference columns in the outer plan.
Review Comment:
```suggestion
Mark this column as an outer column if its expression refers to columns from an outer query or plan
```
--
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-50130][SQL][FOLLOWUP] Simplify the implementation of col.outer() [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan closed pull request #48820: [SPARK-50130][SQL][FOLLOWUP] Simplify the implementation of col.outer()
URL: https://github.com/apache/spark/pull/48820
--
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-50130][SQL][FOLLOWUP] Simplify the resolution of LazyOuterReference [spark]
Posted by "allisonwang-db (via GitHub)" <gi...@apache.org>.
allisonwang-db commented on PR #48820:
URL: https://github.com/apache/spark/pull/48820#issuecomment-2482626978
I think there is another issue with the `outer` API. The `where` or `filter` API can take a SQL expression, but we don't have a SQL API for outer reference. So users can't really use SQL expressions together with the subquery API:
```python
# SELECT a, (SELECT sum(d) FROM r WHERE a = c) FROM l
l.select(
"a",
(
r
.where("a = c") <- SQL expression won't work and no alternative to make this analysis lazy
.select(sf.sum("d"))
.scalar()
),
).show()
```
And this is giving
```
AnalysisException: [UNRESOLVED_COLUMN.WITH_SUGGESTION] A column, variable, or function parameter with name `a` cannot be resolved. Did you mean one of the following? [`c`, `d`]. SQLSTATE: 42703; line 1 pos 0;
'Filter ('a = c#2L)
+- LogicalRDD [c#2L, d#3], 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-50130][SQL][FOLLOWUP] Simplify the implementation of col.outer() [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #48820:
URL: https://github.com/apache/spark/pull/48820#discussion_r1860541110
##########
sql/api/src/main/scala/org/apache/spark/sql/Column.scala:
##########
@@ -1383,20 +1383,27 @@ class Column(val node: ColumnNode) extends Logging {
def over(): Column = over(Window.spec)
/**
- * Marks this column reference as an outer reference for subqueries.
+ * Mark this as an outer Column if its expression may reference columns in the outer plan. This
Review Comment:
```suggestion
* Mark this column as an outer column if its expression refers to columns from an outer query. 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-50130][SQL][FOLLOWUP] Simplify the implementation of col.outer() [spark]
Posted by "allisonwang-db (via GitHub)" <gi...@apache.org>.
allisonwang-db commented on code in PR #48820:
URL: https://github.com/apache/spark/pull/48820#discussion_r1858196893
##########
python/pyspark/sql/column.py:
##########
@@ -1524,7 +1524,11 @@ def over(self, window: "WindowSpec") -> "Column":
@dispatch_col_method
def outer(self) -> "Column":
"""
- Mark this column reference as an outer reference for subqueries.
+ Mark this as an outer Column if its expression may reference columns in the outer plan.
Review Comment:
```suggestion
Mark this column as an outer column if its expression refers to columns from an outer query.
```
--
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-50130][SQL][FOLLOWUP] Simplify the resolution of LazyOuterReference [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on PR #48820:
URL: https://github.com/apache/spark/pull/48820#issuecomment-2469648836
cc @ueshin @HyukjinKwon @xinrong-meng @allisonwang-db
--
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-50130][SQL][FOLLOWUP] Simplify the resolution of LazyOuterReference [spark]
Posted by "ueshin (via GitHub)" <gi...@apache.org>.
ueshin commented on code in PR #48820:
URL: https://github.com/apache/spark/pull/48820#discussion_r1838571341
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/unresolved.scala:
##########
@@ -1004,42 +1004,29 @@ case class UnresolvedTranspose(
copy(child = newChild)
}
-case class UnresolvedOuterReference(
- nameParts: Seq[String])
+case class UnresolvedOuterReference(nameParts: Seq[String])
extends LeafExpression with NamedExpression with Unevaluable {
-
- def name: String =
- nameParts.map(n => if (n.contains(".")) s"`$n`" else n).mkString(".")
-
+ def name: String = nameParts.map(quoteIfNeeded).mkString(".")
Review Comment:
👍
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/QueryExecution.scala:
##########
@@ -69,7 +68,10 @@ class QueryExecution(
// TODO: Move the planner an optimizer into here from SessionState.
protected def planner = sparkSession.sessionState.planner
- lazy val isLazyAnalysis: Boolean = logical.containsAnyPattern(LAZY_ANALYSIS_EXPRESSION)
+ lazy val isLazyAnalysis: Boolean = {
+ // Only check the main query as we can resolve LazyOuterReference inside subquery expressions.
+ logical.exists(_.expressions.exists(_.exists(_.isInstanceOf[LazyOuterReference])))
Review Comment:
oh, `logical.containsAnyPattern(LAZY_ANALYSIS_EXPRESSION)` will check subqueries?
I also intended to check the main query. Thanks for the correction!
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/unresolved.scala:
##########
@@ -1004,42 +1004,29 @@ case class UnresolvedTranspose(
copy(child = newChild)
}
-case class UnresolvedOuterReference(
- nameParts: Seq[String])
+case class UnresolvedOuterReference(nameParts: Seq[String])
extends LeafExpression with NamedExpression with Unevaluable {
-
- def name: String =
- nameParts.map(n => if (n.contains(".")) s"`$n`" else n).mkString(".")
-
+ def name: String = nameParts.map(quoteIfNeeded).mkString(".")
override def exprId: ExprId = throw new UnresolvedException("exprId")
override def dataType: DataType = throw new UnresolvedException("dataType")
override def nullable: Boolean = throw new UnresolvedException("nullable")
override def qualifier: Seq[String] = throw new UnresolvedException("qualifier")
override lazy val resolved = false
-
override def toAttribute: Attribute = throw new UnresolvedException("toAttribute")
override def newInstance(): UnresolvedOuterReference = this
-
final override val nodePatterns: Seq[TreePattern] = Seq(UNRESOLVED_OUTER_REFERENCE)
}
-case class LazyOuterReference(
- nameParts: Seq[String])
+case class LazyOuterReference(nameParts: Seq[String])
extends LeafExpression with NamedExpression with Unevaluable with LazyAnalysisExpression {
-
- def name: String =
- nameParts.map(n => if (n.contains(".")) s"`$n`" else n).mkString(".")
-
+ def name: String = nameParts.map(quoteIfNeeded).mkString(".")
Review Comment:
👍
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/QueryExecution.scala:
##########
@@ -69,7 +68,10 @@ class QueryExecution(
// TODO: Move the planner an optimizer into here from SessionState.
protected def planner = sparkSession.sessionState.planner
- lazy val isLazyAnalysis: Boolean = logical.containsAnyPattern(LAZY_ANALYSIS_EXPRESSION)
+ lazy val isLazyAnalysis: Boolean = {
+ // Only check the main query as we can resolve LazyOuterReference inside subquery expressions.
+ logical.exists(_.expressions.exists(_.exists(_.isInstanceOf[LazyOuterReference])))
Review Comment:
nit: it should be `LazyAnalysisExpression`, although currently only `LazyOuterReference`?
--
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