[SPARK-31393][SQL] Show the correct alias in schema for expression #28164

beliefer · 2020-04-09T07:14:57Z

What changes were proposed in this pull request?

Some alias of expression can not display correctly in schema. This PR will fix them.

TimeWindow
MaxBy
MinBy
UnaryMinus
BitwiseCount

This PR also fix a typo issue, please look at

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

Line 142 in b7cde42

s"\n" +

Note:

MaxBy and MinBy extends MaxMinBy and the latter add a method funcName not needed. We can reuse prettyName to replace funcName.
Spark SQL exists some function no elegant implementation.For example: BitwiseCount override the sql method show below:
override def sql: String = s"bit_count(${child.sql})"
I don't think it's elegant enough. Because Expression gives the following definitions.

  def sql: String = {
    val childrenSQL = children.map(_.sql).mkString(", ")
    s"$prettyName($childrenSQL)"
  }

By this definition, BitwiseCount should override prettyName method.

Why are the changes needed?

Improve the implement of some expression.

Does this PR introduce any user-facing change?

'Yes'. This PR will let user see the correct alias in schema.

How was this patch tested?

Jenkins test.

SparkQA · 2020-04-09T07:46:30Z

Test build #121013 has finished for PR 28164 at commit 08f0906.

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

SparkQA · 2020-04-09T11:52:29Z

Test build #121019 has finished for PR 28164 at commit 7b7f8bf.

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

SparkQA · 2020-04-09T19:12:07Z

Test build #121036 has finished for PR 28164 at commit 67c868b.

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

beliefer · 2020-04-10T01:23:07Z

cc @HyukjinKwon

beliefer · 2020-04-10T13:33:38Z

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


  override def toString: String = s"-$child"

+  override def prettyName: String = "-"


@HyukjinKwon Should we change "-" to "negative" ?

I think it should be negative but should show - when it's used as the sql operator. You should be able to do this with if-else. e.g.)

getTagValue(FunctionRegistry.FUNC_ALIAS).getOrElse("-")

You might need to override sql IIRC.

SparkQA · 2020-04-12T08:10:14Z

Test build #121142 has finished for PR 28164 at commit 43beb20.

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

SparkQA · 2020-04-12T13:04:55Z

Test build #121144 has finished for PR 28164 at commit 67c868b.

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

HyukjinKwon · 2020-04-14T02:32:01Z

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

+
+  override def prettyName: String = "max_by"
+
  override protected def funcName: String = "max_by"


Seems like you can remove this and replace it to prettyName.

Yes. I will replace it.
But wait for #28194 merged.
Then I could update golden file.

SparkQA · 2020-04-30T16:55:26Z

