[
https://issues.apache.org/jira/browse/CALCITE-6492?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17869369#comment-17869369
]
Zoltan Haindrich commented on CALCITE-6492:
-------------------------------------------
I've collected the following notes/ideas/etc:
regarding SqlAggFunction:
*
[SqlAggFunction#isPercentile|https://github.com/apache/calcite/pull/2444/files#diff-0a7cdf58dfd51a188e9bc06c357fb38e4f855862cbf909e7af99779aa96bc606R226]
was added in [CALCITE-4644|https://github.com/apache/calcite/pull/2444]
* I've taken a look at the path it created...I think that ideally it should
have relied on some existing feature to be able to hook into
validateAggregateParams ; to place its validators/etc
* SqlAggFunction#getRollup should be move to an interface; implementing the
interface should mean that there is a rollup fn
* allowsNullTreatment has connections to the parser
* allowsFilter has connection to syntax as well
* getDistinctOptionality kinda the same
AggregateCall
* distinct - current value relates to getDistinctOptionality
* distinctKeys
* filterArg - relates to allowsFilter
* approximate
* ignoreNulls - relates to allowsNullTreatment
based on the above I was experimenting with something which could have made it
simpler to add the `isPercentile` easier.
I've introduced:
* some interfaces to enable the extensability at different places
* for the SqlBasicAggFunction class I've added a container which could hold
these things
* removed the customization `isPercentile` have dispersed and moved it closer
to that function
{code}
public static final SqlAggFunction PERCENTILE_CONT =
SqlBasicAggFunction
.create(SqlKind.PERCENTILE_CONT, ReturnTypes.PERCENTILE_DISC_CONT,
OperandTypes.UNIT_INTERVAL_NUMERIC_LITERAL)
.withFunctionType(SqlFunctionCategory.SYSTEM)
.withGroupOrder(Optionality.MANDATORY)
.withAllowsFraming(false)
.withExtension(AggregateParamsValidator.class, new
PercentileAggregateParamsValidator())
.withExtension(AggCallBindingFactory.class, new
PercentileAggCallBindingFactory());
{code}
I see some value in spelling out the extension interface during
registration...but that could be made implicit as well.
I think more-or-less the same could be done for the other things like:
rollup/distinct/ignoreNulls.
I think if we could handle all those with it that could make it easier to also
add future enhancement (mark natively supported distinct) without disturbing
the main interfaces.
[~julianhyde] what do you think about this approach? I've uploaded my pilot
branch [here|https://github.com/kgyrtkirk/calcite/pull/5] If you would like to
take a quick look.
> Support aggregate functions which could process DISTINCT natively
> -----------------------------------------------------------------
>
> Key: CALCITE-6492
> URL: https://issues.apache.org/jira/browse/CALCITE-6492
> Project: Calcite
> Issue Type: Improvement
> Reporter: Zoltan Haindrich
> Assignee: Zoltan Haindrich
> Priority: Major
>
> This could be usefull if the execution engine natively supports some distinct
> aggregations natively - there is no rewrite necessary for these functions.
> Currently there is support
> [SqlAggFunction#getDistinctOptionality|https://github.com/apache/calcite/blob/0deab6f7e0cb4ec63eae8b59477d6f0fadfd11e8/core/src/main/java/org/apache/calcite/sql/SqlAggFunction.java#L187-L189]
> - which have overlaps with this - possibly the closest would be to set it to
> *IGNORED* if its supported natively...however
> * that's a bit misleading as its not IGNORED; but supported...
> * there is also
> [checkArgument|https://github.com/apache/calcite/blob/0deab6f7e0cb4ec63eae8b59477d6f0fadfd11e8/core/src/main/java/org/apache/calcite/rel/core/AggregateCall.java#L125]
> which ensures that *distinct* is not accepted in tht case.
> More or less the end result would be to also enhance
> AggregateExpandDistinctAggregatesRule with the ability to ignore aggregates.
> note: In Druid
> * if approximationCountDistinct is disabled ; that [enables a calcite rule
> which rewrites *all* disitnct
> aggregates|https://github.com/apache/druid/blob/master/sql/src/main/java/org/apache/druid/sql/calcite/planner/CalciteRulesManager.java#L496-L503]
> * in the meantime there are also some aggregate functions which support
> *distinct* natively like
> [string_agg|https://github.com/apache/druid/blob/c9aae9d8e683c0cc9c4687e526b8270f744c57c2/sql/src/main/java/org/apache/druid/sql/calcite/aggregation/builtin/StringSqlAggregator.java#L154]
> - which doesn't need any rewrites
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
