[SPARK-9634][SPARK-9323][SQL] cleanup unnecessary Aliases in LogicalPlan at the end of analysis #7957

cloud-fan · 2015-08-05T09:07:30Z

Also alias the ExtractValue instead of wrapping it with UnresolvedAlias when resolve attribute in LogicalPlan, as this alias will be trimmed if it's unnecessary.

This also fixes https://issues.apache.org/jira/browse/SPARK-9323 as DataFrame.resolve won't return unresolved expression now.

cloud-fan · 2015-08-05T09:08:34Z

cc @JoshRosen , this anwsers your last question at https://issues.apache.org/jira/browse/SPARK-9323
:)

SparkQA · 2015-08-05T09:21:40Z

Test build #39852 has finished for PR 7957 at commit ad81e92.

This patch fails to build.
This patch merges cleanly.
This patch adds no public classes.

cloud-fan · 2015-08-05T09:41:14Z

Seems master is broken?

SparkQA · 2015-08-05T10:35:52Z

Test build #1353 has finished for PR 7957 at commit ad81e92.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- public static final class BinaryPrefixComparator extends PrefixComparator
- public static final class BinaryPrefixComparatorDesc extends PrefixComparator
- case class SequenceNumberRanges(ranges: Seq[SequenceNumberRange])

cloud-fan · 2015-08-05T13:08:36Z

retest this please.

marmbrus · 2015-08-05T18:41:49Z

I think this works, but is there a reason to not just strip all UnresolvedAliases (and possibly nested Aliases) from the plan like we do with subqueries right after analysis?

SparkQA · 2015-08-05T19:01:42Z

Test build #39861 has finished for PR 7957 at commit ad81e92.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- public static final class BinaryPrefixComparator extends PrefixComparator
- public static final class BinaryPrefixComparatorDesc extends PrefixComparator
- case class SequenceNumberRanges(ranges: Seq[SequenceNumberRange])
- case class ArrayContains(left: Expression, right: Expression)

SparkQA · 2015-08-05T19:22:33Z

Test build #232 has finished for PR 7957 at commit ad81e92.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- public static final class BinaryPrefixComparator extends PrefixComparator
- public static final class BinaryPrefixComparatorDesc extends PrefixComparator
- case class SequenceNumberRanges(ranges: Seq[SequenceNumberRange])

cloud-fan · 2015-08-06T00:24:29Z

Hi @marmbrus , we strip UnresolvedAlias from aggregation and projection at Analyzer, which works well for SQL. It will be good to make this rule more general but I'm afaid that will be hard to write. So I think fixing the different behavior between DataFrame and SQL is also reasonable, i.e. making DataFrame.resolve return resolved expressions.

marmbrus · 2015-08-06T21:02:31Z

Why not this after analysis?

plan transformAllExpressions {
  case UnresolvedAlias(child) => child
  case Alias(child, name) => Alias(child transform { case Alias(c, _) => c }, name)
}

SparkQA · 2015-08-07T01:30:11Z

Test build #40120 has finished for PR 7957 at commit 925c87b.

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

marmbrus · 2015-08-07T01:47:34Z

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

Is this duplicated with ResolveAliases?

Sorry I made a mistake here, we should just remove the UnresolvedAlias, just like what we did in Analyzer.ResolveReferences(we call trimUnresolvedAlias there).

The key problem is we have 2 code path to resolve UnresolvedAttribute, one is Analyzer.ResolveReferences(last case) and the other is DataFrame.resolve, so I think we need to make them consistent, i.e. call trimUnresolvedAlias at DataFrame.resolve or abstract this logic for sql and dataframe.

marmbrus · 2015-08-07T01:50:51Z

Thanks for continuing to work on this :)

cloud-fan · 2015-08-11T17:50:55Z

cc @marmbrus , I think this solution is better :)

SparkQA · 2015-08-11T18:31:25Z

Test build #40480 has finished for PR 7957 at commit 290ff75.

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

SparkQA · 2015-08-11T19:07:22Z

Test build #40487 has finished for PR 7957 at commit 27e2435.

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

cloud-fan · 2015-08-12T03:10:11Z

retest this please.

