[SPARK-37075][SQL] Move UDAF expression building from sql/catalyst to sql/core #34340
Conversation
|
Kubernetes integration test starting |
|
Kubernetes integration test status failure |
| // A builder to create `Expression` from function information. | ||
| trait FunctionExpressionBuilder { | ||
| def makeExpression(name: String, clazz: Class[_], input: Seq[Expression]): Expression | ||
| } |
There was a problem hiding this comment.
Should we add a bit more doc about what parameters are, for an interface?
There was a problem hiding this comment.
sounds good, @beliefer can you add it in your PR? These 3 parameters were not documented before this PR either.
|
This looks a good move. Thanks. |
|
Test build #144469 has finished for PR 34340 at commit
|
|
thanks for the review, merging to master! |
…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>
| // If `super.makeFunctionExpression` throw `InvalidUDFClassException`, we construct | ||
| // Hive UDF/UDAF/UDTF with function definition. Otherwise, we just throw it earlier. | ||
| case _: InvalidUDFClassException => | ||
| makeHiveFunctionExpression(name, clazz, input) |
There was a problem hiding this comment.
@cloud-fan in prior spark version, it will convert decimal to double when exception occuring, #13930 . At now, some hive udfs which receive double throw UDFArgumentException. The udf extends GenericUDF.
There was a problem hiding this comment.
Was it already broken before this PR? The removed code in this PR does not have this handling. Anyway, I'm happy to review the fix if you can create a PR, thanks!
5 participants
What changes were proposed in this pull request?
This PR adds a new internal interface
FunctionExpressionBuilder, to replaceSessionCatalog.makeFunctionExpression. Then we can put the interface implementation in sql/core, to avoid using reflection inSessionCatalog.makeFunctionExpression, because the classUserDefinedAggregateFunctionis not available in sql/catalyst.Why are the changes needed?
code cleanup, and make it easier to support using
Aggregatoras UDAF later (#34303).Does this PR introduce any user-facing change?
no
How was this patch tested?
existing tests