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: [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]
