jihoonson commented on a change in pull request #11379:
URL: https://github.com/apache/druid/pull/11379#discussion_r665129262
##########
File path: sql/src/main/java/org/apache/druid/sql/calcite/rel/DruidQuery.java
##########
@@ -1016,6 +1017,42 @@ public GroupByQuery toGroupByQuery()
grouping.getSubtotals().toSubtotalsSpec(grouping.getDimensionSpecs()),
ImmutableSortedMap.copyOf(plannerContext.getQueryContext())
);
+ // in this case, the timestampResult optimization is not neccessary
Review comment:
Why is the optimization unnecessary in these cases? Or did you mean we
don't optimize for these cases yet?
##########
File path: sql/src/test/java/org/apache/druid/sql/calcite/CalciteQueryTest.java
##########
@@ -10075,7 +10080,7 @@ public void
testGroupByFloorTimeAndOneOtherDimensionWithOrderBy() throws Excepti
Integer.MAX_VALUE
)
)
- .setContext(QUERY_CONTEXT_DEFAULT)
+
.setContext(withTimestampResultContext(QUERY_CONTEXT_DEFAULT, "d0", 0, P1Y))
Review comment:
```suggestion
.setContext(withTimestampResultContext(QUERY_CONTEXT_DEFAULT, "d0", 0,
Granularities.YEAR))
```
##########
File path:
processing/src/main/java/org/apache/druid/query/groupby/strategy/GroupByStrategyV2.java
##########
@@ -285,6 +342,34 @@ public boolean doMergeResults(final GroupByQuery query)
}
}
+ private void fixResultRowWithTimestampResultField(
Review comment:
Hmm, maybe `moveOrReplicateTimestampInRow()` is a better name.
##########
File path: sql/src/main/java/org/apache/druid/sql/calcite/rel/DruidQuery.java
##########
@@ -1016,6 +1017,42 @@ public GroupByQuery toGroupByQuery()
grouping.getSubtotals().toSubtotalsSpec(grouping.getDimensionSpecs()),
ImmutableSortedMap.copyOf(plannerContext.getQueryContext())
);
+ // in this case, the timestampResult optimization is not neccessary
+ if (query.getLimitSpec() instanceof DefaultLimitSpec &&
query.isApplyLimitPushDown()) {
+ return query;
+ }
+ Map<String, Object> theContext = plannerContext.getQueryContext();
+
+ Granularity queryGranularity = null;
+
+ if (!grouping.getDimensions().isEmpty()) {
+ for (DimensionExpression dimensionExpression : grouping.getDimensions())
{
+ Granularity granularity = Expressions.toQueryGranularity(
+ dimensionExpression.getDruidExpression(),
+ plannerContext.getExprMacroTable()
+ );
+ if (granularity == null) {
+ continue;
+ }
+ if (queryGranularity != null) {
+ // group by more than one timestamp_floor
+ // eg: group by timestamp_floor(__time to
DAY),timestamp_floor(__time, to HOUR)
+ queryGranularity = null;
+ break;
+ }
+ queryGranularity = granularity;
+ int timestampDimensionIndexInDimensions =
grouping.getDimensions().indexOf(dimensionExpression);
+ theContext = new HashMap<>();
Review comment:
Why does it clear all planner context?
##########
File path: sql/src/test/java/org/apache/druid/sql/calcite/CalciteQueryTest.java
##########
@@ -13337,7 +13342,7 @@ public void testGroupByTimeAndOtherDimension() throws
Exception
Integer.MAX_VALUE
)
)
- .setContext(QUERY_CONTEXT_DEFAULT)
+
.setContext(withTimestampResultContext(QUERY_CONTEXT_DEFAULT, "d1", 1, P1M))
Review comment:
```suggestion
.setContext(withTimestampResultContext(QUERY_CONTEXT_DEFAULT, "d1", 1,
Granularities.MONTH))
```
##########
File path: sql/src/test/java/org/apache/druid/sql/calcite/CalciteQueryTest.java
##########
@@ -13404,7 +13409,7 @@ public void testGroupingSets() throws Exception
ImmutableList.of()
)
)
- .setContext(QUERY_CONTEXT_DEFAULT)
+
.setContext(withTimestampResultContext(QUERY_CONTEXT_DEFAULT, "d1", 1, P1M))
Review comment:
Please fix here and other places too.
##########
File path: sql/src/test/java/org/apache/druid/sql/calcite/CalciteQueryTest.java
##########
@@ -7966,7 +7971,7 @@ public void
testMultipleExactCountDistinctWithGroupingAndOtherAggregators() thro
ImmutableList.of("d0", "d1"),
ImmutableList.of("d0", "d2")
))
- .setContext(QUERY_CONTEXT_DEFAULT)
+
.setContext(withTimestampResultContext(QUERY_CONTEXT_DEFAULT, "d0", 0, P1D))
Review comment:
```suggestion
.setContext(withTimestampResultContext(QUERY_CONTEXT_DEFAULT, "d0", 0,
Granularities.DAY))
```
##########
File path:
sql/src/test/java/org/apache/druid/sql/calcite/CalciteCorrelatedQueryTest.java
##########
@@ -114,7 +120,9 @@ public void testCorrelatedSubquery(Map<String, Object>
queryContext) throws Exce
"a0",
"a0:a"
)))
-
.setContext(queryContext)
+ .setContext(
+
withTimestampResultContext(queryContext, "d0", P1D)
Review comment:
Please fix other places as well.
##########
File path:
sql/src/test/java/org/apache/druid/sql/calcite/CalciteCorrelatedQueryTest.java
##########
@@ -114,7 +120,9 @@ public void testCorrelatedSubquery(Map<String, Object>
queryContext) throws Exce
"a0",
"a0:a"
)))
-
.setContext(queryContext)
+ .setContext(
+
withTimestampResultContext(queryContext, "d0", P1D)
Review comment:
```suggestion
withTimestampResultContext(queryContext, "d0", Granularities.DAY)
```
--
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]
