[GitHub] [druid] abhishekagarwal87 commented on a change in pull request #12065: Return empty result when a group by gets optimized to a timeseries query

[GitBox <gi...@apache.org>]

abhishekagarwal87 commented on a change in pull request #12065:
URL: https://github.com/apache/druid/pull/12065#discussion_r777366385



##########
File path: sql/src/main/java/org/apache/druid/sql/calcite/rel/Grouping.java
##########
@@ -70,12 +71,25 @@ private Grouping(
       @Nullable final DimFilter havingFilter,
       final RowSignature outputRowSignature
   )
+  {
+    this(dimensions, subtotals, aggregations, havingFilter, outputRowSignature, false);
+  }
+
+  private Grouping(
+      final List<DimensionExpression> dimensions,
+      final Subtotals subtotals,
+      final List<Aggregation> aggregations,
+      @Nullable final DimFilter havingFilter,
+      final RowSignature outputRowSignature,
+      final boolean droppedDimensionsWhileApplyingProject
+  )
   {
     this.dimensions = ImmutableList.copyOf(dimensions);
     this.subtotals = Preconditions.checkNotNull(subtotals, "subtotals");
     this.aggregations = ImmutableList.copyOf(aggregations);
     this.havingFilter = havingFilter;
     this.outputRowSignature = Preconditions.checkNotNull(outputRowSignature, "outputRowSignature");
+    this.droppedDimensionsWhileApplyingProject = droppedDimensionsWhileApplyingProject;

Review comment:
       can we rename it to something else e.g. hadGroupingDimensions or groupDimensionsDropped or something? basically, we need not add `whileApplyingProject` here

##########
File path: sql/src/test/java/org/apache/druid/sql/calcite/CalciteQueryTest.java
##########
@@ -13342,4 +13340,34 @@ public void testCommonVirtualExpressionWithDifferentValueType() throws Exception
         ImmutableList.of()
     );
   }
+
+  // When optimization in Grouping#applyProject is applied, and it reduces a Group By query to a timeseries, we
+  // want it to return empty bucket if no row matches
+  @Test
+  public void testReturnEmptyRowWhenGroupByIsConvertedToTimeseries() throws Exception

Review comment:
       can you add few more queries such as one that groups on a dummy dimension

##########
File path: sql/src/test/java/org/apache/druid/sql/calcite/CalciteQueryTest.java
##########
@@ -13342,4 +13340,34 @@ public void testCommonVirtualExpressionWithDifferentValueType() throws Exception
         ImmutableList.of()
     );
   }
+
+  // When optimization in Grouping#applyProject is applied, and it reduces a Group By query to a timeseries, we
+  // want it to return empty bucket if no row matches
+  @Test
+  public void testReturnEmptyRowWhenGroupByIsConvertedToTimeseries() throws Exception
+  {
+    skipVectorize();
+    testQuery(
+        "SELECT 'A', dim1 from foo WHERE m1 = 50 AND dim1 = 'wat' GROUP BY dim1",

Review comment:
       btw I noticed that it runs as a group-by query if we remove the `m1 = 50` clause. do you know why? 




-- 
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: commits-unsubscribe@druid.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org