[SPARK-31615][SQL] Pretty string output for sql method of RuntimeReplaceable expressions #28420

yaooqinn · 2020-04-30T07:49:50Z

What changes were proposed in this pull request?

The RuntimeReplaceable ones are runtime replaceable, thus, their original parameters are not going to be resolved to PrettyAttribute and remain debug style string if we directly implement their sql methods with their parameters' sql methods.

This PR is raised with suggestions by @maropu and @cloud-fan https://github.com/apache/spark/pull/28402/files#r417656589. In this PR, we re-implement the sql methods of the RuntimeReplaceable ones with toPettySQL

Why are the changes needed?

Consistency of schema output between RuntimeReplaceable expressions and normal ones.

For example, date_format vs to_timestamp, before this PR, they output differently

Before

select date_format(timestamp '2019-10-06', 'yyyy-MM-dd uuuu')
struct<date_format(TIMESTAMP '2019-10-06 00:00:00', yyyy-MM-dd uuuu):string>

select to_timestamp("2019-10-06S10:11:12.12345", "yyyy-MM-dd'S'HH:mm:ss.SSSSSS")
struct<to_timestamp('2019-10-06S10:11:12.12345', 'yyyy-MM-dd\'S\'HH:mm:ss.SSSSSS'):timestamp>

After

select date_format(timestamp '2019-10-06', 'yyyy-MM-dd uuuu')
struct<date_format(TIMESTAMP '2019-10-06 00:00:00', yyyy-MM-dd uuuu):string>



select to_timestamp("2019-10-06T10:11:12'12", "yyyy-MM-dd'T'HH:mm:ss''SSSS")

