[CORE] Remove unnecessary case match in getAttrForAggregateExpr #3629
Conversation
|
Thanks for opening a pull request! Could you open an issue for this pull request on Github Issues? https://github.com/oap-project/gluten/issues Then could you also rename commit message and pull request title in the following format? See also: |
|
Run Gluten Clickhouse CI |
2 similar comments
|
Run Gluten Clickhouse CI |
|
Run Gluten Clickhouse CI |
|
Run Gluten Clickhouse CI |
|
Run Gluten Clickhouse CI |
|
===== Performance report for TPCH SF2000 with Velox backend, for reference only ====
|
| if ExpressionMappings.expressionExtensionTransformer.extensionExpressionsMapping.contains( | ||
| extendedAggFunc.getClass) => | ||
| // get attributes from the extended aggregate functions | ||
| ExpressionMappings.expressionExtensionTransformer | ||
| .getAttrsForExtensionAggregateExpr( | ||
| aggregateFunc, | ||
| mode, | ||
| exp, | ||
| aggregateAttributeList, | ||
| aggregateAttr, | ||
| index) |
There was a problem hiding this comment.
Can not delete this part for the extension, when there are some custom agg functions which have the specified logic to implement.
There was a problem hiding this comment.
I see that CustomSum should also follow this pattern.
There was a problem hiding this comment.
CustomSum is only a simple custom ut case, in out internal product, we need to implement some custom logic.
There was a problem hiding this comment.
Thank you for the explanation. I see. Sorry for removing that portion of the logic.
|
I will raise a pr to fix the extension problem. |
…nctions In PR apache#3629, it removes the logic to support the custom aggregate functions, must be reverted. Close apache#3644.
…nctions In PR apache#3629, it removes the logic to support the custom aggregate functions, must be reverted. Close apache#3644.
What changes were proposed in this pull request?
There are many processes in the
getAttrForAggregateExprsmethod that construct aggregateAttr and index for specific aggFuncs. In fact, the patterns of these processes are consistent. They iterate overinputAggBufferAttributesof aggFunc, transform them, and insert them intoaggregateAttr. Then, they increase the count in index for each insertion. The logic for validating the supported modes of different aggFuncs is the only specific part, which can be extracted separately.How was this patch tested?
Exists CI