[SPARK-31146][SQL] Leverage the helper method for aliasing in built-in SQL expressions #27901

HyukjinKwon · 2020-03-13T09:07:14Z

What changes were proposed in this pull request?

This PR is kind of a followup of #26808. It leverages the helper method for aliasing in built-in SQL expressions to use the alias as its output column name where it's applicable.

Expression, UnaryMathExpression and BinaryMathExpression search the alias in the tags by default.
When the naming is different in its implementation, it has to be overwritten for the expression specifically. E.g., CallMethodViaReflection, Remainder, CurrentTimestamp,
FormatString and XPathDouble.

This PR fixes the aliases of the functions below:

class	alias
`Rand`	`random`
`Ceil`	`ceiling`
`Remainder`	`mod`
`Pow`	`pow`
`Signum`	`sign`
`Chr`	`char`
`Length`	`char_length`
`Length`	`character_length`
`FormatString`	`printf`
`Substring`	`substr`
`Upper`	`ucase`
`XPathDouble`	`xpath_number`
`DayOfMonth`	`day`
`CurrentTimestamp`	`now`
`Size`	`cardinality`
`Sha1`	`sha`
`CallMethodViaReflection`	`java_method`

Note: EqualTo, = and == aliases were excluded because it's unable to leverage this helper method. It should fix the parser.

Note: this PR also excludes some instances such as ToDegrees, ToRadians, UnaryMinus and UnaryPositive that needs an explicit name overwritten to make the scope of this PR smaller.

Why are the changes needed?

To respect expression name.

Does this PR introduce any user-facing change?

Yes, it will change the output column name.

How was this patch tested?

Manually tested, and unittests were added.

dongjoon-hyun · 2020-03-13T19:22:24Z

Retest this please.

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/FunctionRegistry.scala

dongjoon-hyun · 2020-03-13T19:34:52Z

sql/core/src/test/resources/sql-tests/results/postgreSQL/numeric.sql.out

 select ln(1.2345678e-28)
 -- !query schema
-struct<LOG(1.2345678E-28):double>
+struct<log(1.2345678E-28):double>


Can we have ln instead of log?

I actually realised that there are some more instances such as ToDegrees, ToRadians, UnaryMinus and UnaryPositive.. let me exclude these to make the scope my PR addresses smaller

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

dongjoon-hyun · 2020-03-14T00:10:36Z

Oh, R failure is relevant one.

test_sparkSQL.R:1785: failure: column binary mathfunctions
collect(select(df, atan2(df$a, df$b)))[1, "ATAN2(a, b)"] not equal to atan2(1, 5).
target is NULL, current is numeric

maropu · 2020-03-14T07:26:19Z

Looks nice. For easy trackability in commit logs, could you add a list of the functions (that this PR adds alias flags for) in the PR description?

sql/core/src/test/resources/sql-tests/results/postgreSQL/float8.sql.out

HyukjinKwon · 2020-03-15T05:19:21Z

cc @cloud-fan too - I realised that you touched lots of codes related to this during commit history search.

HyukjinKwon · 2020-03-15T07:16:15Z

retest this please

SparkQA · 2020-03-15T11:23:08Z

Test build #119813 has finished for PR 27901 at commit b3f40a1.

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

...alyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/CallMethodViaReflection.scala

viirya

If users code refers the output column name, will such change break user code? Should we update migration guide too?

HyukjinKwon · 2020-03-16T01:47:58Z

I don't think we have made guarantee on the output column name. Also, I would think this is as a rather bug fix. We have already made such changes a lot in the history.

...alyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/CallMethodViaReflection.scala

dongjoon-hyun

+1, LGTM. Thank you, @HyukjinKwon and all.
Merged to master/3.0.

…n SQL expressions ### What changes were proposed in this pull request? This PR is kind of a followup of #26808. It leverages the helper method for aliasing in built-in SQL expressions to use the alias as its output column name where it's applicable. - `Expression`, `UnaryMathExpression` and `BinaryMathExpression` search the alias in the tags by default. - When the naming is different in its implementation, it has to be overwritten for the expression specifically. E.g., `CallMethodViaReflection`, `Remainder`, `CurrentTimestamp`, `FormatString` and `XPathDouble`. This PR fixes the aliases of the functions below: | class | alias | |--------------------------|------------------| |`Rand` |`random` | |`Ceil` |`ceiling` | |`Remainder` |`mod` | |`Pow` |`pow` | |`Signum` |`sign` | |`Chr` |`char` | |`Length` |`char_length` | |`Length` |`character_length`| |`FormatString` |`printf` | |`Substring` |`substr` | |`Upper` |`ucase` | |`XPathDouble` |`xpath_number` | |`DayOfMonth` |`day` | |`CurrentTimestamp` |`now` | |`Size` |`cardinality` | |`Sha1` |`sha` | |`CallMethodViaReflection` |`java_method` | Note: `EqualTo`, `=` and `==` aliases were excluded because it's unable to leverage this helper method. It should fix the parser. Note: this PR also excludes some instances such as `ToDegrees`, `ToRadians`, `UnaryMinus` and `UnaryPositive` that needs an explicit name overwritten to make the scope of this PR smaller. ### Why are the changes needed? To respect expression name. ### Does this PR introduce any user-facing change? Yes, it will change the output column name. ### How was this patch tested? Manually tested, and unittests were added. Closes #27901 from HyukjinKwon/31146. Authored-by: HyukjinKwon <[email protected]> Signed-off-by: Dongjoon Hyun <[email protected]> (cherry picked from commit 6704103) Signed-off-by: Dongjoon Hyun <[email protected]>

HyukjinKwon · 2020-03-17T01:13:25Z

Thank you guys!