struct<to_timestamp(2019-10-06T10:11:12'12, yyyy-MM-dd'T'HH:mm:ss''SSSS):timestamp>

Does this PR introduce any user-facing change?

Yes, the schema output style changed for the runtime replaceable expressions as shown in the above example

How was this patch tested?

regenerate all related tests

…aceable expressions

cloud-fan · 2020-04-30T07:58:52Z

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

@@ -1206,7 +1207,7 @@ case class DatetimeSub(
    interval: Expression,
    child: Expression) extends RuntimeReplaceable {
  override def toString: String = s"$start - $interval"
-  override def sql: String = s"${start.sql} - ${interval.sql}"
+  override def sql: String = s"${toPrettySQL(start)} - ${toPrettySQL(interval)}"


This means RuntimeReplaceable.sql always use pretty sql, while other expressions only use pretty sql when the caller side needs to (by calling toPrettySQL).

Is it possible to make them consistent? Like make start and interval as innerChildren and handle them in usePrettyExpression.

OK, let me see

SparkQA · 2020-04-30T10:06:38Z

Test build #122122 has finished for PR 28420 at commit c0fd2a6.

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

SparkQA · 2020-04-30T11:22:03Z

Test build #122123 has finished for PR 28420 at commit 3e66c25.

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

SparkQA · 2020-04-30T19:55:44Z

Test build #122142 has finished for PR 28420 at commit 29c25b9.

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

maropu

Thanks for the followup, @yaooqinn

maropu · 2020-04-30T23:33:38Z

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

+  override def innerChildren: Seq[Expression] = sys.error("RuntimeReplaceable must implement" +
+    " innerChildren with the original parameters")
+
+  protected val sqlStrSeparator: String = ", "


We need this variable? It seems only Extract uses this.

maropu · 2020-04-30T23:37:46Z

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

@@ -323,6 +323,20 @@ trait RuntimeReplaceable extends UnaryExpression with Unevaluable {
  // two `RuntimeReplaceable` are considered to be semantically equal if their "child" expressions
  // are semantically equal.
  override lazy val canonicalized: Expression = child.canonicalized
+
+  override def innerChildren: Seq[Expression] = sys.error("RuntimeReplaceable must implement" +


Since compilers cannot catch the case where one forget to override this, how about it like this?

// RuntimeReplaceable def exprsReplaced: Seq[Expression] override def innerChildren: Seq[Expression] = exprsReplaced // e.g., ParseToTimestamp override def exprsReplaced: Seq[Expression] = Seq(Some(left), format).flatten

Good Point!

maropu · 2020-04-30T23:40:38Z

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

+  override def sql: String = RuntimeReplaceable.this.prettyName +
+    prettyChildren.map(_.sql).mkString("(", sqlStrSeparator, ")")
+
+  protected var prettyChildren: Seq[Expression] = innerChildren


I think its better not to use var where possible, so how about it like this?

// RuntimeReplaceable def newInstance(innerChildren: Seq[Expression]): RuntimeReplaceable // e.g., ParseToTimestamp override def newInstance(innerChildren: Seq[Expression]): ParseToTimestamp = { innerChildren match { case Seq(left) => ParseToTimestamp(left, None, child) case Seq(left, format) => ParseToTimestamp(left, Some(format), child) } }

Then, update the toPrettySQL in utils.package;

def toPrettySQL(e: Expression): String = e match { case r: RuntimeReplaceable => r.newInstance(r.innerChildren.map(usePrettyExpression)).sql case _ => usePrettyExpression(e).sql }

The newInstance seems not robust to make a clone with Seq[Expression]

I think we can just add def prettySQL: String in RuntimeReplaceable for utils.package to call, and override it wherever needed without creating new instances.

Seems a better and simpler idea that comes up based on your suggestions, WDYT?

I feel having the 'pretty' functionality in RuntimeReplaceable is a bit intrusive. Dropping my idea above is okay, but I think we need a more general solution for getting pretty strings outside RuntimeReplaceable. WDYT? @cloud-fan

cloud-fan · 2020-05-01T06:18:37Z

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

+   */
+  def exprsReplaced: Seq[Expression]
+
+  override def sql: String = RuntimeReplaceable.this.prettyName +


nit: seems we can remove RuntimeReplaceable.this.?

cloud-fan · 2020-05-01T06:20:10Z

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

-  override def prettyName: String = "extract"
+  override def exprsReplaced: Seq[Expression] = Seq(field, source)
+
+  override def sql: String = super.sql.replace(", ", " FROM ")


this is tricky and hard to read, can we just generate the sql string?

cloud-fan · 2020-05-01T06:21:04Z

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

@@ -142,7 +142,10 @@ package object util extends Logging {
    "`" + name.replace("`", "``") + "`"
  }

-  def toPrettySQL(e: Expression): String = usePrettyExpression(e).sql
+  def toPrettySQL(e: Expression): String = e match {
+    case r: RuntimeReplaceable => r.prettySQL


does this work if RuntimeReplaceable is not the root node?

c42a283#diff-6f4edc80e2cc973e748705e85a6053b4R786-R807

fixed and added some nested tests, thanks, good catch!

SparkQA · 2020-05-01T09:24:07Z

Test build #122160 has finished for PR 28420 at commit f6bc861.

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

SparkQA · 2020-05-01T16:15:11Z

Test build #122172 has finished for PR 28420 at commit c42a283.

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

SparkQA · 2020-05-01T21:00:32Z

Test build #122176 has finished for PR 28420 at commit 128ca14.

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

SparkQA · 2020-05-03T07:05:01Z

Test build #122224 has finished for PR 28420 at commit ac511ad.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

yaooqinn · 2020-05-03T07:11:57Z

retest this please

SparkQA · 2020-05-03T09:06:49Z

Test build #122225 has finished for PR 28420 at commit ac511ad.

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

maropu · 2020-05-03T10:38:47Z

retest this please

SparkQA · 2020-05-03T15:15:06Z

Test build #122228 has finished for PR 28420 at commit ac511ad.

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

cloud-fan · 2020-05-05T08:45:11Z

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

+  protected def prettySQL: String =
+    prettyName + exprsReplaced.map(toPrettySQL).mkString("(", ", ", ")")
+
+  def asPrettyAttribute(): PrettyAttribute = PrettyAttribute(prettySQL, dataType)


why not add a def withPrettyAttributes: this.type? The current way requires fewer changes as we can have a default implementation, but it's hacky to convert RuntimeReplaceable to a single PrettyAttribute.

I think it might be better to move this to util.package

SparkQA · 2020-05-05T15:28:45Z

Test build #122315 has finished for PR 28420 at commit e43d9ed.

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

cloud-fan · 2020-05-06T09:30:20Z

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

+   */
+  protected def exprsReplaced: Seq[Expression]
+
+  override def sql: String = prettyName + exprsReplaced.map(_.sql).mkString("(", ", ", ")")


another idea: maybe we can define a mkString method?

def exprsReplaced: Seq[Expression] def mkString(childrenString: Seq[String]): String = childrenString.mkString("(", ", ", ")") override def sql: String = prettyName + mkString(exprsReplaced.map(_.sql))

then in util

case r: RuntimeReplaceable => PrettyAttribute(r.mkString(r.exprsReplaced.map(toPrettySQL)), r.dataType)

SGTM, this way is much better. We should put the prettyName in mkString though.

SparkQA · 2020-05-06T17:46:18Z

Test build #122363 has finished for PR 28420 at commit 5843ac6.

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

yaooqinn · 2020-05-06T17:49:03Z

retest this please

SparkQA · 2020-05-06T22:45:26Z

Test build #122375 has finished for PR 28420 at commit 5843ac6.

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

maropu · 2020-05-06T23:17:53Z

We need to backport this into branch-2.4, too? I personally think printing debug info looks like a minor bug. @cloud-fan @dongjoon-hyun

cloud-fan · 2020-05-07T04:36:14Z

It's a minor bug, but the diff is not small. Maybe master-only is good enough?

maropu · 2020-05-07T05:39:37Z

Looks okay to me.

maropu · 2020-05-07T05:40:52Z

Thanks! Merged to master.

HyukjinKwon · 2020-05-11T08:39:37Z

sql/core/src/test/resources/sql-tests/results/predicate-functions.sql.out

@@ -245,54 +245,54 @@ true
 -- !query
 select to_date('2009-07-30 04:17:52') <= to_date('2009-07-30 04:17:52')
 -- !query schema
-struct<(to_date('2009-07-30 04:17:52') <= to_date('2009-07-30 04:17:52')):boolean>
+struct<(to_date(2009-07-30 04:17:52) <= to_date(2009-07-30 04:17:52)):boolean>


Sorry I happened to leave a comment late but why was this changed? Seems the previous one looks better.

check this

+-- !query +select date_format('2019-10-06', 'yyyy-MM-dd uuuu') +-- !query schema +struct<date_format(CAST(2019-10-06 AS TIMESTAMP), yyyy-MM-dd uuuu):string> +-- !query output +2019-10-06 Sunday

Did we discuss about changing ^ case instead?

Not yet. This PR just makes the RuntimeReplaceable -> PrettyAttribute. I agree with you that the PrettyAttribute is not beautiful in itself.

HyukjinKwon · 2020-05-11T08:45:30Z

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

+
+  def mkString(childrenString: Seq[String]): String = {
+    prettyName + childrenString.mkString("(", ", ", ")")
+  }


Why do we need two extra interface methods just to make the column name pretty? Seems the output isn't even completely pretty.

It's not about pretty or not. It's about consistency. If we call a function with its alias name, then it will not use PrettyAttribute and the string representation is different from if we call the function with the original name.

The consistency is good but the readability here is the problem. See the implementation side:

override def sql: String = s"$prettyName(${field.sql} FROM ${source.sql})" override def prettyName: String = "extract"

vs

override def exprsReplaced: Seq[Expression] = Seq(field, source) override def mkString(childrenString: Seq[String]): String = { prettyName + childrenString.mkString("(", " FROM ", ")") }

Previously, it was fine to only know sql and prettyName but now you should know what exprsReplaced and mkString are. From the naming, it's confusing what that means too.

I would just document that sql and prettyName should be properly overridden at RuntimeReplaceable just like we have the restriction about the child. We could have an util for it as well.

The caller side decides if he wants pretty string or not, by calling Expression.sql or toPrettySQL(Expression). So simply overridden sql doesn't work, unless we add a boolean parameter.

HyukjinKwon · 2020-05-11T09:39:28Z

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

@@ -134,6 +134,8 @@ package object util extends Logging {
      PrettyAttribute(usePrettyExpression(e.child).sql + "." + name, e.dataType)
    case e: GetArrayStructFields =>
      PrettyAttribute(usePrettyExpression(e.child) + "." + e.field.name, e.dataType)
+    case r: RuntimeReplaceable =>


And strictly RuntimeReplaceable itself isn't an attribute ...

We already did the same to GetStructField :)

[SPARK-31615][SQL] Pretty string output for sql method of RuntimeRepl…

c0fd2a6

…aceable expressions

probot-autolabeler bot added the SQL label Apr 30, 2020

nit

3e66c25

cloud-fan reviewed Apr 30, 2020

View reviewed changes

new way

29c25b9

maropu changed the title ~~[SPARK-31615][SQL] Pretty string output for sql method of RuntimeRepl…~~ [SPARK-31615][SQL] Pretty string output for sql method of RuntimeReplaceable expressions Apr 30, 2020

maropu reviewed Apr 30, 2020

View reviewed changes

use prettySQL

f6bc861

cloud-fan reviewed May 1, 2020

View reviewed changes

fix non root replaceable

c42a283

golden file

128ca14

py test

ac511ad

probot-autolabeler bot added the PYTHON label May 3, 2020

cloud-fan reviewed May 5, 2020

View reviewed changes

mv PrettyAttribute to uitl

e43d9ed

cloud-fan reviewed May 6, 2020

View reviewed changes

mkString

5843ac6

cloud-fan approved these changes May 6, 2020

View reviewed changes

maropu approved these changes May 6, 2020

View reviewed changes

maropu closed this in b31ae7b May 7, 2020

HyukjinKwon reviewed May 11, 2020

View reviewed changes

[SPARK-31615][SQL] Pretty string output for sql method of RuntimeReplaceable expressions #28420

[SPARK-31615][SQL] Pretty string output for sql method of RuntimeReplaceable expressions #28420

Conversation

yaooqinn commented Apr 30, 2020 • edited by maropu

What changes were proposed in this pull request?

Why are the changes needed?

Before

After

Does this PR introduce any user-facing change?

How was this patch tested?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Apr 30, 2020

SparkQA commented Apr 30, 2020

SparkQA commented Apr 30, 2020

maropu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maropu Apr 30, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maropu May 1, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yaooqinn May 1, 2020 • edited

Choose a reason for hiding this comment

SparkQA commented May 1, 2020

SparkQA commented May 1, 2020

SparkQA commented May 1, 2020

SparkQA commented May 3, 2020

yaooqinn commented May 3, 2020

SparkQA commented May 3, 2020

maropu commented May 3, 2020

SparkQA commented May 3, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented May 5, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented May 6, 2020

yaooqinn commented May 6, 2020

SparkQA commented May 6, 2020

maropu commented May 6, 2020 • edited

cloud-fan commented May 7, 2020

maropu commented May 7, 2020 • edited

maropu commented May 7, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HyukjinKwon May 11, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yaooqinn commented Apr 30, 2020 •

edited by maropu

maropu Apr 30, 2020 •

edited

maropu May 1, 2020 •

edited

yaooqinn May 1, 2020 •

edited

maropu commented May 6, 2020 •

edited

maropu commented May 7, 2020 •

edited

HyukjinKwon May 11, 2020 •

edited