[SPARK-17426][SQL] Refactor `TreeNode.toJSON` to avoid OOM when converting unknown fields to JSON #14990

clockfly · 2016-09-07T05:01:02Z

What changes were proposed in this pull request?

This PR is a follow up of SPARK-17356. Current implementation of TreeNode.toJSON recursively converts all fields of TreeNode to JSON, even if the field is of type Seq or type Map. This may trigger out of memory exception in cases like:

the Seq or Map can be very big. Converting them to JSON may take huge memory, which may trigger out of memory error.
Some user space input may also be propagated to the Plan. The user space input can be of arbitrary type, and may also be self-referencing. Trying to print user space input to JSON may trigger out of memory error or stack overflow error.

For a code example, please check the Jira description of SPARK-17426.

In this PR, we refactor the TreeNode.toJSON so that we only convert a field to JSON string if the field is a safe type.

How was this patch tested?

Unit test.

SparkQA · 2016-09-07T07:11:13Z

Test build #65020 has finished for PR 14990 at commit 284d780.

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

SparkQA · 2016-09-07T07:17:25Z

Test build #65022 has finished for PR 14990 at commit 33983e5.

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

clockfly · 2016-09-08T00:09:00Z

@cloud-fan Can you take a look?

cloud-fan · 2016-09-08T02:57:26Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/trees/TreeNode.scala

        name -> JInt(children.indexOf(value))
-      case (name, value: Seq[BaseType]) if value.toSet.subsetOf(containsChild) =>
+      // Check the value (Seq[BaseType]) element type first before converting it to a Set.
+      // Otherwise, it may take a lot of memory to convert a super big Seq to Set.


how about use forall here? if values.forall(containsChild)

cloud-fan · 2016-09-08T07:02:07Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/trees/TreeNode.scala

-      ("product-class" -> JString(p.getClass.getName)) :: fieldNames.zip(fieldValues).map {
-        case (name, value) => name -> parseToJson(value)
+      ("product-class" -> JString(p.getClass.getName)) :: fieldNames.zip(fieldValues).collect {
+        // Only converts String fields in Product to JSON


should we handle primitive types?

Other types are not handled, as I think there are not very useful for documentation purpose.

SparkQA · 2016-09-08T08:37:04Z

Test build #65084 has finished for PR 14990 at commit 16e7896.

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

SparkQA · 2016-09-08T09:51:50Z

Test build #65091 has finished for PR 14990 at commit 22b8804.

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

SparkQA · 2016-09-08T10:09:02Z

Test build #65094 has finished for PR 14990 at commit a4b04b4.

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

SparkQA · 2016-09-16T02:31:07Z

Test build #65467 has finished for PR 14990 at commit 996e392.

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

SparkQA · 2016-09-16T02:50:01Z

Test build #65468 has finished for PR 14990 at commit ccdda37.

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

cloud-fan · 2016-09-16T03:32:38Z

LGTM, pending jenkins

SparkQA · 2016-09-16T04:14:17Z

Test build #65473 has finished for PR 14990 at commit 1451753.

This patch fails MiMa tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2016-09-16T04:35:52Z

Test build #65471 has finished for PR 14990 at commit d6838d0.

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

clockfly · 2016-09-16T07:33:38Z

retest this please

SparkQA · 2016-09-16T09:36:35Z

Test build #65480 has finished for PR 14990 at commit 1451753.

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

cloud-fan · 2016-09-16T11:38:35Z

thanks, merging to master!

…rting unknown fields to JSON ## What changes were proposed in this pull request? This PR is a follow up of SPARK-17356. Current implementation of `TreeNode.toJSON` recursively converts all fields of TreeNode to JSON, even if the field is of type `Seq` or type Map. This may trigger out of memory exception in cases like: 1. the Seq or Map can be very big. Converting them to JSON may take huge memory, which may trigger out of memory error. 2. Some user space input may also be propagated to the Plan. The user space input can be of arbitrary type, and may also be self-referencing. Trying to print user space input to JSON may trigger out of memory error or stack overflow error. For a code example, please check the Jira description of SPARK-17426. In this PR, we refactor the `TreeNode.toJSON` so that we only convert a field to JSON string if the field is a safe type. ## How was this patch tested? Unit test. Author: Sean Zhong <seanzhong@databricks.com> Closes apache#14990 from clockfly/json_oom2.

clockfly force-pushed the json_oom2 branch from 284d780 to 33983e5 Compare September 7, 2016 05:05

json oom

33983e5

cloud-fan reviewed Sep 8, 2016
View reviewed changes

address comments

16e7896

cloud-fan reviewed Sep 8, 2016
View reviewed changes

clockfly added 2 commits September 8, 2016 15:50

address comments

22b8804

address comment

a4b04b4

clockfly added 2 commits September 16, 2016 07:35

whitelist for Product

a1c5792

add more UT

ccdda37

clockfly force-pushed the json_oom2 branch from 996e392 to ccdda37 Compare September 16, 2016 00:49

add more UT

d6838d0

add more UT

1451753

asfgit closed this in a425a37 Sep 16, 2016

gatorsmile mentioned this pull request Jun 21, 2017

[SPARK-15491][SQL]fix assertion failure for JDBC DataFrame to JSON #13287

Closed

[SPARK-17426][SQL] Refactor TreeNode.toJSON to avoid OOM when converting unknown fields to JSON #14990

[SPARK-17426][SQL] Refactor TreeNode.toJSON to avoid OOM when converting unknown fields to JSON #14990

Uh oh!

Conversation

clockfly commented Sep 7, 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!

SparkQA commented Sep 7, 2016

Uh oh!

SparkQA commented Sep 7, 2016

Uh oh!

clockfly commented Sep 8, 2016

Uh oh!

cloud-fan Sep 8, 2016

Choose a reason for hiding this comment

Uh oh!

clockfly Sep 8, 2016

Choose a reason for hiding this comment

Uh oh!

cloud-fan Sep 8, 2016

Choose a reason for hiding this comment

Uh oh!

clockfly Sep 8, 2016

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Sep 8, 2016

Uh oh!

SparkQA commented Sep 8, 2016

Uh oh!

SparkQA commented Sep 8, 2016

Uh oh!

SparkQA commented Sep 16, 2016

Uh oh!

SparkQA commented Sep 16, 2016

Uh oh!

cloud-fan commented Sep 16, 2016

Uh oh!

SparkQA commented Sep 16, 2016

Uh oh!

SparkQA commented Sep 16, 2016

Uh oh!

clockfly commented Sep 16, 2016

Uh oh!

SparkQA commented Sep 16, 2016

Uh oh!

cloud-fan commented Sep 16, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

[SPARK-17426][SQL] Refactor `TreeNode.toJSON` to avoid OOM when converting unknown fields to JSON #14990

[SPARK-17426][SQL] Refactor `TreeNode.toJSON` to avoid OOM when converting unknown fields to JSON #14990

clockfly commented Sep 7, 2016 •

edited

Loading