gatorsmile · 2020-03-31T21:05:58Z

We need to update the migration guide. This impacts the output schema of SQL queries.

HyukjinKwon · 2020-04-01T01:16:46Z

~~Sure~~. I am sorry, I rushed to read. I think we haven't documented such output column names in the migration so far, e.g., #26808. Let me leave it out for now.

HyukjinKwon · 2020-04-01T03:23:46Z

And I think we don't also guarantee on output columns names (#27901 (comment)).

…n SQL expressions ### What changes were proposed in this pull request? This PR is kind of a followup of apache#26808. It leverages the helper method for aliasing in built-in SQL expressions to use the alias as its output column name where it's applicable. - `Expression`, `UnaryMathExpression` and `BinaryMathExpression` search the alias in the tags by default. - When the naming is different in its implementation, it has to be overwritten for the expression specifically. E.g., `CallMethodViaReflection`, `Remainder`, `CurrentTimestamp`, `FormatString` and `XPathDouble`. This PR fixes the aliases of the functions below: | class | alias | |--------------------------|------------------| |`Rand` |`random` | |`Ceil` |`ceiling` | |`Remainder` |`mod` | |`Pow` |`pow` | |`Signum` |`sign` | |`Chr` |`char` | |`Length` |`char_length` | |`Length` |`character_length`| |`FormatString` |`printf` | |`Substring` |`substr` | |`Upper` |`ucase` | |`XPathDouble` |`xpath_number` | |`DayOfMonth` |`day` | |`CurrentTimestamp` |`now` | |`Size` |`cardinality` | |`Sha1` |`sha` | |`CallMethodViaReflection` |`java_method` | Note: `EqualTo`, `=` and `==` aliases were excluded because it's unable to leverage this helper method. It should fix the parser. Note: this PR also excludes some instances such as `ToDegrees`, `ToRadians`, `UnaryMinus` and `UnaryPositive` that needs an explicit name overwritten to make the scope of this PR smaller. ### Why are the changes needed? To respect expression name. ### Does this PR introduce any user-facing change? Yes, it will change the output column name. ### How was this patch tested? Manually tested, and unittests were added. Closes apache#27901 from HyukjinKwon/31146. Authored-by: HyukjinKwon <[email protected]> Signed-off-by: Dongjoon Hyun <[email protected]>

This comment has been minimized.

Sign in to view

HyukjinKwon changed the title ~~[SPARK-31146][SQL] Leverage the helper method for aliasing in all SQL expressions~~ [SPARK-31146][SQL] Leverage the helper method for aliasing in built-in SQL expressions Mar 13, 2020

Leverage the helper method for aliasing in all SQL expressions

0809771

HyukjinKwon force-pushed the 31146 branch from 536b747 to 0809771 Compare March 13, 2020 12:06

This comment has been minimized.

Sign in to view

dongjoon-hyun added the SQL label Mar 13, 2020

dongjoon-hyun reviewed Mar 13, 2020

View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/FunctionRegistry.scala Show resolved Hide resolved

dongjoon-hyun reviewed Mar 13, 2020

View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/predicates.scala Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

gatorsmile reviewed Mar 15, 2020

View reviewed changes

sql/core/src/test/resources/sql-tests/results/postgreSQL/float8.sql.out Outdated Show resolved Hide resolved

HyukjinKwon added 2 commits March 15, 2020 14:01

Address comments

d863adc

Remove unused imports

b3f40a1

This comment has been minimized.

Sign in to view

viirya reviewed Mar 15, 2020

View reviewed changes

...alyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/CallMethodViaReflection.scala Show resolved Hide resolved

viirya reviewed Mar 15, 2020

View reviewed changes

cloud-fan reviewed Mar 16, 2020

View reviewed changes

...alyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/CallMethodViaReflection.scala Show resolved Hide resolved

cloud-fan approved these changes Mar 16, 2020

View reviewed changes

dongjoon-hyun approved these changes Mar 16, 2020

View reviewed changes

dongjoon-hyun closed this in 6704103 Mar 16, 2020

HyukjinKwon deleted the 31146 branch July 27, 2020 07:45

[SPARK-31146][SQL] Leverage the helper method for aliasing in built-in SQL expressions #27901

[SPARK-31146][SQL] Leverage the helper method for aliasing in built-in SQL expressions #27901

Uh oh!

Conversation

HyukjinKwon commented Mar 13, 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!

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

dongjoon-hyun commented Mar 13, 2020

Uh oh!

Uh oh!

dongjoon-hyun Mar 13, 2020

Choose a reason for hiding this comment

Uh oh!

HyukjinKwon Mar 15, 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!

Uh oh!

This comment has been minimized.

dongjoon-hyun commented Mar 14, 2020

Uh oh!

maropu commented Mar 14, 2020

Uh oh!

Uh oh!

HyukjinKwon commented Mar 15, 2020

Uh oh!

This comment has been minimized.

HyukjinKwon commented Mar 15, 2020

Uh oh!

SparkQA commented Mar 15, 2020

Uh oh!

Uh oh!

viirya left a comment

Choose a reason for hiding this comment

Uh oh!

HyukjinKwon commented Mar 16, 2020

Uh oh!

Uh oh!

dongjoon-hyun left a comment

Choose a reason for hiding this comment

Uh oh!

HyukjinKwon commented Mar 17, 2020

Uh oh!

gatorsmile commented Mar 31, 2020

Uh oh!

HyukjinKwon commented Apr 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

HyukjinKwon commented Apr 1, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

HyukjinKwon commented Mar 13, 2020 •

edited

Loading

HyukjinKwon Mar 15, 2020 •

edited

Loading

HyukjinKwon commented Apr 1, 2020 •

edited

Loading