Test build #122129 has finished for PR 28164 at commit d549c3c.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
class AuthRpcHandler extends AbstractAuthRpcHandler
public class SaslRpcHandler extends AbstractAuthRpcHandler
public abstract class AbstractAuthRpcHandler extends RpcHandler
class AbortableRpcFuture[T: ClassTag](val future: Future[T], onAbort: Throwable => Unit)
public class JavaUserDefinedScalar
public class SparkAvroKeyOutputFormat extends AvroKeyOutputFormat<GenericRecord>
static class SparkRecordWriterFactory extends RecordWriterFactory<GenericRecord>
class AvroDeserializer(rootAvroType: Schema, rootCatalystType: DataType, rebaseDateTime: Boolean)
class AvroSerializer(
class FMClassifierWrapperWriter(instance: FMClassifierWrapper) extends MLWriter
class FMClassifierWrapperReader extends MLReader[FMClassifierWrapper]
class FMRegressorWrapperWriter(instance: FMRegressorWrapper) extends MLWriter
class FMRegressorWrapperReader extends MLReader[FMRegressorWrapper]
class LinearRegressionWrapperWriter(instance: LinearRegressionWrapper) extends MLWriter
class LinearRegressionWrapperReader extends MLReader[LinearRegressionWrapper]
class ExecutorResourceRequest(object):
class ExecutorResourceRequests(object):
class ResourceProfile(object):
class ResourceProfileBuilder(object):
class TaskResourceRequest(object):
class TaskResourceRequests(object):
case class AggregateWithHaving(
abstract class CurrentTimestampLike() extends LeafExpression with CodegenFallback
case class CurrentTimestamp() extends CurrentTimestampLike
case class Now() extends CurrentTimestampLike
case class YearOfWeek(child: Expression) extends UnaryExpression with ImplicitCastInputTypes
case class DatetimeSub(
case class DateAddInterval(
case class Extract(field: Expression, source: Expression, child: Expression)
case class LengthOfJsonArray(child: Expression) extends UnaryExpression
case class JsonObjectKeys(child: Expression) extends UnaryExpression with CodegenFallback
case class Rand(child: Expression, hideSeed: Boolean = false)
case class Randn(child: Expression, hideSeed: Boolean = false)
case class ShowViews(
class CacheManager extends Logging with AdaptiveSparkPlanHelper
case class AdaptiveExecutionContext(session: SparkSession, qe: QueryExecution)
case class ShowViewsCommand(
class JDBCConfiguration(
class ParquetReadSupport(

SparkQA · 2020-05-01T06:42:09Z

Test build #122154 has finished for PR 28164 at commit 2cd091e.

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

SparkQA · 2020-05-01T09:45:41Z

Test build #122159 has finished for PR 28164 at commit f048601.

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

SparkQA · 2020-05-05T07:05:02Z

Test build #122300 has finished for PR 28164 at commit cc4ee4c.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds the following public classes (experimental):
public class JavaFValueTestExample

beliefer · 2020-05-05T07:11:48Z

retest this please

SparkQA · 2020-05-05T11:58:46Z

Test build #122309 has finished for PR 28164 at commit cc4ee4c.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
public class JavaFValueTestExample

beliefer · 2020-05-06T03:04:30Z

cc @HyukjinKwon

beliefer · 2020-05-11T05:26:39Z

cc @gatorsmile

HyukjinKwon · 2020-05-12T01:24:33Z

Merged to master and branch-3.0.

### What changes were proposed in this pull request? Some alias of expression can not display correctly in schema. This PR will fix them. - `TimeWindow` - `MaxBy` - `MinBy` - `UnaryMinus` - `BitwiseCount` This PR also fix a typo issue, please look at https://github.com/apache/spark/blob/b7cde42b04b21c9bfee6535199cf385855c15853/sql/core/src/test/scala/org/apache/spark/sql/ExpressionsSchemaSuite.scala#L142 Note: 1. `MaxBy` and `MinBy` extends `MaxMinBy` and the latter add a method `funcName` not needed. We can reuse `prettyName` to replace `funcName`. 2. Spark SQL exists some function no elegant implementation.For example: `BitwiseCount` override the sql method show below: `override def sql: String = s"bit_count(${child.sql})"` I don't think it's elegant enough. Because `Expression` gives the following definitions. ``` def sql: String = { val childrenSQL = children.map(_.sql).mkString(", ") s"$prettyName($childrenSQL)" } ``` By this definition, `BitwiseCount` should override `prettyName` method. ### Why are the changes needed? Improve the implement of some expression. ### Does this PR introduce any user-facing change? 'Yes'. This PR will let user see the correct alias in schema. ### How was this patch tested? Jenkins test. Closes #28164 from beliefer/elegant-pretty-name-for-function. Lead-authored-by: beliefer <[email protected]> Co-authored-by: gengjiaan <[email protected]> Signed-off-by: HyukjinKwon <[email protected]> (cherry picked from commit a89006a) Signed-off-by: HyukjinKwon <[email protected]>

beliefer · 2020-05-12T01:57:44Z

@HyukjinKwon Thanks!

Support pretty name

08f0906

Adjust UT

7b7f8bf

regenerate golden files.

67c868b

beliefer commented Apr 10, 2020

View reviewed changes

beliefer force-pushed the elegant-pretty-name-for-function branch from 43beb20 to 67c868b Compare April 12, 2020 08:47

HyukjinKwon reviewed Apr 14, 2020

View reviewed changes

Optimize code and regenerate golden file

d549c3c

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

beliefer added 2 commits May 1, 2020 09:52

Update golden file

2cd091e

Fix typo.

f048601

beliefer mentioned this pull request May 1, 2020

[SPARK-31372][SQL][TEST] Display expression schema for double check. #28194

Closed

beliefer changed the title ~~[SPARK-31393][SQL] Show the correct alias in a more elegant way that override the prettyName~~ [SPARK-31393][SQL] Show the correct alias in schema for expression May 1, 2020

Fix conflict

cc4ee4c

HyukjinKwon approved these changes May 6, 2020

View reviewed changes

HyukjinKwon closed this in a89006a May 12, 2020


		override def toString: String = s"-$child"

		override def prettyName: String = "-"


		override def prettyName: String = "max_by"

		override protected def funcName: String = "max_by"

[SPARK-31393][SQL] Show the correct alias in schema for expression #28164

[SPARK-31393][SQL] Show the correct alias in schema for expression #28164

Uh oh!

Conversation

beliefer commented Apr 9, 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 Apr 9, 2020

Uh oh!

SparkQA commented Apr 9, 2020

Uh oh!

SparkQA commented Apr 9, 2020

Uh oh!

beliefer commented Apr 10, 2020

Uh oh!

beliefer Apr 10, 2020

Choose a reason for hiding this comment

Uh oh!

HyukjinKwon Apr 14, 2020

Choose a reason for hiding this comment

Uh oh!

beliefer Apr 30, 2020

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Apr 12, 2020

Uh oh!

SparkQA commented Apr 12, 2020

Uh oh!

HyukjinKwon Apr 14, 2020

Choose a reason for hiding this comment

Uh oh!

beliefer Apr 14, 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 Apr 30, 2020

Uh oh!

SparkQA commented May 1, 2020

Uh oh!

SparkQA commented May 1, 2020

Uh oh!

SparkQA commented May 5, 2020

Uh oh!

beliefer commented May 5, 2020

Uh oh!

SparkQA commented May 5, 2020

Uh oh!

beliefer commented May 6, 2020

Uh oh!

beliefer commented May 11, 2020

Uh oh!

HyukjinKwon commented May 12, 2020

Uh oh!

beliefer commented May 12, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

beliefer commented Apr 9, 2020 •

edited

Loading

beliefer Apr 14, 2020 •

edited

Loading