yashmayya commented on code in PR #19974:
URL: https://github.com/apache/pinot/pull/19974#discussion_r4339964925
##########
pinot-query-planner/src/test/java/org/apache/pinot/query/ApproximateAggregateRewriteTest.java:
##########
@@ -130,6 +161,11 @@ private static void assertNotRewritten(String plan) {
assertFalse(plan.toLowerCase().contains("smart"), "Unexpected rewrite
in:\n" + plan);
}
+ private static void assertFilteredWithoutCase(String plan) {
+ assertTrue(plan.contains("FILTER"), "Expected a filtered aggregate in:\n"
+ plan);
Review Comment:
Now matches `) FILTER $`. Kept the CASE check: none of these queries has a
CASE outside the aggregate argument. Fixed in 641d718.
##########
pinot-query-planner/src/main/java/org/apache/pinot/calcite/rel/rules/PinotQueryRuleSets.java:
##########
@@ -175,14 +175,17 @@ private PinotQueryRuleSets() {
PinotAggregateFunctionRewriteRule
.instanceWithDescription(PlannerRuleNames.AGGREGATE_FUNCTION_REWRITE),
- // Must stay ahead of PinotAggregateExchangeNodeInsertRule
(POST_LOGICAL), which fixes the leaf-to-final
- // intermediate result format from the function name.
- PinotApproximateAggregateRewriteRule.INSTANCE,
-
// convert CASE-style filtered aggregates into true filtered aggregates
// put it after AGGREGATE_REDUCE_FUNCTIONS where SUM is converted to SUM0
+ // put it before APPROXIMATE_AGGREGATE_REWRITE, which appends a
parameter argument this rule cannot match
AggregateCaseToFilterRule.Config.DEFAULT
- .withDescription(PlannerRuleNames.AGGREGATE_CASE_TO_FILTER).toRule()
+ .withDescription(PlannerRuleNames.AGGREGATE_CASE_TO_FILTER).toRule(),
Review Comment:
If AggregateCaseToFilter is skipped, the exact plan keeps the CASE too, so
both plans count the NULL the same way. On and off still agree. That is the
skip option's existing behavior, not something this order adds. Keeping the
comment as is.
##########
pinot-query-planner/src/main/java/org/apache/pinot/calcite/rel/rules/PinotQueryRuleSets.java:
##########
@@ -175,14 +175,17 @@ private PinotQueryRuleSets() {
PinotAggregateFunctionRewriteRule
.instanceWithDescription(PlannerRuleNames.AGGREGATE_FUNCTION_REWRITE),
- // Must stay ahead of PinotAggregateExchangeNodeInsertRule
(POST_LOGICAL), which fixes the leaf-to-final
- // intermediate result format from the function name.
- PinotApproximateAggregateRewriteRule.INSTANCE,
-
// convert CASE-style filtered aggregates into true filtered aggregates
// put it after AGGREGATE_REDUCE_FUNCTIONS where SUM is converted to SUM0
+ // put it before APPROXIMATE_AGGREGATE_REWRITE, which appends a
parameter argument this rule cannot match
AggregateCaseToFilterRule.Config.DEFAULT
- .withDescription(PlannerRuleNames.AGGREGATE_CASE_TO_FILTER).toRule()
+ .withDescription(PlannerRuleNames.AGGREGATE_CASE_TO_FILTER).toRule(),
+
+ // Must run after AGGREGATE_CASE_TO_FILTER: once the parameter argument
is appended, COUNT(DISTINCT CASE WHEN c
+ // THEN x END) is no longer converted to COUNT(DISTINCT x) FILTER (WHERE
c), and the CASE's NULL counts as a
+ // value. Must stay ahead of PinotAggregateExchangeNodeInsertRule
(POST_LOGICAL), which fixes the leaf-to-final
+ // intermediate result format from the function name.
+ PinotApproximateAggregateRewriteRule.INSTANCE
Review Comment:
Out of scope here. A customizer that reorders BASIC rules can break many
order assumptions, not only this one. The tests cover the default rule set.
##########
pinot-query-planner/src/main/java/org/apache/pinot/calcite/rel/rules/PinotQueryRuleSets.java:
##########
@@ -175,14 +175,17 @@ private PinotQueryRuleSets() {
PinotAggregateFunctionRewriteRule
.instanceWithDescription(PlannerRuleNames.AGGREGATE_FUNCTION_REWRITE),
- // Must stay ahead of PinotAggregateExchangeNodeInsertRule
(POST_LOGICAL), which fixes the leaf-to-final
- // intermediate result format from the function name.
- PinotApproximateAggregateRewriteRule.INSTANCE,
-
// convert CASE-style filtered aggregates into true filtered aggregates
// put it after AGGREGATE_REDUCE_FUNCTIONS where SUM is converted to SUM0
+ // put it before APPROXIMATE_AGGREGATE_REWRITE, which appends a
parameter argument this rule cannot match
AggregateCaseToFilterRule.Config.DEFAULT
- .withDescription(PlannerRuleNames.AGGREGATE_CASE_TO_FILTER).toRule()
+ .withDescription(PlannerRuleNames.AGGREGATE_CASE_TO_FILTER).toRule(),
+
+ // Must run after AGGREGATE_CASE_TO_FILTER: once the parameter argument
is appended, COUNT(DISTINCT CASE WHEN c
Review Comment:
Trimmed both rule-set comments to point at the class doc. Kept the test
docs, since each says what its test checks. Fixed in 641d718.
##########
pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/ApproximateFunctionOverrideIntegrationTest.java:
##########
@@ -46,6 +46,12 @@ public class ApproximateFunctionOverrideIntegrationTest
extends BaseClusterInteg
"SELECT Carrier, COUNT(DISTINCT AirlineID) FROM mytable GROUP BY Carrier
ORDER BY Carrier LIMIT 100";
private static final String PERCENTILE_QUERY =
"SELECT Carrier, PERCENTILE(ArrDelay, 90) FROM mytable GROUP BY Carrier
ORDER BY Carrier LIMIT 100";
+ // The last predicate matches no row, so the exact multi-stage count is 0
and a counted NULL would show up as 1.
+ private static final String CONDITIONAL_DISTINCT_COUNT_QUERY = "SELECT
Carrier, "
+ + "COUNT(DISTINCT CASE WHEN ArrDelay > 0 THEN Origin ELSE NULL END), "
+ + "DISTINCTCOUNT(CASE WHEN ArrDelay > 0 THEN Origin ELSE NULL END), "
Review Comment:
Keeping both spellings and the loop. The planner cases are cheap.
##########
pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/ApproximateFunctionOverrideIntegrationTest.java:
##########
@@ -135,6 +141,28 @@ public void testClusterConfigAppliesWithoutRestart(boolean
useMultiStageQueryEng
}
}
+ /// The multi-stage engine turns `COUNT(DISTINCT CASE WHEN c THEN x END)`
into `COUNT(DISTINCT x) FILTER (WHERE c)`,
+ /// and the rewrite must not stop that, or the NULL of every row that fails
`c` counts as one more value. The
+ /// single-stage engine has no such conversion, but its answer must not
change either.
+ @Test(dataProvider = "useBothQueryEngines")
Review Comment:
Keeping it separate, so a failure points at this case. Each flip takes well
under a second, and the whole class runs in about 12s.
--
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]
