[SPARK-26318][SQL] Deprecate Row.merge #23271

KyleLi1985 · 2018-12-10T08:10:35Z

What changes were proposed in this pull request?

Deprecate Row.merge

How was this patch tested?

N/A

dongjoon-hyun · 2018-12-10T08:18:58Z

ok to test

dongjoon-hyun · 2018-12-10T08:23:43Z

sql/catalyst/src/main/scala/org/apache/spark/sql/Row.scala

+        n = n + 1
+      }
+    }
+    new GenericRow(container)


Hi, @KyleLi1985 . Yep. It's a TODO item. I'm wondering if you have a real use case for this improvement.

definitely, It is important

dongjoon-hyun · 2018-12-10T08:26:02Z

cc @rxin and @cloud-fan

SparkQA · 2018-12-10T12:01:42Z

Test build #99901 has finished for PR 23271 at commit 93c4af4.

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

rxin · 2018-12-10T22:39:55Z

sql/catalyst/src/main/scala/org/apache/spark/sql/Row.scala

+    }
+    val container = Array.ofDim[Any](number)
+    var n = 0
+    for (i <- 0 until size) {


if you want to improve this more, you can try with a while loop here rather than a for loop, without creating the extra 0 until size.

Only primitively use size, subSize, and number information and control the container will improve the performance more.
up to
call 100000000 time Row.merge(row1) need 18064 millisecond
call 100000000 time Row.merge(rows:_*) need 25651 millisecond

rxin · 2018-12-10T22:40:39Z

Does anybody actually use this merge?

cloud-fan · 2018-12-11T01:38:00Z

I have the same question. If it's not used by anyone, maybe we can deprecate it and remove it in the next release

dongjoon-hyun · 2018-12-11T04:32:29Z

+1 for depreciation.

HyukjinKwon · 2018-12-11T05:59:13Z

+1 for deprecation.

viirya · 2018-12-11T08:10:56Z

+1 for deprecation too.

KyleLi1985 · 2018-12-14T15:07:17Z

deprecate this function

SparkQA · 2018-12-14T15:15:54Z

Test build #100155 has finished for PR 23271 at commit cc22a50.

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

KyleLi1985 · 2018-12-14T15:41:05Z

retest this please

SparkQA · 2018-12-14T19:01:22Z

Test build #100156 has finished for PR 23271 at commit c300f43.

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

HyukjinKwon · 2018-12-15T03:53:26Z

sql/catalyst/src/main/scala/org/apache/spark/sql/Row.scala

+        ssSize -= 1
+        container(number) = rows(sSize)(ssSize)
+      }
+    }


I would just leave the function as was. It will be removed in the next release anyway.

yea, it is not used before and is deprecated now. We don't need to do this optimization.

HyukjinKwon · 2018-12-15T03:54:19Z

Let's target to deprecate the API in this PR.

viirya · 2018-12-15T05:12:33Z

And don't forget to update the PR title and description too.

KyleLi1985 · 2018-12-16T11:54:14Z

retest this please

SparkQA · 2018-12-16T15:42:05Z

Test build #100205 has finished for PR 23271 at commit de49469.

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

cloud-fan · 2018-12-17T00:19:02Z

sql/catalyst/src/main/scala/org/apache/spark/sql/Row.scala

