[SPARK-30940][SQL] Remove attributeId in auto-generated arguments when Explain SQL query by Eric5553 · Pull Request #27685 · apache/spark

Eric5553 · 2020-02-24T16:00:22Z

What changes were proposed in this pull request?

When EXPLAIN sql query, the auto-generated argument alias shouldn't include expr/attribute id. This will provide better readability of Explain results. This is a follow-up to address #27368 (comment).

Before

(7) HashAggregate [codegen id : 2]
Input [2]: [key#x, max#x]
Keys [1]: [key#x]
Functions [1]: [max(val#x)]
Aggregate Attributes [1]: [max(val#x)#x]
Results [2]: [key#x, max(val#x)#x AS max(val)#x]

After

(7) HashAggregate [codegen id : 2]
Input [2]: [key#x, max#x]
Keys [1]: [key#x]
Functions [1]: [max(val)]
Aggregate Attributes [1]: [max(val)#x]
Results [2]: [key#x, max(val)#x AS max(val)#x]

Why are the changes needed?

Provide better readability for Explain result

Does this PR introduce any user-facing change?

Update Explain result to a better format

How was this patch tested?

Update existing tests

SparkQA · 2020-02-24T20:12:36Z

Test build #118876 has finished for PR 27685 at commit 3cf84d2.

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

SparkQA · 2020-02-24T21:35:02Z

Test build #118880 has finished for PR 27685 at commit 3cf84d2.

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

SparkQA · 2020-02-28T07:19:12Z

Test build #119063 has finished for PR 27685 at commit 8238b2a.

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

Eric5553 · 2020-02-28T10:08:21Z

@cloud-fan Would you please help review this? Thanks so much!

cloud-fan · 2020-02-28T13:03:42Z

It's still useful to have the attr id as the name can be duplicated
scala> sql("select 1 as a, 2 as a").explain == Physical Plan == *(1) Project [1 AS a#53, 2 AS a#54] +- Scan OneRowRelation[]
I think we should only remove the attr id from the auto-generated alias name. e.g. this should be [max(val)#x]

I see. Thanks for the review :-)
The common modification in AttributeReference will also remove the # in [max(val)#x]. I'm trying to pin point to the exact creation place of auto-generated AttributeReference name.

@cloud-fan I've re-implemented to only remove the auto-generated Attrids I think. Would you please help review again? Thanks so much!

SparkQA · 2020-03-02T21:30:27Z

Test build #119179 has finished for PR 27685 at commit 85f2f01.

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

SparkQA · 2020-03-02T21:43:56Z

Test build #119178 has finished for PR 27685 at commit 85f2f01.

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

SparkQA · 2020-03-03T04:33:09Z

Test build #119198 has finished for PR 27685 at commit e3b105f.

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

SparkQA · 2020-03-03T08:05:02Z

Test build #119203 has finished for PR 27685 at commit e6df5d9.

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

Eric5553 · 2020-03-03T09:51:20Z

retest this please

SparkQA · 2020-03-03T14:58:56Z

Test build #119220 has finished for PR 27685 at commit e6df5d9.

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

SparkQA · 2020-03-09T06:50:39Z

Test build #119547 has finished for PR 27685 at commit 659e7c2.

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

Eric5553 · 2020-03-09T06:56:34Z

cc @cloud-fan @gatorsmile @maropu @maryannxue , thanks!

maropu · 2020-03-09T07:52:10Z

 -- !query output
 org.apache.spark.sql.AnalysisException
-grouping expressions sequence is empty, and 'spark_catalog.default.test_having.`a`' is not an aggregate function. Wrap '(min(spark_catalog.default.test_having.`a`) AS `min(a#x)`, max(spark_catalog.default.test_having.`a`) AS `max(a#x)`)' in windowing function(s) or wrap 'spark_catalog.default.test_having.`a`' in first() (or first_value) if you don't care which value you get.;
+grouping expressions sequence is empty, and 'spark_catalog.default.test_having.`a`' is not an aggregate function. Wrap '(min(spark_catalog.default.test_having.`a`) AS `min(a)`, max(spark_catalog.default.test_having.`a`) AS `max(a)`)' in windowing function(s) or wrap 'spark_catalog.default.test_having.`a`' in first() (or first_value) if you don't care which value you get.;


We need to hide ids even in error messages, too?

I think the flatArgument/flatArgumentsString is tightly bind with toString of Expression/AggregateExpression, which will commonly affect all of them. I'll try more to eliminate the impact, thanks!

The error message explicitly called map(_.sql) instead of default toString. The Alias.sql is using name field which has already been formatted by flatArguments when constructing Alias. So this is also following the flatArgument framework.
As the error message is intended to suggest user with a sql snippet, maybe it's better to not includ #exprId anyway? Thanks.

cloud-fan · 2020-03-09T13:11:10Z

- flatArguments.toSeq, "(", ", ", ")", SQLConf.get.maxToStringFields)
+ flatArgumentStrings.toSeq, "(", ", ", ")", SQLConf.get.maxToStringFields)
+
+ def argumentString: String = toString


is it possible to only add one method? I'm worried about adding to many methods to the framework.

IMO, the argumentString is needed because AttributeReference already overwrite toString, thus we need the new abstract string function to switch to non-exprid format. For flatArgumentStrings, it only have two callers. I refactored the toAggString of AggregateFunction, then we don't need to add the method flatArgumentStrings in Expression but just implement it within toString. See commit b74c500.

SparkQA · 2020-03-09T18:00:31Z

Test build #119570 has finished for PR 27685 at commit 51bb46a.

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

SparkQA · 2020-03-09T19:19:44Z

Test build #119573 has finished for PR 27685 at commit 51bb46a.

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

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

Test build #119597 has finished for PR 27685 at commit b74c500.

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

SparkQA · 2020-03-10T12:05:56Z

Test build #119607 has finished for PR 27685 at commit cee32e4.

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

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

Test build #119916 has finished for PR 27685 at commit 1cbfe49.

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

SparkQA · 2020-03-17T14:59:07Z

Test build #119928 has finished for PR 27685 at commit 10da2c5.

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

Eric5553 · 2020-03-17T15:03:53Z

@cloud-fan Would you please help review the latest change? Thanks so much :-)

Eric5553 · 2020-03-25T03:23:39Z

@cloud-fan @gatorsmile @maryannxue Would you please help review this PR? Thanks so much :-)

maropu · 2020-04-09T08:20:58Z

@Eric5553 Could you resolve the conflict?
@cloud-fan Could you do a final check?

Eric5553 · 2020-04-09T15:08:09Z

Updated. Thanks so much for helping maintain the PR ! @maropu

SparkQA · 2020-04-09T20:04:47Z

Test build #121037 has finished for PR 27685 at commit c7daaa6.

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

github-actions · 2020-09-12T00:43:11Z

We're closing this PR because it hasn't been updated in a while. This isn't a judgement on the merit of the PR in any way. It's just a way of keeping the PR queue manageable.
If you'd like to revive this PR, please reopen it and ask a committer to remove the Stale tag!

Eric5553 force-pushed the RefineExprId branch from 3cf84d2 to 8238b2aCompare February 28, 2020 01:55

dongjoon-hyun added the SQL label Feb 28, 2020

cloud-fan reviewed Feb 28, 2020
View reviewed changes

Eric5553 force-pushed the RefineExprId branch from 8238b2a to 85f2f01Compare March 2, 2020 16:31

Eric5553 changed the title ~~[SPARK-30940][SQL] Remove meaningless attributeId when Explain SQL query~~[SPARK-30940][SQL] Remove attributeId in auto-generated arguments when Explain SQL queryMar 2, 2020

Eric5553 force-pushed the RefineExprId branch from e6df5d9 to 659e7c2Compare March 9, 2020 02:34

maropu reviewed Mar 9, 2020
View reviewed changes

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

maropu reviewed Mar 9, 2020
View reviewed changes

cloud-fan reviewed Mar 9, 2020
View reviewed changes

Eric5553 force-pushed the RefineExprId branch from 51bb46a to b74c500Compare March 10, 2020 02:47

Eric5553 force-pushed the RefineExprId branch from b74c500 to cee32e4Compare March 10, 2020 07:25

maropu approved these changes Mar 10, 2020
View reviewed changes

Eric5553 force-pushed the RefineExprId branch from cee32e4 to 1cbfe49Compare March 17, 2020 06:35

Eric5553 force-pushed the RefineExprId branch from 1cbfe49 to 10da2c5Compare March 17, 2020 10:00

Eric5553 added 4 commits April 9, 2020 23:04

Only remote auto-generated attrId
19ddd4f

refine code to provide better atomicity
62065ac

fix pattern match null
ea136d1

remove one function in Expression
c7daaa6

Eric5553 force-pushed the RefineExprId branch from 10da2c5 to c7daaa6Compare April 9, 2020 15:06

github-actionsBot added the Stale label Sep 12, 2020

github-actionsBot closed this Sep 13, 2020

Uh oh!

Conversation

Eric5553 commented Feb 24, 2020• 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!

SparkQA commented Feb 24, 2020

Uh oh!

SparkQA commented Feb 24, 2020

Uh oh!

SparkQA commented Feb 28, 2020

Uh oh!

Eric5553 commented Feb 28, 2020

Uh oh!

cloud-fanFeb 28, 2020

Choose a reason for hiding this comment

Uh oh!

Eric5553Feb 29, 2020• 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!

Eric5553Mar 3, 2020

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Mar 2, 2020

Uh oh!

SparkQA commented Mar 2, 2020

Uh oh!

SparkQA commented Mar 3, 2020

Uh oh!

SparkQA commented Mar 3, 2020

Uh oh!

Eric5553 commented Mar 3, 2020

Uh oh!

SparkQA commented Mar 3, 2020

Uh oh!

SparkQA commented Mar 9, 2020

Uh oh!

Eric5553 commented Mar 9, 2020

Uh oh!

Uh oh!

maropuMar 9, 2020

Choose a reason for hiding this comment

Uh oh!

Eric5553Mar 9, 2020

Choose a reason for hiding this comment

Uh oh!

Eric5553Mar 10, 2020

Choose a reason for hiding this comment

Uh oh!

cloud-fanMar 9, 2020• 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!

Eric5553Mar 9, 2020• 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!

SparkQA commented Mar 9, 2020

Uh oh!

SparkQA commented Mar 9, 2020

Uh oh!

SparkQA commented Mar 10, 2020

Uh oh!

SparkQA commented Mar 10, 2020

Uh oh!

SparkQA commented Mar 17, 2020

Uh oh!

SparkQA commented Mar 17, 2020

Uh oh!

Eric5553 commented Mar 17, 2020

Uh oh!

Eric5553 commented Mar 25, 2020

Uh oh!

maropu commented Apr 9, 2020

Uh oh!

Eric5553 commented Apr 9, 2020

Eric5553 commented Feb 24, 2020•
edited
Loading

Eric5553Feb 29, 2020•
edited
Loading

cloud-fanMar 9, 2020•
edited
Loading

Eric5553Mar 9, 2020•
edited
Loading