clintropolis commented on a change in pull request #11014:
URL: https://github.com/apache/druid/pull/11014#discussion_r602633106
##########
File path: sql/src/test/java/org/apache/druid/sql/calcite/CalciteQueryTest.java
##########
@@ -131,6 +131,8 @@
import java.util.Map;
import java.util.stream.Collectors;
+import static
org.apache.druid.sql.calcite.planner.PlannerConfig.CTX_KEY_USE_GROUPING_SET_FOR_EXACT_DISTINCT;
Review comment:
iirc, I think we typically prefer to not use static imports, I know this
is enforced in some places, but maybe not in test code because of some junit
stuffs?
##########
File path: docs/configuration/index.md
##########
@@ -1675,6 +1675,7 @@ The Druid SQL server is configured through the following
properties on the Broke
|`druid.sql.planner.maxTopNLimit`|Maximum threshold for a [TopN
query](../querying/topnquery.md). Higher limits will be planned as [GroupBy
queries](../querying/groupbyquery.md) instead.|100000|
|`druid.sql.planner.metadataRefreshPeriod`|Throttle for metadata
refreshes.|PT1M|
|`druid.sql.planner.useApproximateCountDistinct`|Whether to use an approximate
cardinality algorithm for `COUNT(DISTINCT foo)`.|true|
+|`druid.sql.planner.useGroupingSetForExactDistinct`|Only relevant when
`useApproximateCountDistinct` is disabled. If set to true, exact distinct
queries are re-written using grouping sets. Otherwise, exact distinct queries
are re-written using joins. This should be set to true for group by query with
multiple exact distinct aggregations. This flag can be overridden per
query.|false|
Review comment:
naively this seems better maybe than using joins... is the reason to
make it false by default in case there are any regressions I guess? I only ask
because things that are cool, but off by default tend to take a long time to
make it to being turned on, if ever.
##########
File path:
processing/src/test/java/org/apache/druid/query/groupby/GroupByQueryMergeBufferTest.java
##########
@@ -278,11 +285,12 @@ public void testDoubleNestedGroupBy()
.setContext(ImmutableMap.of(QueryContexts.TIMEOUT_KEY, TIMEOUT))
.build();
+ Assert.assertEquals(2,
GroupByStrategyV2.countRequiredMergeBufferNum(query));
GroupByQueryRunnerTestHelper.runQuery(FACTORY, runner, query);
// This should be 0 because the broker needs 2 buffers and the queryable
node needs one.
- Assert.assertEquals(0, MERGE_BUFFER_POOL.getMinRemainBufferNum());
- Assert.assertEquals(3, MERGE_BUFFER_POOL.getPoolSize());
+ Assert.assertEquals(1, MERGE_BUFFER_POOL.getMinRemainBufferNum());
+ Assert.assertEquals(4, MERGE_BUFFER_POOL.getPoolSize());
Review comment:
nit: this comment isn't accurate anymore
--
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.
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
