[SPARK-25727][SQL] Add outputOrdering to otherCopyArgs in InMemoryRelation by gatorsmile · Pull Request #22715 · apache/spark

gatorsmile · 2018-10-13T18:59:02Z

What changes were proposed in this pull request?

Add outputOrdering to otherCopyArgs in InMemoryRelation so that this field will be copied when we doing the tree transformation.

 val data = Seq(100).toDF("count").cache()
data.queryExecution.optimizedPlan.toJSON

The above code can generate the following error:

assertion failed: InMemoryRelation fields: output, cacheBuilder, statsOfPlanToCache, outputOrdering, values: List(count#178), CachedRDDBuilder(true,10000,StorageLevel(disk, memory, deserialized, 1 replicas),*(1) Project [value#176 AS count#178]
+- LocalTableScan [value#176]
,None), Statistics(sizeInBytes=12.0 B, hints=none)
java.lang.AssertionError: assertion failed: InMemoryRelation fields: output, cacheBuilder, statsOfPlanToCache, outputOrdering, values: List(count#178), CachedRDDBuilder(true,10000,StorageLevel(disk, memory, deserialized, 1 replicas),*(1) Project [value#176 AS count#178]
+- LocalTableScan [value#176]
,None), Statistics(sizeInBytes=12.0 B, hints=none)
at scala.Predef$.assert(Predef.scala:170)
at org.apache.spark.sql.catalyst.trees.TreeNode.jsonFields(TreeNode.scala:611)
at org.apache.spark.sql.catalyst.trees.TreeNode.org$apache$spark$sql$catalyst$trees$TreeNode$$collectJsonValue$1(TreeNode.scala:599)
at org.apache.spark.sql.catalyst.trees.TreeNode.jsonValue(TreeNode.scala:604)
at org.apache.spark.sql.catalyst.trees.TreeNode.toJSON(TreeNode.scala:590)

How was this patch tested?

Added a test

mgaido91 · 2018-10-13T19:49:50Z

thanks for this fix @gatorsmile. Can we add a UT for this? Moreover, shall we add it also to LogicalRDD ?

gatorsmile · 2018-10-13T21:50:38Z

@mgaido91 Anything is missing in LogicalRDD?

SparkQA · 2018-10-13T22:24:12Z

Test build #97346 has finished for PR 22715 at commit 1d30cc5.

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

SparkQA · 2018-10-14T02:55:25Z

Test build #97350 has finished for PR 22715 at commit cff78e9.

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

dongjoon-hyun

+1, LGTM

dongjoon-hyun · 2018-10-14T05:09:46Z

Merged to master/branch-2.4.

…ation ## What changes were proposed in this pull request? Add `outputOrdering ` to `otherCopyArgs` in InMemoryRelation so that this field will be copied when we doing the tree transformation. ``` val data = Seq(100).toDF("count").cache() data.queryExecution.optimizedPlan.toJSON ``` The above code can generate the following error: ``` assertion failed: InMemoryRelation fields: output, cacheBuilder, statsOfPlanToCache, outputOrdering, values: List(count#178), CachedRDDBuilder(true,10000,StorageLevel(disk, memory, deserialized, 1 replicas),*(1) Project [value#176 AS count#178] +- LocalTableScan [value#176] ,None), Statistics(sizeInBytes=12.0 B, hints=none) java.lang.AssertionError: assertion failed: InMemoryRelation fields: output, cacheBuilder, statsOfPlanToCache, outputOrdering, values: List(count#178), CachedRDDBuilder(true,10000,StorageLevel(disk, memory, deserialized, 1 replicas),*(1) Project [value#176 AS count#178] +- LocalTableScan [value#176] ,None), Statistics(sizeInBytes=12.0 B, hints=none) at scala.Predef$.assert(Predef.scala:170) at org.apache.spark.sql.catalyst.trees.TreeNode.jsonFields(TreeNode.scala:611) at org.apache.spark.sql.catalyst.trees.TreeNode.org$apache$spark$sql$catalyst$trees$TreeNode$$collectJsonValue$1(TreeNode.scala:599) at org.apache.spark.sql.catalyst.trees.TreeNode.jsonValue(TreeNode.scala:604) at org.apache.spark.sql.catalyst.trees.TreeNode.toJSON(TreeNode.scala:590) ``` ## How was this patch tested? Added a test Closes#22715 from gatorsmile/copyArgs1. Authored-by: gatorsmile <gatorsmile@gmail.com> Signed-off-by: Dongjoon Hyun <dongjoon@apache.org> (cherry picked from commit 6c3f2c6) Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>

dongjoon-hyun · 2018-10-14T05:12:29Z

Thank you, @gatorsmile and @mgaido91 .

cloud-fan · 2018-10-15T02:19:26Z

 }

- override protected def otherCopyArgs: Seq[AnyRef] = Seq(statsOfPlanToCache)
+ override protected def otherCopyArgs: Seq[AnyRef] = Seq(statsOfPlanToCache, outputOrdering)


The thing I don't understand is why we put the outputOrdering in the curry constructor at the first place...

mgaido91 · 2018-10-15T10:34:38Z

sorry @gatorsmile that is fine, my bad. A late LGTM to this, despite probably @cloud-fan 's comment make, we probably should have just put it as a part of the main constructor...

cloud-fan · 2018-10-15T10:49:55Z

Hi @mgaido91 , since you are the major author of this part, do you have time to open a PR and move outputOrdering to the main constructor? Thanks in advance!

mgaido91 · 2018-10-15T11:02:23Z

@cloud-fan , sure, I'll submit a follow-up PR for this. Thanks.

…nMemoryRelation ## What changes were proposed in this pull request? The PR addresses [the comment](#22715 (comment)) in the previous one. `outputOrdering` becomes a field of `InMemoryRelation`. ## How was this patch tested? existing UTs Closes#22726 from mgaido91/SPARK-25727_followup. Authored-by: Marco Gaido <marcogaido91@gmail.com> Signed-off-by: gatorsmile <gatorsmile@gmail.com>

…ation ## What changes were proposed in this pull request? Add `outputOrdering ` to `otherCopyArgs` in InMemoryRelation so that this field will be copied when we doing the tree transformation. ``` val data = Seq(100).toDF("count").cache() data.queryExecution.optimizedPlan.toJSON ``` The above code can generate the following error: ``` assertion failed: InMemoryRelation fields: output, cacheBuilder, statsOfPlanToCache, outputOrdering, values: List(count#178), CachedRDDBuilder(true,10000,StorageLevel(disk, memory, deserialized, 1 replicas),*(1) Project [value#176 AS count#178] +- LocalTableScan [value#176] ,None), Statistics(sizeInBytes=12.0 B, hints=none) java.lang.AssertionError: assertion failed: InMemoryRelation fields: output, cacheBuilder, statsOfPlanToCache, outputOrdering, values: List(count#178), CachedRDDBuilder(true,10000,StorageLevel(disk, memory, deserialized, 1 replicas),*(1) Project [value#176 AS count#178] +- LocalTableScan [value#176] ,None), Statistics(sizeInBytes=12.0 B, hints=none) at scala.Predef$.assert(Predef.scala:170) at org.apache.spark.sql.catalyst.trees.TreeNode.jsonFields(TreeNode.scala:611) at org.apache.spark.sql.catalyst.trees.TreeNode.org$apache$spark$sql$catalyst$trees$TreeNode$$collectJsonValue$1(TreeNode.scala:599) at org.apache.spark.sql.catalyst.trees.TreeNode.jsonValue(TreeNode.scala:604) at org.apache.spark.sql.catalyst.trees.TreeNode.toJSON(TreeNode.scala:590) ``` ## How was this patch tested? Added a test Closesapache#22715 from gatorsmile/copyArgs1. Authored-by: gatorsmile <gatorsmile@gmail.com> Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>

…nMemoryRelation ## What changes were proposed in this pull request? The PR addresses [the comment](apache#22715 (comment)) in the previous one. `outputOrdering` becomes a field of `InMemoryRelation`. ## How was this patch tested? existing UTs Closesapache#22726 from mgaido91/SPARK-25727_followup. Authored-by: Marco Gaido <marcogaido91@gmail.com> Signed-off-by: gatorsmile <gatorsmile@gmail.com>

fix
1d30cc5

gatorsmile mentioned this pull request Oct 13, 2018
[SPARK-23375][SQL] Eliminate unneeded Sort in Optimizer #20560
Closed

gatorsmile changed the title ~~[SPARK-23375][FOLLOW-UP] Add outputOrdering to otherCopyArgs~~[SPARK-25727] Add outputOrdering to otherCopyArgsOct 13, 2018

add a test
cff78e9

gatorsmile changed the title ~~[SPARK-25727] Add outputOrdering to otherCopyArgs~~[SPARK-25727][SQL] Add outputOrdering to otherCopyArgsOct 13, 2018

gatorsmile changed the title ~~[SPARK-25727][SQL] Add outputOrdering to otherCopyArgs~~[SPARK-25727][SQL] Add outputOrdering to otherCopyArgs in InMemoryRelationOct 14, 2018

dongjoon-hyun approved these changes Oct 14, 2018
View reviewed changes

asfgit closed this in 6c3f2c6Oct 14, 2018

cloud-fan reviewed Oct 15, 2018
View reviewed changes

mgaido91 mentioned this pull request Oct 15, 2018
[SPARK-25727][FOLLOWUP] Move outputOrdering to case class field for InMemoryRelation #22726
Closed

Navigation Menu

Uh oh!

[SPARK-25727][SQL] Add outputOrdering to otherCopyArgs in InMemoryRelation - #22715

[SPARK-25727][SQL] Add outputOrdering to otherCopyArgs in InMemoryRelation#22715
gatorsmile wants to merge 2 commits into
apache:masterfrom
gatorsmile:copyArgs1

gatorsmile commented Oct 13, 2018•
edited
Loading

Uh oh!

mgaido91 commented Oct 13, 2018

Uh oh!

gatorsmile commented Oct 13, 2018

Uh oh!

SparkQA commented Oct 13, 2018

Uh oh!

SparkQA commented Oct 14, 2018

Uh oh!

dongjoon-hyun left a comment

Uh oh!

dongjoon-hyun commented Oct 14, 2018

Uh oh!

dongjoon-hyun commented Oct 14, 2018

Uh oh!

cloud-fanOct 15, 2018

Uh oh!

mgaido91 commented Oct 15, 2018

Uh oh!

cloud-fan commented Oct 15, 2018

Uh oh!

mgaido91 commented Oct 15, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Uh oh!

Conversation

gatorsmile commented Oct 13, 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!

mgaido91 commented Oct 13, 2018

Uh oh!

gatorsmile commented Oct 13, 2018

Uh oh!

SparkQA commented Oct 13, 2018

Uh oh!

SparkQA commented Oct 14, 2018

Uh oh!

dongjoon-hyun left a comment

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun commented Oct 14, 2018

Uh oh!

dongjoon-hyun commented Oct 14, 2018

Uh oh!

cloud-fanOct 15, 2018

Choose a reason for hiding this comment

Uh oh!

mgaido91 commented Oct 15, 2018

Uh oh!

cloud-fan commented Oct 15, 2018

Uh oh!

mgaido91 commented Oct 15, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

gatorsmile commented Oct 13, 2018•
edited
Loading