[SPARK-37018][SQL] Spark SQL should support create function with Aggregator #34303

beliefer · 2021-10-17T07:47:19Z

What changes were proposed in this pull request?

Spark SQL not supports to create function of Aggregator yet and deprecated UserDefinedAggregateFunction.
If we want remove UserDefinedAggregateFunction, Spark SQL should provide a new option.

Why are the changes needed?

We need to provide a new way to create user defined aggregate function so as remove UserDefinedAggregateFunction in future.

Does this PR introduce any user-facing change?

Yes. Users will create user defined aggregate function by implement Aggregator.

How was this patch tested?

New tests.

SparkQA · 2021-10-17T09:12:08Z

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

SparkQA · 2021-10-17T09:56:05Z

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

SparkQA · 2021-10-17T10:32:42Z

Test build #144330 has finished for PR 34303 at commit 3702b9b.

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

sql/hive/src/test/scala/org/apache/spark/sql/hive/execution/HiveSQLViewSuite.scala

…veSQLViewSuite.scala Co-authored-by: Hyukjin Kwon <gurwls223@gmail.com>

beliefer · 2021-10-18T04:06:19Z

ping @cloud-fan

SparkQA · 2021-10-18T05:17:48Z

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

SparkQA · 2021-10-18T05:59:34Z

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

SparkQA · 2021-10-18T06:47:36Z

Test build #144346 has finished for PR 34303 at commit 7b11c6e.

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

cloud-fan · 2021-10-18T09:05:21Z

sql/hive/src/main/scala/org/apache/spark/sql/hive/HiveSessionCatalog.scala

+        // ScalaAggregator or Hive UDF/UDAF/UDTF with function definition. Otherwise,
+        // we just throw it earlier.
+        // Unfortunately we need to use reflection here because Aggregator
+        // and ScalaAggregator are defined in sql/core module.


classes in sql/core are available in sql/hive. What's the problem you hit?

or you want to move the code to SessionCatalog? then reflection makes sense

Oh. Thank you for your remind.

SparkQA · 2021-10-18T09:40:16Z

Test build #144365 has finished for PR 34303 at commit 136b8f5.

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

beliefer · 2021-10-18T10:17:10Z

retest this please

SparkQA · 2021-10-18T10:20:11Z

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

SparkQA · 2021-10-18T11:14:55Z

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

SparkQA · 2021-10-18T11:15:41Z

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

SparkQA · 2021-10-18T12:15:01Z

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

SparkQA · 2021-10-18T12:38:08Z

Test build #144366 has finished for PR 34303 at commit 136b8f5.

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

cloud-fan · 2021-10-18T14:15:33Z

sql/hive/src/main/scala/org/apache/spark/sql/hive/HiveSessionCatalog.scala

