[SPARK-44930][SQL] Deterministic ApplyFunctionExpression should be foldable #42629
Conversation
|
Hi, @cloud-fan @rdblue @beliefer @LuciferYang @sunchao could you help to review this? Thanks a lot. |
| @@ -425,3 +447,14 @@ case class SerializableBoxedInt(intVal: Int) { | |||
| class NotSerializableBoxedInt(intVal: Int) { | |||
| def addAsInt(other: Int): Int = intVal + other | |||
| } | |||
|
|
|||
| case object CharLength extends ScalarFunction[Int] { | |||
There was a problem hiding this comment.
Could we reuse the CharLength defined at JDBCV2Suite?
There was a problem hiding this comment.
It is located in sql/core module.
There was a problem hiding this comment.
It doesn't matter. You can move it from catalyst to core.
There was a problem hiding this comment.
The CharLength is defined in sql/core. Do you mean to move it from sql/core to sql/catalyst?
There was a problem hiding this comment.
We can just add a new test case in DataSourceV2FunctionSuite to check ApplyFunctionExpression#foldable.
There was a problem hiding this comment.
Moved the UT in DataSourceV2FunctionSuite
| errorClass = null, | ||
| parameters = Map.empty | ||
| ) | ||
| intercept[UnsupportedOperationException](sql("SELECT testcat.ns.strlen('abc')").collect()) |
There was a problem hiding this comment.
Exception throws in constant folding stage instead of execution stage.
There was a problem hiding this comment.
shall we still use checkError?
There was a problem hiding this comment.
checkError require an exception with the type of SparkThrowable
| @@ -341,7 +333,7 @@ class DataSourceV2FunctionSuite extends DatasourceV2SQLBase { | |||
| test("scalar function: no implementation found") { | |||
| catalog("testcat").asInstanceOf[SupportsNamespaces].createNamespace(Array("ns"), emptyProps) | |||
| addFunction(Identifier.of(Array("ns"), "strlen"), StrLen(StrLenNoImpl)) | |||
| intercept[SparkException](sql("SELECT testcat.ns.strlen('abc')").collect()) | |||
| intercept[UnsupportedOperationException](sql("SELECT testcat.ns.strlen('abc')").collect()) | |||
| @@ -415,6 +417,26 @@ class ConstantFoldingSuite extends PlanTest { | |||
| } | |||
| } | |||
| } | |||
|
|
|||
| test("Fold deterministic ApplyFunctionExpression") { | |||
There was a problem hiding this comment.
nit: can we put JIRA name here too?
|
thanks, mering to master/3.5! |
…ldable ### What changes were proposed in this pull request? Currently, ApplyFunctionExpression is unfoldable because inherits the default value from Expression. However, it should be foldable for a deterministic ApplyFunctionExpression. ### Why are the changes needed? This could help optimize the usage for V2 UDF applying to constant expressions. ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? New UT. ### Was this patch authored or co-authored using generative AI tooling? No. Closes #42629 from ConeyLiu/constant-fold-v2-udf. Authored-by: xianyangliu <xianyangliu@tencent.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com> (cherry picked from commit 994389f) Signed-off-by: Wenchen Fan <wenchen@databricks.com>
|
Thanks @cloud-fan @beliefer @sunchao |
What changes were proposed in this pull request?
Currently, ApplyFunctionExpression is unfoldable because inherits the default value from Expression. However, it should be foldable for a deterministic ApplyFunctionExpression.
Why are the changes needed?
This could help optimize the usage for V2 UDF applying to constant expressions.
Does this PR introduce any user-facing change?
No
How was this patch tested?
New UT.
Was this patch authored or co-authored using generative AI tooling?
No.