[SPARK-20690][SQL] Subqueries in FROM should have alias names by viirya · Pull Request #17935 · apache/spark

viirya · 2017-05-10T08:32:03Z

What changes were proposed in this pull request?

We add missing attributes into Filter in Analyzer. But we shouldn't do it through subqueries like this:

select 1 from  (select 1 from onerow t1 LIMIT 1) where  t1.c1=1

This query works in current codebase. However, the outside where clause shouldn't be able to refer t1.c1 attribute.

The root cause is we allow subqueries in FROM have no alias names previously, it is confusing and isn't supported by various databases such as MySQL, Postgres, Oracle. We shouldn't support it too.

How was this patch tested?

Jenkins tests.

Please review http://spark.apache.org/contributing.html before opening a pull request.

SparkQA · 2017-05-10T09:14:08Z

Test build #76743 has finished for PR 17935 at commit bb16061.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2017-05-10T10:45:14Z

Test build #76745 has finished for PR 17935 at commit 974217f.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2017-05-11T03:49:37Z

Test build #76771 has finished for PR 17935 at commit 51aafde.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
case class ColumnStatsMap(originalMap: AttributeMap[ColumnStat])
s\"($

SparkQA · 2017-05-11T05:23:58Z

Test build #76772 has finished for PR 17935 at commit c19cbbb.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2017-05-11T06:30:22Z

Test build #76783 has started for PR 17935 at commit a904214.

viirya · 2017-05-11T07:08:17Z

retest this please.

SparkQA · 2017-05-11T09:35:55Z

Test build #76785 has finished for PR 17935 at commit a904214.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

viirya · 2017-05-11T13:51:57Z

cc @cloud-fan

cloud-fan · 2017-05-12T05:04:23Z

+        sql(
+          """
+            | select 1
+            | from   (select 1 from onerow t1 LIMIT 1)


I'm surprised we support this syntax, I think the FROM clause must have an alias.

I checked with postgres, it will throw exception subquery in FROM must have an alias, can you check with other databases? Thanks!

mysql:

mysql> select 1 from (select 1 from test); ERROR 1248 (42000): Every derived table must have its own alias

https://cwiki.apache.org/confluence/display/Hive/LanguageManual+SubQueries

Hive supports subqueries only in the FROM clause (through Hive 0.12). The subquery has to be given a name because every table in a FROM clause must have a name.

Hive also requires an alias name.

https://docs.oracle.com/cd/E17952_01/mysql-5.1-en/from-clause-subqueries.html

The [AS] name clause is mandatory, because every table in a FROM clause must have a name. Any columns in the subquery select list must have unique names.

Oracle also requires it.

cc @hvanhovell shall we change the parser? I think it's hard to reason about the semantic of an anonymous subquery

Yeah, in this change I remove qualifier after an anonymous subquery. Not sure if it is what we always want.

I think we should change the parser and require alias for subquery.

Yeah, this seems confusing. Subqueries should be have an alias. Let's try to add that.

cloud-fan · 2017-05-16T05:06:20Z

      Row(3, 3.0, 2, 3.0) :: Row(3, 3.0, 2, 3.0) :: Nil)
  }

+  test("SPARK-20690: Do not add missing attributes through subqueries") {


do we still need this test? I think it's all covered by the parser test

Yes. We can remove this.

cloud-fan · 2017-05-16T05:16:10Z

 relationPrimary
    : tableIdentifier sample? (AS? strictIdentifier)?      #tableName
-    | '(' queryNoWith ')' sample? (AS? strictIdentifier)?  #aliasedQuery
+    | '(' queryNoWith ')' sample? (AS? strictIdentifier)   #aliasedQuery


shall we also update AstBuilder for this?

Should we also force an alias for the next line? '(' relation ')' sample? (AS? strictIdentifier)?

I also have this question when I change this.

MySQL supports select 1 from (test); but doesn't support select 1 from (test) as a ;

Postgres doesn't support both syntax.

Hive supports select * from (test); but doesn't support select * from (test) as a;.

Seems aliased relation is not commonly supported. So I leave it untouched.

Yay, unclear semantics. Ok, that is fine for now.

gatorsmile · 2017-05-16T07:47:09Z

+ * Aliased subquery.
+ *
+ * @param alias the alias name for this subquery.
+ * @param child the LogicalPlan


Nit: the LogicalPlan -> the logical plan of this subquery

gatorsmile · 2017-05-16T08:01:36Z

        }

-      case f @ Filter(cond, child) if child.resolved =>
+      case f @ Filter(cond, child) if !f.resolved && child.resolved =>


Only added for Filter? How about Sort in the same rule?

hvanhovell

One small question, otherwise LGTM.

hvanhovell · 2017-05-16T14:20:27Z

 relationPrimary
    : tableIdentifier sample? (AS? strictIdentifier)?      #tableName
-    | '(' queryNoWith ')' sample? (AS? strictIdentifier)?  #aliasedQuery
+    | '(' queryNoWith ')' sample? (AS? strictIdentifier)   #aliasedQuery


Should we also force an alias for the next line? '(' relation ')' sample? (AS? strictIdentifier)?

SparkQA · 2017-05-16T16:48:11Z

Test build #76969 has finished for PR 17935 at commit 1e595b2.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2017-05-17T03:14:54Z

Test build #76994 has finished for PR 17935 at commit 9ec0414.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

viirya · 2017-05-17T04:31:25Z

Sure.

…

On May 17, 2017 12:10 PM, "Wenchen Fan" ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In sql/core/src/test/scala/org/apache/spark/sql/CachedTableSuite.scala <#17935 (comment)>: > @@ -631,13 +631,13 @@ class CachedTableSuite extends QueryTest with SQLTestUtils with SharedSQLContext val ds2 = sql( """ - |SELECT * FROM (SELECT max(c1) FROM t1 GROUP BY c1) + |SELECT * FROM (SELECT max(c1) as c1 FROM t1 GROUP BY c1) t1 can you pick another name? t1 appeared twice... — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#17935 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAEM97DecogWUFu4HTI-KqSoijpKuJPsks5r6nMcgaJpZM4NWVlc> .

cloud-fan · 2017-05-17T04:58:04Z

thanks, merging to master!

cloud-fan · 2017-05-17T04:59:24Z

oops, you pushed a new commit... But it should be fine as the change is very small.

viirya · 2017-05-17T05:00:05Z

Thanks @cloud-fan @hvanhovell @gatorsmile @wzhfy

viirya · 2017-05-17T05:00:49Z

@cloud-fan Yeah, I change the aliased name in one test. I tested it locally.

SparkQA · 2017-05-17T06:53:49Z

Test build #77007 has finished for PR 17935 at commit 9b117f4.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

viirya · 2017-05-17T08:42:46Z

No problem. The new commit passes tests.

## What changes were proposed in this pull request? We add missing attributes into Filter in Analyzer. But we shouldn't do it through subqueries like this: select 1 from (select 1 from onerow t1 LIMIT 1) where t1.c1=1 This query works in current codebase. However, the outside where clause shouldn't be able to refer `t1.c1` attribute. The root cause is we allow subqueries in FROM have no alias names previously, it is confusing and isn't supported by various databases such as MySQL, Postgres, Oracle. We shouldn't support it too. ## How was this patch tested? Jenkins tests. Please review http://spark.apache.org/contributing.html before opening a pull request. Author: Liang-Chi Hsieh <viirya@gmail.com> Closes apache#17935 from viirya/SPARK-20690.

JoshRosen · 2017-05-30T00:52:21Z

I was trying to run a test case from another database which does support unaliased subqueries in the FROM clause and hit a confusing parser error due to this patch's behavior change. While I agree that we shouldn't necessarily support this syntax, I think that the current error message that we're returning isn't very good so I've file https://issues.apache.org/jira/browse/SPARK-20916 to improve it.

viirya · 2017-05-30T01:12:38Z

@JoshRosen Thanks for filing this issue. I'll look into it.

ash211 · 2017-06-05T22:21:36Z

@JoshRosen what was the other type of database you were using?

JoshRosen · 2017-06-08T21:03:31Z

@ash211, I was attempting to re-use test cases from CockroachDB (which is surprisingly permissive in the SQL it accepts compared to Postgres).

rxin · 2017-06-29T17:47:22Z

Guys - isn't this API breaking?

rxin · 2017-06-29T17:49:28Z

Also the description / title is completely different from the JIRA ticket.

viirya · 2017-06-29T23:47:59Z

@rxin Sorry I forgot to change JIRA ticket. I changed it now.

rxin · 2017-06-30T01:31:34Z

Other committers please revert this change until we find a solution or verify that almost no users write queries like this.

viirya · 2017-06-30T02:19:54Z

I'm ok to revert this. Just a little reference.

Seems it is required to have alias name for derived table in SQL 2003 grammar:
https://github.com/ronsavage/SQL/blob/master/sql-2003-2.bnf#L2221

And as investigating before, MySQL, Postgres, Oracle require alias name for derived table.

So it seems to me that the syntax of no alias name is incorrect and the users tending to write queries like this is expected to be few.

For your reference.

rxin · 2017-06-30T05:03:42Z

I don't think that argument is useful at all. For example, none of the other databases support the DataFrame API. Does that mean few users will write DataFrame code?

viirya · 2017-06-30T05:26:41Z

Sorry, to be accurate, for the syntax of derived table in SQL, the databases I listed above are commonly seen in the market, and they don't support it without alias name. SQL 2003 grammar also doesn't support it.

Based on the above, I'd tend to think that derived table without alias name is not a widely used syntax among database users.

But we know there's exception such as CockroachDB, as @JoshRosen pointed out. And you're right that, it's possible there're already many Spark users using this syntax, as we support it.

This isn't an argument to object reverting, rather just a reference for you to decide on this issue.

rxin · 2017-06-30T05:34:10Z

The reason I found out about this is because the one of the widely circulated TPC-DS benchmark harness online uses this.

viirya force-pushed the SPARK-20690 branch from bb16061 to 974217f Compare May 10, 2017 09:02

Analyzer shouldn't add missing attributes through subqueries.

c079946

viirya force-pushed the SPARK-20690 branch from 974217f to c079946 Compare May 11, 2017 03:43

Merge remote-tracking branch 'upstream/master' into SPARK-20690

51aafde

Fix merging.

c19cbbb

Fix test.

a904214

viirya changed the title ~~[SPARK-20690][SQL][WIP] Analyzer shouldn't add missing attributes through subqueries~~ [SPARK-20690][SQL] Analyzer shouldn't add missing attributes through subqueries May 11, 2017

cloud-fan reviewed May 12, 2017

View reviewed changes

viirya added 2 commits May 16, 2017 02:51

Require alias name for aliasedQuery.

1e25926

Merge remote-tracking branch 'upstream/master' into SPARK-20690

307ca1d

viirya changed the title ~~[SPARK-20690][SQL] Analyzer shouldn't add missing attributes through subqueries~~ [SPARK-20690][SQL] Subqueries in FROM should have alias names May 16, 2017

cloud-fan reviewed May 16, 2017

View reviewed changes

gatorsmile reviewed May 16, 2017

View reviewed changes

Address comments.

1e595b2

hvanhovell approved these changes May 16, 2017

View reviewed changes

Fix tests.

9ec0414

Address comment.

9b117f4

asfgit closed this in 7463a88 May 17, 2017

viirya deleted the SPARK-20690 branch December 27, 2023 18:20

Conversation

viirya commented May 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

SparkQA commented May 10, 2017

Uh oh!

SparkQA commented May 10, 2017

Uh oh!

SparkQA commented May 11, 2017

Uh oh!

SparkQA commented May 11, 2017

Uh oh!

SparkQA commented May 11, 2017

Uh oh!

viirya commented May 11, 2017

Uh oh!

SparkQA commented May 11, 2017

Uh oh!

viirya commented May 11, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hvanhovell left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented May 16, 2017

Uh oh!

SparkQA commented May 17, 2017

Uh oh!

viirya commented May 17, 2017 via email

Uh oh!

cloud-fan commented May 17, 2017

Uh oh!

cloud-fan commented May 17, 2017

Uh oh!

viirya commented May 17, 2017

Uh oh!

viirya commented May 17, 2017

Uh oh!

SparkQA commented May 17, 2017

Uh oh!

viirya commented May 17, 2017

viirya commented May 10, 2017 •

edited

Loading

viirya commented Jun 30, 2017 •

edited

Loading

viirya commented Jun 30, 2017 •

edited

Loading