[SPARK-15094][SPARK-14803][SQL] Remove extra Project added in EliminateSerialization #12926

viirya · 2016-05-05T07:44:57Z

What changes were proposed in this pull request?

We will eliminate the pair of DeserializeToObject and SerializeFromObject in Optimizer and add extra Project. However, when DeserializeToObject's outputObjectType is ObjectType and its cls can't be processed by unsafe project, it will be failed.

To fix it, we can simply remove the extra Project and replace the output attribute of DeserializeToObject in another rule.

How was this patch tested?

DatasetSuite.

…lass for Project.

viirya · 2016-05-05T07:45:23Z

This is another approach to #12898.

SparkQA · 2016-05-05T08:55:18Z

Test build #57858 has finished for PR 12926 at commit 48e6b6d.

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

SparkQA · 2016-05-05T10:56:27Z

Test build #57873 has finished for PR 12926 at commit 737c518.

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

…ialization-projection Conflicts: sql/core/src/test/scala/org/apache/spark/sql/DatasetSuite.scala

SparkQA · 2016-05-06T04:50:01Z

Test build #57955 has finished for PR 12926 at commit 3d0554d.

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

viirya · 2016-05-06T06:31:35Z

cc @cloud-fan @yhuai @rxin

cloud-fan · 2016-05-06T06:59:09Z

I think a simpler and better approach is just removing this alias only project in next batch.

viirya · 2016-05-06T08:05:08Z

@cloud-fan But how about to preserve DeserializeToObject 's output expr id?

cloud-fan · 2016-05-06T08:17:20Z

Sorry, when I say "remove", I mean a safe removal that we transform the plan tree and replace attributes produced by alias with the original attributes.

viirya · 2016-05-08T03:13:13Z

@cloud-fan ok. let me try it.

…alization.

cloud-fan · 2016-05-09T07:51:24Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/Optimizer.scala


+/**
+ * Removes extra Project added in EliminateSerialization rule.
+ */


How about we make it more general? e.g. RemoveAliasOnlyProject, so that not only object operator can benefit from it.

SparkQA · 2016-05-09T08:37:52Z

Test build #58127 has finished for PR 12926 at commit 4b0773a.

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

SparkQA · 2016-05-11T06:14:41Z

Test build #58326 has finished for PR 12926 at commit 29a0c70.

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

cloud-fan · 2016-05-11T08:20:11Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/Optimizer.scala

