[SPARK-44913][SQL] DS V2 supports push down V2 UDF that has magic method by ConeyLiu · Pull Request #42612 · apache/spark

ConeyLiu · 2023-08-22T15:02:06Z

What changes were proposed in this pull request?

Right now we only support pushing down the V2 UDF that has not a magic method. Because the V2 UDF will be analyzed into the ApplyFunctionExpression which could be translated and pushed down. However, a V2 UDF that has the magic method will be analyzed into StaticInvoke or Invoke that can not be translated into V2 expression and then can not be pushed down to the data source. The magic method is suggested.

Why are the changes needed?

This PR adds the support of pushing down the V2 UDF that has a magic method.

Does this PR introduce any user-facing change?

Yes, now the V2 UDF with the magic method could be pushed down.

How was this patch tested?

New UTs.

Was this patch authored or co-authored using generative AI tooling?

No

ConeyLiu · 2023-08-22T15:03:47Z

Hi, @cloud-fan @rdblue @beliefer @LuciferYang could you help to review this? Thanks a lot.

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

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

ConeyLiu · 2023-08-29T03:40:41Z

Gentle ping @cloud-fan @sunchao, could you please also take a look at this? Really appreciate.

sunchao · 2023-09-15T17:20:12Z

Apologies @ConeyLiu , just saw this PR. I think this makes sense. Could you rebase it? I'll review afterwards.

ConeyLiu · 2023-09-16T06:25:42Z

@sunchao that's OK, rebased.

sunchao · 2023-09-19T00:08:14Z

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

Hmm instead of adding these two parameters, can we instead check staticObject and functionName?

+1, we can check if staticObject is a subclass of BoundFunction

For V2 UDF, the staticObject here is the class of the BoundFunction, and the functionName is the magic name: invoke. However, we need the name and canonicalName to build the UserDefinedScalarFunc.

we can instantiate staticObject and call its canonicalName function?

Seems is not easy. The BoundFunction is created by calling the bind method of UnboundFunction which is loaded by FunctionCatalog. And we have no guarantee the BoundFunction has no-args constructor to create with reflect.

It's better to reuse StaticInoke so that any optimizations for it can still apply. How about we add one more parameter function: Option[ScalarFunction[_]] to it instead of two strings?

I was thinking extending StaticInvoke somehow like

case class V2StaticInvoke(wrapped: StaticInvoke, name: String, canonicalName: String) extends InvokeLike

but this way we still need to override quite a few members in InvokeLike so may not worth it.

How about we add one more parameter function: Option[ScalarFunction[_]] to it instead of two strings?

Changed to this way. And it is the initial implementation.

@cloud-fan @sunchao Although adding ScalarFunction as a new parameter is convenient, it makes StaticInvoke look very strange.
We already call the scalarFunc.getClass, scalarFunc.resultType(), scalarFunc.isResultNullable and scalarFunc.isDeterministic before passed the new ScalarFunction.

I'm OK to keep as it is. For me it is just a nit and one parameter makes the change less intrusive.

sunchao

LGTM

beliefer

LGTM too.

cloud-fan · 2023-09-21T05:37:16Z

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

+ * @param scalarFunction the [[ScalarFunction]] object if this is calling the magic method of the
+ *                       [[ScalarFunction]] otherwise is unset.
 */
 case class StaticInvoke(


we can override stringArgs in StaticInvoke, to exclude the new parameter if it's None, to avoid the golden file changes.

sunchao · 2023-09-29T23:37:19Z

Thanks! merged to master branch

ConeyLiu · 2023-10-02T13:40:49Z

Thanks @sunchao @beliefer @cloud-fan

cloud-fan · 2023-10-04T06:45:28Z

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

+    if (scalarFunction.nonEmpty) {
+      super.stringArgs
+    } else {
+      super.stringArgs.take(8)


this is fragile. I think super.stringArgs.dropRight(1) is better.

@cloud-fan I have submitted a follow-up PR for this.

… magic method ### What changes were proposed in this pull request? This is follow up #42612 and to address the comment #42612 (comment) ### Why are the changes needed? To address comments. ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? Existing UTs. ### Was this patch authored or co-authored using generative AI tooling? No Closes #43262 from ConeyLiu/42612-followup. Authored-by: Xianyang Liu <[email protected]> Signed-off-by: Wenchen Fan <[email protected]>

github-actions bot added the SQL label Aug 22, 2023

beliefer reviewed Aug 23, 2023

View reviewed changes

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

beliefer reviewed Aug 23, 2023

View reviewed changes

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

github-actions bot added the CONNECT label Aug 23, 2023

ConeyLiu mentioned this pull request Aug 25, 2023

Spark 3.4: Rewrite procedure throw better exception when filter expression cannot translate apache/iceberg#8394

Merged

ConeyLiu force-pushed the push-down-udf-with-magic branch from c6f0a84 to 50f09cc Compare September 16, 2023 06:22

sunchao reviewed Sep 19, 2023

View reviewed changes

sunchao approved these changes Sep 20, 2023

View reviewed changes

beliefer approved these changes Sep 21, 2023

View reviewed changes

ConeyLiu and others added 4 commits September 21, 2023 11:18

push down V2 UDF that has magic method

0105d8e

fixes and address comments

6015dd0

fixes

e86a631

address comments

12982ff

ConeyLiu force-pushed the push-down-udf-with-magic branch from a7060ef to 12982ff Compare September 21, 2023 03:19

cloud-fan reviewed Sep 21, 2023

View reviewed changes

overwrite stringArgs

e0d9827

github-actions bot removed the CONNECT label Sep 21, 2023

sunchao closed this in bef11d8 Sep 29, 2023

ConeyLiu deleted the push-down-udf-with-magic branch October 2, 2023 13:40

cloud-fan reviewed Oct 4, 2023

View reviewed changes

ConeyLiu mentioned this pull request Oct 7, 2023

[SPARK-44913][SQL][FOLLOWUP] DS V2 supports push down V2 UDF that has magic method #43262

Closed

chris-twiner mentioned this pull request Jan 26, 2024

Spark 4.0 / DBR 14.2+ - bleeding edge changes typelevel/frameless#787

Open

Conversation

ConeyLiu commented Aug 22, 2023

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?

Was this patch authored or co-authored using generative AI tooling?

Uh oh!

ConeyLiu commented Aug 22, 2023

Uh oh!

Uh oh!

Uh oh!

ConeyLiu commented Aug 29, 2023

Uh oh!

sunchao commented Sep 15, 2023

Uh oh!

ConeyLiu commented Sep 16, 2023

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

beliefer Sep 20, 2023 • 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!

Choose a reason for hiding this comment

Uh oh!

sunchao left a comment

Choose a reason for hiding this comment

Uh oh!

beliefer left a comment

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!

sunchao commented Sep 29, 2023

Uh oh!

ConeyLiu commented Oct 2, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

beliefer Sep 20, 2023 •

edited

Loading