yashmayya commented on code in PR #14211:
URL: https://github.com/apache/pinot/pull/14211#discussion_r1797267398
##########
pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/MultiStageEngineIntegrationTest.java:
##########
@@ -1018,6 +1013,28 @@ public void testMVNumericCastInFilter() throws Exception
{
assertEquals(jsonNode.get("resultTable").get("rows").get(0).get(0).asInt(),
15482);
}
+ @Test
+ public void testFilteredAggregationWithNoValueMatchingAggregationFilter()
+ throws Exception {
+ // Write the query in a way where the aggregation will not be pushed to
the leaf stage, so that we can test the
+ // MultistageGroupByExecutor
Review Comment:
Oh nice, I wasn't aware of the existence of this hint.
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/MultistageGroupByExecutor.java:
##########
@@ -241,6 +241,11 @@ private void processAggregate(TransferableBlock block) {
aggFunction.aggregateGroupBySV(numMatchedRows, filteredIntKeys,
groupByResultHolder, blockValSetMap);
}
}
+ if (intKeys == null) {
+ // _groupIdGenerator should still have all the groups even if there
are only filtered aggregates for SQL
+ // compliant results.
+ generateGroupByKeys(block);
+ }
Review Comment:
Yup, this is actually left over from my earlier choice of making it optional
for the multi-stage engine too. I can move it back if we decide we're okay with
keeping this the default behavior for the multi-stage engine.
--
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: commits-unsubscr...@pinot.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org
