[SQL] [SPARK-6620] Speed up toDF() and rdd() functions by constructing converters in ScalaReflection #5279

vlyubin · 2015-03-31T03:18:51Z

cc @marmbrus

vlyubin · 2015-03-31T03:20:07Z

sql/catalyst/src/main/scala/org/apache/spark/sql/types/DataTypeConversions.scala

This isn't quite related to this PR, but I don't think it was necessary to use GenericMutableRow here.

I agree. The original version uses a mutable row mostly because of the updates in the while loop I guess.

SparkQA · 2015-03-31T03:23:21Z

Test build #29441 has started for PR 5279 at commit ab7585b.

SparkQA · 2015-03-31T04:45:52Z

Test build #29441 has finished for PR 5279 at commit ab7585b.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.
This patch does not change any dependencies.

AmplabJenkins · 2015-03-31T04:45:55Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/29441/
Test PASSed.

rxin · 2015-03-31T05:20:29Z

I know you are probably still working on this - any benchmark numbers?

rxin · 2015-03-31T05:20:55Z

cc @davies since you guys are both changing this part of the code lately.

liancheng · 2015-03-31T15:12:33Z

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

Please break the line after the first =>.

liancheng · 2015-03-31T15:13:52Z

Went though very quickly for the first time, left some styling comments.

SparkQA · 2015-03-31T19:43:26Z

Test build #29499 has started for PR 5279 at commit 6a35425.

SparkQA · 2015-03-31T21:06:28Z

Test build #29499 has finished for PR 5279 at commit 6a35425.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.
This patch does not change any dependencies.

AmplabJenkins · 2015-03-31T21:06:32Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/29499/
Test PASSed.

vlyubin · 2015-04-01T01:28:12Z

Here are the benchmark numbers: http://pastie.org/private/6vg7kg2yfwop2ov5zu2eq
TLDR: On real cluster the new implementation made rdd() 1.9 times faster and toDF() 2.2 times faster.

SparkQA · 2015-04-01T01:38:22Z

Test build #29514 has started for PR 5279 at commit 99d333a.

SparkQA · 2015-04-01T03:00:22Z

Test build #29514 has finished for PR 5279 at commit 99d333a.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.
This patch does not change any dependencies.

AmplabJenkins · 2015-04-01T03:00:25Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/29514/
Test PASSed.

vlyubin · 2015-04-03T02:36:27Z

ping

rxin · 2015-04-03T02:37:58Z

@vlyubin who are you pinging? is this still "WIP"?

vlyubin · 2015-04-03T18:47:48Z

@rxin Sorry, I just removed the WIP tag. There isn't anything more to add, as it turned out that there are no places here where we could use SpecificMutableRow to speed things up.
So I'm pinging whoever wants to add more comments on the code ...

rxin · 2015-04-03T18:50:36Z

Jenkins, retest this please.

aarondav · 2015-04-08T16:57:24Z

sql/core/src/main/scala/org/apache/spark/sql/execution/LocalTableScan.scala

Is there a reason this pattern cannot be

val converter = CatalystTypeConverters.createToScalaConverter(schema) rows.map(converter).toArray

?

It looks like this pretty efficiently handles this situation in the same way that you've extracted it here (even with calling convertRowWithConverters).

Not really, I'll update these.

SparkQA · 2015-04-08T22:28:29Z

Test build #29896 has started for PR 5279 at commit dec6802.

SparkQA · 2015-04-08T22:32:42Z

Test build #29897 has started for PR 5279 at commit c327bc9.

SparkQA · 2015-04-09T00:20:44Z

Test build #29896 has finished for PR 5279 at commit dec6802.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.
This patch does not change any dependencies.

AmplabJenkins · 2015-04-09T00:20:49Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/29896/
Test PASSed.

SparkQA · 2015-04-09T00:29:03Z

Test build #29897 has finished for PR 5279 at commit c327bc9.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.
This patch does not change any dependencies.

AmplabJenkins · 2015-04-09T00:29:07Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/29897/
Test PASSed.

liancheng · 2015-04-09T19:13:47Z

@vlyubin Would you mind to add [SQL] to the PR title? Thanks!

aarondav · 2015-04-09T23:13:00Z

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

nit: use
convertedMap(keyConverter(entry.getKey)) = valueConverter(entry.getValue)
to avoid creating a tuple.

aarondav · 2015-04-09T23:15:43Z

LGTM, @marmbrus would you mind doing a final pass?

SparkQA · 2015-04-10T00:23:32Z

Test build #29988 has started for PR 5279 at commit 11a20ec.

SparkQA · 2015-04-10T02:13:25Z

Test build #29988 has finished for PR 5279 at commit 11a20ec.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.
This patch does not change any dependencies.