+              deserializer,
+              ClassTag(cls))
+
+            val e = classOf[ScalaAggregator[_, _, _]].getConstructor(classOf[Seq[Expression]],


can't we just new ScalaAggregator?

Thanks for your reminder.

cloud-fan · 2021-10-18T14:16:29Z

sql/hive/src/main/scala/org/apache/spark/sql/hive/HiveSessionCatalog.scala

+            val baseClassType = typeOf[Aggregator[_, _, _]].typeSymbol.asClass
+            val baseType = internal.thisType(classType).baseType(baseClassType)
+            val tpe = baseType.typeArgs.head
+            val cls = mirror.runtimeClass(tpe)


did you copy the code above from somewhere?

The code references

spark/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/encoders/ExpressionEncoder.scala

Line 55 in 4072a22

val serializer = ScalaReflection.serializerForType(tpe)

cloud-fan · 2021-10-18T14:17:57Z

sql/hive/src/test/scala/org/apache/spark/sql/hive/execution/HiveSQLViewSuite.scala

+        functionName -> true) {
+        // create a function in default database
+        sql("USE DEFAULT")
+        sql(s"CREATE FUNCTION $functionName AS '$avgFuncClass'")


can we do some basic test to make sure the function can be called? and with compatible input types to test implicit cast, incompatible input types to make sure the type check works.

SparkQA · 2021-10-19T05:36:48Z

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

SparkQA · 2021-10-19T06:19:36Z

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

SparkQA · 2021-10-19T06:23:57Z

Test build #144393 has finished for PR 34303 at commit 815c65c.

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

… sql/core ### What changes were proposed in this pull request? This PR adds a new internal interface `FunctionExpressionBuilder`, to replace `SessionCatalog.makeFunctionExpression`. Then we can put the interface implementation in sql/core, to avoid using reflection in `SessionCatalog.makeFunctionExpression`, because the class `UserDefinedAggregateFunction` is not available in sql/catalyst. ### Why are the changes needed? code cleanup, and make it easier to support using `Aggregator` as UDAF later (#34303). ### Does this PR introduce _any_ user-facing change? no ### How was this patch tested? existing tests Closes #34340 from cloud-fan/function. Authored-by: Wenchen Fan <wenchen@databricks.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com>

beliefer · 2021-10-21T08:08:40Z

Because #34340 reactor the architecture of register user-defined function, I opened #34352 replaces this one.

…sql/core This PR adds a new internal interface `FunctionExpressionBuilder`, to replace `SessionCatalog.makeFunctionExpression`. Then we can put the interface implementation in sql/core, to avoid using reflection in `SessionCatalog.makeFunctionExpression`, because the class `UserDefinedAggregateFunction` is not available in sql/catalyst. code cleanup, and make it easier to support using `Aggregator` as UDAF later (apache#34303). no existing tests Closes apache#34340 from cloud-fan/function. Authored-by: Wenchen Fan <wenchen@databricks.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com>

beliefer and others added 2 commits October 15, 2021 19:12

test

fc47d96

Update code

3702b9b

github-actions bot added the SQL label Oct 17, 2021

HyukjinKwon reviewed Oct 18, 2021

View reviewed changes

sql/hive/src/test/scala/org/apache/spark/sql/hive/execution/HiveSQLViewSuite.scala Outdated Show resolved Hide resolved

Update sql/hive/src/test/scala/org/apache/spark/sql/hive/execution/Hi…

7b11c6e

…veSQLViewSuite.scala Co-authored-by: Hyukjin Kwon <gurwls223@gmail.com>

cloud-fan reviewed Oct 18, 2021

View reviewed changes

Update code

136b8f5

cloud-fan reviewed Oct 18, 2021

View reviewed changes

Update code

815c65c

cloud-fan mentioned this pull request Oct 20, 2021

[SPARK-37075][SQL] Move UDAF expression building from sql/catalyst to sql/core #34340

Closed

beliefer mentioned this pull request Oct 21, 2021

[SPARK-37018][SQL] Spark SQL should support create function with Aggregator #34352

Closed

beliefer closed this Oct 21, 2021

[SPARK-37018][SQL] Spark SQL should support create function with Aggregator #34303

[SPARK-37018][SQL] Spark SQL should support create function with Aggregator #34303

Uh oh!

Conversation

beliefer commented Oct 17, 2021

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 Oct 17, 2021

Uh oh!

SparkQA commented Oct 17, 2021

Uh oh!

SparkQA commented Oct 17, 2021

Uh oh!

Uh oh!

beliefer commented Oct 18, 2021

Uh oh!

SparkQA commented Oct 18, 2021

Uh oh!

SparkQA commented Oct 18, 2021

Uh oh!

SparkQA commented Oct 18, 2021

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!

SparkQA commented Oct 18, 2021

Uh oh!

beliefer commented Oct 18, 2021

Uh oh!

SparkQA commented Oct 18, 2021

Uh oh!

SparkQA commented Oct 18, 2021

Uh oh!

SparkQA commented Oct 18, 2021

Uh oh!

SparkQA commented Oct 18, 2021

Uh oh!

SparkQA commented Oct 18, 2021

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!

SparkQA commented Oct 19, 2021

Uh oh!

SparkQA commented Oct 19, 2021

Uh oh!

SparkQA commented Oct 19, 2021

Uh oh!

beliefer commented Oct 21, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants