[SPARK-26103][SQL] Limit the length of debug strings for query plans #23169

DaveDeCaprio · 2018-11-28T18:04:25Z

What changes were proposed in this pull request?

The PR puts in a limit on the size of a debug string generated for a tree node. Helps to fix out of memory errors when large plans have huge debug strings. In addition to SPARK-26103, this should also address SPARK-23904 and SPARK-25380. AN alternative solution was proposed in #23076, but that solution doesn't address all the cases that can cause a large query. This limit is only on calls treeString that don't pass a Writer, which makes it play nicely with #22429, #23018 and #23039. Full plans can be written to files, but truncated plans will be used when strings are held in memory, such as for the UI.

A new configuration parameter called spark.sql.debug.maxPlanLength was added to control the length of the plans.
When plans are truncated, "..." is printed to indicate that it isn't a full plan
A warning is printed out the first time a truncated plan is displayed. The warning explains what happened and how to adjust the limit.

How was this patch tested?

Unit tests were created for the new SizeLimitedWriter. Also a unit test for TreeNode was created that checks that a long plan is correctly truncated.

merge in spark

…iter

DaveDeCaprio · 2018-11-28T18:10:21Z

@MaxGekk and @hvanhovell, this is an alternative solution for #23076. It limits overall plan length when generating the full string in memory, but not if a specific writer is passed in.

gatorsmile · 2018-11-28T22:49:24Z

ok to test

SparkQA · 2018-11-28T22:51:48Z

Test build #99410 has finished for PR 23169 at commit 5528ca1.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2018-11-28T23:20:36Z

Test build #99414 has finished for PR 23169 at commit 3171cf3.

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

HeartSaVioR

Thanks for your effort on addressing this!

Would this patch address the issue on UI side too, or it will be addressed in another PR?

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/SizeLimitedWriter.scala

sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/trees/TreeNodeSuite.scala

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/package.scala

SparkQA · 2018-11-29T03:08:22Z

Test build #99418 has finished for PR 23169 at commit 3ffdc6a.

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

SparkQA · 2018-11-29T03:40:47Z

