[SPARK-33989][SQL] Strip auto-generated cast when using Cast.sql #31034

ulysses-you · 2021-01-05T08:50:49Z

What changes were proposed in this pull request?

This PR aims to strip auto-generated cast. The main logic is:

Add tag if Cast is specified by user.
Wrap PrettyAttribute in usePrettyExpression.

Why are the changes needed?

Make sql consistent with dsl. Here is an inconsistent example before this PR:

-- output field name: FLOOR(1)
spark.emptyDataFrame.select(floor(lit(1)))

-- output field name: FLOOR(CAST(1 AS DOUBLE))
spark.sql("select floor(1)")

Note that, we don't remove the Cast so the auto-generated Cast can still work. The only changed place is usePrettyExpression, we use PrettyAttribute replace Cast to give a better sql string.

Does this PR introduce any user-facing change?

Yes, the default field name may change.

How was this patch tested?

Add test and pass exists test.

…3989

ulysses-you · 2021-01-05T09:01:05Z

Seems the change is quite big but mostly are golden files cc @maropu @cloud-fan

SparkQA · 2021-01-05T09:52:02Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/38256/

SparkQA · 2021-01-05T10:17:45Z

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/38256/

SparkQA · 2021-01-05T12:50:38Z

Test build #133667 has finished for PR 31034 at commit 7b00641.

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

maropu · 2021-01-05T13:26:56Z

