gianm commented on a change in pull request #10653:
URL: https://github.com/apache/druid/pull/10653#discussion_r538587464
##########
File path:
processing/src/main/java/org/apache/druid/query/groupby/strategy/GroupByStrategyV2.java
##########
@@ -442,8 +425,8 @@ public boolean doMergeResults(final GroupByQuery query)
}
GroupByQuery subtotalQuery = baseSubtotalQuery
- .withLimitSpec(subtotalQueryLimitSpec)
- .withDimensionSpecs(newDimensions);
+ .withLimitSpec(subtotalQueryLimitSpec);
+ //.withDimensionSpecs(newDimensions);
Review comment:
Please don't include commented-out code.
##########
File path: sql/src/test/java/org/apache/druid/sql/calcite/CalciteQueryTest.java
##########
@@ -12159,23 +12159,23 @@ public void
testGroupingAggregatorWithPostAggregator() throws Exception
List<Object[]> resultList;
if (NullHandling.sqlCompatible()) {
resultList = ImmutableList.of(
- new Object[]{NULL_STRING, 2L, 0L, "INDIVIDUAL"},
- new Object[]{"", 1L, 0L, "INDIVIDUAL"},
- new Object[]{"a", 2L, 0L, "INDIVIDUAL"},
- new Object[]{"abc", 1L, 0L, "INDIVIDUAL"},
+ new Object[]{NULL_STRING, 2L, 0L, NULL_STRING},
+ new Object[]{"", 1L, 0L, ""},
+ new Object[]{"a", 2L, 0L, "a"},
+ new Object[]{"abc", 1L, 0L, "abc"},
new Object[]{NULL_STRING, 6L, 1L, "ALL"}
);
} else {
resultList = ImmutableList.of(
- new Object[]{"", 3L, 0L, "INDIVIDUAL"},
- new Object[]{"a", 2L, 0L, "INDIVIDUAL"},
- new Object[]{"abc", 1L, 0L, "INDIVIDUAL"},
+ new Object[]{"", 3L, 0L, ""},
+ new Object[]{"a", 2L, 0L, "a"},
+ new Object[]{"abc", 1L, 0L, "abc"},
new Object[]{NULL_STRING, 6L, 1L, "ALL"}
);
}
testQuery(
"SELECT dim2, SUM(cnt), GROUPING(dim2), \n"
- + "CASE WHEN GROUPING(dim2) = 1 THEN 'ALL' ELSE 'INDIVIDUAL' END\n"
+ + "CASE WHEN GROUPING(dim2) = 1 THEN 'ALL' ELSE dim2 END\n"
Review comment:
Why did you change this test case? (As opposed to introducing a new test
case.)
##########
File path:
processing/src/main/java/org/apache/druid/query/groupby/strategy/GroupByStrategyV2.java
##########
@@ -408,27 +408,10 @@ public boolean doMergeResults(final GroupByQuery query)
// Dimension spec including dimension name and output name
final List<DimensionSpec> subTotalDimensionSpec = new
ArrayList<>(dimsInSubtotalSpec.size());
final List<DimensionSpec> dimensions = query.getDimensions();
- final List<DimensionSpec> newDimensions = new ArrayList<>();
- for (int i = 0; i < dimensions.size(); i++) {
- DimensionSpec dimensionSpec = dimensions.get(i);
+ for (DimensionSpec dimensionSpec : dimensions) {
if (dimsInSubtotalSpec.contains(dimensionSpec.getOutputName())) {
- newDimensions.add(
- new DefaultDimensionSpec(
- dimensionSpec.getOutputName(),
- dimensionSpec.getOutputName(),
- dimensionSpec.getOutputType()
- )
- );
subTotalDimensionSpec.add(dimensionSpec);
- } else {
- // Insert dummy dimension so all subtotals queries have ResultRows
with the same shape.
Review comment:
Is this concern no longer valid?
IIRC, it was necessary because otherwise the ResultRows would be different
lengths and so the final results wouldn't be correct.
----------------------------------------------------------------
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]