Test build #99426 has finished for PR 23169 at commit 45a60fc.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
class WriterSizeException(val extraChars: Long, val charLimit: Long) extends Exception(

Added the limit in for query execution

SparkQA · 2018-11-29T03:56:09Z

Test build #99427 has finished for PR 23169 at commit 9678799.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds no public classes.

DaveDeCaprio · 2018-11-29T04:02:29Z

I added changes to QueryExecution in the latest commit to address the UI issue.

SparkQA · 2018-11-29T07:39:12Z

Test build #99430 has finished for PR 23169 at commit a5af842.

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

felixcheung

I didn't follow on all the long discussion, but I"m worry that having max len by default and blindly truncating plan string will break some of our important use cases that requires the full plan string?

DaveDeCaprio · 2018-11-29T13:36:48Z

If you have an idea of what those use cases are I could take a look and see if there is an impact. If not, we could turn it off by default (set the max length to Long.Max).

…t is specifically configured.

…an-size # Conflicts: # sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala

DaveDeCaprio · 2018-12-03T23:41:58Z

Ok @felixcheung , I've updated this PR so that the default behavior does not change - full plan strings are always printed.
This should be fully backwards compatible. Plan strings will only be truncated if you specifically configure them to be.

SparkQA · 2018-12-04T01:47:37Z

Test build #99632 has finished for PR 23169 at commit f0f75c2.

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

SparkQA · 2018-12-04T03:12:30Z

Test build #99631 has finished for PR 23169 at commit a4be985.

This patch fails Spark unit tests.
This patch does not merge cleanly.
This patch adds no public classes.

HeartSaVioR · 2018-12-04T05:32:48Z

retest this, please

HeartSaVioR · 2018-12-04T05:34:41Z

@DaveDeCaprio

You might miss to roll back change in test.
https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/99632/testReport/org.apache.spark.sql.catalyst.trees/TreeNodeSuite/treeString_limits_plan_length/

I also think you need to add a new test with setting configuration to some value and see whether it works properly.

vanzin

@hvanhovell @MaxGekk if you don't comment here I'll assume you're ok with the changes.

sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/StringUtils.scala

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/util/StringUtilsSuite.scala

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/StringUtils.scala

MaxGekk · 2019-03-06T21:47:08Z

I think we should indicate to users that a plan was cut otherwise the truncated plan can confuse them. For example, truncatedString outputs "... N more fields". It would be nice if PlanStringConcat prints something like this.

DaveDeCaprio · 2019-03-08T20:22:39Z

I've removed the check that only prints the warning once, and added an indicator to the end of the truncated string saying how much has been removed.

I decided to enforce that the plan string would always be within the limit, even counting the message at the end saying it was truncated. This was a bit of extra code but I think is more the behavior people would expect.

SparkQA · 2019-03-08T20:23:19Z

Test build #103222 has finished for PR 23169 at commit 4f56e48.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2019-03-08T20:32:58Z

Test build #103223 has finished for PR 23169 at commit a090fbb.

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

MaxGekk

LGTM

SparkQA · 2019-03-09T00:08:19Z

Test build #103231 has finished for PR 23169 at commit dcb4eb0.

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

SparkQA · 2019-03-09T22:55:42Z

Test build #103268 has finished for PR 23169 at commit b4cb7bf.

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

vanzin

Just style issues. We generally prefer using === and !== in tests.

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/StringUtils.scala

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/util/StringUtilsSuite.scala

SparkQA · 2019-03-11T23:23:19Z

Test build #103346 has finished for PR 23169 at commit db0db18.

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

sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala

… 0 length plans.

SparkQA · 2019-03-13T06:16:32Z

Test build #103402 has finished for PR 23169 at commit e4afa26.

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

vanzin · 2019-03-13T16:58:15Z

Merging to master.

…nfig key. ## What changes were proposed in this pull request? This is a follow-up of #23169. We should've used string-interpolation to show the config key in the warn message. ## How was this patch tested? Existing tests. Closes #24217 from ueshin/issues/SPARK-26103/s. Authored-by: Takuya UESHIN <[email protected]> Signed-off-by: Hyukjin Kwon <[email protected]>

tooptoop4 · 2019-09-10T21:30:56Z

@DaveDeCaprio @vanzin PR says merged but jira is open?

HeartSaVioR · 2019-09-11T01:13:11Z

@tooptoop4 SPARK-26103 has been marked as resolved. Looks like you're referring to SPARK-25380 - SPARK-26103 would help for SPARK-25380 but the issue is not identical (SPARK-25380 concerns there're so many generated plans stored in memory).

The PR puts in a limit on the size of a debug string generated for a tree node. Helps to fix out of memory errors when large plans have huge debug strings. In addition to SPARK-26103, this should also address SPARK-23904 and SPARK-25380. AN alternative solution was proposed in apache#23076, but that solution doesn't address all the cases that can cause a large query. This limit is only on calls treeString that don't pass a Writer, which makes it play nicely with apache#22429, apache#23018 and apache#23039. Full plans can be written to files, but truncated plans will be used when strings are held in memory, such as for the UI. - A new configuration parameter called spark.sql.debug.maxPlanLength was added to control the length of the plans. - When plans are truncated, "..." is printed to indicate that it isn't a full plan - A warning is printed out the first time a truncated plan is displayed. The warning explains what happened and how to adjust the limit. Unit tests were created for the new SizeLimitedWriter. Also a unit test for TreeNode was created that checks that a long plan is correctly truncated. Closes apache#23169 from DaveDeCaprio/text-plan-size. Lead-authored-by: Dave DeCaprio <[email protected]> Co-authored-by: David DeCaprio <[email protected]> Signed-off-by: Marcelo Vanzin <[email protected]>

DaveDeCaprio added 4 commits November 27, 2018 13:18

Merge pull request #1 from apache/master

22bd4bd

merge in spark

Added a configurable limit on the maximum length of a plan debug string.

b7f964d

Removed unneeded imports

3f4fe1f

Moved withSizeLimitedWriter to treeString function that uses StringWr…

