[SPARK-24488] [SQL] Fix issue when generator is aliased multiple times #21508

bkrieger · 2018-06-07T20:17:25Z

What changes were proposed in this pull request?

Currently, the Analyzer throws an exception if you try to nest a generator. However, it special cases generators "nested" in an alias, and allows that. If you try to alias a generator twice, it is not caught by the special case, so an exception is thrown.

This PR trims the unnecessary, non-top-level aliases, so that the generator is allowed.

How was this patch tested?

new tests in AnalysisSuite.

hvanhovell · 2018-06-08T10:51:37Z

ok to test

hvanhovell · 2018-06-08T11:01:15Z

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

-      case MultiAlias(_: Generator, _) => false
-      case other => hasGenerator(other)
+    private def hasNestedGenerator(expr: NamedExpression): Boolean = {
+      CleanupAliases.trimNonTopLevelAliases(expr) match {


CleanupAliases.trimNonTopLevelAliases only strips Alias expressions. Should we also handle the other two cases?

Updated to handle the MultiAlias and UnresolvedAlias, and updated the unit test to test all 3.

SparkQA · 2018-06-08T14:28:10Z

Test build #91564 has finished for PR 21508 at commit 44ae34d.

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

SparkQA · 2018-06-08T18:24:43Z

Test build #91568 has finished for PR 21508 at commit 46c4a55.

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

SparkQA · 2018-06-08T18:41:56Z

Test build #91569 has finished for PR 21508 at commit f174263.

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

gatorsmile · 2018-06-10T23:21:49Z

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

+      }
+    }
+
+    def trimNonTopLevelAliases(e: Expression): Expression = e match {


Instead of duplicating the function here, could we just fixing CleanupAliases.trimNonTopLevelAliases

Sure- I didn't want to break any existing functionality, but I can do that instead.

gatorsmile · 2018-06-10T23:25:42Z

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

+    }
+
+    def trimNonTopLevelAliases(e: Expression): Expression = e match {
+      case a: UnresolvedAlias =>


Do we need to handle UnresolvedAlias?

In my use case, no. But I wasn't sure if another use case would care.

I don't think it'll hurt to handle it.

SparkQA · 2018-06-11T19:03:31Z

Test build #91665 has finished for PR 21508 at commit abd1457.

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

gatorsmile · 2018-06-11T20:30:39Z

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

-      case other => hasGenerator(other)
+    private def hasNestedGenerator(expr: NamedExpression): Boolean = {
+      CleanupAliases.trimNonTopLevelAliases(expr) match {
+        case UnresolvedAlias(_: Generator, _) => false


If we do not have a valid case here, we should not add it. Here, I think we just need to handle the resolved alias.

hasNestedGenerator already handled UnresolvedAlias. I'll change CleanupAliases back to only handling resolved aliases.

SparkQA · 2018-06-11T22:12:38Z

Test build #91674 has finished for PR 21508 at commit 5d5e8e5.

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

bkrieger · 2018-06-11T22:16:07Z

The test failure looks like a flake to me?

SparkQA · 2018-06-12T22:40:25Z

Test build #91725 has finished for PR 21508 at commit e9605dc.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
logInfo(s\"Using output committer class $
public class JavaPowerIterationClusteringExample
class PowerIterationClustering(HasMaxIter, HasWeightCol, JavaParams, JavaMLReadable,

bkrieger · 2018-06-13T13:48:39Z

@gatorsmile @hvanhovell is this good to merge?

bkrieger · 2018-06-15T19:47:38Z

@gatorsmile @hvanhovell can you take a last look at this? I think it's good to merge.

bkrieger · 2018-06-18T15:45:29Z

@gatorsmile @hvanhovell Gentle ping. Let me know if there's someone else who would be better to review.

bkrieger · 2018-06-21T20:36:09Z

@gatorsmile @hvanhovell can you take another look at this?

mccheah · 2018-06-25T21:22:33Z

@gatorsmile @hvanhovell, I'm working with @bkrieger and we need this patch soon. May we please get a sign off or else any suggested changes here?

SparkQA · 2018-07-17T18:55:49Z

Test build #93177 has finished for PR 21508 at commit 3021918.

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

bkrieger · 2018-07-20T21:05:19Z

@gatorsmile @hvanhovell any chance you can take a look at this?

gatorsmile · 2018-07-20T21:57:01Z

cc @maropu Help review this?

hvanhovell

LGTM, merging to master. Sorry for the hold-up.

Currently, the Analyzer throws an exception if your try to nest a generator. However, it special cases generators "nested" in an alias, and allows that. If you try to alias a generator twice, it is not caught by the special case, so an exception is thrown. This PR trims the unnecessary, non-top-level aliases, so that the generator is allowed. new tests in AnalysisSuite. Author: Brandon Krieger <[email protected]> Closes apache#21508 from bkrieger/bk/SPARK-24488.

SPARK-24488

44ae34d

hvanhovell reviewed Jun 8, 2018

View reviewed changes

Brandon Krieger added 2 commits June 8, 2018 10:32

unresolved and multi too

46c4a55

more unit test

f174263

gatorsmile reviewed Jun 10, 2018

View reviewed changes

share code

abd1457

gatorsmile reviewed Jun 11, 2018

View reviewed changes

only handle resolved

5d5e8e5

Merge branch 'master' into bk/SPARK-24488

e9605dc

Merge branch 'master' into bk/SPARK-24488

3021918

hvanhovell approved these changes Jul 20, 2018

View reviewed changes

asfgit closed this in 597bdef Jul 20, 2018

bkrieger deleted the bk/SPARK-24488 branch July 23, 2018 13:48

bkrieger mentioned this pull request Jul 23, 2018

[SPARK-24488][SQL] Fix issue when generator is aliased multiple times palantir/spark#390

Closed

[SPARK-24488] [SQL] Fix issue when generator is aliased multiple times #21508

[SPARK-24488] [SQL] Fix issue when generator is aliased multiple times #21508

Uh oh!

Conversation

bkrieger commented Jun 7, 2018 • 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!

hvanhovell commented Jun 8, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Jun 8, 2018

Uh oh!

SparkQA commented Jun 8, 2018

Uh oh!

SparkQA commented Jun 8, 2018

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!

SparkQA commented Jun 11, 2018

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!

SparkQA commented Jun 11, 2018

Uh oh!

bkrieger commented Jun 11, 2018

Uh oh!

SparkQA commented Jun 12, 2018

Uh oh!

bkrieger commented Jun 13, 2018

Uh oh!

bkrieger commented Jun 15, 2018

Uh oh!

bkrieger commented Jun 18, 2018

Uh oh!

bkrieger commented Jun 21, 2018

Uh oh!

mccheah commented Jun 25, 2018

Uh oh!

SparkQA commented Jul 17, 2018

Uh oh!

bkrieger commented Jul 20, 2018

Uh oh!

gatorsmile commented Jul 20, 2018

Uh oh!

hvanhovell left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

bkrieger commented Jun 7, 2018 •

edited

Loading