+      return false
+    } else {
+      projectList.map(_.asInstanceOf[Alias]).zip(childOutput).forall { case (a, o) =>
+        a.child match {


isn't it just a semanticEquals attr?

SparkQA · 2016-05-11T09:44:20Z

Test build #58351 has finished for PR 12926 at commit 85fba17.

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

SparkQA · 2016-05-11T10:05:45Z

Test build #58353 has finished for PR 12926 at commit ea55398.

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

cloud-fan · 2016-05-11T14:25:16Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/Optimizer.scala

+ */
+object RemoveAliasOnlyProject extends Rule[LogicalPlan] {
+  // Check if projectList in the Project node has the same attribute names and ordering
+  // as its child node.


nit: isAliasOnly

cloud-fan · 2016-05-11T14:31:56Z

mostly LGTM except some style comments

zsxwing · 2016-05-11T22:19:28Z

Looks pretty good!

viirya · 2016-05-12T03:23:18Z

@cloud-fan @zsxwing Thanks! I've addressed your comments now.

SparkQA · 2016-05-12T04:35:02Z

Test build #58440 has finished for PR 12926 at commit c3748ba.

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

cloud-fan · 2016-05-12T04:56:47Z

test this please.

SparkQA · 2016-05-12T06:24:39Z

Test build #58447 has finished for PR 12926 at commit c3748ba.

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

cloud-fan · 2016-05-12T06:51:15Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/Optimizer.scala

-      }
+      // Adds an extra Project here, to preserve the output expr id of `DeserializeToObject`.
+      // We will remove it later in RemoveAliasOnlyProject rule.
+      val objAttr = Alias(s.child.output.head, "obj")(exprId = d.output.head.exprId)


nit: use Alias(s.child.output.head, s.child.output.head.name)(exprId = d.output.head.exprId) to make sure the alias name is same with the attribute name

ok. update later.

SparkQA · 2016-05-12T11:32:29Z

Test build #58474 has finished for PR 12926 at commit 882fc66.

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

cloud-fan · 2016-05-12T12:37:34Z

LGTM

yhuai · 2016-05-12T17:10:32Z

Thanks. Merging to master and branch 2.0.

…teSerialization ## What changes were proposed in this pull request? We will eliminate the pair of `DeserializeToObject` and `SerializeFromObject` in `Optimizer` and add extra `Project`. However, when DeserializeToObject's outputObjectType is ObjectType and its cls can't be processed by unsafe project, it will be failed. To fix it, we can simply remove the extra `Project` and replace the output attribute of `DeserializeToObject` in another rule. ## How was this patch tested? `DatasetSuite`. Author: Liang-Chi Hsieh <[email protected]> Closes #12926 from viirya/fix-eliminate-serialization-projection. (cherry picked from commit 470de74) Signed-off-by: Yin Huai <[email protected]>

viirya added 2 commits May 5, 2016 06:59

Avoid extra Project when DeserializeToObject outputs an unsupported c…

cf53a43

…lass for Project.

Add ObjectProject.

48e6b6d

Fix test.

737c518

Merge remote-tracking branch 'upstream/master' into fix-eliminate-ser…

3d0554d

…ialization-projection Conflicts: sql/core/src/test/scala/org/apache/spark/sql/DatasetSuite.scala

Replace projection that is used to preserve expr id in eliminate seri…

4b0773a

…alization.

cloud-fan reviewed May 9, 2016
View reviewed changes

Address comment.

29a0c70

Fix test.

85fba17

viirya changed the title ~~[SPARK-15094][SPARK-14803][SQL] Add ObjectProject for EliminateSerialization~~ [SPARK-15094][SPARK-14803][SQL] Remove extra Project added in EliminateSerialization May 11, 2016

cloud-fan reviewed May 11, 2016
View reviewed changes

Address comment.

ea55398

cloud-fan reviewed May 11, 2016
View reviewed changes

Address comments.

c3748ba

cloud-fan reviewed May 12, 2016
View reviewed changes

Address comments.

882fc66

asfgit closed this in 470de74 May 12, 2016

JoshRosen mentioned this pull request Sep 7, 2016

[SPARK-14803][SQL][Optimizer] A bug in EliminateSerialization rule in Catalyst Optimizer. #12575

Closed

viirya deleted the fix-eliminate-serialization-projection branch December 27, 2023 18:19

[SPARK-15094][SPARK-14803][SQL] Remove extra Project added in EliminateSerialization #12926

[SPARK-15094][SPARK-14803][SQL] Remove extra Project added in EliminateSerialization #12926

Uh oh!

Conversation

viirya commented May 5, 2016 • 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!

viirya commented May 5, 2016

Uh oh!

SparkQA commented May 5, 2016

Uh oh!

SparkQA commented May 5, 2016

Uh oh!

SparkQA commented May 6, 2016

Uh oh!

viirya commented May 6, 2016

Uh oh!

cloud-fan commented May 6, 2016

Uh oh!

viirya commented May 6, 2016

Uh oh!

cloud-fan commented May 6, 2016

Uh oh!

viirya commented May 8, 2016

Uh oh!

cloud-fan May 9, 2016

Choose a reason for hiding this comment

Uh oh!

SparkQA commented May 9, 2016

Uh oh!

SparkQA commented May 11, 2016

Uh oh!

cloud-fan May 11, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented May 11, 2016

Uh oh!

SparkQA commented May 11, 2016

Uh oh!

cloud-fan May 11, 2016

Choose a reason for hiding this comment

Uh oh!

cloud-fan commented May 11, 2016

Uh oh!

zsxwing commented May 11, 2016

Uh oh!

viirya commented May 12, 2016

Uh oh!

SparkQA commented May 12, 2016

Uh oh!

cloud-fan commented May 12, 2016

Uh oh!

SparkQA commented May 12, 2016

Uh oh!

cloud-fan May 12, 2016

Choose a reason for hiding this comment

Uh oh!

viirya May 12, 2016

Choose a reason for hiding this comment

Uh oh!

SparkQA commented May 12, 2016

Uh oh!

cloud-fan commented May 12, 2016

Uh oh!

yhuai commented May 12, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

viirya commented May 5, 2016 •

edited

Loading

cloud-fan May 11, 2016 •

edited

Loading