30e4348

…iter

DaveDeCaprio mentioned this pull request Nov 28, 2018

[SPARK-26103][SQL] Added maxDepth to limit the length of text plans #23076

Closed

Fixed formatting

5528ca1

Coverted to javadoc style multiline comment

3171cf3

Fixed scalastyle formatting of imports

3ffdc6a

HeartSaVioR reviewed Nov 29, 2018

View reviewed changes

Have size limit cut off right at the correct number of characters.

45a60fc

Changed name to remove the debug from the config parameter name

9678799

Added the limit in for query execution

Fixed formatting error

a5af842

felixcheung reviewed Nov 29, 2018

View reviewed changes

DaveDeCaprio added 2 commits December 3, 2018 17:23

Changed length default to Long.MaxValue to turn off behavior unless i…

a4be985

…t is specifically configured.

Merge branch 'master' of https://github.com/apache/spark into text-pl…

f0f75c2

…an-size # Conflicts: # sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala

vanzin reviewed Mar 6, 2019

View reviewed changes

MaxGekk requested changes Mar 6, 2019

View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/StringUtils.scala Show resolved Hide resolved

DaveDeCaprio added 3 commits March 8, 2019 14:13

Addressed code review comments.

daf02f2

Missed some Boolean->Unit updates

7d89388

Missed some Boolean->Unit updates

4f56e48

Code formatting

a090fbb

Fixed Fatal Warning.

dcb4eb0

MaxGekk approved these changes Mar 8, 2019

View reviewed changes

Fixed failing unit tests

b4cb7bf

vanzin reviewed Mar 11, 2019

View reviewed changes

DaveDeCaprio added 2 commits March 11, 2019 14:12

Style/formatting issues from code review

4fec590

Style/formatting issues from code review

db0db18

vanzin reviewed Mar 12, 2019

View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala Outdated Show resolved Hide resolved

DaveDeCaprio added 2 commits March 12, 2019 20:48

Changed plan string length to bytesConf and gave a better message for…

b5b30f3

… 0 length plans.

Tabs to spaces

e4afa26

vanzin closed this in 812ad55 Mar 13, 2019

ueshin mentioned this pull request Mar 26, 2019

[SPARK-26103][SQL][FOLLOW-UP] Use string-interpolation to show the config key. #24217

Closed

[SPARK-26103][SQL] Limit the length of debug strings for query plans #23169

[SPARK-26103][SQL] Limit the length of debug strings for query plans #23169

Uh oh!

Conversation

DaveDeCaprio commented Nov 28, 2018

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

DaveDeCaprio commented Nov 28, 2018

Uh oh!

gatorsmile commented Nov 28, 2018

Uh oh!

SparkQA commented Nov 28, 2018

Uh oh!

SparkQA commented Nov 28, 2018

Uh oh!

HeartSaVioR left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

SparkQA commented Nov 29, 2018

Uh oh!

SparkQA commented Nov 29, 2018

Uh oh!

SparkQA commented Nov 29, 2018

Uh oh!

DaveDeCaprio commented Nov 29, 2018

Uh oh!

SparkQA commented Nov 29, 2018

Uh oh!

felixcheung left a comment

Choose a reason for hiding this comment

Uh oh!

DaveDeCaprio commented Nov 29, 2018

Uh oh!

DaveDeCaprio commented Dec 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SparkQA commented Dec 4, 2018

Uh oh!

SparkQA commented Dec 4, 2018

Uh oh!

HeartSaVioR commented Dec 4, 2018

Uh oh!

HeartSaVioR commented Dec 4, 2018

Uh oh!

vanzin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

MaxGekk commented Mar 6, 2019

Uh oh!

DaveDeCaprio commented Mar 8, 2019

Uh oh!

SparkQA commented Mar 8, 2019

Uh oh!

SparkQA commented Mar 8, 2019

Uh oh!

MaxGekk left a comment

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Mar 9, 2019

Uh oh!

SparkQA commented Mar 9, 2019

Uh oh!

vanzin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

DaveDeCaprio commented Dec 3, 2018 •

edited

Loading