[SPARK-30184][SQL] Implement a helper method for aliasing functions #26808

amanomer · 2019-12-09T09:46:24Z

What changes were proposed in this pull request?

This PR is to use expressionWithAlias for remaining functions for which alias name can be used. Remaining functions are:
Average, First, Last, ApproximatePercentile, StddevSamp, VarianceSamp

PR #26712 introduced expressionWithAlias

Why are the changes needed?

Error message is wrong when alias name is used for above mentioned functions.

Does this PR introduce any user-facing change?

No

How was this patch tested?

Manually

amanomer · 2019-12-09T14:33:06Z

cc @cloud-fan @srowen @maropu @gengliangwang

dongjoon-hyun · 2019-12-09T22:26:36Z

ok to test

SparkQA · 2019-12-09T23:27:09Z

Test build #115054 has finished for PR 26808 at commit 7d5f4be.

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

maropu · 2019-12-10T00:13:42Z

...alyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/RewriteDistinctAggregates.scala

        val result = AggregateExpression(
-          aggregate.First(evalWithinGroup(regularGroupId, operator.toAttribute), Literal(true)),
+          aggregate.First("first",
+            evalWithinGroup(regularGroupId, operator.toAttribute), Literal(true)),


a bit verbose to always set "first" for First, so how about defining auxiliary constructor?

def this(child: Expression, ignoreNullsExpr: Expression) = this("first", child, Literal.create(false, BooleanType))

maropu

If the tests pass, LGTM except for one minor comment

maropu · 2019-12-10T00:15:49Z

btw, why did you include fixes for Average and ApproximatePercentile in this pr?

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

cloud-fan · 2019-12-10T02:28:13Z

sql/core/src/main/scala/org/apache/spark/sql/functions.scala

   */
  def first(e: Column, ignoreNulls: Boolean): Column = withAggregateFunction {
-    new First(e.expr, Literal(ignoreNulls))
+    new First("first", e.expr, Literal(ignoreNulls))


We can add a constructor to provide a default name.

amanomer · 2019-12-10T04:47:42Z

btw, why did you include fixes for Average and ApproximatePercentile in this pr?

Average and ApproximatePercentile can be called with alias name as well.

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

Lines 303 to 304 in be867e8

    
           expression[ApproximatePercentile]("percentile_approx"), 
        
           expression[ApproximatePercentile]("approx_percentile"),

SparkQA · 2019-12-10T05:34:56Z

Test build #115081 has finished for PR 26808 at commit 759262d.

This patch fails to build.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2019-12-10T06:23:31Z

Test build #115082 has finished for PR 26808 at commit 5ec101f.

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

cloud-fan · 2019-12-10T16:19:15Z

...alyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/RewriteDistinctAggregates.scala

        // Select the result of the first aggregate in the last aggregate.
        val result = AggregateExpression(
-          aggregate.First(evalWithinGroup(regularGroupId, operator.toAttribute), Literal(true)),
+          new aggregate.First(evalWithinGroup(regularGroupId, operator.toAttribute), Literal(true)),


so adding more constructors doesn't help us to reduce diff, how about

case class First(funcName: String, child: Expression, ignoreNullsExpr: Expression) { def this(funcName: String, child: Expression) } object First { def apply(child: Expression, ignoreNullsExpr: Expression) ... def apply(child: Expression) ... }

SparkQA · 2019-12-10T18:03:26Z

Test build #115106 has finished for PR 26808 at commit f210bb9.

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

SparkQA · 2019-12-10T18:51:42Z

Test build #115109 has finished for PR 26808 at commit cc234b9.

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

maropu · 2019-12-11T05:42:11Z

sql/core/src/test/scala/org/apache/spark/sql/SQLQueryTestSuite.scala

    //   1. Maven can't get correct resource directory when resources in other jars.
    //   2. We test subclasses in the hive-thriftserver module.
    val sparkHome = {
+      sys.props.put("spark.test.home", "/home/root1/spark")


opps...I'll revert this

maropu

LGTM except for one comment.

SparkQA · 2019-12-11T05:51:42Z

Test build #115142 has finished for PR 26808 at commit 92e381b.

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

SparkQA · 2019-12-11T06:09:59Z

Test build #115141 has finished for PR 26808 at commit 7617ec3.

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

cloud-fan · 2019-12-11T07:09:29Z

...alyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/aggregate/LastTestSuite.scala

  val evaluator = DeclarativeAggregateEvaluator(Last(input, Literal(false)), Seq(input))
-  val evaluatorIgnoreNulls = DeclarativeAggregateEvaluator(Last(input, Literal(true)), Seq(input))
+  val evaluatorIgnoreNulls = DeclarativeAggregateEvaluator(
+    Last(input, Literal(true)), Seq(input))


nit: unnecessary change

SparkQA · 2019-12-18T19:11:50Z

Test build #115524 has finished for PR 26808 at commit 43ce929.

This patch fails to build.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2019-12-18T19:46:50Z

Test build #115526 has finished for PR 26808 at commit e25271e.

This patch fails to build.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2019-12-18T23:52:10Z

Test build #115528 has finished for PR 26808 at commit 5d057c8.

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

cloud-fan · 2019-12-19T08:43:47Z

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


  /** See usage above. */
-  private def expression[T <: Expression](name: String)
+  private def expression[T <: Expression](name: String, isAliasName: Boolean = false)


nit: setAlias: Boolean = false

cloud-fan · 2019-12-19T08:45:11Z

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

    throw new UnsupportedOperationException(s"$nodeName does not implement simpleStringWithNodeId")
  }
+
+  val FUNC_ALIAS = TreeNodeTag[String]("functionAliasName")


We can define it in object FunctionRegistry. This is kind of a map key, and we don't need to create a different instance for each expression.

cloud-fan · 2019-12-19T08:46:32Z

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

    expression[HyperLogLogPlusPlus]("approx_count_distinct"),
-    expression[Average]("avg"),
+    expressionWithAlias[Average]("avg"),
+    expressionWithAlias[Average]("mean"),


Seems we don't need expressionWithAlias at all, just call expression[Average]("mean", setAlias = true).

And expression[Average]("avg") can remain unchanged, as avg is already the default name.

And we don't need to reorder them now.

cloud-fan · 2019-12-19T08:48:18Z

...c/main/scala/org/apache/spark/sql/catalyst/expressions/aggregate/ApproximatePercentile.scala

  }

-  override def prettyName: String = "percentile_approx"
+  override def nodeName: String = getTagValue(FUNC_ALIAS).getOrElse("percentile_approx")


let's override prettyName as before.

cloud-fan · 2019-12-19T08:49:05Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/aggregate/First.scala

  override lazy val evaluateExpression: AttributeReference = first

-  override def toString: String = s"first($child)${if (ignoreNulls) " ignore nulls"}"
+  override def nodeName: String = getTagValue(FUNC_ALIAS).getOrElse("first")


ditto, override prettyName seems better.

cloud-fan · 2019-12-19T08:49:52Z

in general looks good, thanks for updating!

amanomer · 2019-12-19T09:23:22Z

cc @cloud-fan

SparkQA · 2019-12-19T14:01:44Z

Test build #115559 has finished for PR 26808 at commit 700a84d.

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

amanomer · 2019-12-19T14:13:46Z

Tests have passed. cc @cloud-fan @maropu

cloud-fan · 2019-12-20T04:49:39Z

thanks, merging to master!

…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]>

…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]>