(I'm neutral on this though) why is the name consistency important? Is there the concrete scenario where the name inconsistency can cause any issue?

SparkQA · 2021-01-06T02:55:34Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/38286/

SparkQA · 2021-01-06T03:25:21Z

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/38286/

SparkQA · 2021-01-06T06:14:35Z

Test build #133698 has finished for PR 31034 at commit a160d62.

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

cloud-fan · 2021-01-06T08:18:01Z

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

-    // back to SQL query string.
-    case _: ArrayType | _: MapType | _: StructType => child.sql
-    case _ => s"CAST(${child.sql} AS ${dataType.sql})"
+  override def sql: String = {


Let's not change the sql method.

ResolveAlias calls toPrettySQL to generate the alias name, and we should strip auto-generated alias in toPrettySQL.

SparkQA · 2021-01-07T14:13:31Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/38386/

SparkQA · 2021-01-07T14:46:06Z

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/38386/

SparkQA · 2021-01-07T16:21:56Z

Test build #133796 has finished for PR 31034 at commit 3823289.

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

SparkQA · 2021-01-11T13:22:27Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/38516/

SparkQA · 2021-01-11T13:52:58Z

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/38516/

SparkQA · 2021-01-11T15:09:45Z

Test build #133924 has finished for PR 31034 at commit ab827fc.

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

SparkQA · 2021-01-11T17:29:56Z

Test build #133929 has finished for PR 31034 at commit e5294fe.

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

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/analysis/ResolveAliasesSuite.scala

cloud-fan · 2021-01-12T16:47:55Z

I like this idea to exclude auto-added cast in the auto-generated alias. This should be a safe change as the auto-generated alias is mostly for display purposes. Let's get more feedback though. cc @maropu @viirya @dongjoon-hyun @HyukjinKwon

HyukjinKwon · 2021-01-13T00:51:41Z

~~I'm okay. I wonder if it's better and possible to have that cast when we explain alone, and use the stripped output for column names but I don't feel strongly on this.~~, oh, I took a look again. That's already what this PR does.

SparkQA · 2021-01-13T02:15:22Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/38575/

SparkQA · 2021-01-13T02:44:02Z

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/38575/

SparkQA · 2021-01-13T06:31:10Z

Test build #133987 has finished for PR 31034 at commit 3c23778.

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

ulysses-you · 2021-01-13T06:58:41Z

How about add a config like extended or simple to decide whether show the Cast or not ? That sounds more like a debug config which maybe can help user. But after another thought, it seems a little redundent since we have already given the analyzed output data type in extended explain.

viirya · 2021-01-13T07:30:35Z

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

    case r: RuntimeReplaceable =>
      PrettyAttribute(r.mkString(r.exprsReplaced.map(toPrettySQL)), r.dataType)
+    case c: CastBase if !c.getTagValue(Cast.USER_SPECIFIED_CAST).getOrElse(false) =>
+      PrettyAttribute(usePrettyExpression(c.child).sql, c.dataType)


So the purpose is to strip Cast only from toPrettySQL? Only for changing column name but the actual Cast expression still works?

Could you elaborate it clearly in the PR description?

Yes, it's the only place which we changed. Update the description.

viirya · 2021-01-13T07:36:39Z

I like this idea to exclude auto-added cast in the auto-generated alias. This should be a safe change as the auto-generated alias is mostly for display purposes. Let's get more feedback though. cc @maropu @viirya @dongjoon-hyun @HyukjinKwon

If the change is restricted to only auto-generated alias, it sounds okay. But this still sounds like a breaking change as the column name is changed? Do we need to update migration guide for this?

cloud-fan · 2021-01-13T09:17:22Z

Do we need to update migration guide for this?

yea better to add. But I do wonder if people will do

df.select("`CAST(a AS INT)`")

to select that column...

docs/sql-migration-guide.md

SparkQA · 2021-01-13T11:24:20Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/38596/

SparkQA · 2021-01-13T11:28:57Z

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/38596/

SparkQA · 2021-01-13T13:41:40Z

Test build #134005 has finished for PR 31034 at commit 3217d5b.

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

SparkQA · 2021-01-13T15:20:41Z

Test build #134008 has finished for PR 31034 at commit 2d13353.

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

cloud-fan · 2021-01-14T15:27:11Z

thanks, merging to master!

HyukjinKwon · 2021-01-15T01:29:39Z

i took a close look too. LGTM.

ulysses-you · 2021-01-15T02:33:50Z

thanks all !

ulysses-you added 9 commits January 4, 2021 20:44

init

d66ecc1

add cast tag

52440cd

add test

eb0ad21

sql output golden file

c82c928

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

68fe16e

…3989

test name

e398dd3

plan golden file

a67e915

fix sql output

dd891e5

fix sql output

7b00641

fix sql-expression-schem

ad6a6f7

github-actions bot added the SQL label Jan 5, 2021

ulysses-you added 2 commits January 6, 2021 09:49

fix explainSuite

264f5bb

fix context.py

a160d62

github-actions bot added CORE PYTHON labels Jan 6, 2021

cloud-fan reviewed Jan 6, 2021

View reviewed changes

ulysses-you added 3 commits January 7, 2021 20:25

use PrettyAttribute

dc367bd

fix ExplainSuite

fd097a7

fix session.py

3823289

cloud-fan reviewed Jan 12, 2021

View reviewed changes

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/analysis/ResolveAliasesSuite.scala Outdated Show resolved Hide resolved

cloud-fan reviewed Jan 12, 2021

View reviewed changes

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/analysis/ResolveAliasesSuite.scala Outdated Show resolved Hide resolved

HyukjinKwon mentioned this pull request Jan 13, 2021

[SPARK-34053][INFRA][FOLLOW-UP] Disables canceling push/schedule workflows #31153

Closed

parameter name

3c23778

viirya reviewed Jan 13, 2021

View reviewed changes

migration docs

3217d5b

github-actions bot added the DOCS label Jan 13, 2021

cloud-fan reviewed Jan 13, 2021

View reviewed changes

docs/sql-migration-guide.md Outdated Show resolved Hide resolved

docs

2d13353

cloud-fan closed this in 92e5cfd Jan 14, 2021

ulysses-you deleted the SPARK-33989 branch January 15, 2021 02:33

[SPARK-33989][SQL] Strip auto-generated cast when using Cast.sql #31034

[SPARK-33989][SQL] Strip auto-generated cast when using Cast.sql #31034

Uh oh!

Conversation

ulysses-you commented Jan 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Uh oh!

ulysses-you commented Jan 5, 2021

Uh oh!

SparkQA commented Jan 5, 2021

Uh oh!

SparkQA commented Jan 5, 2021

Uh oh!

SparkQA commented Jan 5, 2021

Uh oh!

maropu commented Jan 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SparkQA commented Jan 6, 2021

Uh oh!

SparkQA commented Jan 6, 2021

Uh oh!

SparkQA commented Jan 6, 2021

Uh oh!

cloud-fan Jan 6, 2021

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Jan 7, 2021

Uh oh!

SparkQA commented Jan 7, 2021

Uh oh!

SparkQA commented Jan 7, 2021

Uh oh!

SparkQA commented Jan 11, 2021

Uh oh!

SparkQA commented Jan 11, 2021

Uh oh!

SparkQA commented Jan 11, 2021

Uh oh!

SparkQA commented Jan 11, 2021

Uh oh!

Uh oh!

Uh oh!

cloud-fan commented Jan 12, 2021

Uh oh!

HyukjinKwon commented Jan 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SparkQA commented Jan 13, 2021

Uh oh!

SparkQA commented Jan 13, 2021

Uh oh!

SparkQA commented Jan 13, 2021

Uh oh!

ulysses-you commented Jan 13, 2021

Uh oh!

viirya Jan 13, 2021

Choose a reason for hiding this comment

Uh oh!

viirya Jan 13, 2021

Choose a reason for hiding this comment

Uh oh!

ulysses-you Jan 13, 2021

Choose a reason for hiding this comment

Uh oh!

viirya commented Jan 13, 2021

Uh oh!

cloud-fan commented Jan 13, 2021

Uh oh!

Uh oh!

SparkQA commented Jan 13, 2021

Uh oh!

SparkQA commented Jan 13, 2021

Uh oh!

SparkQA commented Jan 13, 2021

Uh oh!

ulysses-you commented Jan 5, 2021 •

edited

Loading

maropu commented Jan 5, 2021 •

edited

Loading

HyukjinKwon commented Jan 13, 2021 •

edited

Loading