[CALCITE-7640] Enumerable engine should execute aggregates whose implementor comes from a custom RexImplementorTable#5074
[CALCITE-7640] Enumerable engine should execute aggregates whose implementor comes from a custom RexImplementorTable#5074darpan-e6 wants to merge 6 commits into
Conversation
…ementor comes from a custom RexImplementorTable CALCITE-7631 made RexImplementorTable composable for scalar code generation and constant reduction, but the aggregate path still resolves implementors through the RexImpTable singleton: EnumerableAggregate's constructor rejects any aggregate the built-ins do not know, and AggImpState generates code against the singleton. A custom aggregate implementor supplied through a chained table is therefore never used. Resolve aggregate implementors through the injected table, defaulting to the built-ins. Add RexImplementorTables.of(RelOptCluster) (reads the planner context, else the built-in table), move the availability check from the EnumerableAggregate constructor into EnumerableAggregateRule (keeping only the table-independent structural checks in the node), and thread the resolved table into AggImpState and its callers. Behaviour is unchanged when no table is registered.
| }; | ||
| } | ||
|
|
||
| /** Builds {@code SELECT g, MY_CUSTOM_AGG(x) FROM (VALUES ...) GROUP BY g} on a |
There was a problem hiding this comment.
Maybe you can show the full query here
| /** Builds {@code SELECT g, MY_CUSTOM_AGG(x) FROM (VALUES ...) GROUP BY g} on a | ||
| * planner whose context carries {@code implementorTable} (or the built-in | ||
| * table when null), and optimizes it to the enumerable convention. */ | ||
| private static RelNode planAggregate( |
There was a problem hiding this comment.
This function should have a more precise name, like createAndOptimizeTestProgram.
| .build(); | ||
|
|
||
| final Planner planner = Frameworks.getPlanner(config); | ||
| final SqlNode parse = planner.parse("SELECT g, MY_CUSTOM_AGG(x) AS c\n" |
There was a problem hiding this comment.
why here use the parser and above build the plan by hand?
…gle helper Address review feedback on the test: use one plan-construction style -- parsing SQL through the Frameworks API -- for both the positive and negative cases via a single createAndOptimizeTestProgram helper, instead of mixing a hand-built RelBuilder plan with a parsed one. The full query is shown in the parse call, and the helper has a precise name.
|
| RexImpTable.INSTANCE.get(aggCall.getAggregation(), false); | ||
| if (implementor2 == null) { | ||
| throw new InvalidRelException( | ||
| "aggregation " + aggCall.getAggregation() + " not supported"); |
There was a problem hiding this comment.
Can you explain why you think this block of code should be deleted?
There was a problem hiding this comment.
The check is still needed, but it needs to use the active RexImplementorTable from the planner context rather than RexImpTable.INSTANCE. I moved it to EnumerableAggregateRule.convert because implementor availability is a convention-conversion feasibility check: if the active table cannot implement an aggregate, the rule should return null and let the planner try another path.
Keeping this in the EnumerableAggregate constructor would make the constructor depend on planner-context lookup. The constructor now keeps only table-independent structural checks such as DISTINCT / WITHIN DISTINCT. Code generation later resolves through the same active table via RexImplementorTables.of(cluster).
| } | ||
|
|
||
| public AggImpState(int aggIdx, AggregateCall call, boolean windowContext, | ||
| RexImplementorTable implementorTable) { |
There was a problem hiding this comment.
Why is implementorTable a constructor parameter when it does not become a field? Why not use a constructor method?
There was a problem hiding this comment.
implementorTable is only needed while constructing AggImpState, to resolve the AggImplementor; after that the state keeps the resolved implementor, so I did not store the table as a field.
My reason for passing it in was to avoid making AggImpState itself depend on planner context. The callers already have the relevant cluster/context, so they resolve the active table with RexImplementorTables.of(cluster) and pass it here. If AggImpState retrieved the table internally through a static helper, the constructor would need either the planner/cluster or would have to fall back to the singleton, which is the path this change is trying to avoid.
That said, if you prefer resolving the table inside the constructor by passing the cluster instead, I can change it that way.
3 participants
Jira Link
CALCITE-7640
Summary
Follow-up to CALCITE-7631. That change made
RexImplementorTablecomposable for scalar code generation and constant reduction, but aggregate code generation still resolved aggregate implementors through theRexImpTablesingleton.As a result, a custom aggregate operator contributed through a
SqlOperatorTablecould validate as SQL, but the enumerable engine could not plan or execute it unless the aggregate was known by the built-in table.Implementation Details
RexImplementorTables.of(RelOptCluster), which returns theRexImplementorTableregistered on the plannerContext, falling back toRexImpTable.instance()when no custom table is registered. This preserves existing behavior for users that do not provide custom implementors.EnumerableAggregateconstructor toEnumerableAggregateRule.convert. The rule has access to the planner context, so it can check the active implementor table and returnnullwhen enumerable cannot implement an aggregate.EnumerableAggregateconstructor checks limited to table-independent structural restrictions, such asDISTINCTandWITHIN DISTINCT.RexImplementorTableintoAggImpStateso code generation uses the same table that the rule used during planning.EnumerableAggregate,EnumerableSortedAggregate,EnumerableWindow, and interpreterAggregateNode, to resolve implementors throughRexImplementorTables.of(cluster).AggImpStateconstructor as deprecated, delegating to the built-in table for compatibility.There are two separate registrations involved:
SqlOperatorTableregisters the SQL operator so parsing and validation can recognizeMY_CUSTOM_AGG(...).RexImplementorTableregisters the Java/codegen implementor so enumerable planning and execution can implement that aggregate.Tests
EnumerableCustomAggregateTestcovers the ticket scenarios:FrameworksAPI.RexImplementorTable.Focused verification: