[SPARK-20311][SQL] Support aliases for table value functions #17666

maropu · 2017-04-18T01:09:44Z

What changes were proposed in this pull request?

This pr added parsing rules to support aliases in table value functions.

How was this patch tested?

Added tests in PlanParserSuite.

SparkQA · 2017-04-18T02:39:40Z

Test build #75876 has finished for PR 17666 at commit a611a13.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
case class UnresolvedTableValuedFunction(

SparkQA · 2017-04-18T06:32:36Z

Test build #75888 has started for PR 17666 at commit 539a9e8.

maropu · 2017-04-18T07:10:13Z

Jenkins, retest this please.

SparkQA · 2017-04-18T09:28:44Z

Test build #75889 has finished for PR 17666 at commit 539a9e8.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
case class UnresolvedTableValuedFunction(

hvanhovell · 2017-04-18T13:21:01Z

sql/catalyst/src/main/antlr4/org/apache/spark/sql/catalyst/parser/SqlBase.g4

+    | '(' queryNoWith ')' sample? (AS? strictIdentifier)?                                    #aliasedQuery
+    | '(' relation ')' sample? (AS? strictIdentifier)?                                       #aliasedRelation
+    | inlineTable                                                                            #inlineTableDefault2
+    | identifier '(' (expression (',' expression)*)? ')' (AS? identifier identifierList?)?   #tableValuedFunction


Perhaps we should put the multi-alias in a separate rule? Since it is also used by inline table.

Why did you add the multi-alias anyway?

I missed the point. okay, I'll reconsider this. Thanks!

It makes sense to add this. Lets keep the multi-alias for now.

oh, it seems I misunderstood what you pointed out. You meant should we need to support a query like SELECT * FROM [[tvf]] AS t(a, b, ...) in this pr? Yea, I know we currently support range only as a table value function though, I also think it'd be better to put a more general rule in this file. So, +1 for keeping this.

Then, I'll update this pr to separate this rule and share it with the inline table rule.

SparkQA · 2017-04-18T17:58:58Z

Test build #75903 has finished for PR 17666 at commit 8d3a037.

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

maropu · 2017-04-18T23:35:43Z

@hvanhovell Could you check again?

maropu · 2017-04-21T01:25:48Z

ping

maropu · 2017-04-24T23:53:32Z

@hvanhovell ping

maropu · 2017-05-08T12:24:55Z

@hvanhovell @gatorsmile ping

gatorsmile · 2017-05-08T16:52:25Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/unresolved.scala

@@ -69,7 +69,10 @@ case class UnresolvedInlineTable(
 *   select * from range(10);


Please update this example.

gatorsmile · 2017-05-08T16:55:08Z

sql/catalyst/src/main/antlr4/org/apache/spark/sql/catalyst/parser/SqlBase.g4

+    | '(' queryNoWith ')' sample? (AS? strictIdentifier)?             #aliasedQuery
+    | '(' relation ')' sample? (AS? strictIdentifier)?                #aliasedRelation
+    | inlineTable                                                     #inlineTableDefault2
+    | identifier '(' (expression (',' expression)*)? ')' tableAlias   #tableValuedFunction


Please follow inlineTable to add a separate rule.

gatorsmile · 2017-05-08T18:23:21Z

...lyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveTableValuedFunctions.scala

      })
  )

+  private def validateInputDimension(tvf: UnresolvedTableValuedFunction, expectedNumCols: Int)


Conceptually, expectedNumCols needs to be part of type/class TVF. This is the num of TVF's output arguments.

BTW, we need to add comments to this function.

gatorsmile · 2017-05-08T18:34:43Z

...lyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveTableValuedFunctions.scala

-        Range(0, end, 1, None)
+      tvf("end" -> LongType) { case (tvf, args @ Seq(end: Long)) =>
+        validateInputDimension(tvf, 1)
+        Range(0, end, 1, None, tvf.outputNames.headOption)


Instead of changing the output of Range, I think we can simply add a Project above Range for column alias, like what we do in the Dataframe API: spark.range(100).toDF("i")

great suggestion! I could make it simpler (https://github.com/apache/spark/pull/17666/files#diff-e2023817ad13201adeb45b74e99d29f8R129). Thought?

hvanhovell · 2017-05-08T21:28:28Z

...alyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/basicLogicalOperators.scala

-  def apply(start: Long, end: Long, step: Long, numSlices: Option[Int]): Range = {
-    val output = StructType(StructField("id", LongType, nullable = false) :: Nil).toAttributes
+
+  def apply(start: Long, end: Long, step: Long, numSlices: Option[Int], outputName: Option[String])


Why make this an option? It is not optional right? The next function should either call Range(start, end, step, Some(numSlices), "id") or this function should have a default parameter.

yea, ok. I'll update. Thanks!

hvanhovell · 2017-05-08T21:30:49Z

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/analysis/AnalysisSuite.scala

+
+  test("SPARK-20311 range(N) as alias") {
+    def rangeWithAliases(outputNames: Seq[String]): LogicalPlan = {
+      SubqueryAlias("t", UnresolvedTableValuedFunction("range", Literal(7) :: Nil, outputNames))


Should we also test different cases?

ok, I'll add more tests.

I a bit worry though, this fix satisfies your intention? 625dbda

hvanhovell · 2017-05-08T21:45:31Z

...lyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveTableValuedFunctions.scala

-  private def tvf(args: (String, DataType)*)(pf: PartialFunction[Seq[Any], LogicalPlan])
-      : (ArgumentList, Seq[Any] => LogicalPlan) = {
+  private def tvf(args: (String, DataType)*)(
+      pf: PartialFunction[(UnresolvedTableValuedFunction, Seq[Any]), LogicalPlan])


Hmmm... this signature is kind of complex. Can we try to use some kind of class/case class that encapsulates this?

I also think that we should separate the aliasing from constructing the table valued function. See @gatorsmile's earlier comment.

ok, I'll re-consider the signature.

SparkQA · 2017-05-09T03:22:12Z

Test build #76604 has finished for PR 17666 at commit 399d823.

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

felixcheung · 2017-05-09T03:35:09Z

Please rebase to pick up fix for the R tests.

Though again, I'm not sure why it is running R tests for this PR - is the change detection logic broken somehow?

maropu · 2017-05-09T03:57:47Z

ok, thanks! no, I think I do not touch on that.

cloud-fan · 2017-05-09T04:16:38Z

LGTM

SparkQA · 2017-05-09T04:18:05Z

Test build #76602 has finished for PR 17666 at commit f494e41.

This patch passes all tests.
This patch does not merge cleanly.
This patch adds no public classes.

SparkQA · 2017-05-09T04:19:12Z

Test build #76608 has finished for PR 17666 at commit 625dbda.

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

gatorsmile · 2017-05-09T05:35:38Z

LGTM.

Thank you! @maropu

SparkQA · 2017-05-09T06:19:38Z

Test build #76615 has finished for PR 17666 at commit 81bef3b.

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

gatorsmile · 2017-05-09T07:03:06Z

retest this please

SparkQA · 2017-05-09T09:45:45Z

Test build #76650 has finished for PR 17666 at commit 81bef3b.

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

## What changes were proposed in this pull request? This pr added parsing rules to support aliases in table value functions. ## How was this patch tested? Added tests in `PlanParserSuite`. Author: Takeshi Yamamuro <yamamuro@apache.org> Closes #17666 from maropu/SPARK-20311. (cherry picked from commit 714811d) Signed-off-by: Wenchen Fan <wenchen@databricks.com>

cloud-fan · 2017-05-09T12:24:21Z

thanks, merging to master/2.2!

yhuai · 2017-05-09T21:40:33Z

@maropu Sorry. I think this PR introduces a regression.

scala> spark.sql("select * from range(1, 10) cross join range(1, 10)").explain
== Physical Plan ==
org.apache.spark.sql.AnalysisException: Detected cartesian product for INNER join between logical plans
Range (1, 10, step=1, splits=None)
and
Range (1, 10, step=1, splits=None)
Join condition is missing or trivial.
Use the CROSS JOIN syntax to allow cartesian products between these relations.;

I think we are taking the cross as the alias.

I reverted your change locally and the query worked. I am attaching the expected analyzed plan below.

scala> spark.sql("select * from range(1, 10) cross join range(1, 10)").queryExecution.analyzed
res1: org.apache.spark.sql.catalyst.plans.logical.LogicalPlan =
Project [id#8L, id#9L]
+- Join Cross
   :- Range (1, 10, step=1, splits=None)
   +- Range (1, 10, step=1, splits=None)

yhuai · 2017-05-09T21:44:48Z

I am going to revert this PR from master and branch-2.2. I need to revert it because it is in branch-2.2 and 2.2 is in the RC staging.

yhuai · 2017-05-09T21:50:53Z

I have reverted this change from both master and branch-2.2. I have reopened the jira.

maropu · 2017-05-09T23:14:06Z

@yhuai okay, thanks for letting me know! I'll make a new pr to fix.

## What changes were proposed in this pull request? This pr added parsing rules to support aliases in table value functions. The previous pr (apache#17666) has been reverted because of the regression. This new pr fixed the regression and add tests in `SQLQueryTestSuite`. ## How was this patch tested? Added tests in `PlanParserSuite` and `SQLQueryTestSuite`. Author: Takeshi Yamamuro <yamamuro@apache.org> Closes apache#17928 from maropu/SPARK-20311-3.

## What changes were proposed in this pull request? This pr added parsing rules to support aliases in table value functions. ## How was this patch tested? Added tests in `PlanParserSuite`. Author: Takeshi Yamamuro <yamamuro@apache.org> Closes apache#17666 from maropu/SPARK-20311.

## What changes were proposed in this pull request? This pr added parsing rules to support aliases in table value functions. The previous pr (apache#17666) has been reverted because of the regression. This new pr fixed the regression and add tests in `SQLQueryTestSuite`. ## How was this patch tested? Added tests in `PlanParserSuite` and `SQLQueryTestSuite`. Author: Takeshi Yamamuro <yamamuro@apache.org> Closes apache#17928 from maropu/SPARK-20311-3.

maropu force-pushed the SPARK-20311 branch from a611a13 to 539a9e8 Compare April 18, 2017 06:28

hvanhovell reviewed Apr 18, 2017

View reviewed changes

gatorsmile reviewed May 8, 2017

View reviewed changes

hvanhovell reviewed May 8, 2017

View reviewed changes

maropu added 3 commits May 9, 2017 09:55

Support aliases for table value functions

02e7007

Add a rule for table aliases

50fc519

Apply comments

399d823

maropu force-pushed the SPARK-20311 branch from f494e41 to 399d823 Compare May 9, 2017 00:57

Add more test cases

81bef3b

maropu force-pushed the SPARK-20311 branch from 625dbda to 81bef3b Compare May 9, 2017 04:01

asfgit closed this in 714811d May 9, 2017

maropu mentioned this pull request May 10, 2017

[SPARK-20311][SQL] Support aliases for table value functions #17928

Closed

		@@ -69,7 +69,10 @@ case class UnresolvedInlineTable(
		* select * from range(10);

[SPARK-20311][SQL] Support aliases for table value functions #17666

[SPARK-20311][SQL] Support aliases for table value functions #17666

Conversation

maropu commented Apr 18, 2017

What changes were proposed in this pull request?

How was this patch tested?

SparkQA commented Apr 18, 2017

SparkQA commented Apr 18, 2017

maropu commented Apr 18, 2017

SparkQA commented Apr 18, 2017

hvanhovell Apr 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maropu Apr 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Apr 18, 2017

maropu commented Apr 18, 2017

maropu commented Apr 21, 2017

maropu commented Apr 24, 2017

maropu commented May 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hvanhovell May 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented May 9, 2017

felixcheung commented May 9, 2017

maropu commented May 9, 2017

cloud-fan commented May 9, 2017

SparkQA commented May 9, 2017

SparkQA commented May 9, 2017

gatorsmile commented May 9, 2017

SparkQA commented May 9, 2017

gatorsmile commented May 9, 2017

SparkQA commented May 9, 2017

cloud-fan commented May 9, 2017

yhuai commented May 9, 2017

yhuai commented May 9, 2017 • edited Loading

yhuai commented May 9, 2017

maropu commented May 9, 2017

hvanhovell Apr 18, 2017 •

edited

Loading

maropu Apr 18, 2017 •

edited

Loading

maropu commented May 8, 2017 •

edited

Loading

hvanhovell May 8, 2017 •

edited

Loading

yhuai commented May 9, 2017 •

edited

Loading