…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]>

amanomer added 2 commits December 9, 2019 15:03

expressionWithAlias for First, Last, StddevSamp, VarianceSamp

29fdaba

Fixed errors

7d5f4be

dongjoon-hyun added the SQL label Dec 9, 2019

maropu reviewed Dec 10, 2019

View reviewed changes

maropu approved these changes Dec 10, 2019

View reviewed changes

cloud-fan reviewed Dec 10, 2019

View reviewed changes

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

cloud-fan reviewed Dec 10, 2019

View reviewed changes

amanomer added 2 commits December 10, 2019 10:42

+constructor First

7ba7802

+constructor Last

759262d

Fixed build error

5ec101f

amanomer added 2 commits December 10, 2019 21:15

Fixed TC

f210bb9

ScalaStyle Fix

cc234b9

cloud-fan reviewed Dec 10, 2019

View reviewed changes

amanomer added 2 commits December 11, 2019 08:58

Fixed TC

7617ec3

Reduce constructors

92e381b

maropu reviewed Dec 11, 2019

View reviewed changes

maropu approved these changes Dec 11, 2019

View reviewed changes

nit

a2d75de

cloud-fan reviewed Dec 11, 2019

View reviewed changes

amanomer force-pushed the fncAlias branch from 5485fcd to 8e9e42b Compare December 18, 2019 19:01

small fix

36418e2

amanomer force-pushed the fncAlias branch from 43ce929 to 36418e2 Compare December 18, 2019 19:33

amanomer force-pushed the fncAlias branch from e25271e to 36418e2 Compare December 18, 2019 19:51

cloud-fan reviewed Dec 19, 2019

View reviewed changes

amanomer added 4 commits December 19, 2019 14:33

move FUNC_ALIAS to FunctionRegistry

ce8ea17

Remove expressionWithAlias

4b536dd

revert reorder

737f33a

override prettyName instead of nodeName

1920940

amanomer force-pushed the fncAlias branch from 5d057c8 to 1920940 Compare December 19, 2019 09:17

Merge branch 'master' into fncAlias

700a84d

cloud-fan approved these changes Dec 19, 2019

View reviewed changes

cloud-fan closed this in 726f6d3 Dec 20, 2019

HyukjinKwon mentioned this pull request Mar 13, 2020

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

Closed

[SPARK-30184][SQL] Implement a helper method for aliasing functions #26808

[SPARK-30184][SQL] Implement a helper method for aliasing functions #26808

Uh oh!

Conversation

amanomer commented Dec 9, 2019 • 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!

amanomer commented Dec 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dongjoon-hyun commented Dec 9, 2019

Uh oh!

SparkQA commented Dec 9, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maropu left a comment • 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!

maropu commented Dec 10, 2019

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amanomer commented Dec 10, 2019

Uh oh!

SparkQA commented Dec 10, 2019

Uh oh!

SparkQA commented Dec 10, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Dec 10, 2019

Uh oh!

SparkQA commented Dec 10, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maropu left a comment

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Dec 11, 2019

Uh oh!

SparkQA commented Dec 11, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Dec 18, 2019

Uh oh!

SparkQA commented Dec 18, 2019

Uh oh!

SparkQA commented Dec 18, 2019

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!

cloud-fan commented Dec 19, 2019

Uh oh!

amanomer commented Dec 9, 2019 •

edited

Loading

amanomer commented Dec 9, 2019 •

edited

Loading

maropu left a comment •

edited

Loading

amanomer commented Dec 19, 2019 •

edited

Loading