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

[DaveDeCaprio]

Nested query plans can get extremely large (hundreds of megabytes).

What changes were proposed in this pull request?

The PR puts in a limit on the nesting depth of trees to be printed when writing a plan string.

• The default limit is 15, which allows for reasonably nested plans.
• A new configuration parameter called spark.debug.maxToStringTreeDepth was added to control the depth.
• When plans are truncated, "..." is printed to indicate that tree elements were removed.
• 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?

A new unit test in QueryExecutionSuite which creates a highly nested plan and then ensures that the printed plan is not too long.

Please review http://spark.apache.org/contributing.html before opening a pull request.

[AmplabJenkins]

Can one of the admins verify this patch?

[DaveDeCaprio]

This contribution is my original work and I license the work to the project under the project’s open source license.

[DaveDeCaprio]

Not sure TreeNode should have the baggage of its own logger. The catalyst.trees package object extends logger and there is a comment that tree nodes should just use that logger, but I don't see a way to do that since loggers are protected.

I could add a log function to the package object and then use that from TreeNode:

private[trees] def logWarning 

[DaveDeCaprio]

I wasn't sure where to put this code, so made a TreeNode companion object. If there is a better place let me know.

[DaveDeCaprio]

Prefix and addSuffix are ignored in the original code but I can't tell why. Seems like they should be passed through from parent to child here. I adjusted, but it might not be a bug or maybe should go in a separate PR

Suggested change

	child.generateTreeString(depth, lastChildren, writer, verbose, prefix, addSuffix, maxDepth)
	child.generateTreeString(depth, lastChildren, writer, verbose, prefix = "", addSuffix = false, maxDepth)

[DaveDeCaprio]

@MaxGekk and @hvanhovell, I think this PR is complementary to the PR for SPARK-26023 which recently made allowed for file dumping of queries.

It might make sense to only have the default depth apply to the toString function and not writing to a file. We could have a parameter to toFile to control the depth, but that would default to Int.MaxValue. toString would use the configured parameter.

[MaxGekk]

@DaveDeCaprio What is the practical use case for limiting the plans? My initial goal in the PRs #22429, #23018 and #23039 is to dump whole plans and gather more info for debugging. Your PR has an opposite purpose, it seems.

[DaveDeCaprio]

The goal of this is to avoid creation of massive strings if toString gets called on a QueryExecution. In our use we have some large plans that generate plan strings in the hundreds of megabytes. These plans are efficiently executed because of caching and partitioning, but we are running into out of memory errors because of the QueryExecution.toString.

In our case, writing to a file would help the memory issue, but we'd still be writing 200Mb+ files for each query. (Note these queries take seconds to run because 99% of the work is cached, and we run a bunch of them).

[DaveDeCaprio]

I originally wrote this against 2.3.0 where the toString was always called as part of newExecutionId. I'm not sure if that still happens in master.

[DaveDeCaprio]

Any case where we use truncatedString to limit the output columns I also think makes sense to limit the plan depth.

[HeartSaVioR]

I'm seeing both sides of needs: while I think dumping full plan into file is a good feature for debugging specific issue, retaining full plans for representing them to UI page have been a headache and three regarding issues (SPARK-23904, SPARK-25380, SPARK-26103) are filed in 3 months which doesn't look like a thing we can say end users should take a workaround.

One thing we may be aware is that huge plan is not generated not only from nested join, but also from lots of columns, like SPARK-23904. For SPARK-25380 we are not aware of which parts generate huge plan. So we might feel easier and flexible to just truncate to specific size rather than applying conditions.

[DaveDeCaprio]

Limiting on pure size makes sense. I'd be willing to implement that in a different PR. Now that we are using a writer it should be pretty easy to do that. That was harder in the Spark 2.4 branch which is why I took the approach I did. A few questions about that: * I assume we still want a config parameter for maximum size. Any suggestions on name: spark.debug.maxPlanStringSizeInMB? * Is there a way to represent byte sizes in config? * Should the limit be on each element of the plan or the overall plan? Logical Plan, Physical Plan, Optimized Logical Plan, etc? I kind of like the idea of putting it on the treeString method itself since then it gets applied whereever that method is called rather than only in queryExecution.toString. Dave From: Jungtaek Lim <[email protected]> Sent: Sunday, November 18, 2018 11:07 PM To: apache/spark <[email protected]> Cc: David DeCaprio <[email protected]>; Mention <[email protected]> Subject: Re: [apache/spark] [SPARK-26103][SQL] Added maxDepth to limit the length of text plans (#23076) I'm seeing both sides of needs: while I think dumping full plan into file is a good feature for debugging specific issue, retaining full plans for representing them to UI page have been a headache and three regarding issues (SPARK-23904 <https://issues.apache.org/jira/browse/SPARK-23904> , SPARK-25380 <https://issues.apache.org/jira/browse/SPARK-25380> , SPARK-26103 <https://issues.apache.org/jira/browse/SPARK-26103> ) are filed in 3 months which doesn't look like a thing we can say end users should take a workaround. One thing we may be aware is that huge plan is not generated not only from nested join, but also from lots of columns, like SPARK-23904. For SPARK-25380 we are not aware of which parts generate huge plan. So we might feel easier and flexible to just truncate to specific size rather than applying conditions. - You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#23076 (comment)> , or mute the thread <https://github.com/notifications/unsubscribe-auth/AAzVuuhxE9ejYmlbwLnamQkHK NAGV8zpks5uwjxbgaJpZM4Ynzoz> . <https://github.com/notifications/beacon/AAzVulccV4Ui9iSrDibkHyHFCy06Tz8Cks5 uwjxbgaJpZM4Ynzoz.gif> <https://t.sidekickopen72.com/s1t/o/5/f18dQhb0S7kC8dDMPbW2n0x6l2B9gXrN7sKj6v 5KRN6W56jT_C3Lq__nW1pctGF2VxGHQf197v5Y04?si=7000000001068294&pi=d758f5d1-2c3 4-4ded-8e49-c782907b9060>

[MaxGekk]

I assume we still want a config parameter for maximum size. Any suggestions on name: spark.debug.maxPlanStringSizeInMB?

I would propose to make it more SQL specific, and put the config to the spark.sql.debug namespace. Especially in the light of moving truncatedString to sql/catalyst (see #23039 )

[DaveDeCaprio]

Closing this pull request in favor of #23169