AmplabJenkins · 2015-04-10T02:13:30Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/29988/
Test PASSed.

marmbrus · 2015-04-10T19:12:42Z

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

Wrong comment style.

SparkQA · 2015-04-10T21:18:45Z

Test build #30051 has started for PR 5279 at commit e75a387.

SparkQA · 2015-04-10T23:09:31Z

Test build #30051 has finished for PR 5279 at commit e75a387.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.
This patch does not change any dependencies.

AmplabJenkins · 2015-04-10T23:09:36Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/30051/
Test PASSed.

cloud-fan · 2015-05-18T02:36:06Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/ScalaUdf.scala

why use lazy val here?

vlyubin reviewed Mar 31, 2015
View reviewed changes

liancheng reviewed Mar 31, 2015
View reviewed changes

vlyubin force-pushed the speedup branch from 6a35425 to 99d333a Compare April 1, 2015 01:32

vlyubin changed the title ~~[WIP] [SPARK-6620] Speed up toDF() and rdd() functions by constructing converters in ScalaReflection~~ [SPARK-6620] Speed up toDF() and rdd() functions by constructing converters in ScalaReflection Apr 3, 2015

aarondav reviewed Apr 8, 2015
View reviewed changes

vlyubin added 2 commits April 8, 2015 15:22

Addresed review feedback

dec6802

Moved the only remaining function from DataTypeConversions to DateUtils

c327bc9

vlyubin changed the title ~~[SPARK-6620] Speed up toDF() and rdd() functions by constructing converters in ScalaReflection~~ [SQL] [SPARK-6620] Speed up toDF() and rdd() functions by constructing converters in ScalaReflection Apr 9, 2015

aarondav reviewed Apr 9, 2015
View reviewed changes

Avoid creating a tuple

11a20ec

marmbrus reviewed Apr 10, 2015
View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/CatalystTypeConverters.scala Outdated

Copy link

Contributor

marmbrus Apr 10, 2015

Choose a reason for hiding this comment

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

Wrong comment style.

Changes to ScalaUDF

e75a387

asfgit closed this in 67d0688 Apr 10, 2015

rayortigas mentioned this pull request Apr 27, 2015

[SPARK-7160][SQL] Support converting DataFrames to typed RDDs. #5713

Closed

rayortigas mentioned this pull request May 18, 2015

[SPARK-7691] [SQL] Refactor CatalystTypeConverter to use type-specific row accessors #6222

Closed

cloud-fan reviewed May 18, 2015
View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/ScalaUdf.scala

Copy link

Contributor

cloud-fan May 18, 2015

Choose a reason for hiding this comment

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

why use lazy val here?

cloud-fan mentioned this pull request May 18, 2015

[SQL][minor] use private[this] for internal field in ScalaUdf #6235

Closed

[SQL] [SPARK-6620] Speed up toDF() and rdd() functions by constructing converters in ScalaReflection #5279

[SQL] [SPARK-6620] Speed up toDF() and rdd() functions by constructing converters in ScalaReflection #5279

Uh oh!

Conversation

vlyubin commented Mar 31, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Mar 31, 2015

Uh oh!

SparkQA commented Mar 31, 2015

Uh oh!

AmplabJenkins commented Mar 31, 2015

Uh oh!

rxin commented Mar 31, 2015

Uh oh!

rxin commented Mar 31, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

liancheng commented Mar 31, 2015

Uh oh!

SparkQA commented Mar 31, 2015

Uh oh!

SparkQA commented Mar 31, 2015

Uh oh!

AmplabJenkins commented Mar 31, 2015

Uh oh!

vlyubin commented Apr 1, 2015

Uh oh!

SparkQA commented Apr 1, 2015

Uh oh!

SparkQA commented Apr 1, 2015

Uh oh!

AmplabJenkins commented Apr 1, 2015

Uh oh!

vlyubin commented Apr 3, 2015

Uh oh!

rxin commented Apr 3, 2015

Uh oh!

vlyubin commented Apr 3, 2015

Uh oh!

rxin commented Apr 3, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Apr 8, 2015

Uh oh!

SparkQA commented Apr 8, 2015

Uh oh!

SparkQA commented Apr 9, 2015

Uh oh!

AmplabJenkins commented Apr 9, 2015

Uh oh!

SparkQA commented Apr 9, 2015

Uh oh!

AmplabJenkins commented Apr 9, 2015

Uh oh!

liancheng commented Apr 9, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aarondav commented Apr 9, 2015

Uh oh!

SparkQA commented Apr 10, 2015

Uh oh!

SparkQA commented Apr 10, 2015

Uh oh!

AmplabJenkins commented Apr 10, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Apr 10, 2015

Uh oh!

SparkQA commented Apr 10, 2015

Uh oh!