imply-cheddar commented on code in PR #14748:
URL: https://github.com/apache/druid/pull/14748#discussion_r1309500249
##########
processing/src/main/java/org/apache/druid/query/groupby/GroupByQueryRunnerFactory.java:
##########
@@ -108,4 +124,57 @@ public GroupingEngine getGroupingEngine()
{
return groupingEngine;
}
+
+ /**
+ * Wraps the sequence around if for this query a summary row might be needed
in case the input becomes empty.
+ */
+ public static Sequence<ResultRow> wrapSummaryRowIfNeeded(GroupByQuery query,
Sequence<ResultRow> process)
+ {
+ if (!summaryRowPreconditions(query)) {
+ return process;
+ }
+
+ final AtomicBoolean t = new AtomicBoolean();
+
+ return Sequences.concat(
+ Sequences.map(process, ent -> {
+ t.set(true);
+ return ent;
+ }),
+ Sequences.simple(() -> {
+ if (t.get()) {
+ return Collections.emptyIterator();
+ }
+ return summaryRowIterator(query);
+ }));
+ }
+
+ private static boolean summaryRowPreconditions(GroupByQuery query)
+ {
+ LimitSpec limit = query.getLimitSpec();
+ if (limit instanceof DefaultLimitSpec) {
+ DefaultLimitSpec limitSpec = (DefaultLimitSpec) limit;
+ if (limitSpec.getLimit() == 0 || limitSpec.getOffset() > 0) {
+ return false;
+ }
+ }
+ if (!query.getDimensions().isEmpty()) {
+ return false;
+ }
+ if (query.getGranularity().isFinerThan(Granularities.ALL)) {
+ return false;
+ }
+ return true;
+ }
+
+ private static Iterator<ResultRow> summaryRowIterator(GroupByQuery q)
+ {
+ List<AggregatorFactory> aggSpec = q.getAggregatorSpecs();
+ Object[] values = new Object[aggSpec.size()];
+ for (int i = 0; i < aggSpec.size(); i++) {
+ values[i] = aggSpec.get(i).factorize(new
AllNullColumnSelectorFactory()).get();
+ }
+ return Collections.singleton(ResultRow.of(values)).iterator();
Review Comment:
There's a `Collections.singletonIterator` that you can use instead. It's a
nit, but will save on an object allocation.
##########
processing/src/main/java/org/apache/druid/query/groupby/GroupByQueryQueryToolChest.java:
##########
@@ -175,10 +175,13 @@ private Sequence<ResultRow> mergeGroupByResults(
ResponseContext context
)
{
+ Sequence<ResultRow> process;
if (isNestedQueryPushDown(query)) {
- return mergeResultsWithNestedQueryPushDown(query, resource, runner,
context);
+ process = mergeResultsWithNestedQueryPushDown(query, resource, runner,
context);
+ } else {
+ process = mergeGroupByResultsWithoutPushDown(query, resource, runner,
context);
}
- return mergeGroupByResultsWithoutPushDown(query, resource, runner,
context);
+ return GroupByQueryRunnerFactory.wrapSummaryRowIfNeeded(query, process);
Review Comment:
I'm surprised that this was required, which test caused you to need this
change? I say this because the only way you should be able to get a completely
empty sequence here is if the "leaf nodes" are producing completely empty
sequences. The change in the other place should ensure that no leaf node ever
produces a completely empty sequence, meaning that this change shouldn't be
necessary...
--
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]
