You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by GitBox <gi...@apache.org> on 2020/06/04 20:37:08 UTC
[GitHub] [druid] gianm commented on a change in pull request #9986: Fix groupBy with literal in subquery grouping
gianm commented on a change in pull request #9986:
URL: https://github.com/apache/druid/pull/9986#discussion_r435531102
##########
File path: sql/src/test/java/org/apache/druid/sql/calcite/CalciteQueryTest.java
##########
@@ -13419,6 +13420,73 @@ public void testNvlColumns() throws Exception
);
}
+ @Test
+ public void testGroupByWithLiteralInSubqueryGrouping() throws Exception
+ {
+ testQuery(
+ "SELECT \n"
+ + " t1, t2\n"
+ + " FROM\n"
+ + " ( SELECT\n"
+ + " 'dummy' as t1,\n"
+ + " CASE\n"
+ + " WHEN \n"
+ + " dim4 = 'b'\n"
+ + " THEN dim4\n"
+ + " ELSE NULL\n"
+ + " END AS t2\n"
+ + " FROM\n"
+ + " numfoo\n"
+ + " GROUP BY\n"
+ + " dim4\n"
+ + " )\n"
+ + " GROUP BY\n"
+ + " t1,t2\n",
+ ImmutableList.of(
+ GroupByQuery.builder()
+ .setDataSource(
+ GroupByQuery.builder()
+ .setDataSource(CalciteTests.DATASOURCE3)
+ .setInterval(querySegmentSpec(Filtration.eternity()))
+ .setGranularity(Granularities.ALL)
+ .setDimensions(new DefaultDimensionSpec("dim4", "_d0", ValueType.STRING))
+ .setContext(QUERY_CONTEXT_DEFAULT)
+ .build()
+ )
+ .setVirtualColumns(
+ expressionVirtualColumn(
+ "v0",
+ "\'dummy\'",
+ ValueType.STRING
+ ),
+ expressionVirtualColumn(
+ "v1",
+ "case_searched((\"_d0\" == 'b'),\"_d0\",null)",
+ ValueType.STRING
+ )
+ ) .setInterval(querySegmentSpec(Filtration.eternity()))
Review comment:
This indentation is kind of weird. I'm not sure if checkstyle will flag it, but watch out.
##########
File path: sql/src/test/java/org/apache/druid/sql/calcite/CalciteQueryTest.java
##########
@@ -13419,6 +13420,73 @@ public void testNvlColumns() throws Exception
);
}
+ @Test
+ public void testGroupByWithLiteralInSubqueryGrouping() throws Exception
+ {
+ testQuery(
+ "SELECT \n"
+ + " t1, t2\n"
+ + " FROM\n"
+ + " ( SELECT\n"
+ + " 'dummy' as t1,\n"
+ + " CASE\n"
+ + " WHEN \n"
+ + " dim4 = 'b'\n"
+ + " THEN dim4\n"
+ + " ELSE NULL\n"
+ + " END AS t2\n"
+ + " FROM\n"
+ + " numfoo\n"
+ + " GROUP BY\n"
+ + " dim4\n"
+ + " )\n"
+ + " GROUP BY\n"
+ + " t1,t2\n",
+ ImmutableList.of(
+ GroupByQuery.builder()
+ .setDataSource(
+ GroupByQuery.builder()
+ .setDataSource(CalciteTests.DATASOURCE3)
+ .setInterval(querySegmentSpec(Filtration.eternity()))
+ .setGranularity(Granularities.ALL)
+ .setDimensions(new DefaultDimensionSpec("dim4", "_d0", ValueType.STRING))
+ .setContext(QUERY_CONTEXT_DEFAULT)
+ .build()
+ )
+ .setVirtualColumns(
+ expressionVirtualColumn(
+ "v0",
+ "\'dummy\'",
+ ValueType.STRING
+ ),
+ expressionVirtualColumn(
+ "v1",
+ "case_searched((\"_d0\" == 'b'),\"_d0\",null)",
+ ValueType.STRING
+ )
+ ) .setInterval(querySegmentSpec(Filtration.eternity()))
+ .setDimensions(
+ dimensions(
+ new DefaultDimensionSpec("v0", "d0", ValueType.STRING),
+ new DefaultDimensionSpec("v1", "d1", ValueType.STRING)
+ )
+ )
+ .setGranularity(Granularities.ALL)
+ .setContext(QUERY_CONTEXT_DEFAULT)
+ .build()
+ ),
+ NullHandling.replaceWithDefault() ?
+ ImmutableList.of(
+ new Object[]{"dummy", ""},
+ new Object[]{"dummy", "b"}
+ ) :
+ ImmutableList.of(
+ new Object[]{"dummy", null},
Review comment:
FYI: You could simplify this by using `NULL_STRING` for the null / "" and avoiding the ternary.
##########
File path: sql/src/main/java/org/apache/druid/sql/calcite/rel/DruidQuery.java
##########
@@ -424,16 +425,39 @@ private static Grouping computeGrouping(
/**
* Builds a {@link Subtotals} object based on {@link Aggregate#getGroupSets()}.
+ *
+ * @throws CannotBuildQueryException if subtotals cannot be computed
*/
private static Subtotals computeSubtotals(
final PartialDruidQuery partialQuery,
+ final PlannerContext plannerContext,
final RowSignature rowSignature
)
{
final Aggregate aggregate = partialQuery.getAggregate();
// dimBitMapping maps from input field position to group set position (dimension number).
- final int[] dimBitMapping = new int[rowSignature.size()];
+ final int[] dimBitMapping;
+ if (partialQuery.getSelectProject() != null) {
+ int fieldCount = 0;
+ for (final RexNode rexNode : partialQuery.getSelectProject().getChildExps()) {
+ final DruidExpression expression = Expressions.toDruidExpression(
+ plannerContext,
+ rowSignature,
+ rexNode
+ );
+
+ if (expression == null) {
+ throw new CannotBuildQueryException(partialQuery.getSelectProject(), rexNode);
Review comment:
I think there's no need to do this check, because it isn't needed to build the Subtotals object, and `computeDimensions` will handle the case where we're grouping on something where dimensions are based on expressions that are not translatable.
----------------------------------------------------------------
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