[GitHub] [incubator-druid] gianm commented on a change in pull request #8775: Fix NPE for subquery with limit
gianm commented on a change in pull request #8775: Fix NPE for subquery with
limit
URL: https://github.com/apache/incubator-druid/pull/8775#discussion_r341681453
##
File path: sql/src/test/java/org/apache/druid/sql/calcite/CalciteQueryTest.java
##
@@ -7344,6 +7344,36 @@ public void testUsingSubqueryWithExtractionFns() throws
Exception
);
}
+ @Test
+ public void testUsingSubqueryWithLimit() throws Exception
+ {
+testQuery(
+"SELECT COUNT(*) AS cnt FROM ( SELECT * FROM druid.foo LIMIT 10 )
tmpA",
+ImmutableList.of(),
+ImmutableList.of()
+);
+ }
+
+ @Test
+ public void testUsingSubqueryWithoutLimit() throws Exception
+ {
+testQuery(
+"SELECT COUNT(*) AS cnt FROM ( SELECT * FROM druid.foo ) tmpA",
Review comment:
This one looks good.
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: commits-unsubscr...@druid.apache.org
For additional commands, e-mail: commits-h...@druid.apache.org
[GitHub] [incubator-druid] gianm commented on a change in pull request #8775: Fix NPE for subquery with limit
gianm commented on a change in pull request #8775: Fix NPE for subquery with
limit
URL: https://github.com/apache/incubator-druid/pull/8775#discussion_r341681386
##
File path: sql/src/test/java/org/apache/druid/sql/calcite/CalciteQueryTest.java
##
@@ -7344,6 +7344,36 @@ public void testUsingSubqueryWithExtractionFns() throws
Exception
);
}
+ @Test
+ public void testUsingSubqueryWithLimit() throws Exception
+ {
+testQuery(
+"SELECT COUNT(*) AS cnt FROM ( SELECT * FROM druid.foo LIMIT 10 )
tmpA",
Review comment:
Whoa, this looks wrong. The query doesn't have a native query and it returns
an empty result set.
I think that probably, this query should return an error, because it
represents a kind of query structure Druid can't do right now (aggregation on
top of a non-aggregating subquery).
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: commits-unsubscr...@druid.apache.org
For additional commands, e-mail: commits-h...@druid.apache.org
[GitHub] [incubator-druid] gianm commented on a change in pull request #8775: Fix NPE for subquery with limit
gianm commented on a change in pull request #8775: Fix NPE for subquery with
limit
URL: https://github.com/apache/incubator-druid/pull/8775#discussion_r341271916
##
File path:
sql/src/test/java/org/apache/druid/sql/calcite/http/SqlResourceTest.java
##
@@ -671,6 +671,46 @@ public void testResourceLimitExceeded() throws Exception
checkSqlRequestLog(false);
}
+ @Test
+ public void testSubQueryWithLimit() throws Exception
+ {
+final List> rows = doPost(
+new SqlQuery(
+"SELECT COUNT(*) AS cnt FROM ( SELECT * FROM ( SELECT * FROM
druid.foo LIMIT 10 ) tmpA ) tmpB",
+null,
+false,
+null
+)
+).rhs;
+
+Assert.assertEquals(
+ImmutableList.of(),
+rows
+);
+checkSqlRequestLog(true);
+ }
+
+ @Test
+ public void testSubQueryWithoutLimit() throws Exception
Review comment:
Could you move these tests into `CalciteQueryTest` please? Its `testQuery`
method provides a nice way to test this stuff, including testing that it plans
into an appropriate native query.
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: commits-unsubscr...@druid.apache.org
For additional commands, e-mail: commits-h...@druid.apache.org
[GitHub] [incubator-druid] gianm commented on a change in pull request #8775: Fix NPE for subquery with limit
gianm commented on a change in pull request #8775: Fix NPE for subquery with
limit
URL: https://github.com/apache/incubator-druid/pull/8775#discussion_r340945547
##
File path:
sql/src/main/java/org/apache/druid/sql/calcite/rel/DruidOuterQueryRel.java
##
@@ -129,8 +131,10 @@ public DruidQuery toDruidQuery(final boolean
finalizeAggregations)
}
final RowSignature sourceRowSignature = subQuery.getOutputRowSignature();
+final GroupByQuery groupByQuery = subQuery.toGroupByQuery();
+final DataSource dataSource = groupByQuery == null ?
subQuery.getDataSource() : new QueryDataSource(groupByQuery);
Review comment:
I don't think this is correct. It is ignoring everything about `subQuery`
except the underlying dataSource. It means that if the underlying query is
doing something "interesting", like a limit, or expression, etc, then it will
be ignored.
I'd suggest doing this instead, to at least return a "nice" null instead of
throwing NPE. Then that will get understood by the planner as an unplannable
query.
```java
if (groupByQuery == null) {
return null;
}
```
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: commits-unsubscr...@druid.apache.org
For additional commands, e-mail: commits-h...@druid.apache.org
[GitHub] [incubator-druid] gianm commented on a change in pull request #8775: Fix NPE for subquery with limit
gianm commented on a change in pull request #8775: Fix NPE for subquery with
limit
URL: https://github.com/apache/incubator-druid/pull/8775#discussion_r340945489
##
File path:
sql/src/test/java/org/apache/druid/sql/calcite/http/SqlResourceTest.java
##
@@ -671,6 +671,48 @@ public void testResourceLimitExceeded() throws Exception
checkSqlRequestLog(false);
}
+ @Test
+ public void testSubQueryWithLimit() throws Exception
+ {
+final List> rows = doPost(
+new SqlQuery(
+"SELECT COUNT(*) AS cnt FROM ( SELECT * FROM ( SELECT * FROM
druid.foo LIMIT 10 ) tmpA ) tmpB",
Review comment:
I think a query like this is actually legitimately unplannable in Druid
today. It is an aggregation query on top of a non-aggregating subquery.
Currently, groupBy subqueries must also be groupBys.
I think that to really fix this bug, we need to really support groupBys with
something other than a groupBy as a subquery. To make this work, we could
potentially build on point (3) in "native queries" of the join proposal
https://github.com/apache/incubator-druid/issues/8728:
> Allow joining on to "query" datasources as well. To make this work, we’ll
need to add a sense of a ‘standard translation’ of results from certain query
types into flat schemas that we can offer column selectors on top of. There may
be more than one way to do this, since certain query types (notably, topN and
scan) return nested results in some cases. We could do this by adding a new
QueryToolChest method.
The "standard translation" could also be used to support running a groupBy
on top of a non-aggregating subquery.
Something to think about for the future.
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: commits-unsubscr...@druid.apache.org
For additional commands, e-mail: commits-h...@druid.apache.org
