You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by nsyca <gi...@git.apache.org> on 2017/01/13 11:41:14 UTC
[GitHub] spark pull request #16572: [SPARK-18863][SQL] Output non-aggregate expressio...
GitHub user nsyca opened a pull request:
https://github.com/apache/spark/pull/16572
[SPARK-18863][SQL] Output non-aggregate expressions without GROUP BY in a subquery does not yield an error
## What changes were proposed in this pull request?
This PR will report proper error messages when a subquery expression contain an invalid plan. This problem is fixed by calling CheckAnalysis for the plan inside a subquery.
## How was this patch tested?
Existing tests and two new test cases on 2 forms of subquery, namely, scalar subquery and in/exists subquery.
````
-- TC 01.01
-- The column t2b in the SELECT of the subquery is invalid
-- because it is neither an aggregate function nor a GROUP BY column.
select t1a, t2b
from t1, t2
where t1b = t2c
and t2b = (select max(avg)
from (select t2b, avg(t2b) avg
from t2
where t2a = t1.t1b
)
)
;
-- TC 01.02
-- Invalid due to the column t2b not part of the output from table t2.
select *
from t1
where t1a in (select min(t2a)
from t2
group by t2c
having t2c in (select max(t3c)
from t3
group by t3b
having t3b > t2b ))
;
````
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/nsyca/spark 18863
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/16572.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #16572
----
commit b98865127a39bde885f9b1680cfe608629d59d51
Author: Nattavut Sutyanyong <ns...@gmail.com>
Date: 2016-07-29T21:43:56Z
[SPARK-16804][SQL] Correlated subqueries containing LIMIT return incorrect results
## What changes were proposed in this pull request?
This patch fixes the incorrect results in the rule ResolveSubquery in Catalyst's Analysis phase.
## How was this patch tested?
./dev/run-tests
a new unit test on the problematic pattern.
commit 069ed8f8e5f14dca7a15701945d42fc27fe82f3c
Author: Nattavut Sutyanyong <ns...@gmail.com>
Date: 2016-07-29T21:50:02Z
[SPARK-16804][SQL] Correlated subqueries containing LIMIT return incorrect results
## What changes were proposed in this pull request?
This patch fixes the incorrect results in the rule ResolveSubquery in Catalyst's Analysis phase.
## How was this patch tested?
./dev/run-tests
a new unit test on the problematic pattern.
commit edca333c081e6d4e53a91b496fba4a3ef4ee89ac
Author: Nattavut Sutyanyong <ns...@gmail.com>
Date: 2016-07-30T00:28:15Z
New positive test cases
commit 64184fdb77c1a305bb2932e82582da28bb4c0e53
Author: Nattavut Sutyanyong <ns...@gmail.com>
Date: 2016-08-01T13:20:09Z
Fix unit test case failure
commit 29f82b05c9e40e7934397257c674b260a8e8a996
Author: Nattavut Sutyanyong <ns...@gmail.com>
Date: 2016-08-05T17:42:01Z
blocking TABLESAMPLE
commit ac43ab47907a1ccd6d22f920415fbb4de93d4720
Author: Nattavut Sutyanyong <ns...@gmail.com>
Date: 2016-08-05T21:10:19Z
Fixing code styling
commit 631d396031e8bf627eb1f4872a4d3a17c144536c
Author: Nattavut Sutyanyong <ns...@gmail.com>
Date: 2016-08-07T18:39:44Z
Correcting Scala test style
commit 7eb9b2dbba3633a1958e38e0019e3ce816300514
Author: Nattavut Sutyanyong <ns...@gmail.com>
Date: 2016-08-08T02:31:09Z
One (last) attempt to correct the Scala style tests
commit 1387cf51541408ac20048064fa5e559836af932c
Author: Nattavut Sutyanyong <ns...@gmail.com>
Date: 2016-08-12T20:11:50Z
Merge remote-tracking branch 'upstream/master'
commit 3faa2d5edc030495f8b870d2c017cb714c17b6a7
Author: Nattavut Sutyanyong <ns...@gmail.com>
Date: 2016-12-14T16:35:52Z
Merge remote-tracking branch 'upstream/master'
commit a30863457ef49f99aff001b1987da75093c20f86
Author: Nattavut Sutyanyong <ns...@gmail.com>
Date: 2016-12-30T17:18:18Z
Merge remote-tracking branch 'upstream/master'
commit 2f463de8d4bf566e5fd59f39ddef6ceba5cfc894
Author: Nattavut Sutyanyong <ns...@gmail.com>
Date: 2017-01-01T16:18:54Z
first fix (incomplete)
commit 6e2f686f8e516e63235e1e6ccb13bdf8a9e6e314
Author: Nattavut Sutyanyong <ns...@gmail.com>
Date: 2017-01-03T20:06:12Z
first attempt
commit f1524b99aff70e688e4763db7898da53286a321e
Author: Nattavut Sutyanyong <ns...@gmail.com>
Date: 2017-01-03T22:08:03Z
Merge remote-tracking branch 'upstream/master'
commit 6dfa8e5e132fbc489e20e3181a2a2faaa339ec3a
Author: Nattavut Sutyanyong <ns...@gmail.com>
Date: 2017-01-04T01:58:47Z
Merge branch 'master' into 18863
commit e9bdde6e1268170ef89c2a3f402dbd766b5cad00
Author: Nattavut Sutyanyong <ns...@gmail.com>
Date: 2017-01-05T15:31:08Z
New test cases
commit deec874947a7028aa4a7bef0a1b5898609a6d79c
Author: Nattavut Sutyanyong <ns...@gmail.com>
Date: 2017-01-05T16:04:36Z
Masking exprIDs
commit 5c36dce4df0051cdf1957ac448b354db0ee22e2d
Author: Nattavut Sutyanyong <ns...@gmail.com>
Date: 2017-01-13T01:05:45Z
Merge remote-tracking branch 'upstream/master'
commit 98cbd606e215b9e1e57978e604755368e1bf6948
Author: Nattavut Sutyanyong <ns...@gmail.com>
Date: 2017-01-13T01:07:17Z
Merge branch 'master' into 18863
commit bcae3363db60cdf93d1bb9b741f96ec0e088cf0b
Author: Nattavut Sutyanyong <ns...@gmail.com>
Date: 2017-01-13T01:12:55Z
reverse back accidental change
commit 51f7fb92e47e92208f4e7b2d3cd6d9745177509e
Author: Nattavut Sutyanyong <ns...@gmail.com>
Date: 2017-01-13T01:16:05Z
port from SPARK-19017
commit 24397cf6c8728b4dfff22da14dc909dbb3b0a4e5
Author: Nattavut Sutyanyong <ns...@gmail.com>
Date: 2017-01-13T02:19:11Z
remove unrelated comment
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #16572: [SPARK-18863][SQL] Output non-aggregate expressions with...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/16572
Merged build finished. Test PASSed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #16572: [SPARK-18863][SQL] Output non-aggregate expressio...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/spark/pull/16572
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #16572: [SPARK-18863][SQL] Output non-aggregate expressio...
Posted by nsyca <gi...@git.apache.org>.
Github user nsyca commented on a diff in the pull request:
https://github.com/apache/spark/pull/16572#discussion_r95981409
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/CheckAnalysis.scala ---
@@ -117,66 +117,72 @@ trait CheckAnalysis extends PredicateHelper {
failAnalysis(s"Window specification $s is not valid because $m")
case None => w
}
- case s @ ScalarSubquery(query, conditions, _)
+
+ case e @ PredicateSubquery(query, _, _, _) =>
+ checkAnalysis(query)
+ e
+
+ case s @ ScalarSubquery(query, conditions, _) =>
// If no correlation, the output must be exactly one column
- if (conditions.isEmpty && query.output.size != 1) =>
+ if (conditions.isEmpty && query.output.size != 1) {
failAnalysis(
s"Scalar subquery must return only one column, but got ${query.output.size}")
-
- case s @ ScalarSubquery(query, conditions, _) if conditions.nonEmpty =>
-
- // Collect the columns from the subquery for further checking.
- var subqueryColumns = conditions.flatMap(_.references).filter(query.output.contains)
-
- def checkAggregate(agg: Aggregate): Unit = {
- // Make sure correlated scalar subqueries contain one row for every outer row by
- // enforcing that they are aggregates which contain exactly one aggregate expressions.
- // The analyzer has already checked that subquery contained only one output column,
- // and added all the grouping expressions to the aggregate.
- val aggregates = agg.expressions.flatMap(_.collect {
- case a: AggregateExpression => a
- })
- if (aggregates.isEmpty) {
- failAnalysis("The output of a correlated scalar subquery must be aggregated")
- }
-
- // SPARK-18504/SPARK-18814: Block cases where GROUP BY columns
- // are not part of the correlated columns.
- val groupByCols = AttributeSet(agg.groupingExpressions.flatMap(_.references))
- val correlatedCols = AttributeSet(subqueryColumns)
- val invalidCols = groupByCols -- correlatedCols
- // GROUP BY columns must be a subset of columns in the predicates
- if (invalidCols.nonEmpty) {
- failAnalysis(
- "A GROUP BY clause in a scalar correlated subquery " +
- "cannot contain non-correlated columns: " +
- invalidCols.mkString(","))
- }
}
+ else if (conditions.nonEmpty) {
+ // Collect the columns from the subquery for further checking.
+ var subqueryColumns = conditions.flatMap(_.references).filter(query.output.contains)
+
+ def checkAggregate(agg: Aggregate): Unit = {
+ // Make sure correlated scalar subqueries contain one row for every outer row by
+ // enforcing that they are aggregates containing exactly one aggregate expression.
+ // The analyzer has already checked that subquery contained only one output column,
+ // and added all the grouping expressions to the aggregate.
+ val aggregates = agg.expressions.flatMap(_.collect {
+ case a: AggregateExpression => a
+ })
+ if (aggregates.isEmpty) {
+ failAnalysis("The output of a correlated scalar subquery must be aggregated")
+ }
- // Skip subquery aliases added by the Analyzer and the SQLBuilder.
- // For projects, do the necessary mapping and skip to its child.
- def cleanQuery(p: LogicalPlan): LogicalPlan = p match {
- case s: SubqueryAlias => cleanQuery(s.child)
- case p: Project =>
- // SPARK-18814: Map any aliases to their AttributeReference children
- // for the checking in the Aggregate operators below this Project.
- subqueryColumns = subqueryColumns.map {
- xs => p.projectList.collectFirst {
- case e @ Alias(child : AttributeReference, _) if e.exprId == xs.exprId =>
- child
- }.getOrElse(xs)
+ // SPARK-18504/SPARK-18814: Block cases where GROUP BY columns
+ // are not part of the correlated columns.
+ val groupByCols = AttributeSet(agg.groupingExpressions.flatMap(_.references))
+ val correlatedCols = AttributeSet(subqueryColumns)
+ val invalidCols = groupByCols -- correlatedCols
+ // GROUP BY columns must be a subset of columns in the predicates
+ if (invalidCols.nonEmpty) {
+ failAnalysis(
+ "A GROUP BY clause in a scalar correlated subquery " +
+ "cannot contain non-correlated columns: " +
+ invalidCols.mkString(","))
}
+ }
- cleanQuery(p.child)
- case child => child
- }
+ // Skip subquery aliases added by the Analyzer and the SQLBuilder.
+ // For projects, do the necessary mapping and skip to its child.
+ def cleanQuery(p: LogicalPlan): LogicalPlan = p match {
+ case s: SubqueryAlias => cleanQuery(s.child)
+ case p: Project =>
+ // SPARK-18814: Map any aliases to their AttributeReference children
+ // for the checking in the Aggregate operators below this Project.
+ subqueryColumns = subqueryColumns.map {
+ xs => p.projectList.collectFirst {
+ case e @ Alias(child : AttributeReference, _) if e.exprId == xs.exprId =>
+ child
+ }.getOrElse(xs)
+ }
+
+ cleanQuery(p.child)
+ case child => child
+ }
- cleanQuery(query) match {
- case a: Aggregate => checkAggregate(a)
- case Filter(_, a: Aggregate) => checkAggregate(a)
- case fail => failAnalysis(s"Correlated scalar subqueries must be Aggregated: $fail")
+ cleanQuery(query) match {
+ case a: Aggregate => checkAggregate(a)
+ case Filter(_, a: Aggregate) => checkAggregate(a)
+ case fail => failAnalysis(s"Correlated scalar subqueries must be Aggregated: $fail")
+ }
}
+ checkAnalysis(query)
--- End diff --
The best way to view this block of code changes is using a `diff` with `-b`. The main part is to call `checkAnalysis` for both PredicateSubquery and ScalaSubquery.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #16572: [SPARK-18863][SQL] Output non-aggregate expressions with...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/16572
**[Test build #71314 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/71314/testReport)** for PR 16572 at commit [`24397cf`](https://github.com/apache/spark/commit/24397cf6c8728b4dfff22da14dc909dbb3b0a4e5).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #16572: [SPARK-18863][SQL] Output non-aggregate expressions with...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/16572
**[Test build #71960 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/71960/testReport)** for PR 16572 at commit [`010d27a`](https://github.com/apache/spark/commit/010d27a79be684668012fd796d21a085308dd828).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #16572: [SPARK-18863][SQL] Output non-aggregate expressions with...
Posted by hvanhovell <gi...@git.apache.org>.
Github user hvanhovell commented on the issue:
https://github.com/apache/spark/pull/16572
@nsyca it would be nice to make subquery expression part of tree traversal. It does seem risky to me, a lot functions using traversal maintain some state and do not account for the existence of subqueries, so I am a bit weary of trying this out (as it might break stuff in very subtle ways).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #16572: [SPARK-18863][SQL] Output non-aggregate expressions with...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/16572
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/71960/
Test PASSed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #16572: [SPARK-18863][SQL] Output non-aggregate expressions with...
Posted by nsyca <gi...@git.apache.org>.
Github user nsyca commented on the issue:
https://github.com/apache/spark/pull/16572
Note the way the plans inside subqueries are not treated as part of the tree traversal is a common problem. Besides this problem, another was reported in SPARK-19093. Also the way Spark needs to implement a nested call to the Optimizer via the rule `OptimizedSubqueries` to work on those plans is another instance.
One possible solution is to overwrite the tree traversal (`transformUp`, `transformDown`) of class `LogicalPlan` to include those subqueries' plans.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #16572: [SPARK-18863][SQL] Output non-aggregate expressio...
Posted by hvanhovell <gi...@git.apache.org>.
Github user hvanhovell commented on a diff in the pull request:
https://github.com/apache/spark/pull/16572#discussion_r97676813
--- Diff: sql/core/src/test/scala/org/apache/spark/sql/SQLQueryTestSuite.scala ---
@@ -163,7 +163,12 @@ class SQLQueryTestSuite extends QueryTest with SharedSQLContext {
s"-- Number of queries: ${outputs.size}\n\n\n" +
outputs.zipWithIndex.map{case (qr, i) => qr.toString(i)}.mkString("\n\n\n") + "\n"
}
- stringToFile(new File(testCase.resultFile), goldenOutput)
+ val resultFile = new File(testCase.resultFile);
+ val parent = resultFile.getParentFile();
+ if (!parent.exists()) {
+ assert(parent.mkdirs(), "Could not create directory: " + parent)
+ }
+ stringToFile(resultFile, goldenOutput)
--- End diff --
This has been merged.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #16572: [SPARK-18863][SQL] Output non-aggregate expressions with...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/16572
**[Test build #71314 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/71314/testReport)** for PR 16572 at commit [`24397cf`](https://github.com/apache/spark/commit/24397cf6c8728b4dfff22da14dc909dbb3b0a4e5).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #16572: [SPARK-18863][SQL] Output non-aggregate expressio...
Posted by nsyca <gi...@git.apache.org>.
Github user nsyca commented on a diff in the pull request:
https://github.com/apache/spark/pull/16572#discussion_r95982097
--- Diff: sql/core/src/test/scala/org/apache/spark/sql/SQLQueryTestSuite.scala ---
@@ -163,7 +163,12 @@ class SQLQueryTestSuite extends QueryTest with SharedSQLContext {
s"-- Number of queries: ${outputs.size}\n\n\n" +
outputs.zipWithIndex.map{case (qr, i) => qr.toString(i)}.mkString("\n\n\n") + "\n"
}
- stringToFile(new File(testCase.resultFile), goldenOutput)
+ val resultFile = new File(testCase.resultFile);
+ val parent = resultFile.getParentFile();
+ if (!parent.exists()) {
+ assert(parent.mkdirs(), "Could not create directory: " + parent)
+ }
+ stringToFile(resultFile, goldenOutput)
--- End diff --
This addition is ported from the code from [PR-16467](https://github.com/apache/spark/pull/16467) of [SPARK-19017](https://issues.apache.org/jira/browse/SPARK-19017) reviewed by @hvanhovell.
This change is required after the introduction of test files in sub-directories by [SPARK-18871](https://issues.apache.org/jira/browse/SPARK-18871).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #16572: [SPARK-18863][SQL] Output non-aggregate expressions with...
Posted by hvanhovell <gi...@git.apache.org>.
Github user hvanhovell commented on the issue:
https://github.com/apache/spark/pull/16572
Note that the diff is better to read using githubs `w=1` flag: https://github.com/apache/spark/pull/16572/files?w=1
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #16572: [SPARK-18863][SQL] Output non-aggregate expressions with...
Posted by hvanhovell <gi...@git.apache.org>.
Github user hvanhovell commented on the issue:
https://github.com/apache/spark/pull/16572
LGTM - merging to master/2.1/2.0. Thanks!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #16572: [SPARK-18863][SQL] Output non-aggregate expressions with...
Posted by nsyca <gi...@git.apache.org>.
Github user nsyca commented on the issue:
https://github.com/apache/spark/pull/16572
@hvanhovell, I agree it does look risky with this approach. There are a lot of dependencies here. I am pitching in the idea to get your initial thought. Let me do some background and I will share once I have a better idea on this approach.
Thanks again for your time.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #16572: [SPARK-18863][SQL] Output non-aggregate expressio...
Posted by hvanhovell <gi...@git.apache.org>.
Github user hvanhovell commented on a diff in the pull request:
https://github.com/apache/spark/pull/16572#discussion_r97676592
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/CheckAnalysis.scala ---
@@ -117,66 +117,72 @@ trait CheckAnalysis extends PredicateHelper {
failAnalysis(s"Window specification $s is not valid because $m")
case None => w
}
- case s @ ScalarSubquery(query, conditions, _)
+
+ case e @ PredicateSubquery(query, _, _, _) =>
+ checkAnalysis(query)
+ e
+
+ case s @ ScalarSubquery(query, conditions, _) =>
// If no correlation, the output must be exactly one column
- if (conditions.isEmpty && query.output.size != 1) =>
+ if (conditions.isEmpty && query.output.size != 1) {
failAnalysis(
s"Scalar subquery must return only one column, but got ${query.output.size}")
-
- case s @ ScalarSubquery(query, conditions, _) if conditions.nonEmpty =>
-
- // Collect the columns from the subquery for further checking.
- var subqueryColumns = conditions.flatMap(_.references).filter(query.output.contains)
-
- def checkAggregate(agg: Aggregate): Unit = {
- // Make sure correlated scalar subqueries contain one row for every outer row by
- // enforcing that they are aggregates which contain exactly one aggregate expressions.
- // The analyzer has already checked that subquery contained only one output column,
- // and added all the grouping expressions to the aggregate.
- val aggregates = agg.expressions.flatMap(_.collect {
- case a: AggregateExpression => a
- })
- if (aggregates.isEmpty) {
- failAnalysis("The output of a correlated scalar subquery must be aggregated")
- }
-
- // SPARK-18504/SPARK-18814: Block cases where GROUP BY columns
- // are not part of the correlated columns.
- val groupByCols = AttributeSet(agg.groupingExpressions.flatMap(_.references))
- val correlatedCols = AttributeSet(subqueryColumns)
- val invalidCols = groupByCols -- correlatedCols
- // GROUP BY columns must be a subset of columns in the predicates
- if (invalidCols.nonEmpty) {
- failAnalysis(
- "A GROUP BY clause in a scalar correlated subquery " +
- "cannot contain non-correlated columns: " +
- invalidCols.mkString(","))
- }
}
+ else if (conditions.nonEmpty) {
+ // Collect the columns from the subquery for further checking.
+ var subqueryColumns = conditions.flatMap(_.references).filter(query.output.contains)
+
+ def checkAggregate(agg: Aggregate): Unit = {
+ // Make sure correlated scalar subqueries contain one row for every outer row by
+ // enforcing that they are aggregates containing exactly one aggregate expression.
+ // The analyzer has already checked that subquery contained only one output column,
+ // and added all the grouping expressions to the aggregate.
+ val aggregates = agg.expressions.flatMap(_.collect {
+ case a: AggregateExpression => a
+ })
+ if (aggregates.isEmpty) {
+ failAnalysis("The output of a correlated scalar subquery must be aggregated")
+ }
- // Skip subquery aliases added by the Analyzer and the SQLBuilder.
- // For projects, do the necessary mapping and skip to its child.
- def cleanQuery(p: LogicalPlan): LogicalPlan = p match {
- case s: SubqueryAlias => cleanQuery(s.child)
- case p: Project =>
- // SPARK-18814: Map any aliases to their AttributeReference children
- // for the checking in the Aggregate operators below this Project.
- subqueryColumns = subqueryColumns.map {
- xs => p.projectList.collectFirst {
- case e @ Alias(child : AttributeReference, _) if e.exprId == xs.exprId =>
- child
- }.getOrElse(xs)
+ // SPARK-18504/SPARK-18814: Block cases where GROUP BY columns
+ // are not part of the correlated columns.
+ val groupByCols = AttributeSet(agg.groupingExpressions.flatMap(_.references))
+ val correlatedCols = AttributeSet(subqueryColumns)
+ val invalidCols = groupByCols -- correlatedCols
+ // GROUP BY columns must be a subset of columns in the predicates
+ if (invalidCols.nonEmpty) {
+ failAnalysis(
+ "A GROUP BY clause in a scalar correlated subquery " +
+ "cannot contain non-correlated columns: " +
+ invalidCols.mkString(","))
}
+ }
- cleanQuery(p.child)
- case child => child
- }
+ // Skip subquery aliases added by the Analyzer and the SQLBuilder.
+ // For projects, do the necessary mapping and skip to its child.
+ def cleanQuery(p: LogicalPlan): LogicalPlan = p match {
+ case s: SubqueryAlias => cleanQuery(s.child)
+ case p: Project =>
+ // SPARK-18814: Map any aliases to their AttributeReference children
+ // for the checking in the Aggregate operators below this Project.
+ subqueryColumns = subqueryColumns.map {
+ xs => p.projectList.collectFirst {
+ case e @ Alias(child : AttributeReference, _) if e.exprId == xs.exprId =>
+ child
+ }.getOrElse(xs)
+ }
+
+ cleanQuery(p.child)
+ case child => child
+ }
- cleanQuery(query) match {
- case a: Aggregate => checkAggregate(a)
- case Filter(_, a: Aggregate) => checkAggregate(a)
- case fail => failAnalysis(s"Correlated scalar subqueries must be Aggregated: $fail")
+ cleanQuery(query) match {
+ case a: Aggregate => checkAggregate(a)
+ case Filter(_, a: Aggregate) => checkAggregate(a)
+ case fail => failAnalysis(s"Correlated scalar subqueries must be Aggregated: $fail")
+ }
}
+ checkAnalysis(query)
--- End diff --
This has been merged.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #16572: [SPARK-18863][SQL] Output non-aggregate expressions with...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/16572
**[Test build #71960 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/71960/testReport)** for PR 16572 at commit [`010d27a`](https://github.com/apache/spark/commit/010d27a79be684668012fd796d21a085308dd828).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #16572: [SPARK-18863][SQL] Output non-aggregate expressio...
Posted by nsyca <gi...@git.apache.org>.
Github user nsyca commented on a diff in the pull request:
https://github.com/apache/spark/pull/16572#discussion_r97694229
--- Diff: sql/core/src/test/scala/org/apache/spark/sql/SQLQueryTestSuite.scala ---
@@ -223,7 +228,10 @@ class SQLQueryTestSuite extends QueryTest with SharedSQLContext {
} catch {
case a: AnalysisException if a.plan.nonEmpty =>
// Do not output the logical plan tree which contains expression IDs.
- (StructType(Seq.empty), Seq(a.getClass.getName, a.getSimpleMessage))
+ // Also implement a crude way of masking expression IDs in the error message
+ // with a generic pattern "###".
+ (StructType(Seq.empty),
+ Seq(a.getClass.getName, a.getSimpleMessage.replaceAll("#[0-9]+", "###")))
--- End diff --
Fixed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #16572: [SPARK-18863][SQL] Output non-aggregate expressio...
Posted by hvanhovell <gi...@git.apache.org>.
Github user hvanhovell commented on a diff in the pull request:
https://github.com/apache/spark/pull/16572#discussion_r97675872
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/CheckAnalysis.scala ---
@@ -117,66 +117,72 @@ trait CheckAnalysis extends PredicateHelper {
failAnalysis(s"Window specification $s is not valid because $m")
case None => w
}
- case s @ ScalarSubquery(query, conditions, _)
+
+ case e @ PredicateSubquery(query, _, _, _) =>
--- End diff --
It might be better to add a catch all `SubqueryExpression` case after the ScalarSubquery case, instead of adding one specifically aimed at a predicate subquery.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #16572: [SPARK-18863][SQL] Output non-aggregate expressions with...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/16572
Merged build finished. Test PASSed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #16572: [SPARK-18863][SQL] Output non-aggregate expressio...
Posted by nsyca <gi...@git.apache.org>.
Github user nsyca commented on a diff in the pull request:
https://github.com/apache/spark/pull/16572#discussion_r97694210
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/CheckAnalysis.scala ---
@@ -117,66 +117,72 @@ trait CheckAnalysis extends PredicateHelper {
failAnalysis(s"Window specification $s is not valid because $m")
case None => w
}
- case s @ ScalarSubquery(query, conditions, _)
+
+ case e @ PredicateSubquery(query, _, _, _) =>
--- End diff --
Fixed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #16572: [SPARK-18863][SQL] Output non-aggregate expressions with...
Posted by nsyca <gi...@git.apache.org>.
Github user nsyca commented on the issue:
https://github.com/apache/spark/pull/16572
Thank you for your time reviewing this PR.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #16572: [SPARK-18863][SQL] Output non-aggregate expressions with...
Posted by nsyca <gi...@git.apache.org>.
Github user nsyca commented on the issue:
https://github.com/apache/spark/pull/16572
cc @hvanhovell.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #16572: [SPARK-18863][SQL] Output non-aggregate expressions with...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/16572
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/71314/
Test PASSed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #16572: [SPARK-18863][SQL] Output non-aggregate expressio...
Posted by hvanhovell <gi...@git.apache.org>.
Github user hvanhovell commented on a diff in the pull request:
https://github.com/apache/spark/pull/16572#discussion_r97676523
--- Diff: sql/core/src/test/scala/org/apache/spark/sql/SQLQueryTestSuite.scala ---
@@ -223,7 +228,10 @@ class SQLQueryTestSuite extends QueryTest with SharedSQLContext {
} catch {
case a: AnalysisException if a.plan.nonEmpty =>
// Do not output the logical plan tree which contains expression IDs.
- (StructType(Seq.empty), Seq(a.getClass.getName, a.getSimpleMessage))
+ // Also implement a crude way of masking expression IDs in the error message
+ // with a generic pattern "###".
+ (StructType(Seq.empty),
+ Seq(a.getClass.getName, a.getSimpleMessage.replaceAll("#[0-9]+", "###")))
--- End diff --
why not use the same regex/replacement as on line 223?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org