xiangfu0 commented on code in PR #19988:
URL: https://github.com/apache/pinot/pull/19988#discussion_r4382932155
##########
pinot-query-planner/src/main/java/org/apache/pinot/calcite/rel/rules/PinotAggregateTypeArgumentRewriteRule.java:
##########
@@ -62,10 +69,11 @@ private
PinotAggregateTypeArgumentRewriteRule(RelBuilderFactory factory) {
@Override
public boolean matches(RelOptRuleCall call) {
- // Requiring a call without its data type is also what makes the rule
terminate.
+ // Requiring a call without its data type or with the DISTINCT flag is
also what makes the rule terminate.
Aggregate aggRel = call.rel(0);
RelNode input = aggRel.getInput();
- return aggRel.getAggCallList().stream().anyMatch(aggCall ->
getDataTypeArgument(aggCall, input) != null);
+ return aggRel.getAggCallList().stream()
+ .anyMatch(aggCall -> getDataTypeArgument(aggCall, input) != null ||
isDistinctArrayAgg(aggCall));
Review Comment:
Done in f051cc8369: DISTINCT now becomes the `true` isDistinct argument in
`buildAggCall` of both `PinotAggregateExchangeNodeInsertRule` and
`AggregatePushdownRule`, next to LISTAGG, so this rule no longer touches
explicit-type calls. A no-type call next to them over a UNION ALL still hid
their literals, so the rule now adds its literal to every union input instead
of a project above it. Your query, plus no-type `ARRAY_AGG(DISTINCT x)` and
`PERCENTILE(x, 50)` in the same aggregate, is a runtime case (it fails on the
previous head) and a plan case.
_🤖 Addressed by [Claude Code](https://claude.com/claude-code)_
##########
pinot-query-planner/src/main/java/org/apache/pinot/query/QueryEnvironment.java:
##########
@@ -603,9 +603,15 @@ private static HepProgram getOptProgram(PinotRuleSet
ruleSet, Set<String> skipRu
// Run the Calcite CORE rules using 1 HepInstruction per rule. We use 1
HepInstruction per rule for simplicity:
// the rules used here can rest assured that they are the only ones
evaluated in a dedicated graph-traversal.
for (RelOptRule relOptRule : basicRules) {
+ // Supplies the data type argument before AggregateCaseToFilter, which
rewrites only one-argument calls: its
+ // ARRAY_AGG(expression) FILTER (WHERE condition) form of ARRAY_AGG(CASE
WHEN ...) fails at runtime.
+ if
(relOptRule.toString().equals(CommonConstants.Broker.PlannerRuleNames.AGGREGATE_CASE_TO_FILTER))
{
+
hepProgramBuilder.addRuleInstance(PinotAggregateTypeArgumentRewriteRule.INSTANCE);
Review Comment:
Done in f051cc8369: `PinotApproximateAggregateRewriteRule` now appends its
parameters to an existing project beneath the aggregate.
`ApproximateAggregateRewriteTest` checks that FIRST_WITH_TIME's `'INT'`, the
percentile and both parameter strings stay inlined with
`skipPlannerRules='ProjectMerge'`; it fails without the change.
_🤖 Addressed by [Claude Code](https://claude.com/claude-code)_
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