SparkQA · 2015-08-12T03:48:05Z

Test build #40581 has finished for PR 7957 at commit 4cd4fad.

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

SparkQA · 2015-08-12T10:00:22Z

Test build #1483 has finished for PR 7957 at commit 4cd4fad.

This patch fails Spark unit tests.
This patch does not merge cleanly.
This patch adds no public classes.

cloud-fan · 2015-08-12T14:28:49Z

retest this please.

SparkQA · 2015-08-12T15:09:05Z

Test build #40634 has finished for PR 7957 at commit 4cd4fad.

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

marmbrus · 2015-08-12T17:25:25Z

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

test CreateStructUnsafe too?

marmbrus · 2015-08-12T17:26:25Z

LGTM

SparkQA · 2015-08-12T18:02:36Z

Test build #1496 has finished for PR 7957 at commit 4cd4fad.

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

cloud-fan · 2015-08-13T03:34:44Z

sql/core/src/main/scala/org/apache/spark/sql/DataFrame.scala

cc @rxin , in MLlib sometimes we need to set metadata for the new column, thus we will alias the new column with metadata before call withColumn and in withColumn we alias this clolumn again. Here I added a new parameter to allow user set metadata.

Can you do this in a different PR? Also we should do it without using Option and default arguments so that it works well in Java.

SparkQA · 2015-08-13T05:18:20Z

Test build #40726 has finished for PR 7957 at commit 36c5b8b.

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

SparkQA · 2015-08-13T14:22:56Z

Test build #40761 has finished for PR 7957 at commit 4aa631d.

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

cloud-fan · 2015-08-13T14:39:52Z

retest this please.

SparkQA · 2015-08-13T17:14:19Z

Test build #40764 has finished for PR 7957 at commit 4aa631d.

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

marmbrus · 2015-08-14T04:04:54Z

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

Can you remove everything except for this and the related tests? I'd like to pull this into the release branch without new features.

I have opened #8159 to improve the withColumn, but left the code here to see if we can pass the tests.

This PR did 2 things:

use Alias instead of UnresolvedAlias when resolve nested column in LogicalPlan.resolve

clean unnecessary aliases at the end of analysis

If we only do 1, some tests will fail as we need to trim aliases in the middle of getField chain. If we only do 2, it can't fix any bugs. So I put them together here.

I've open #8215 which is basically your patch without the mllib changes.

marmbrus · 2015-08-14T20:54:45Z

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

Not for this release, but this makes me think that we are abusing aliases. I would rather that resolved expressions past the analyzer move the names out of the subexpressions and into the CreateStruct expression itself.

cloud-fan · 2015-08-15T02:34:22Z

closing in favor of #8215

…lPlan at the end of analysis Also alias the ExtractValue instead of wrapping it with UnresolvedAlias when resolve attribute in LogicalPlan, as this alias will be trimmed if it's unnecessary. Based on #7957 without the changes to mllib, but instead maintaining earlier behavior when using `withColumn` on expressions that already have metadata. Author: Wenchen Fan <cloud0fan@outlook.com> Author: Michael Armbrust <michael@databricks.com> Closes #8215 from marmbrus/pr/7957. (cherry picked from commit ec29f20) Signed-off-by: Reynold Xin <rxin@databricks.com>

…lPlan at the end of analysis Also alias the ExtractValue instead of wrapping it with UnresolvedAlias when resolve attribute in LogicalPlan, as this alias will be trimmed if it's unnecessary. Based on #7957 without the changes to mllib, but instead maintaining earlier behavior when using `withColumn` on expressions that already have metadata. Author: Wenchen Fan <cloud0fan@outlook.com> Author: Michael Armbrust <michael@databricks.com> Closes #8215 from marmbrus/pr/7957.

…lPlan at the end of analysis Also alias the ExtractValue instead of wrapping it with UnresolvedAlias when resolve attribute in LogicalPlan, as this alias will be trimmed if it's unnecessary. Based on apache#7957 without the changes to mllib, but instead maintaining earlier behavior when using `withColumn` on expressions that already have metadata. Author: Wenchen Fan <cloud0fan@outlook.com> Author: Michael Armbrust <michael@databricks.com> Closes apache#8215 from marmbrus/pr/7957.

cloud-fan force-pushed the resolve branch from ad81e92 to 925c87b Compare August 7, 2015 01:04

marmbrus reviewed Aug 7, 2015
View reviewed changes

marmbrus mentioned this pull request Aug 10, 2015

[SPARK-9778][SQL] remove unnecessary evaluation from SortOrder #8066

Closed

cloud-fan force-pushed the resolve branch from 925c87b to 290ff75 Compare August 11, 2015 17:46

cloud-fan changed the title ~~[SPARK-9634][SPARK-9323][SQL] resolve UnresolvedAlias in DataFrame.resolve~~ [SPARK-9634][SPARK-9323][SQL] cleanup unnecessary Aliases in LogicalPlan at the end of analysis Aug 11, 2015

cloud-fan force-pushed the resolve branch from 35ff8c0 to 27e2435 Compare August 11, 2015 18:12

cloud-fan added 2 commits August 12, 2015 09:40

cleanup unnecessary Aliases in LogicalPlan at the end of analysis

73f84c0

enhance test

4cd4fad

cloud-fan force-pushed the resolve branch from 27e2435 to 4cd4fad Compare August 12, 2015 01:41

marmbrus reviewed Aug 12, 2015
View reviewed changes

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

Copy link

Contributor

marmbrus Aug 12, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

test CreateStructUnsafe too?

fix mllib

36c5b8b

cloud-fan force-pushed the resolve branch from 0140fc8 to 36c5b8b Compare August 13, 2015 03:30

cloud-fan reviewed Aug 13, 2015
View reviewed changes

cloud-fan mentioned this pull request Aug 13, 2015

[SPARK-9929][SQL] support metadata in withColumn #8159

Closed

fix window function

4aa631d

marmbrus reviewed Aug 14, 2015
View reviewed changes

marmbrus mentioned this pull request Aug 14, 2015

[SPARK-9634][SPARK-9323][SQL] cleanup unnecessary Aliases in LogicalPlan at the end of analysis #8215

Closed

marmbrus reviewed Aug 14, 2015
View reviewed changes

cloud-fan closed this Aug 15, 2015

[SPARK-9634][SPARK-9323][SQL] cleanup unnecessary Aliases in LogicalPlan at the end of analysis #7957

[SPARK-9634][SPARK-9323][SQL] cleanup unnecessary Aliases in LogicalPlan at the end of analysis #7957

Uh oh!

Conversation

cloud-fan commented Aug 5, 2015

Uh oh!

cloud-fan commented Aug 5, 2015

Uh oh!

SparkQA commented Aug 5, 2015

Uh oh!

cloud-fan commented Aug 5, 2015

Uh oh!

SparkQA commented Aug 5, 2015

Uh oh!

cloud-fan commented Aug 5, 2015

Uh oh!

marmbrus commented Aug 5, 2015

Uh oh!

SparkQA commented Aug 5, 2015

Uh oh!

SparkQA commented Aug 5, 2015

Uh oh!

cloud-fan commented Aug 6, 2015

Uh oh!

marmbrus commented Aug 6, 2015

Uh oh!

SparkQA commented Aug 7, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marmbrus commented Aug 7, 2015

Uh oh!

cloud-fan commented Aug 11, 2015

Uh oh!

SparkQA commented Aug 11, 2015

Uh oh!

SparkQA commented Aug 11, 2015

Uh oh!

cloud-fan commented Aug 12, 2015

Uh oh!

SparkQA commented Aug 12, 2015

Uh oh!

SparkQA commented Aug 12, 2015

Uh oh!

cloud-fan commented Aug 12, 2015

Uh oh!

SparkQA commented Aug 12, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marmbrus commented Aug 12, 2015

Uh oh!

SparkQA commented Aug 12, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Aug 13, 2015

Uh oh!

SparkQA commented Aug 13, 2015

Uh oh!

cloud-fan commented Aug 13, 2015

Uh oh!

SparkQA commented Aug 13, 2015

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!

cloud-fan commented Aug 15, 2015

Uh oh!

Reviewers

Assignees