You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "yashmayya (via GitHub)" <gi...@apache.org> on 2026/10/20 19:02:29 UTC
[PR] Run the MSE approximate aggregate rewrite after CASE-to-FILTER conversion [pinot]
yashmayya opened a new pull request, #19974:
URL: https://github.com/apache/pinot/pull/19974
Fixes a wrong-result bug in the multi-stage approximate rewrite from #19510.
## Bug
`BASIC_RULES` ran `PinotApproximateAggregateRewriteRule` before `AggregateCaseToFilterRule`. With `useApproximateFunction` on and distinct count params set, `COUNT(DISTINCT CASE WHEN c THEN x ELSE NULL END)` became `DISTINCTCOUNTSMARTHLL(CASE ..., '<params>')` first. `AggregateCaseToFilterRule` only matches a single-argument call, so it no longer turned the CASE into `FILTER (WHERE c)`. Each row that fails `c` then adds its NULL as one more value.
Example: a predicate that matches no row gives `0` with the rewrite off and `1` with it on, far below any threshold. `DISTINCTCOUNT(CASE ...)` had the same problem.
Without params the call stays single-argument, so the conversion still ran and the result was correct.
Plan before (params `threshold=17`):
```
PinotLogicalAggregate(group=[{}], agg#0=[DISTINCTCOUNTSMARTHLL($0, _UTF-8'threshold=17')], aggType=[LEAF])
LogicalProject($f0=[CASE(>($2, 0), $0, null:VARCHAR)], $f1=[_UTF-8'threshold=17'])
```
After:
```
PinotLogicalAggregate(group=[{}], agg#0=[DISTINCTCOUNTSMARTHLL($0, _UTF-8'threshold=17') FILTER $1], aggType=[LEAF])
LogicalProject($f0=[$0], $f1=[>($2, 0)], $f2=[_UTF-8'threshold=17'])
```
## Fix
Move `PinotApproximateAggregateRewriteRule` after `AggregateCaseToFilterRule`. It stays in `BASIC`, ahead of `PinotAggregateExchangeNodeInsertRule` in `POST_LOGICAL`. The rewrite already keeps `filterArg`, and the params project still sits directly under the aggregate.
## Tests
- `ApproximateAggregateRewriteTest#testKeepsCaseToFilterConversion`: `COUNT(DISTINCT CASE ...)` and `DISTINCTCOUNT(CASE ...)`, with and without GROUP BY. The rewritten plan must have the FILTER and no CASE. Without params it must equal the exact plan apart from the function name. Fails before the move.
- `ApproximateFunctionOverrideIntegrationTest#testConditionalDistinctCountMatchesExact`: same result with the rewrite on and off, on both engines. On MSE the no-match predicate must return 0. Before the move, MSE returns `["AA",85,85,1]` instead of `["AA",84,84,0]`.
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Run the MSE approximate aggregate rewrite after CASE-to-FILTER conversion [pinot]
Posted by "yashmayya (via GitHub)" <gi...@apache.org>.
yashmayya commented on code in PR #19974:
URL: https://github.com/apache/pinot/pull/19974#discussion_r4339963033
##########
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")
+ public void testConditionalDistinctCountMatchesExact(boolean useMultiStageQueryEngine)
+ throws Exception {
+ setUseMultiStageQueryEngine(useMultiStageQueryEngine);
+ JsonNode exact = postQuery(CONDITIONAL_DISTINCT_COUNT_QUERY);
+ assertApplied(exact, false);
+ if (useMultiStageQueryEngine) {
+ for (JsonNode row : rows(exact)) {
+ assertEquals(row.get(3).asLong(), 0L, exact.toString());
+ }
+ }
+
+ setApproximateFunctionOverride("threshold=1000000");
Review Comment:
Good catch. One `ConfigAccessor.set(scope, Map)` call now writes all three keys. Fixed in 641d718.
##########
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")
+ public void testConditionalDistinctCountMatchesExact(boolean useMultiStageQueryEngine)
+ throws Exception {
+ setUseMultiStageQueryEngine(useMultiStageQueryEngine);
+ JsonNode exact = postQuery(CONDITIONAL_DISTINCT_COUNT_QUERY);
+ assertApplied(exact, false);
+ if (useMultiStageQueryEngine) {
+ for (JsonNode row : rows(exact)) {
+ assertEquals(row.get(3).asLong(), 0L, exact.toString());
+ }
+ }
+
+ setApproximateFunctionOverride("threshold=1000000");
+ waitForOverride(true);
+ JsonNode approximate = postQuery(CONDITIONAL_DISTINCT_COUNT_QUERY);
Review Comment:
Added: no exceptions on both responses, and non-empty rows on the exact one. Fixed in 641d718.
##########
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")
+ public void testConditionalDistinctCountMatchesExact(boolean useMultiStageQueryEngine)
+ throws Exception {
+ setUseMultiStageQueryEngine(useMultiStageQueryEngine);
+ JsonNode exact = postQuery(CONDITIONAL_DISTINCT_COUNT_QUERY);
+ assertApplied(exact, false);
+ if (useMultiStageQueryEngine) {
Review Comment:
Moved the zero check to the rewritten response, so it now checks the fix directly. Fixed in 641d718.
##########
pinot-query-planner/src/test/java/org/apache/pinot/query/ApproximateAggregateRewriteTest.java:
##########
@@ -108,6 +110,35 @@ public void testAppendsParamsUnderAJoinAndASubquery() {
assertTrue(subquery.contains("threshold=23"), subquery);
}
+ /// `AggregateCaseToFilterRule` turns `COUNT(DISTINCT CASE WHEN c THEN x END)` into `COUNT(DISTINCT x) FILTER (WHERE
+ /// c)`, so the rows that fail `c` add nothing to the count. It only matches a single-argument call, so the rewrite
+ /// has to run after it: with the parameters appended first, the CASE stays and its NULL counts as one more value.
+ @Test
+ public void testKeepsCaseToFilterConversion() {
+ QueryEnvironment exactEnv = newQueryEnvironment(false, "", "");
+ QueryEnvironment approximateEnv = newQueryEnvironment(true, "", "");
+ QueryEnvironment approximateWithParamsEnv = newQueryEnvironment(true, "threshold=17", "");
+ for (String query : List.of(
Review Comment:
Added `COUNT(DISTINCT CASE ...) FILTER (WHERE col2 = 'x')` to the loop. It covers the branch that ANDs an explicit FILTER with the CASE condition. Fixed in 641d718.
##########
pinot-query-planner/src/test/java/org/apache/pinot/query/ApproximateAggregateRewriteTest.java:
##########
@@ -108,6 +110,35 @@ public void testAppendsParamsUnderAJoinAndASubquery() {
assertTrue(subquery.contains("threshold=23"), subquery);
}
+ /// `AggregateCaseToFilterRule` turns `COUNT(DISTINCT CASE WHEN c THEN x END)` into `COUNT(DISTINCT x) FILTER (WHERE
+ /// c)`, so the rows that fail `c` add nothing to the count. It only matches a single-argument call, so the rewrite
+ /// has to run after it: with the parameters appended first, the CASE stays and its NULL counts as one more value.
+ @Test
+ public void testKeepsCaseToFilterConversion() {
+ QueryEnvironment exactEnv = newQueryEnvironment(false, "", "");
+ QueryEnvironment approximateEnv = newQueryEnvironment(true, "", "");
+ QueryEnvironment approximateWithParamsEnv = newQueryEnvironment(true, "threshold=17", "");
+ for (String query : List.of(
+ "SELECT COUNT(DISTINCT CASE WHEN col3 > 0 THEN col1 ELSE NULL END) FROM a",
+ "SELECT col2, COUNT(DISTINCT CASE WHEN col3 > 0 THEN col1 ELSE NULL END) FROM a GROUP BY col2",
+ "SELECT DISTINCTCOUNT(CASE WHEN col3 > 0 THEN col1 ELSE NULL END) FROM a",
+ "SELECT col2, DISTINCTCOUNT(CASE WHEN col3 > 0 THEN col1 END) FROM a GROUP BY col2")) {
+ String exact = explain(exactEnv, query);
+ assertFilteredWithoutCase(exact);
+
+ // Without parameters the only difference from the exact plan is the function name.
+ String approximate = explain(approximateEnv, query);
+ assertRewritten(approximate, "distinctcountsmarthll");
+ assertEquals(approximate.replace("DISTINCTCOUNTSMARTHLL", "DISTINCTCOUNT"), exact);
+
+ // This is the case that kept the CASE, because the parameter makes it a two-argument call.
+ String approximateWithParams = explain(approximateWithParamsEnv, query);
+ assertRewritten(approximateWithParams, "distinctcountsmarthll");
+ assertTrue(approximateWithParams.contains("threshold=17"), approximateWithParams);
+ assertFilteredWithoutCase(approximateWithParams);
+ }
+ }
+
/// The physical optimizer splits aggregates in its own rule, which also keys the leaf-to-final intermediate format
/// off the function name, so the rewrite has to hold on that path too.
Review Comment:
Added a physical optimizer run to the loop. Fixed in 641d718.
##########
pinot-query-planner/src/main/java/org/apache/pinot/calcite/rel/rules/PinotApproximateAggregateRewriteRule.java:
##########
@@ -56,6 +56,11 @@
/// It must run before [PinotAggregateExchangeNodeInsertRule], which derives the leaf-to-final intermediate result
/// format from the function name, hence `Phase.BASIC` rather than that rule's `POST_LOGICAL`. That is also why the
/// runtime is not a valid place to do this rewrite.
+///
+/// It must also run after `AggregateCaseToFilterRule`, which only matches a single-argument call. With the parameter
Review Comment:
True, Case A1 also needs `allowsFilter()` and `skipsNullInputs()`. But with this order, CASE-to-FILTER always sees the original call, in both the exact and the rewritten plan. So those gates give the same answer either way. The doc only names the gate the rewrite breaks, which is why the order matters. Keeping it as is.
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Run the MSE approximate aggregate rewrite after CASE-to-FILTER conversion [pinot]
Posted by "yashmayya (via GitHub)" <gi...@apache.org>.
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Run the MSE approximate aggregate rewrite after CASE-to-FILTER conversion [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on code in PR #19974:
URL: https://github.com/apache/pinot/pull/19974#discussion_r4329667620
##########
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")
+ public void testConditionalDistinctCountMatchesExact(boolean useMultiStageQueryEngine)
+ throws Exception {
+ setUseMultiStageQueryEngine(useMultiStageQueryEngine);
+ JsonNode exact = postQuery(CONDITIONAL_DISTINCT_COUNT_QUERY);
+ assertApplied(exact, false);
+ if (useMultiStageQueryEngine) {
+ for (JsonNode row : rows(exact)) {
+ assertEquals(row.get(3).asLong(), 0L, exact.toString());
+ }
+ }
+
+ setApproximateFunctionOverride("threshold=1000000");
+ waitForOverride(true);
+ JsonNode approximate = postQuery(CONDITIONAL_DISTINCT_COUNT_QUERY);
Review Comment:
Neither the exact nor the approximate run checks the response's `exceptions` array or that rows are non-empty, so an error response carrying an empty `resultTable` passes every assertion vacuously: the `row.get(3)==0` loop never executes and `assertSameResult` compares two equal empty arrays. A regression that makes the filtered `DISTINCTCOUNTSMARTHLL` plan fail on servers would go green here. The sibling test guards exactly this with `assertEquals(response.get("exceptions").size(), 0)` at line 138 — this test needs the same, plus a rows-non-empty assertion.
##########
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")
+ public void testConditionalDistinctCountMatchesExact(boolean useMultiStageQueryEngine)
+ throws Exception {
+ setUseMultiStageQueryEngine(useMultiStageQueryEngine);
+ JsonNode exact = postQuery(CONDITIONAL_DISTINCT_COUNT_QUERY);
+ assertApplied(exact, false);
+ if (useMultiStageQueryEngine) {
+ for (JsonNode row : rows(exact)) {
+ assertEquals(row.get(3).asLong(), 0L, exact.toString());
+ }
+ }
+
+ setApproximateFunctionOverride("threshold=1000000");
Review Comment:
`setApproximateFunctionOverride` writes three separate ZK cluster-config keys, but `waitForOverride(true)` only polls the enabled flag. If the broker processes the `USE_APPROXIMATE_FUNCTION` callback before the `DISTINCT_COUNT_PARAMS` write lands, the planner sees enabled=true with empty params, skips the param project, and plans single-arg `DISTINCTCOUNTSMARTHLL` — the shape that was never broken — so the test passes without exercising the two-argument path this PR fixes. (`DefaultClusterConfigChangeHandler` re-reads the whole record per callback and `ApproximateFunctionOverrideProvider` falls back to empty params.) Writing all three keys in one `ConfigAccessor.set(scope, Map)` call closes the race; it never causes a spurious failure, only a silent coverage hole.
##########
pinot-query-planner/src/test/java/org/apache/pinot/query/ApproximateAggregateRewriteTest.java:
##########
@@ -108,6 +110,35 @@ public void testAppendsParamsUnderAJoinAndASubquery() {
assertTrue(subquery.contains("threshold=23"), subquery);
}
+ /// `AggregateCaseToFilterRule` turns `COUNT(DISTINCT CASE WHEN c THEN x END)` into `COUNT(DISTINCT x) FILTER (WHERE
+ /// c)`, so the rows that fail `c` add nothing to the count. It only matches a single-argument call, so the rewrite
+ /// has to run after it: with the parameters appended first, the CASE stays and its NULL counts as one more value.
+ @Test
+ public void testKeepsCaseToFilterConversion() {
+ QueryEnvironment exactEnv = newQueryEnvironment(false, "", "");
+ QueryEnvironment approximateEnv = newQueryEnvironment(true, "", "");
+ QueryEnvironment approximateWithParamsEnv = newQueryEnvironment(true, "threshold=17", "");
+ for (String query : List.of(
Review Comment:
No test (planner or integration) covers an explicit `FILTER` clause under the rewrite with params — neither `COUNT(DISTINCT x) FILTER (WHERE d)` nor the `AggregateCaseToFilterRule` `filterArg >= 0` branch that ANDs a pre-existing FILTER with the CASE condition — although the class doc now advertises that the rewrite keeps the call's filter. Every query here derives its filter solely from the CASE conversion, so a future change to the params-project construction in `PinotApproximateAggregateRewriteRule.onMatch` (e.g. projecting only referenced fields, or inserting the literal before input refs) could invalidate `filterArg` for user-written FILTER clauses undetected. One query with an explicit FILTER would pin it.
##########
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")
+ public void testConditionalDistinctCountMatchesExact(boolean useMultiStageQueryEngine)
+ throws Exception {
+ setUseMultiStageQueryEngine(useMultiStageQueryEngine);
+ JsonNode exact = postQuery(CONDITIONAL_DISTINCT_COUNT_QUERY);
+ assertApplied(exact, false);
+ if (useMultiStageQueryEngine) {
Review Comment:
This direct `row.get(3)==0` "counted NULL shows up as 1" assertion runs only on the pre-override exact response, where the rewrite never fires regardless of rule order — on un-fixed code it passes either way, since `AggregateCaseToFilterRule` converts fine without the rewrite. Detection of the actual regression rests solely on `assertSameResult`, which (combined with the params-propagation race above) leaves no direct `==0` check on a response that exercises the rewrite. Duplicating this per-row zero check on the `approximate` response would assert the fix itself.
##########
pinot-query-planner/src/main/java/org/apache/pinot/calcite/rel/rules/PinotApproximateAggregateRewriteRule.java:
##########
@@ -56,6 +56,11 @@
/// It must run before [PinotAggregateExchangeNodeInsertRule], which derives the leaf-to-final intermediate result
/// format from the function name, hence `Phase.BASIC` rather than that rule's `POST_LOGICAL`. That is also why the
/// runtime is not a valid place to do this rewrite.
+///
+/// It must also run after `AggregateCaseToFilterRule`, which only matches a single-argument call. With the parameter
Review Comment:
The "only matches a single-argument call" rationale is incomplete for the `DISTINCTCOUNT`/`DISTINCTCOUNTMV` spellings: their conversion goes through Calcite's Case-A1 branch, which is additionally gated on the aggregation's `allowsFilter()` and `skipsNullInputs()` defaults (both true on `PinotSqlAggFunction` by inheritance) — and `skipsNullInputs()==true` mis-describes Pinot's actual DISTINCTCOUNT null behavior and is pinned nowhere as a contract. If `PinotSqlAggFunction` ever overrides `skipsNullInputs()` to reflect runtime semantics (or Calcite tightens the Case-A1 gate), `DISTINCTCOUNT(CASE ...)` silently stops converting while `COUNT(DISTINCT CASE ...)` still converts via the distinct-COUNT branch, and the two spellings diverge by the NULL count under the override. Worth mentioning the real gate here so a maintainer isn't steered toward arity alone.
##########
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:
Nit: this query carries both spellings (`COUNT(DISTINCT CASE ...)` and `DISTINCTCOUNT(CASE ...)`) of the same aggregate over the same expression, and the planner test's four-query loop re-runs the same rule interaction across GROUP BY variants that exercise no additional code path in either rule. If trimming, note the dual spelling does pin the two distinct Calcite conversion branches (distinct-COUNT vs Case-A1), so keep one query per spelling rather than dropping a spelling entirely.
##########
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:
Nit (CI wall-clock only): this method adds four full cluster-config override flip/poll cycles (2 engines × set+clear, each polling every 100ms up to 60s) on a cluster whose existing `testClusterConfigAppliesWithoutRestart` already performs the identical `threshold=1000000` override lifecycle per engine. Appending `CONDITIONAL_DISTINCT_COUNT_QUERY` to that test's exact/approximate pair would add zero flips.
##########
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:
Nit: the same ordering mechanism (param appended → single-argument match fails → NULL counted) is restated near-verbatim in five places — the rule's class doc, the comments on both rule entries here, the planner test doc, and the integration test doc/query comment. When the mechanism changes (e.g. a Calcite upgrade lets `AggregateCaseToFilterRule` match multi-argument calls), all five must move in lockstep. One canonical statement (the rule's class doc) with one-line pointers elsewhere would do the same job.
##########
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:
Keying on raw `plan.contains("FILTER")` / `!plan.contains("CASE")` over the whole explain string is correct for today's four queries but a trap for extension: a correctly converted `SUM(CASE ...)` plan legitimately contains `CASE` in its null-handling final project (see `QueryCompilationTest.testAggregateCaseToFilter`'s golden plan), and a WHERE-bearing query would satisfy `contains("FILTER")` without the aggregate being filtered. Matching `) FILTER $` or the aggregate-call digest would make the helper self-contained.
##########
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:
(Speculative, surfacing for the record.) The CASE-to-FILTER-before-rewrite constraint is enforced only by list position in `BASIC_RULES` plus behavior tests that run the default rule set. The `RuleSetCustomizer` SPI (ServiceLoader plugins, and this file's own TODO to consolidate lists into customizers) can re-seed or append `Phase.BASIC` rules with no structural check that the rewrite stays after CASE-affecting rules — a customizer that reintroduces an Aggregate-over-CASE shape after this rule would over-count silently with `distinctCountParams` configured, since `AggregateCaseToFilterRule` never reruns.
##########
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:
The correctness guarantee this ordering buys is silently voided by `plannerRule_skipAggregateCaseToFilter=true` (query option) or by disabling `AggregateCaseToFilter` in the broker's default-disabled rules: the plan reverts to exactly the pre-PR shape where the CASE's NULL counts as one extra value per group. Verified the bypassed plan executes without crashing — it just over-counts with no indication the skip reintroduced the bug. A sentence in the ordering comment documenting the bypass would be the minimal fix.
##########
pinot-query-planner/src/test/java/org/apache/pinot/query/ApproximateAggregateRewriteTest.java:
##########
@@ -108,6 +110,35 @@ public void testAppendsParamsUnderAJoinAndASubquery() {
assertTrue(subquery.contains("threshold=23"), subquery);
}
+ /// `AggregateCaseToFilterRule` turns `COUNT(DISTINCT CASE WHEN c THEN x END)` into `COUNT(DISTINCT x) FILTER (WHERE
+ /// c)`, so the rows that fail `c` add nothing to the count. It only matches a single-argument call, so the rewrite
+ /// has to run after it: with the parameters appended first, the CASE stays and its NULL counts as one more value.
+ @Test
+ public void testKeepsCaseToFilterConversion() {
+ QueryEnvironment exactEnv = newQueryEnvironment(false, "", "");
+ QueryEnvironment approximateEnv = newQueryEnvironment(true, "", "");
+ QueryEnvironment approximateWithParamsEnv = newQueryEnvironment(true, "threshold=17", "");
+ for (String query : List.of(
+ "SELECT COUNT(DISTINCT CASE WHEN col3 > 0 THEN col1 ELSE NULL END) FROM a",
+ "SELECT col2, COUNT(DISTINCT CASE WHEN col3 > 0 THEN col1 ELSE NULL END) FROM a GROUP BY col2",
+ "SELECT DISTINCTCOUNT(CASE WHEN col3 > 0 THEN col1 ELSE NULL END) FROM a",
+ "SELECT col2, DISTINCTCOUNT(CASE WHEN col3 > 0 THEN col1 END) FROM a GROUP BY col2")) {
+ String exact = explain(exactEnv, query);
+ assertFilteredWithoutCase(exact);
+
+ // Without parameters the only difference from the exact plan is the function name.
+ String approximate = explain(approximateEnv, query);
+ assertRewritten(approximate, "distinctcountsmarthll");
+ assertEquals(approximate.replace("DISTINCTCOUNTSMARTHLL", "DISTINCTCOUNT"), exact);
+
+ // This is the case that kept the CASE, because the parameter makes it a two-argument call.
+ String approximateWithParams = explain(approximateWithParamsEnv, query);
+ assertRewritten(approximateWithParams, "distinctcountsmarthll");
+ assertTrue(approximateWithParams.contains("threshold=17"), approximateWithParams);
+ assertFilteredWithoutCase(approximateWithParams);
+ }
+ }
+
/// The physical optimizer splits aggregates in its own rule, which also keys the leaf-to-final intermediate format
/// off the function name, so the rewrite has to hold on that path too.
Review Comment:
The new filterArg + trailing-literal plan shape is never exercised under the v2 physical optimizer: `testKeepsCaseToFilterConversion` never passes `usePhysicalOptimizer=true` and `testRewritesUnderThePhysicalOptimizer` has no CASE query, so `AggregatePushdownRule`'s leaf/final split of `DISTINCTCOUNTSMARTHLL(x, 'params') FILTER (c)` is untested. Structural reading shows it mirrors the default rule (filterArg kept at leaf, literal inlined via `findImmediateProjects`), so this is a coverage gap rather than a known break — one physical-optimizer variant in the new test's loop would close it.
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Run the MSE approximate aggregate rewrite after CASE-to-FILTER conversion [pinot]
Posted by "yashmayya (via GitHub)" <gi...@apache.org>.
yashmayya merged PR #19974:
URL: https://github.com/apache/pinot/pull/19974
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Run the MSE approximate aggregate rewrite after CASE-to-FILTER conversion [pinot]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #19974:
URL: https://github.com/apache/pinot/pull/19974#issuecomment-6303350158
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/19974?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 58.92%. Comparing base ([`607576c`](https://app.codecov.io/gh/apache/pinot/commit/607576c7a8b6ea19dd7eda2e5614701738d1db1c?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`430fbed`](https://app.codecov.io/gh/apache/pinot/commit/430fbedf3be3e68628a9798344f0b178438f306f?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 1 commits behind head on master.
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #19974 +/- ##
============================================
+ Coverage 58.90% 58.92% +0.02%
+ Complexity 7 1 -6
============================================
Files 2733 2730 -3
Lines 171798 171792 -6
Branches 28310 28310
============================================
+ Hits 101196 101227 +31
+ Misses 62036 62010 -26
+ Partials 8566 8555 -11
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/19974/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [integration](https://app.codecov.io/gh/apache/pinot/pull/19974/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration1](https://app.codecov.io/gh/apache/pinot/pull/19974/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration2](https://app.codecov.io/gh/apache/pinot/pull/19974/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [java-25](https://app.codecov.io/gh/apache/pinot/pull/19974/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `58.92% <100.00%> (+0.02%)` | :arrow_up: |
| [lane-a](https://app.codecov.io/gh/apache/pinot/pull/19974/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [lane-b](https://app.codecov.io/gh/apache/pinot/pull/19974/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [temurin](https://app.codecov.io/gh/apache/pinot/pull/19974/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `58.92% <100.00%> (+0.02%)` | :arrow_up: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/19974/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `58.92% <100.00%> (+0.02%)` | :arrow_up: |
| [unittests1](https://app.codecov.io/gh/apache/pinot/pull/19974/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `58.92% <100.00%> (+0.02%)` | :arrow_up: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/pinot/pull/19974?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org