@@ -57,6 +57,8 @@ object Row {
  /**
   * Merge multiple rows into a single row, one after another.
   */
+
+  @deprecated("This method is deprecated and will be removed in future versions.", "2.5.0")


next release is 3.0.0

cloud-fan · 2018-12-17T00:19:29Z

can we use "deprecate Row.merge" as title?

KyleLi1985 · 2018-12-17T05:25:08Z

Done

KyleLi1985 · 2018-12-17T05:25:18Z

retest this please

SparkQA · 2018-12-17T08:05:01Z

Test build #100221 has finished for PR 23271 at commit ed55406.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

viirya · 2018-12-17T08:39:59Z

retest this please.

viirya · 2018-12-17T08:41:33Z

Please update the PR description before merging.

SparkQA · 2018-12-17T10:35:05Z

Test build #100230 has finished for PR 23271 at commit ed55406.

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

viirya · 2018-12-17T10:37:29Z

retest this please.

KyleLi1985 · 2018-12-17T12:03:22Z

retest this please.

I check the test output, the fail is strange.
Is there some trick things I should care about?

SparkQA · 2018-12-17T12:25:52Z

Test build #100238 has finished for PR 23271 at commit ed55406.

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

HyukjinKwon · 2018-12-17T13:11:14Z

retest this please

HyukjinKwon · 2018-12-17T13:51:57Z

sql/catalyst/src/main/scala/org/apache/spark/sql/Row.scala

@@ -57,6 +57,8 @@ object Row {
  /**
   * Merge multiple rows into a single row, one after another.
   */
+


I would remove this newline

@KyleLi1985 ^

SparkQA · 2018-12-17T16:58:12Z

Test build #100241 has finished for PR 23271 at commit ed55406.

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

felixcheung

don't we need a note in the sql migration guide or something?

felixcheung · 2018-12-19T08:10:49Z

sql/catalyst/src/main/scala/org/apache/spark/sql/Row.scala

@@ -57,6 +57,8 @@ object Row {
  /**
   * Merge multiple rows into a single row, one after another.
   */
+


@KyleLi1985 ^

HyukjinKwon · 2018-12-19T08:14:55Z

I think it's okay .. virtually no one uses it .. We could also leave a note when we actually remove it out. It's going to warn that it's deprecated during build or in IDE I guess anyway.

SparkQA · 2018-12-19T19:22:48Z

Test build #100305 has finished for PR 23271 at commit ab19141.

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

dongjoon-hyun · 2018-12-19T19:38:03Z

Retest this please.

SparkQA · 2018-12-19T23:38:39Z

Test build #100315 has finished for PR 23271 at commit ab19141.

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

HyukjinKwon · 2018-12-20T05:22:16Z

Merged to master.

## What changes were proposed in this pull request? Deprecate Row.merge ## How was this patch tested? N/A Closes apache#23271 from KyleLi1985/master. Authored-by: 李亮 <liang.li.work@outlook.com> Signed-off-by: Hyukjin Kwon <gurwls223@apache.org>

dongjoon-hyun reviewed Dec 10, 2018

View reviewed changes

rxin reviewed Dec 10, 2018

View reviewed changes

KyleLi1985 force-pushed the master branch from 93c4af4 to cc22a50 Compare December 14, 2018 15:06

KyleLi1985 force-pushed the master branch from cc22a50 to c300f43 Compare December 14, 2018 15:39

HyukjinKwon reviewed Dec 15, 2018

View reviewed changes

KyleLi1985 force-pushed the master branch from c300f43 to de49469 Compare December 16, 2018 11:49

KyleLi1985 changed the title ~~[SPARK-26318][SQL] Enhance function merge performance in Row~~ [SPARK-26318][SQL] Enhance Row deprecate function merge Dec 16, 2018

cloud-fan reviewed Dec 17, 2018

View reviewed changes

KyleLi1985 changed the title ~~[SPARK-26318][SQL] Enhance Row deprecate function merge~~ [SPARK-26318][SQL] Deprecate Row.merge Dec 17, 2018

KyleLi1985 force-pushed the master branch from de49469 to ed55406 Compare December 17, 2018 05:25

HyukjinKwon reviewed Dec 17, 2018

View reviewed changes

HyukjinKwon approved these changes Dec 17, 2018

View reviewed changes

viirya approved these changes Dec 17, 2018

View reviewed changes

felixcheung reviewed Dec 19, 2018

View reviewed changes

Enhance function merge performance in Row

ab19141

KyleLi1985 force-pushed the master branch from ed55406 to ab19141 Compare December 19, 2018 09:03

dongjoon-hyun approved these changes Dec 19, 2018

View reviewed changes

asfgit closed this in 04d8e3a Dec 20, 2018

HyukjinKwon mentioned this pull request Oct 10, 2019

[SPARK-29347][SQL] Add JSON serialization for external Rows #26013

Closed

[SPARK-26318][SQL] Deprecate Row.merge #23271

[SPARK-26318][SQL] Deprecate Row.merge #23271

Conversation

KyleLi1985 commented Dec 10, 2018 • edited Loading

What changes were proposed in this pull request?

How was this patch tested?

dongjoon-hyun commented Dec 10, 2018

dongjoon-hyun Dec 10, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dongjoon-hyun commented Dec 10, 2018

SparkQA commented Dec 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rxin commented Dec 10, 2018

cloud-fan commented Dec 11, 2018

dongjoon-hyun commented Dec 11, 2018

HyukjinKwon commented Dec 11, 2018

viirya commented Dec 11, 2018

KyleLi1985 commented Dec 14, 2018

SparkQA commented Dec 14, 2018

KyleLi1985 commented Dec 14, 2018

SparkQA commented Dec 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HyukjinKwon commented Dec 15, 2018

viirya commented Dec 15, 2018

KyleLi1985 commented Dec 16, 2018

SparkQA commented Dec 16, 2018

Choose a reason for hiding this comment

cloud-fan commented Dec 17, 2018

KyleLi1985 commented Dec 17, 2018

KyleLi1985 commented Dec 17, 2018

SparkQA commented Dec 17, 2018

viirya commented Dec 17, 2018

viirya commented Dec 17, 2018

SparkQA commented Dec 17, 2018

viirya commented Dec 17, 2018

KyleLi1985 commented Dec 17, 2018

SparkQA commented Dec 17, 2018

HyukjinKwon commented Dec 17, 2018

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

SparkQA commented Dec 17, 2018

felixcheung left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HyukjinKwon commented Dec 19, 2018

SparkQA commented Dec 19, 2018

dongjoon-hyun commented Dec 19, 2018

SparkQA commented Dec 19, 2018

HyukjinKwon commented Dec 20, 2018

KyleLi1985 commented Dec 10, 2018 •

edited

Loading

dongjoon-hyun Dec 10, 2018 •

edited

Loading