[GitHub] [druid] clintropolis commented on a change in pull request #11081: Nested GroupBy query got wrong/empty result when using virtual column and filter

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

clintropolis commented on a change in pull request #11081:
URL: https://github.com/apache/druid/pull/11081#discussion_r610305869



##########
File path: processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/RowBasedGrouperHelper.java
##########
@@ -398,8 +399,10 @@ public static ColumnSelectorFactory createResultRowBasedColumnSelectorFactory(
     );
 
     final SettableSupplier<ResultRow> rowSupplier = new SettableSupplier<>();
-    final ColumnSelectorFactory columnSelectorFactory =
-        RowBasedGrouperHelper.createResultRowBasedColumnSelectorFactory(subquery, rowSupplier);
+    final ColumnSelectorFactory columnSelectorFactory = new VirtualizedColumnSelectorFactory(
+        RowBasedGrouperHelper.createResultRowBasedColumnSelectorFactory(subquery, rowSupplier),
+        query.getVirtualColumns()
+    );

Review comment:
       
   ```suggestion
       final ColumnSelectorFactory columnSelectorFactory = query.getVirtualColumns().wrap(
           RowBasedGrouperHelper.createResultRowBasedColumnSelectorFactory(subquery, rowSupplier)
       );
   ```
   would be more consistent with how other methods such as `createGrouperAccumulatorPair` are doing this 
   
   Since `VirtualColumns.wrap` is currently the only 'production' method that is calling `VirtualizedColumnSelectorFactory` constructor right now, so I think we should probably change this to use `wrap`




-- 
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:
users@infra.apache.org



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