You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by GitBox <gi...@apache.org> on 2020/04/27 13:48:37 UTC
[GitHub] [spark] cloud-fan opened a new pull request #28371: [SPARK-31535][SQL][FOLLOWUP] Simplify name conflict check in CTE resolution
cloud-fan opened a new pull request #28371:
URL: https://github.com/apache/spark/pull/28371
<!--
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'.
-->
### 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/28318, to make the code more readable:
1. add some comments to explain the trick
2. simplify the code to use a boolean flag instead of 2 string sets.
3. consider case sensitivity as well.
### 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.
2. If you fix a bug, you can clarify why it is a bug.
-->
refactor
### Does this PR introduce any user-facing change?
<!--
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 no, write 'No'.
-->
yes, fix the name conflict check with case sensitivity.
### 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.
-->
new tests
----------------------------------------------------------------
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.
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
[GitHub] [spark] cloud-fan commented on pull request #28371: [SPARK-31535][SQL][FOLLOWUP] Simplify name conflict check in CTE resolution
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on pull request #28371:
URL: https://github.com/apache/spark/pull/28371#issuecomment-619999399
cc @peter-toth @xuanyuanking @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.
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
[GitHub] [spark] SparkQA removed a comment on pull request #28371: [SPARK-31577][SQL] Fix case-sensitivity and forward name conflict problems when check name conflicts of CTE relations
Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #28371:
URL: https://github.com/apache/spark/pull/28371#issuecomment-620091239
**[Test build #121920 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/121920/testReport)** for PR 28371 at commit [`24a487d`](https://github.com/apache/spark/commit/24a487dce69a0bc55148164d650259268b45dc4f).
----------------------------------------------------------------
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.
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
[GitHub] [spark] cloud-fan commented on a change in pull request #28371: [SPARK-31535][SQL][FOLLOWUP] Simplify name conflict check in CTE resolution
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #28371:
URL: https://github.com/apache/spark/pull/28371#discussion_r415829102
##########
File path: sql/core/src/test/resources/sql-tests/results/cte-nested.sql.out
##########
@@ -64,10 +64,9 @@ WITH
)
SELECT * FROM t2
-- !query schema
-struct<>
+struct<scalarsubquery():int>
-- !query output
-org.apache.spark.sql.AnalysisException
-Name t is ambiguous in nested CTE. Please set spark.sql.legacy.ctePrecedencePolicy to CORRECTED so that name defined in inner CTE takes precedence. If set it to LEGACY, outer CTE definitions will take precedence. See more details in SPARK-28228.;
Review comment:
I tried this query with Spark 2.4.5 (need to replace `t(c) AS (SELECT 1)` with `t AS (SELECT 1 c)` as Spark 2.4 doesn't support `t(c)` syntax), it fails the parser. So we don't need to fail here.
----------------------------------------------------------------
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.
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
[GitHub] [spark] AmplabJenkins removed a comment on pull request #28371: [SPARK-31577][SQL] Fix case-sensitivity and forward name conflict problems when check name conflicts of CTE relations
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #28371:
URL: https://github.com/apache/spark/pull/28371#issuecomment-620191367
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/121904/
Test FAILed.
----------------------------------------------------------------
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.
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
[GitHub] [spark] peter-toth commented on pull request #28371: [SPARK-31577][SQL] Fix various problems when check name conflicts of CTE relations
Posted by GitBox <gi...@apache.org>.
peter-toth commented on pull request #28371:
URL: https://github.com/apache/spark/pull/28371#issuecomment-620074663
That flag is a nice trick so LGTM, I just left a minor note.
The other 2 fixes also look good.
----------------------------------------------------------------
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.
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
[GitHub] [spark] SparkQA commented on pull request #28371: [SPARK-31577][SQL] Fix various problems when check name conflicts of CTE relations
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #28371:
URL: https://github.com/apache/spark/pull/28371#issuecomment-620091239
**[Test build #121920 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/121920/testReport)** for PR 28371 at commit [`24a487d`](https://github.com/apache/spark/commit/24a487dce69a0bc55148164d650259268b45dc4f).
----------------------------------------------------------------
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.
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
[GitHub] [spark] AmplabJenkins removed a comment on pull request #28371: [SPARK-31577][SQL] Fix case-sensitivity and forward name conflict problems when check name conflicts of CTE relations
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #28371:
URL: https://github.com/apache/spark/pull/28371#issuecomment-620208929
----------------------------------------------------------------
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.
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
[GitHub] [spark] peter-toth commented on a change in pull request #28371: [SPARK-31577][SQL] Fix various problems when check name conflicts of CTE relations
Posted by GitBox <gi...@apache.org>.
peter-toth commented on a change in pull request #28371:
URL: https://github.com/apache/spark/pull/28371#discussion_r415932806
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/CTESubstitution.scala
##########
@@ -41,34 +43,45 @@ object CTESubstitution extends Rule[LogicalPlan] {
}
/**
- * Check the plan to be traversed has naming conflicts in nested CTE or not, traverse through
- * child, innerChildren and subquery expressions for the current plan.
+ * Spark 3.0 changes the CTE relations resolution, and inner relations take precedence. This is
+ * correct but we need to warn users about this behavior change under EXCEPTION mode, when we see
+ * CTE relations with conflicting names.
+ *
+ * Note that, before Spark 3.0 the parser didn't support CTE in the FROM clause. For example,
+ * `WITH ... SELECT * FROM (WITH ... SELECT ...)` was not supported. We should not fail for this
+ * case, as Spark versions before 3.0 can't run it anyway. The parameter `startOfQuery` is used
+ * to indicate where we can define CTE relations before Spark 3.0, and we should only check
+ * name conflicts when `startOfQuery` is true.
*/
private def assertNoNameConflictsInCTE(
plan: LogicalPlan,
- outerCTERelationNames: Set[String] = Set.empty,
- namesInSubqueries: Set[String] = Set.empty): Unit = {
+ outerCTERelationNames: Seq[String] = Nil,
+ startOfQuery: Boolean = true): Unit = {
+ val resolver = SQLConf.get.resolver
plan match {
- case w @ With(child, relations) =>
- val newNames = relations.map {
- case (cteName, _) =>
- if (outerCTERelationNames.contains(cteName)) {
- throw new AnalysisException(s"Name $cteName is ambiguous in nested CTE. " +
+ case With(child, relations) =>
+ val newNames = mutable.ArrayBuffer.empty[String]
+ newNames ++= outerCTERelationNames
+ relations.foreach {
+ case (name, relation) =>
+ if (startOfQuery && outerCTERelationNames.exists(resolver(_, name))) {
+ throw new AnalysisException(s"Name $name is ambiguous in nested CTE. " +
s"Please set ${LEGACY_CTE_PRECEDENCE_POLICY.key} to CORRECTED so that name " +
"defined in inner CTE takes precedence. If set it to LEGACY, outer CTE " +
"definitions will take precedence. See more details in SPARK-28228.")
- } else {
- cteName
}
- }.toSet ++ namesInSubqueries
- assertNoNameConflictsInCTE(child, outerCTERelationNames, newNames)
- w.innerChildren.foreach(assertNoNameConflictsInCTE(_, newNames, newNames))
+ assertNoNameConflictsInCTE(relation, newNames)
Review comment:
Do you think we could use `relation.child` here and drop the `SubqueryAlias` case below?
----------------------------------------------------------------
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.
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
[GitHub] [spark] AmplabJenkins removed a comment on pull request #28371: [SPARK-31535][SQL][FOLLOWUP] Simplify name conflict check in CTE resolution
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #28371:
URL: https://github.com/apache/spark/pull/28371#issuecomment-620001729
----------------------------------------------------------------
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.
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
[GitHub] [spark] AmplabJenkins removed a comment on pull request #28371: [SPARK-31577][SQL] Fix various problems when check name conflicts of CTE relations
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #28371:
URL: https://github.com/apache/spark/pull/28371#issuecomment-620091854
----------------------------------------------------------------
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.
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
[GitHub] [spark] AmplabJenkins commented on pull request #28371: [SPARK-31577][SQL] Fix various problems when check name conflicts of CTE relations
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #28371:
URL: https://github.com/apache/spark/pull/28371#issuecomment-620087834
----------------------------------------------------------------
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.
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
[GitHub] [spark] AmplabJenkins commented on pull request #28371: [SPARK-31577][SQL] Fix case-sensitivity and forward name conflict problems when check name conflicts of CTE relations
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #28371:
URL: https://github.com/apache/spark/pull/28371#issuecomment-620281499
----------------------------------------------------------------
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.
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
[GitHub] [spark] dongjoon-hyun commented on pull request #28371: [SPARK-31577][SQL] Fix case-sensitivity and forward name conflict problems when check name conflicts of CTE relations
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on pull request #28371:
URL: https://github.com/apache/spark/pull/28371#issuecomment-620293684
All Scala/Java/Python test passed, but it's timeouted R testing.
I ran the R UT manually.
```
══ testthat results ═══════════════════════════════════════════════════════════
[ OK: 13 | SKIPPED: 0 | WARNINGS: 0 | FAILED: 0 ]
✔ | OK F W S | Context
✔ | 11 | binary functions [1.5 s]
✔ | 4 | functions on binary files [1.3 s]
✔ | 2 | broadcast variables [0.3 s]
✔ | 5 | functions in client.R
✔ | 46 | test functions in sparkR.R [4.4 s]
✔ | 2 | include R packages [0.2 s]
✔ | 2 | JVM API [0.1 s]
✔ | 75 | MLlib classification algorithms, except for tree-based algorithms [55.2 s]
✔ | 70 | MLlib clustering algorithms [23.8 s]
✔ | 6 | MLlib frequent pattern mining [1.8 s]
✔ | 8 | MLlib recommendation algorithms [4.6 s]
✔ | 136 | MLlib regression algorithms, except for tree-based algorithms [50.6 s]
✔ | 8 | MLlib statistics algorithms [0.4 s]
✔ | 94 | MLlib tree-based algorithms [40.7 s]
✔ | 29 | parallelize() and collect() [0.4 s]
✔ | 428 | basic RDD functions [15.0 s]
✔ | 39 | SerDe functionality [2.2 s]
✔ | 20 | partitionBy, groupByKey, reduceByKey etc. [2.1 s]
✔ | 4 | functions in sparkR.R
✔ | 16 | SparkSQL Arrow optimization [10.5 s]
✔ | 6 | test show SparkDataFrame when eager execution is enabled. [0.7 s]
✔ | 1177 | SparkSQL functions [106.5 s]
✔ | 42 | Structured Streaming [53.8 s]
✔ | 16 | tests RDD function take() [0.5 s]
✔ | 14 | the textFile() function [1.3 s]
✔ | 46 | functions in utils.R [0.3 s]
✔ | 0 1 | Windows-specific tests
────────────────────────────────────────────────────────────────────────────────
test_Windows.R:22: skip: sparkJars tag in SparkContext
Reason: This test is only for Windows, skipped
────────────────────────────────────────────────────────────────────────────────
══ Results ═════════════════════════════════════════════════════════════════════
Duration: 378.6 s
OK: 2306
Failed: 0
Warnings: 0
Skipped: 1
```
Thank you so much for this fix, @cloud-fan and @peter-toth .
Merged to master/3.0.
----------------------------------------------------------------
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.
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
[GitHub] [spark] AmplabJenkins removed a comment on pull request #28371: [SPARK-31577][SQL] Fix case-sensitivity and forward name conflict problems when check name conflicts of CTE relations
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #28371:
URL: https://github.com/apache/spark/pull/28371#issuecomment-620281505
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/121920/
Test FAILed.
----------------------------------------------------------------
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.
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
[GitHub] [spark] cloud-fan commented on a change in pull request #28371: [SPARK-31577][SQL] Fix various problems when check name conflicts of CTE relations
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #28371:
URL: https://github.com/apache/spark/pull/28371#discussion_r415953305
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/CTESubstitution.scala
##########
@@ -41,34 +43,45 @@ object CTESubstitution extends Rule[LogicalPlan] {
}
/**
- * Check the plan to be traversed has naming conflicts in nested CTE or not, traverse through
- * child, innerChildren and subquery expressions for the current plan.
+ * Spark 3.0 changes the CTE relations resolution, and inner relations take precedence. This is
+ * correct but we need to warn users about this behavior change under EXCEPTION mode, when we see
+ * CTE relations with conflicting names.
+ *
+ * Note that, before Spark 3.0 the parser didn't support CTE in the FROM clause. For example,
+ * `WITH ... SELECT * FROM (WITH ... SELECT ...)` was not supported. We should not fail for this
+ * case, as Spark versions before 3.0 can't run it anyway. The parameter `startOfQuery` is used
+ * to indicate where we can define CTE relations before Spark 3.0, and we should only check
+ * name conflicts when `startOfQuery` is true.
*/
private def assertNoNameConflictsInCTE(
plan: LogicalPlan,
- outerCTERelationNames: Set[String] = Set.empty,
- namesInSubqueries: Set[String] = Set.empty): Unit = {
+ outerCTERelationNames: Seq[String] = Nil,
+ startOfQuery: Boolean = true): Unit = {
+ val resolver = SQLConf.get.resolver
plan match {
- case w @ With(child, relations) =>
- val newNames = relations.map {
- case (cteName, _) =>
- if (outerCTERelationNames.contains(cteName)) {
- throw new AnalysisException(s"Name $cteName is ambiguous in nested CTE. " +
+ case With(child, relations) =>
+ val newNames = mutable.ArrayBuffer.empty[String]
+ newNames ++= outerCTERelationNames
+ relations.foreach {
+ case (name, relation) =>
+ if (startOfQuery && outerCTERelationNames.exists(resolver(_, name))) {
+ throw new AnalysisException(s"Name $name is ambiguous in nested CTE. " +
s"Please set ${LEGACY_CTE_PRECEDENCE_POLICY.key} to CORRECTED so that name " +
"defined in inner CTE takes precedence. If set it to LEGACY, outer CTE " +
"definitions will take precedence. See more details in SPARK-28228.")
- } else {
- cteName
}
- }.toSet ++ namesInSubqueries
- assertNoNameConflictsInCTE(child, outerCTERelationNames, newNames)
- w.innerChildren.foreach(assertNoNameConflictsInCTE(_, newNames, newNames))
+ assertNoNameConflictsInCTE(relation, newNames)
Review comment:
good idea! updated.
----------------------------------------------------------------
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.
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
[GitHub] [spark] AmplabJenkins commented on pull request #28371: [SPARK-31535][SQL][FOLLOWUP] Simplify name conflict check in CTE resolution
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #28371:
URL: https://github.com/apache/spark/pull/28371#issuecomment-620001729
----------------------------------------------------------------
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.
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
[GitHub] [spark] AmplabJenkins removed a comment on pull request #28371: [SPARK-31577][SQL] Fix case-sensitivity and forward name conflict problems when check name conflicts of CTE relations
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #28371:
URL: https://github.com/apache/spark/pull/28371#issuecomment-620191359
Merged build finished. Test FAILed.
----------------------------------------------------------------
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.
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
[GitHub] [spark] SparkQA removed a comment on pull request #28371: [SPARK-31577][SQL] Fix case-sensitivity and forward name conflict problems when check name conflicts of CTE relations
Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #28371:
URL: https://github.com/apache/spark/pull/28371#issuecomment-620010568
**[Test build #121906 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/121906/testReport)** for PR 28371 at commit [`c4d0a69`](https://github.com/apache/spark/commit/c4d0a69c7ac20cfe5ad68c97f66f6a312a2e2a19).
----------------------------------------------------------------
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.
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
[GitHub] [spark] dongjoon-hyun commented on a change in pull request #28371: [SPARK-31577][SQL] Fix various problems when check name conflicts of CTE relations
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on a change in pull request #28371:
URL: https://github.com/apache/spark/pull/28371#discussion_r415948760
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/CTESubstitution.scala
##########
@@ -41,34 +43,45 @@ object CTESubstitution extends Rule[LogicalPlan] {
}
/**
- * Check the plan to be traversed has naming conflicts in nested CTE or not, traverse through
- * child, innerChildren and subquery expressions for the current plan.
+ * Spark 3.0 changes the CTE relations resolution, and inner relations take precedence. This is
+ * correct but we need to warn users about this behavior change under EXCEPTION mode, when we see
+ * CTE relations with conflicting names.
+ *
+ * Note that, before Spark 3.0 the parser didn't support CTE in the FROM clause. For example,
+ * `WITH ... SELECT * FROM (WITH ... SELECT ...)` was not supported. We should not fail for this
+ * case, as Spark versions before 3.0 can't run it anyway. The parameter `startOfQuery` is used
+ * to indicate where we can define CTE relations before Spark 3.0, and we should only check
+ * name conflicts when `startOfQuery` is true.
*/
private def assertNoNameConflictsInCTE(
plan: LogicalPlan,
- outerCTERelationNames: Set[String] = Set.empty,
- namesInSubqueries: Set[String] = Set.empty): Unit = {
+ outerCTERelationNames: Seq[String] = Nil,
+ startOfQuery: Boolean = true): Unit = {
+ val resolver = SQLConf.get.resolver
plan match {
- case w @ With(child, relations) =>
- val newNames = relations.map {
- case (cteName, _) =>
- if (outerCTERelationNames.contains(cteName)) {
- throw new AnalysisException(s"Name $cteName is ambiguous in nested CTE. " +
+ case With(child, relations) =>
+ val newNames = mutable.ArrayBuffer.empty[String]
+ newNames ++= outerCTERelationNames
+ relations.foreach {
+ case (name, relation) =>
+ if (startOfQuery && outerCTERelationNames.exists(resolver(_, name))) {
Review comment:
My bad. I missed to check case-sensitivity.
----------------------------------------------------------------
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.
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
[GitHub] [spark] AmplabJenkins removed a comment on pull request #28371: [SPARK-31577][SQL] Fix case-sensitivity and forward name conflict problems when check name conflicts of CTE relations
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #28371:
URL: https://github.com/apache/spark/pull/28371#issuecomment-620281499
Merged build finished. Test FAILed.
----------------------------------------------------------------
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.
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
[GitHub] [spark] AmplabJenkins commented on pull request #28371: [SPARK-31577][SQL] Fix case-sensitivity and forward name conflict problems when check name conflicts of CTE relations
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #28371:
URL: https://github.com/apache/spark/pull/28371#issuecomment-620191359
----------------------------------------------------------------
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.
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
[GitHub] [spark] dongjoon-hyun commented on pull request #28371: [SPARK-31577][SQL] Fix case-sensitivity and forward name conflict problems when check name conflicts of CTE relations
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on pull request #28371:
URL: https://github.com/apache/spark/pull/28371#issuecomment-620107765
Thank you so much, @cloud-fan .
----------------------------------------------------------------
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.
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
[GitHub] [spark] dongjoon-hyun edited a comment on pull request #28371: [SPARK-31577][SQL] Fix case-sensitivity and forward name conflict problems when check name conflicts of CTE relations
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun edited a comment on pull request #28371:
URL: https://github.com/apache/spark/pull/28371#issuecomment-620293684
All Scala/Java/Python test passed, but it's timeouted at R testing.
I ran the R UT manually.
```
══ testthat results ═══════════════════════════════════════════════════════════
[ OK: 13 | SKIPPED: 0 | WARNINGS: 0 | FAILED: 0 ]
✔ | OK F W S | Context
✔ | 11 | binary functions [1.5 s]
✔ | 4 | functions on binary files [1.3 s]
✔ | 2 | broadcast variables [0.3 s]
✔ | 5 | functions in client.R
✔ | 46 | test functions in sparkR.R [4.4 s]
✔ | 2 | include R packages [0.2 s]
✔ | 2 | JVM API [0.1 s]
✔ | 75 | MLlib classification algorithms, except for tree-based algorithms [55.2 s]
✔ | 70 | MLlib clustering algorithms [23.8 s]
✔ | 6 | MLlib frequent pattern mining [1.8 s]
✔ | 8 | MLlib recommendation algorithms [4.6 s]
✔ | 136 | MLlib regression algorithms, except for tree-based algorithms [50.6 s]
✔ | 8 | MLlib statistics algorithms [0.4 s]
✔ | 94 | MLlib tree-based algorithms [40.7 s]
✔ | 29 | parallelize() and collect() [0.4 s]
✔ | 428 | basic RDD functions [15.0 s]
✔ | 39 | SerDe functionality [2.2 s]
✔ | 20 | partitionBy, groupByKey, reduceByKey etc. [2.1 s]
✔ | 4 | functions in sparkR.R
✔ | 16 | SparkSQL Arrow optimization [10.5 s]
✔ | 6 | test show SparkDataFrame when eager execution is enabled. [0.7 s]
✔ | 1177 | SparkSQL functions [106.5 s]
✔ | 42 | Structured Streaming [53.8 s]
✔ | 16 | tests RDD function take() [0.5 s]
✔ | 14 | the textFile() function [1.3 s]
✔ | 46 | functions in utils.R [0.3 s]
✔ | 0 1 | Windows-specific tests
────────────────────────────────────────────────────────────────────────────────
test_Windows.R:22: skip: sparkJars tag in SparkContext
Reason: This test is only for Windows, skipped
────────────────────────────────────────────────────────────────────────────────
══ Results ═════════════════════════════════════════════════════════════════════
Duration: 378.6 s
OK: 2306
Failed: 0
Warnings: 0
Skipped: 1
```
Thank you so much for this fix, @cloud-fan and @peter-toth .
Merged to master/3.0.
----------------------------------------------------------------
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.
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
[GitHub] [spark] AmplabJenkins commented on pull request #28371: [SPARK-31577][SQL] Fix various problems when check name conflicts of CTE relations
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #28371:
URL: https://github.com/apache/spark/pull/28371#issuecomment-620091854
----------------------------------------------------------------
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.
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
[GitHub] [spark] SparkQA commented on pull request #28371: [SPARK-31577][SQL] Fix case-sensitivity and forward name conflict problems when check name conflicts of CTE relations
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #28371:
URL: https://github.com/apache/spark/pull/28371#issuecomment-620280857
**[Test build #121920 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/121920/testReport)** for PR 28371 at commit [`24a487d`](https://github.com/apache/spark/commit/24a487dce69a0bc55148164d650259268b45dc4f).
* This patch **fails from timeout after a configured wait of `400m`**.
* This patch merges cleanly.
* This patch adds no public classes.
----------------------------------------------------------------
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.
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
[GitHub] [spark] SparkQA commented on pull request #28371: [SPARK-31577][SQL] Fix case-sensitivity and forward name conflict problems when check name conflicts of CTE relations
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #28371:
URL: https://github.com/apache/spark/pull/28371#issuecomment-620189990
**[Test build #121904 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/121904/testReport)** for PR 28371 at commit [`dd6cec5`](https://github.com/apache/spark/commit/dd6cec59085c4cb8bd93733b0d724cd07fb73cce).
* This patch **fails PySpark unit tests**.
* This patch merges cleanly.
* This patch adds no public classes.
----------------------------------------------------------------
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.
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
[GitHub] [spark] AmplabJenkins commented on pull request #28371: [SPARK-31577][SQL] Fix various problems when check name conflicts of CTE relations
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #28371:
URL: https://github.com/apache/spark/pull/28371#issuecomment-620011435
----------------------------------------------------------------
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.
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
[GitHub] [spark] AmplabJenkins commented on pull request #28371: [SPARK-31577][SQL] Fix case-sensitivity and forward name conflict problems when check name conflicts of CTE relations
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #28371:
URL: https://github.com/apache/spark/pull/28371#issuecomment-620208929
----------------------------------------------------------------
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.
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
[GitHub] [spark] SparkQA commented on pull request #28371: [SPARK-31535][SQL][FOLLOWUP] Simplify name conflict check in CTE resolution
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #28371:
URL: https://github.com/apache/spark/pull/28371#issuecomment-620000996
**[Test build #121904 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/121904/testReport)** for PR 28371 at commit [`dd6cec5`](https://github.com/apache/spark/commit/dd6cec59085c4cb8bd93733b0d724cd07fb73cce).
----------------------------------------------------------------
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.
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
[GitHub] [spark] SparkQA commented on pull request #28371: [SPARK-31577][SQL] Fix various problems when check name conflicts of CTE relations
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #28371:
URL: https://github.com/apache/spark/pull/28371#issuecomment-620010568
**[Test build #121906 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/121906/testReport)** for PR 28371 at commit [`c4d0a69`](https://github.com/apache/spark/commit/c4d0a69c7ac20cfe5ad68c97f66f6a312a2e2a19).
----------------------------------------------------------------
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.
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
[GitHub] [spark] SparkQA removed a comment on pull request #28371: [SPARK-31577][SQL] Fix case-sensitivity and forward name conflict problems when check name conflicts of CTE relations
Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #28371:
URL: https://github.com/apache/spark/pull/28371#issuecomment-620000996
**[Test build #121904 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/121904/testReport)** for PR 28371 at commit [`dd6cec5`](https://github.com/apache/spark/commit/dd6cec59085c4cb8bd93733b0d724cd07fb73cce).
----------------------------------------------------------------
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.
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
[GitHub] [spark] AmplabJenkins removed a comment on pull request #28371: [SPARK-31577][SQL] Fix various problems when check name conflicts of CTE relations
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #28371:
URL: https://github.com/apache/spark/pull/28371#issuecomment-620087834
----------------------------------------------------------------
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.
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
[GitHub] [spark] SparkQA commented on pull request #28371: [SPARK-31577][SQL] Fix case-sensitivity and forward name conflict problems when check name conflicts of CTE relations
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #28371:
URL: https://github.com/apache/spark/pull/28371#issuecomment-620207761
**[Test build #121906 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/121906/testReport)** for PR 28371 at commit [`c4d0a69`](https://github.com/apache/spark/commit/c4d0a69c7ac20cfe5ad68c97f66f6a312a2e2a19).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
----------------------------------------------------------------
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.
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
[GitHub] [spark] dongjoon-hyun commented on a change in pull request #28371: [SPARK-31577][SQL] Fix various problems when check name conflicts of CTE relations
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on a change in pull request #28371:
URL: https://github.com/apache/spark/pull/28371#discussion_r415944837
##########
File path: sql/core/src/test/resources/sql-tests/results/cte-nested.sql.out
##########
@@ -64,10 +64,9 @@ WITH
)
SELECT * FROM t2
-- !query schema
-struct<>
+struct<scalarsubquery():int>
-- !query output
-org.apache.spark.sql.AnalysisException
-Name t is ambiguous in nested CTE. Please set spark.sql.legacy.ctePrecedencePolicy to CORRECTED so that name defined in inner CTE takes precedence. If set it to LEGACY, outer CTE definitions will take precedence. See more details in SPARK-28228.;
Review comment:
Got 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.
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
[GitHub] [spark] AmplabJenkins removed a comment on pull request #28371: [SPARK-31577][SQL] Fix various problems when check name conflicts of CTE relations
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #28371:
URL: https://github.com/apache/spark/pull/28371#issuecomment-620011435
----------------------------------------------------------------
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.
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
[GitHub] [spark] cloud-fan commented on a change in pull request #28371: [SPARK-31535][SQL][FOLLOWUP] Simplify name conflict check in CTE resolution
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #28371:
URL: https://github.com/apache/spark/pull/28371#discussion_r415829102
##########
File path: sql/core/src/test/resources/sql-tests/results/cte-nested.sql.out
##########
@@ -64,10 +64,9 @@ WITH
)
SELECT * FROM t2
-- !query schema
-struct<>
+struct<scalarsubquery():int>
-- !query output
-org.apache.spark.sql.AnalysisException
-Name t is ambiguous in nested CTE. Please set spark.sql.legacy.ctePrecedencePolicy to CORRECTED so that name defined in inner CTE takes precedence. If set it to LEGACY, outer CTE definitions will take precedence. See more details in SPARK-28228.;
Review comment:
I tried this query with Spark 2.4.5 (need to replace `t(c) AS (SELECT 1)` with `t AS (SELECT 1 c)` as Spark 2.4 doesn't support `t(c) syntax`), it fails the parser. So we don't need to fail here.
----------------------------------------------------------------
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.
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