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/03/01 06:56:17 UTC
[PR] [SPARK-47241][SQL] Fix rule order issues for ExtractGenerator [spark]
cloud-fan opened a new pull request, #45350:
URL: https://github.com/apache/spark/pull/45350
<!--
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.
-->
The rule `ExtractGenerator` does not define any trigger condition when rewriting generator functions in `Project`, which makes the behavior quite unstable and heavily depends on the execution order of analyzer rules.
Two bugs I've found so far:
1. By design, we want to forbid users from using more than one generator function in SELECT. However, we can't really enforce it if two generator functions are not resolved at the same time: the rule thinks there is only one generate function (the other is still unresolved), then rewrite it. The other one gets resolved later and gets rewritten later.
2. When a generator function is put after `SELECT *`, it's possible that `*` is not expanded yet when we enter `ExtractGenerator`. The rule rewrites the generator function: insert a `Generate` operator below, and add a new column to the projectList for the generator function output. Then we expand `*` to the child plan output which is `Generate`, we end up with two identical columns for the generate function output.
This PR fixes it by adding a trigger condition when rewriting generator functions in `Project`: the projectList should be resolved or a generator function. This is the same trigger condition we used for `Aggregate`. To avoid breaking changes, this PR also allows multiple generator functions in `Project`, which works totally fine.
### 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.
4. If you fix a bug, you can clarify why it is a bug.
-->
bug fix
### 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'.
-->
Yes, now multiple generator functions are allowed in `Project`. And there won't be duplicated columns for generator function output.
### 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.
-->
new test
### 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-47241][SQL] Fix rule order issues for ExtractGenerator [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #45350:
URL: https://github.com/apache/spark/pull/45350#discussion_r1515379202
##########
sql/core/src/test/scala/org/apache/spark/sql/GeneratorFunctionSuite.scala:
##########
@@ -553,6 +552,32 @@ class GeneratorFunctionSuite extends QueryTest with SharedSparkSession {
checkAnswer(df, Row(0.7604953758285915d))
}
}
+
+ test("SPARK-47241: two generator functions in SELECT") {
+ def testTwoGenerators(needImplicitCast: Boolean): Unit = {
+ val df = sql(
+ s"""
+ |SELECT
+ |explode(array('a', 'b')) as c1,
+ |explode(array(0L, ${if (needImplicitCast) "0L + 1" else "1L"})) as c2
+ |""".stripMargin)
+ checkAnswer(df, Seq(Row("a", 0L), Row("a", 1L), Row("b", 0L), Row("b", 1L)))
+ }
+ testTwoGenerators(needImplicitCast = true)
+ testTwoGenerators(needImplicitCast = false)
+ }
+
+ test("SPARK-47241: generator function after SELECT *") {
Review Comment:
```suggestion
test("SPARK-47241: generator function after wildcard in SELECT") {
```
--
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-47241][SQL] Fix rule order issues for ExtractGenerator [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #45350:
URL: https://github.com/apache/spark/pull/45350#discussion_r1515379047
##########
sql/core/src/test/scala/org/apache/spark/sql/GeneratorFunctionSuite.scala:
##########
@@ -553,6 +552,32 @@ class GeneratorFunctionSuite extends QueryTest with SharedSparkSession {
checkAnswer(df, Row(0.7604953758285915d))
}
}
+
+ test("SPARK-47241: two generator functions in SELECT") {
+ def testTwoGenerators(needImplicitCast: Boolean): Unit = {
+ val df = sql(
+ s"""
+ |SELECT
+ |explode(array('a', 'b')) as c1,
+ |explode(array(0L, ${if (needImplicitCast) "0L + 1" else "1L"})) as c2
+ |""".stripMargin)
+ checkAnswer(df, Seq(Row("a", 0L), Row("a", 1L), Row("b", 0L), Row("b", 1L)))
+ }
+ testTwoGenerators(needImplicitCast = true)
+ testTwoGenerators(needImplicitCast = false)
+ }
+
+ test("SPARK-47241: generator function after SELECT *") {
+ val df = sql(
+ s"""
+ |SELECT *, explode(array('a', 'b')) as c1
+ |FROM
+ |(
+ | SELECT id FROM range(1) GROUP BY 1
+ |)
+ |""".stripMargin)
+ checkAnswer(df, Seq(Row(0, "a"), Row(0, "b")))
+ }
Review Comment:
good suggestion!
--
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-47241][SQL] Fix rule order issues for ExtractGenerator [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on PR #45350:
URL: https://github.com/apache/spark/pull/45350#issuecomment-1972615533
cc @viirya @gengliangwang
--
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-47241][SQL] Fix rule order issues for ExtractGenerator [spark]
Posted by "rajatrj20 (via GitHub)" <gi...@apache.org>.
rajatrj20 commented on PR #45350:
URL: https://github.com/apache/spark/pull/45350#issuecomment-2089760654
@cloud-fan This change broke an existing behaviour. When a aliased generator field A is referenced in some another field B in project list, it will create a situation where the B will wait for A to resolve as it needs to know the field identifier of A and A will wait for B to resolve since we have check in place which needs all fields other than generator field to be resolved.
This is how you can reproduce this:
```
CREATE TABLE nestedTable1 (array_int_col array<int>, array_str_col array<string>, map_str_col map<string,string>, map_int_col map<string,int>, struct_col struct<col_3:string,col_1:int>, col_3 string, col_1 int) USING iceberg
INSERT INTO nestedTable1 (array_int_col, array_str_col, map_str_col, map_int_col, struct_col, col_3, col_1) VALUES (array(1, 2, 3, 4), array('a', 'b', 'c'), map('k1', 'v1', 'k2', 'v2'), map('k', 1), struct('a', 1), 'a', 1)
SELECT col_1, EXPLODE(MAP_KEYS(map_str_col)) AS key, map_str_col[key] AS value FROM nestedTable1;
```
This results in following final analyzed plan:
```
'Project [col_1#192, 'EXPLODE(map_keys(map_str_col#188)) AS key#329, map_str_col#188[lateralAliasReference(key)] AS value#330]
+- RelationV2[array_int_col#186, array_str_col#187, map_str_col#188, map_int_col#189, struct_col#190, col_3#191, col_1#192] spark_catalog.default.nestedtable1
```
Without this change the analyzed plan looks like:
```
'Project [col_1#192, key#331, map_str_col#188['key] AS value#330]
+- Generate explode(map_keys(map_str_col#188)), false, [key#331]
+- RelationV2[array_int_col#186, array_str_col#187, map_str_col#188, map_int_col#189, struct_col#190, col_3#191, col_1#192] spark_catalog.default.nestedtable1
```
--
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-47241][SQL] Fix rule order issues for ExtractGenerator [spark]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on code in PR #45350:
URL: https://github.com/apache/spark/pull/45350#discussion_r1514904624
##########
sql/core/src/test/scala/org/apache/spark/sql/GeneratorFunctionSuite.scala:
##########
@@ -553,6 +552,32 @@ class GeneratorFunctionSuite extends QueryTest with SharedSparkSession {
checkAnswer(df, Row(0.7604953758285915d))
}
}
+
+ test("SPARK-47241: two generator functions in SELECT") {
+ def testTwoGenerators(needImplicitCast: Boolean): Unit = {
+ val df = sql(
+ s"""
+ |SELECT
+ |explode(array('a', 'b')) as c1,
+ |explode(array(0L, ${if (needImplicitCast) "0L + 1" else "1L"})) as c2
+ |""".stripMargin)
+ checkAnswer(df, Seq(Row("a", 0L), Row("a", 1L), Row("b", 0L), Row("b", 1L)))
+ }
+ testTwoGenerators(needImplicitCast = true)
+ testTwoGenerators(needImplicitCast = false)
+ }
+
+ test("SPARK-47241: generator function after SELECT *") {
+ val df = sql(
+ s"""
+ |SELECT *, explode(array('a', 'b')) as c1
+ |FROM
+ |(
+ | SELECT id FROM range(1) GROUP BY 1
+ |)
+ |""".stripMargin)
+ checkAnswer(df, Seq(Row(0, "a"), Row(0, "b")))
+ }
Review Comment:
If you refer the order in project list, maybe `generator function after wildcard in SELECT`?
--
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-47241][SQL] Fix rule order issues for ExtractGenerator [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #45350:
URL: https://github.com/apache/spark/pull/45350#discussion_r1515378012
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala:
##########
@@ -2876,28 +2876,36 @@ class Analyzer(override val catalogManager: CatalogManager) extends RuleExecutor
}
}
+ // We must wait until all expressions except for generator functions are resolved before
+ // rewriting generator functions in Project/Aggregate. This is necessary to make this rule
+ // stable for different execution orders of analyzer rules. See also SPARK-47241.
+ private def canRewriteGenerator(namedExprs: Seq[NamedExpression]): Boolean = {
+ namedExprs.forall { ne =>
+ ne.resolved || {
+ trimNonTopLevelAliases(ne) match {
+ case AliasedGenerator(_, _, _) => true
+ case _ => false
+ }
+ }
+ }
+ }
+
def apply(plan: LogicalPlan): LogicalPlan = plan.resolveOperatorsUpWithPruning(
_.containsPattern(GENERATOR), ruleId) {
case Project(projectList, _) if projectList.exists(hasNestedGenerator) =>
val nestedGenerator = projectList.find(hasNestedGenerator).get
throw QueryCompilationErrors.nestedGeneratorError(trimAlias(nestedGenerator))
- case Project(projectList, _) if projectList.count(hasGenerator) > 1 =>
- val generators = projectList.filter(hasGenerator).map(trimAlias)
- throw QueryCompilationErrors.moreThanOneGeneratorError(generators, "SELECT")
-
case Aggregate(_, aggList, _) if aggList.exists(hasNestedGenerator) =>
val nestedGenerator = aggList.find(hasNestedGenerator).get
throw QueryCompilationErrors.nestedGeneratorError(trimAlias(nestedGenerator))
case Aggregate(_, aggList, _) if aggList.count(hasGenerator) > 1 =>
val generators = aggList.filter(hasGenerator).map(trimAlias)
- throw QueryCompilationErrors.moreThanOneGeneratorError(generators, "aggregate")
+ throw QueryCompilationErrors.moreThanOneGeneratorError(generators)
Review Comment:
I find `aggregate clause` confusing, as what end users write is a `SELECT` query with GROUP BY or aggregate functions.
--
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-47241][SQL] Fix rule order issues for ExtractGenerator [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan closed pull request #45350: [SPARK-47241][SQL] Fix rule order issues for ExtractGenerator
URL: https://github.com/apache/spark/pull/45350
--
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-47241][SQL] Fix rule order issues for ExtractGenerator [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on PR #45350:
URL: https://github.com/apache/spark/pull/45350#issuecomment-2090237466
> SELECT col_1, EXPLODE(MAP_KEYS(map_str_col)) AS key, map_str_col[key] AS value FROM nestedTable1;
I think this can be supported with LCA. cc @anchovYu
--
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-47241][SQL] Fix rule order issues for ExtractGenerator [spark]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on code in PR #45350:
URL: https://github.com/apache/spark/pull/45350#discussion_r1514897535
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala:
##########
@@ -2876,28 +2876,36 @@ class Analyzer(override val catalogManager: CatalogManager) extends RuleExecutor
}
}
+ // We must wait until all expressions except for generator functions are resolved before
+ // rewriting generator functions in Project/Aggregate. This is necessary to make this rule
+ // stable for different execution orders of analyzer rules. See also SPARK-47241.
+ private def canRewriteGenerator(namedExprs: Seq[NamedExpression]): Boolean = {
+ namedExprs.forall { ne =>
+ ne.resolved || {
+ trimNonTopLevelAliases(ne) match {
+ case AliasedGenerator(_, _, _) => true
+ case _ => false
+ }
+ }
+ }
+ }
+
def apply(plan: LogicalPlan): LogicalPlan = plan.resolveOperatorsUpWithPruning(
_.containsPattern(GENERATOR), ruleId) {
case Project(projectList, _) if projectList.exists(hasNestedGenerator) =>
val nestedGenerator = projectList.find(hasNestedGenerator).get
throw QueryCompilationErrors.nestedGeneratorError(trimAlias(nestedGenerator))
- case Project(projectList, _) if projectList.count(hasGenerator) > 1 =>
- val generators = projectList.filter(hasGenerator).map(trimAlias)
- throw QueryCompilationErrors.moreThanOneGeneratorError(generators, "SELECT")
-
case Aggregate(_, aggList, _) if aggList.exists(hasNestedGenerator) =>
val nestedGenerator = aggList.find(hasNestedGenerator).get
throw QueryCompilationErrors.nestedGeneratorError(trimAlias(nestedGenerator))
case Aggregate(_, aggList, _) if aggList.count(hasGenerator) > 1 =>
val generators = aggList.filter(hasGenerator).map(trimAlias)
- throw QueryCompilationErrors.moreThanOneGeneratorError(generators, "aggregate")
+ throw QueryCompilationErrors.moreThanOneGeneratorError(generators)
Review Comment:
This is still for aggregate, but I saw you remove the `clause` field in the error message?
--
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-47241][SQL] Fix rule order issues for ExtractGenerator [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on PR #45350:
URL: https://github.com/apache/spark/pull/45350#issuecomment-1983026959
thanks for the review, merging to master/3.5!
--
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-47241][SQL] Fix rule order issues for ExtractGenerator [spark]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on code in PR #45350:
URL: https://github.com/apache/spark/pull/45350#discussion_r1515378800
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala:
##########
@@ -2876,28 +2876,36 @@ class Analyzer(override val catalogManager: CatalogManager) extends RuleExecutor
}
}
+ // We must wait until all expressions except for generator functions are resolved before
+ // rewriting generator functions in Project/Aggregate. This is necessary to make this rule
+ // stable for different execution orders of analyzer rules. See also SPARK-47241.
+ private def canRewriteGenerator(namedExprs: Seq[NamedExpression]): Boolean = {
+ namedExprs.forall { ne =>
+ ne.resolved || {
+ trimNonTopLevelAliases(ne) match {
+ case AliasedGenerator(_, _, _) => true
+ case _ => false
+ }
+ }
+ }
+ }
+
def apply(plan: LogicalPlan): LogicalPlan = plan.resolveOperatorsUpWithPruning(
_.containsPattern(GENERATOR), ruleId) {
case Project(projectList, _) if projectList.exists(hasNestedGenerator) =>
val nestedGenerator = projectList.find(hasNestedGenerator).get
throw QueryCompilationErrors.nestedGeneratorError(trimAlias(nestedGenerator))
- case Project(projectList, _) if projectList.count(hasGenerator) > 1 =>
- val generators = projectList.filter(hasGenerator).map(trimAlias)
- throw QueryCompilationErrors.moreThanOneGeneratorError(generators, "SELECT")
-
case Aggregate(_, aggList, _) if aggList.exists(hasNestedGenerator) =>
val nestedGenerator = aggList.find(hasNestedGenerator).get
throw QueryCompilationErrors.nestedGeneratorError(trimAlias(nestedGenerator))
case Aggregate(_, aggList, _) if aggList.count(hasGenerator) > 1 =>
val generators = aggList.filter(hasGenerator).map(trimAlias)
- throw QueryCompilationErrors.moreThanOneGeneratorError(generators, "aggregate")
+ throw QueryCompilationErrors.moreThanOneGeneratorError(generators)
Review Comment:
Hmm, okay.
--
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-47241][SQL] Fix rule order issues for ExtractGenerator [spark]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on code in PR #45350:
URL: https://github.com/apache/spark/pull/45350#discussion_r1514903490
##########
sql/core/src/test/scala/org/apache/spark/sql/GeneratorFunctionSuite.scala:
##########
@@ -553,6 +552,32 @@ class GeneratorFunctionSuite extends QueryTest with SharedSparkSession {
checkAnswer(df, Row(0.7604953758285915d))
}
}
+
+ test("SPARK-47241: two generator functions in SELECT") {
+ def testTwoGenerators(needImplicitCast: Boolean): Unit = {
+ val df = sql(
+ s"""
+ |SELECT
+ |explode(array('a', 'b')) as c1,
+ |explode(array(0L, ${if (needImplicitCast) "0L + 1" else "1L"})) as c2
+ |""".stripMargin)
+ checkAnswer(df, Seq(Row("a", 0L), Row("a", 1L), Row("b", 0L), Row("b", 1L)))
+ }
+ testTwoGenerators(needImplicitCast = true)
+ testTwoGenerators(needImplicitCast = false)
+ }
+
+ test("SPARK-47241: generator function after SELECT *") {
+ val df = sql(
+ s"""
+ |SELECT *, explode(array('a', 'b')) as c1
+ |FROM
+ |(
+ | SELECT id FROM range(1) GROUP BY 1
+ |)
+ |""".stripMargin)
+ checkAnswer(df, Seq(Row(0, "a"), Row(0, "b")))
+ }
Review Comment:
When I read the PR description, I began wondering what it is "after" `SELECT *`, because I think the `Generate` will be under `Project`:
```
== Physical Plan ==
*(1) Project [id#1L, c1#2, c1#2]
+- *(1) Generate explode([a,b]), [id#1L], false, [c1#2]
+- *(1) Range (0, 1, step=1, splits=16)
```
It maybe "before"?
--
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-47241][SQL] Fix rule order issues for ExtractGenerator [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on PR #45350:
URL: https://github.com/apache/spark/pull/45350#issuecomment-1981124178
cc @MaxGekk @yaooqinn @dongjoon-hyun
--
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-47241][SQL] Fix rule order issues for ExtractGenerator [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #45350:
URL: https://github.com/apache/spark/pull/45350#discussion_r1515378900
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala:
##########
@@ -2876,28 +2876,36 @@ class Analyzer(override val catalogManager: CatalogManager) extends RuleExecutor
}
}
+ // We must wait until all expressions except for generator functions are resolved before
+ // rewriting generator functions in Project/Aggregate. This is necessary to make this rule
+ // stable for different execution orders of analyzer rules. See also SPARK-47241.
+ private def canRewriteGenerator(namedExprs: Seq[NamedExpression]): Boolean = {
+ namedExprs.forall { ne =>
+ ne.resolved || {
+ trimNonTopLevelAliases(ne) match {
+ case AliasedGenerator(_, _, _) => true
+ case _ => false
+ }
+ }
+ }
+ }
+
def apply(plan: LogicalPlan): LogicalPlan = plan.resolveOperatorsUpWithPruning(
_.containsPattern(GENERATOR), ruleId) {
case Project(projectList, _) if projectList.exists(hasNestedGenerator) =>
val nestedGenerator = projectList.find(hasNestedGenerator).get
throw QueryCompilationErrors.nestedGeneratorError(trimAlias(nestedGenerator))
- case Project(projectList, _) if projectList.count(hasGenerator) > 1 =>
- val generators = projectList.filter(hasGenerator).map(trimAlias)
- throw QueryCompilationErrors.moreThanOneGeneratorError(generators, "SELECT")
-
case Aggregate(_, aggList, _) if aggList.exists(hasNestedGenerator) =>
val nestedGenerator = aggList.find(hasNestedGenerator).get
throw QueryCompilationErrors.nestedGeneratorError(trimAlias(nestedGenerator))
case Aggregate(_, aggList, _) if aggList.count(hasGenerator) > 1 =>
val generators = aggList.filter(hasGenerator).map(trimAlias)
- throw QueryCompilationErrors.moreThanOneGeneratorError(generators, "aggregate")
+ throw QueryCompilationErrors.moreThanOneGeneratorError(generators)
Review Comment:
Another reason is we can't always figure out if it's aggregate or nor. If there is no GROUP BY, the plan is still `Project` and we may fail before analyzer rewrite it to `Aggregate`, then we report `SELECT clause` anyway.
--
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-47241][SQL] Fix rule order issues for ExtractGenerator [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #45350:
URL: https://github.com/apache/spark/pull/45350#discussion_r1515378900
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala:
##########
@@ -2876,28 +2876,36 @@ class Analyzer(override val catalogManager: CatalogManager) extends RuleExecutor
}
}
+ // We must wait until all expressions except for generator functions are resolved before
+ // rewriting generator functions in Project/Aggregate. This is necessary to make this rule
+ // stable for different execution orders of analyzer rules. See also SPARK-47241.
+ private def canRewriteGenerator(namedExprs: Seq[NamedExpression]): Boolean = {
+ namedExprs.forall { ne =>
+ ne.resolved || {
+ trimNonTopLevelAliases(ne) match {
+ case AliasedGenerator(_, _, _) => true
+ case _ => false
+ }
+ }
+ }
+ }
+
def apply(plan: LogicalPlan): LogicalPlan = plan.resolveOperatorsUpWithPruning(
_.containsPattern(GENERATOR), ruleId) {
case Project(projectList, _) if projectList.exists(hasNestedGenerator) =>
val nestedGenerator = projectList.find(hasNestedGenerator).get
throw QueryCompilationErrors.nestedGeneratorError(trimAlias(nestedGenerator))
- case Project(projectList, _) if projectList.count(hasGenerator) > 1 =>
- val generators = projectList.filter(hasGenerator).map(trimAlias)
- throw QueryCompilationErrors.moreThanOneGeneratorError(generators, "SELECT")
-
case Aggregate(_, aggList, _) if aggList.exists(hasNestedGenerator) =>
val nestedGenerator = aggList.find(hasNestedGenerator).get
throw QueryCompilationErrors.nestedGeneratorError(trimAlias(nestedGenerator))
case Aggregate(_, aggList, _) if aggList.count(hasGenerator) > 1 =>
val generators = aggList.filter(hasGenerator).map(trimAlias)
- throw QueryCompilationErrors.moreThanOneGeneratorError(generators, "aggregate")
+ throw QueryCompilationErrors.moreThanOneGeneratorError(generators)
Review Comment:
Another reason is we can't always figure out if it's aggregate or not. If there is no GROUP BY, the plan is still `Project` and we may fail before analyzer rewrite it to `Aggregate`, then we report `SELECT clause` anyway.
--
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