[SPARK-26504][SQL] Rope-wise dumping of Spark plans #23406

MaxGekk · 2018-12-29T13:16:07Z

What changes were proposed in this pull request?

Proposed new class StringConcat for converting a sequence of strings to string with one memory allocation in the toString method. StringConcat replaces StringBuilderWriter in methods of dumping of Spark plans and codegen to strings.

All Writer arguments are replaced by String => Unit in methods related to Spark plans stringification.

How was this patch tested?

It was tested by existing suites QueryExecutionSuite, DebuggingSuite as well as new tests for StringConcat in StringUtilsSuite.

MaxGekk · 2018-12-29T13:16:37Z

@hvanhovell Please, take a look at the PR.

SparkQA · 2018-12-29T17:13:58Z

Test build #100537 has finished for PR 23406 at commit f621de7.

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

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

SparkQA · 2018-12-29T21:25:06Z

Test build #100539 has finished for PR 23406 at commit ca4aed8.

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

hvanhovell · 2018-12-29T21:37:32Z

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

  }
+
+  class StringRope {
+    private var list = List.empty[String]


Let's use a ListBuffer or an ArrayBuffer here. Those have (amortized) constant time appends and do not force you to reverse the collection when building the string.

I supposed this reverse is relatively cheap O(n). Coping a string would be more expensive than adding element to the head of list inside of the reverse() method. In any case, need to traverse over the list in toString.

I replaced List by ArrayBuffer

hvanhovell · 2018-12-29T21:51:32Z

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


-  private def writeOrError(writer: Writer)(f: Writer => Unit): Unit = {
-    try f(writer)
+  private def appendOrError(append: String => Unit)(f: (String => Unit) => Unit): Unit = {


You always call this method on a QueryPlan. Can we specialize this method for this scenario and pass the plan and all the needed treeString parameters?

Different question. Is it possible that treeString already writes to the appender before throwing an exception. It it does the output might look pretty weird, because it will and contain a part of the tree and the exception thrown.

because it will and contain a part of the tree and the exception thrown

In the case of writing to a file, I think it is possible. I believe it will be a nice feature in trouble shooting. I would image a huge (maybe wrong) plan causes OOMs at some point. If we write some part of the plan to file, it would be helpful in debugging.

Can we specialize this method for this scenario and pass the plan and all the needed treeString parameters?

@hvanhovell Do you mean changes like in the PR MaxGekk#16 ? Unfortunately it doesn't work well because plan's constructor can produce AnalysisException which cannot be handled with this approach.

Yeah that would be it. You can pass in a call-by-name parameter if you want to capture errors during construction. I prefer his over having some overly generic function.

Will be ok if I move appendOrError to the companion object of QueryPlan? Like:

object QueryPlan extends PredicateHelper { /** * Converts the query plan to string and appends it via provided function. */ def append[T <: QueryPlan[T]]( plan: => QueryPlan[T], append: String => Unit, verbose: Boolean, addSuffix: Boolean, maxFields: Int = SQLConf.get.maxToStringFields): Unit = { () }

Sure that works.

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

hvanhovell · 2018-12-29T21:53:19Z

cc @rednaxelafx

SparkQA · 2018-12-30T15:33:01Z

Test build #100556 has finished for PR 23406 at commit 544b80e.

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

hvanhovell · 2018-12-30T16:27:42Z

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

+     */
+    override def toString: String = {
+      val buffer = new StringBuffer(length)
+


Why the new line?

hvanhovell · 2018-12-30T17:11:07Z

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

  }
+
+  /**
+   * Concatenation of sequence of strings to final string with cheap append method


I am wondering if StringConcatenation is a better name for this class, as this class is technically a rope (that is a binary tree).

StringRope looks nicer ;-)

... and is wrong :P...

StringConcat?

hvanhovell

LGTM - pending jenkins

SparkQA · 2018-12-31T01:31:44Z

Test build #100573 has finished for PR 23406 at commit 074e9b8.

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

rednaxelafx

Looks good overall. One minor comment on using StringBuffer

rednaxelafx · 2018-12-31T01:45:02Z

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

+     * returns concatenated string.
+     */
+    override def toString: String = {
+      val result = new StringBuffer(length)


Can we just use java.lang.StringBuilder here for the sake of reducing one useless allocation of the Scala wrapper?
Which StringBuffer is this anyway? If you're using the Scala scala.collection.mutable.*, it should have been StringBuilder, right?

Replaced by java.lang.StringBuilder

SparkQA · 2018-12-31T13:27:20Z

Test build #100587 has finished for PR 23406 at commit 29b62bf.

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

hvanhovell · 2018-12-31T15:39:03Z

Merging to master. Thanks!

srowen · 2018-12-31T15:47:27Z

Late but looks good to me.

MaxGekk · 2018-12-31T16:13:35Z

@hvanhovell @srowen @rednaxelafx Happy New Year!!!

## What changes were proposed in this pull request? Proposed new class `StringConcat` for converting a sequence of strings to string with one memory allocation in the `toString` method. `StringConcat` replaces `StringBuilderWriter` in methods of dumping of Spark plans and codegen to strings. All `Writer` arguments are replaced by `String => Unit` in methods related to Spark plans stringification. ## How was this patch tested? It was tested by existing suites `QueryExecutionSuite`, `DebuggingSuite` as well as new tests for `StringConcat` in `StringUtilsSuite`. Closes apache#23406 from MaxGekk/rope-plan. Authored-by: Maxim Gekk <[email protected]> Signed-off-by: Herman van Hovell <[email protected]>

Proposed new class `StringConcat` for converting a sequence of strings to string with one memory allocation in the `toString` method. `StringConcat` replaces `StringBuilderWriter` in methods of dumping of Spark plans and codegen to strings. All `Writer` arguments are replaced by `String => Unit` in methods related to Spark plans stringification. It was tested by existing suites `QueryExecutionSuite`, `DebuggingSuite` as well as new tests for `StringConcat` in `StringUtilsSuite`. Closes apache#23406 from MaxGekk/rope-plan. Authored-by: Maxim Gekk <[email protected]> Signed-off-by: Herman van Hovell <[email protected]>

MaxGekk added 5 commits December 29, 2018 12:42

Use append function instead of Writer

de38cf8

Fix typo

af5c9ed

StringRope on List

0a36a26

Tests for StringRope

07ae72d

Porting on StringRope

f621de7

Rope other methods

ca4aed8

hvanhovell reviewed Dec 29, 2018

View reviewed changes

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

hvanhovell reviewed Dec 29, 2018

View reviewed changes

sql/core/src/main/scala/org/apache/spark/sql/execution/QueryExecution.scala Show resolved Hide resolved

MaxGekk added 3 commits December 30, 2018 12:29

Added docs

d727959

Replacing List by ArrayBuffer

544b80e

Inline stringOrError

059dcf4

hvanhovell reviewed Dec 30, 2018

View reviewed changes

MaxGekk added 3 commits December 30, 2018 22:26

Move appendOrError to QueryPlan

fe3bbcf

Renaming StringRope to StringConcat

707c80b

rope -> concat in tests

074e9b8

hvanhovell approved these changes Dec 30, 2018

View reviewed changes

rednaxelafx reviewed Dec 31, 2018

View reviewed changes

StringBuffer -> StringBuilder

29b62bf

asfgit closed this in 89c92cc Dec 31, 2018

MaxGekk deleted the rope-plan branch August 17, 2019 13:35

[SPARK-26504][SQL] Rope-wise dumping of Spark plans #23406

[SPARK-26504][SQL] Rope-wise dumping of Spark plans #23406

Uh oh!

Conversation

MaxGekk commented Dec 29, 2018 • 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!

MaxGekk commented Dec 29, 2018

Uh oh!

SparkQA commented Dec 29, 2018

Uh oh!

Uh oh!

SparkQA commented Dec 29, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

hvanhovell commented Dec 29, 2018

Uh oh!

SparkQA commented Dec 30, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hvanhovell left a comment

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Dec 31, 2018

Uh oh!

rednaxelafx left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Dec 31, 2018

Uh oh!

hvanhovell commented Dec 31, 2018

Uh oh!

srowen commented Dec 31, 2018

Uh oh!

MaxGekk commented Dec 31, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

MaxGekk commented Dec 29, 2018 •

edited

Loading

MaxGekk Dec 29, 2018 •

edited

Loading