[GLUTEN-3705][CORE] Support mapping one custom aggregate function to more than one backend functions #3708
Conversation
…more than one backend functions Support mapping one custom aggregate function to more than one backend functions, like first/last function, they will be mapped to two backend function names according to the ignoreNulls parameter.
|
Run Gluten Clickhouse CI |
1 similar comment
|
Run Gluten Clickhouse CI |
|
|
||
| val inputTypes: Seq[DataType] = aggregateFunc.children.map(child => child.dataType) | ||
| val inputTypes: Seq[DataType] = aggregateFunc.children.map(child => child.dataType) |
There was a problem hiding this comment.
The inputTypes looks as same as that produced by buildCustomAggregateFunction. Maybe, we can move it outside the if/else block.
There was a problem hiding this comment.
In some cases, the inputTypes will be obtained using the other logic, it's better to support using custom logic to get inputTypes
| Some("custom_sum_double") | ||
| } | ||
| case _ => | ||
| throw new UnsupportedOperationException( |
There was a problem hiding this comment.
Return None if we check the returned Option on calling side, as mentioned in other comment.
| val (substraitAggFuncName, inputTypes) = | ||
| if ( | ||
| ExpressionMappings.expressionExtensionTransformer.extensionExpressionsMapping.contains( | ||
| aggregateFunc.getClass) |
There was a problem hiding this comment.
Maybe, we can remove this condition and just depend on the returned Option from buildCustomAggregateFunction. If the Option is not empty, we use the contained value. Otherwise, go to the next to find normal agg function name.
There was a problem hiding this comment.
the buildCustomAggregateFunction only handles the custom expressions (functions and agg functions) which defines in the extensionExpressionsMapping, so it's better check first.
|
===== Performance report for TPCH SF2000 with Velox backend, for reference only ====
|
What changes were proposed in this pull request?
Support mapping one custom aggregate function to more than one backend functions, like first/last function, they will be mapped to two backend function names according to the ignoreNulls parameter.
Close #3705 .
(Fixes: #3705)
How was this patch tested?
(Please explain how this patch was tested. E.g. unit tests, integration tests, manual tests)
(If this patch involves UI changes, please attach a screenshot; otherwise, remove this)