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 2022/04/10 05:22:52 UTC
[GitHub] [druid] somu-imply opened a new pull request, #12418: Handling planning with alias for time for group by and order by
somu-imply opened a new pull request, #12418:
URL: https://github.com/apache/druid/pull/12418
The query
`select __time as t, val from table group by 1,2 order by 1` fails to plan.
The reason is that this query during the explain phase translates into a scan query which uses the alias `t` but the check in the scan query allows group by only on the `__time` column. This causes a mismatch as the planner cannot understand that `t` and `__time` means the same thing.
This is solved by the rowtype being set from the output type of the subquery in such a case. There is a fallback for the other cases.
This PR has:
- [ ] been self-reviewed.
- [ ] using the [concurrency checklist](https://github.com/apache/druid/blob/master/dev/code-review/concurrency.md) (Remove this item if the PR doesn't have any relation to concurrency.)
- [ ] added documentation for new or modified features or behaviors.
- [ ] added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
- [ ] added or updated version, license, or notice information in [licenses.yaml](https://github.com/apache/druid/blob/master/dev/license.md)
- [ ] added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
- [ ] added unit tests or modified existing tests to cover new code paths, ensuring the threshold for [code coverage](https://github.com/apache/druid/blob/master/dev/code-review/code-coverage.md) is met.
- [ ] added integration tests.
- [ ] been tested in a test Druid cluster.
--
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
[GitHub] [druid] abhishekagarwal87 commented on pull request #12418: Handling planning with alias for time for group by and order by
Posted by GitBox <gi...@apache.org>.
abhishekagarwal87 commented on PR #12418:
URL: https://github.com/apache/druid/pull/12418#issuecomment-1094228130
> the check in the scan query allows group by only on the __time column
Did you mean to say that scan query allows order by only on the __time column? What if we ignore erroneous plans during planning? So we could changes `DruidRules` class to something like below
```
if (outerQueryRel.isValidDruidQuery()) {
- call.transformTo(outerQueryRel);
+ try {
+ call.transformTo(outerQueryRel);
+ } catch (Exception ex) {
+ if (Throwables.getCauseOfType(ex, CannotBuildQueryException.class) == null) {
+ throw ex;
+ }
+ // Do nothing
+ }
}
}
```
I tested the query with above change and it plans successfully.
--
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
[GitHub] [druid] abhishekagarwal87 commented on pull request #12418: Handling planning with alias for time for group by and order by
Posted by GitBox <gi...@apache.org>.
abhishekagarwal87 commented on PR #12418:
URL: https://github.com/apache/druid/pull/12418#issuecomment-1094571068
hmm. I believe the part of the PR that fixes the bug, is using the row signature of the subquery. If you remove that and just explain the subQuery, the overall query will still fail with the same exception.
--
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
[GitHub] [druid] abhishekagarwal87 commented on pull request #12418: Handling planning with alias for time for group by and order by
Posted by GitBox <gi...@apache.org>.
abhishekagarwal87 commented on PR #12418:
URL: https://github.com/apache/druid/pull/12418#issuecomment-1094530397
Hi Somu, In this case, the transformation can be deemed invalid and the assumption is that calcite will find another plan as it continues exploring. I am assuming that ignoring an exception during transformation is fine but it can be confirmed by asking on the calcite dev mailing list (they are very responsive).
The reason for suggesting this approach is that it can cover more failures scenarios such as the one you put out in the description. Otherwise, we might end up with lot of scenario-specific handling in different parts of the code.
Also, as this change is being tested, it might be worth adding test cases such as below
- join between two scan queries with a join on alias of time column
```(select __time as dim1, dim2 from table1 group by 1, 2 order by 1) as T1 INNER JOIN (select __time as a1, a2 from table2 group by 1, 2 order by 1) as T2 ON T1.dim1 = T2.a1```
- Verify that the output field names returned are correct in the header.
--
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
[GitHub] [druid] abhishekagarwal87 merged pull request #12418: Handling planning with alias for time for group by and order by
Posted by GitBox <gi...@apache.org>.
abhishekagarwal87 merged PR #12418:
URL: https://github.com/apache/druid/pull/12418
--
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
[GitHub] [druid] somu-imply commented on pull request #12418: Handling planning with alias for time for group by and order by
Posted by GitBox <gi...@apache.org>.
somu-imply commented on PR #12418:
URL: https://github.com/apache/druid/pull/12418#issuecomment-1098361410
@clintropolis and @abhishekagarwal87 I have updated the code and have added a test case with alias for inner joins as asked in an earlier review comment
--
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
[GitHub] [druid] somu-imply commented on a diff in pull request #12418: Handling planning with alias for time for group by and order by
Posted by GitBox <gi...@apache.org>.
somu-imply commented on code in PR #12418:
URL: https://github.com/apache/druid/pull/12418#discussion_r850573795
##########
sql/src/test/java/org/apache/druid/sql/calcite/CalciteQueryTest.java:
##########
@@ -6719,6 +6719,12 @@ public void testMinMaxAvgDailyCountWithLimit() throws Exception
)
)
.setInterval(querySegmentSpec(Filtration.eternity()))
+ .setLimitSpec(
Review Comment:
Yes the previous would have been `limitSpec=NoopLimitSpec` but now it changed to `limitSpec=DefaultLimitSpec{columns='[]', offset=0, limit=1}` . But considering the query has a LIMIT 1 at the end, I thought this was acceptable. I updated the limit to a different value and it shows up in the DefaultLimitSpec
--
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
[GitHub] [druid] clintropolis commented on pull request #12418: Handling planning with alias for time for group by and order by
Posted by GitBox <gi...@apache.org>.
clintropolis commented on PR #12418:
URL: https://github.com/apache/druid/pull/12418#issuecomment-1097140531
>hmm. I believe the part of the PR that fixes the bug, is using the row signature of the subquery. If you remove that and just explain the subQuery, the overall query will still fail with the same exception.
Sorry yes, I confused myself, it is the outer scan query that is invalid, not the subquery here because the outer cannot order by the aliased time of the subquery, i mixed them up. But, it still is reporting that the query is valid because of the way this explain works, while calling `toDruidQuery` on the same fails with the `CannotBuildQueryException` we see when we try to transform the query after the valid check, so to me that seems it is not in fact a valid query.
The worry I think is that changing explain to also explain the subquery might result in extra work being done which could result in increased planning time for nested queries, which is reasonable to be concerned about. Though, i also wonder if by not actually validating here if we are allowing other work to proceed which should not have gotten so far, like in this case if explain failed then it would never proceed to the rule transformation to fail again. I'm not really sure what is best though... or how to know
--
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
[GitHub] [druid] somu-imply commented on a diff in pull request #12418: Handling planning with alias for time for group by and order by
Posted by GitBox <gi...@apache.org>.
somu-imply commented on code in PR #12418:
URL: https://github.com/apache/druid/pull/12418#discussion_r850575723
##########
sql/src/test/java/org/apache/druid/sql/calcite/CalciteQueryTest.java:
##########
@@ -7002,7 +7008,7 @@ public void testExplainExactCountDistinctOfSemiJoinResult() throws Exception
+ " )\n"
+ ")";
final String legacyExplanation =
- "DruidOuterQueryRel(query=[{\"queryType\":\"timeseries\",\"dataSource\":{\"type\":\"table\",\"name\":\"__subquery__\"},\"intervals\":{\"type\":\"intervals\",\"intervals\":[\"-146136543-09-08T08:23:32.096Z/146140482-04-24T15:36:27.903Z\"]},\"descending\":false,\"virtualColumns\":[],\"filter\":null,\"granularity\":{\"type\":\"all\"},\"aggregations\":[{\"type\":\"count\",\"name\":\"a0\"}],\"postAggregations\":[],\"limit\":2147483647,\"context\":{\"defaultTimeout\":300000,\"maxScatterGatherBytes\":9223372036854775807,\"sqlCurrentTimestamp\":\"2000-01-01T00:00:00Z\",\"sqlQueryId\":\"dummy\",\"vectorize\":\"false\",\"vectorizeVirtualColumns\":\"false\"}}], signature=[{a0:LONG}])\n"
Review Comment:
I am not sure why the timeseries changed to a groupBy. But it may be due to the change in DUMMY_DATA_SOURCE moving from a table source to a query data source which led the planner to do something else.
--
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
[GitHub] [druid] abhishekagarwal87 commented on pull request #12418: Handling planning with alias for time for group by and order by
Posted by GitBox <gi...@apache.org>.
abhishekagarwal87 commented on PR #12418:
URL: https://github.com/apache/druid/pull/12418#issuecomment-1097605710
I am definitely concerned about increased planning time. The druidRel would have been validated already but we would still explain it repeatedly. This could be a problem for queries that have many levels of nesting. Again, the problem is not in the `explain` itself. It's just that `isValidDruidQuery` and `transformTo` are calling `explain` on query with different inputs. Those inputs are different because of `transformTo` function.
I will still recommend the other approach so that we can just ignore bad in-flight transformations and move on to other rules.
Also, the scan query need not require ordering on the time column when the underlying data source is a query data source. That is actually bound to give wrong ordering because underlying data itself is not sorted by time.
--
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
[GitHub] [druid] clintropolis commented on pull request #12418: Handling planning with alias for time for group by and order by
Posted by GitBox <gi...@apache.org>.
clintropolis commented on PR #12418:
URL: https://github.com/apache/druid/pull/12418#issuecomment-1094560432
I think changing `DruidRules` maybe isn't the best fix here, because what is going on is a bit of a mismatch of expectations on a method to check if a query is valid. [This code in `DruidRel`](https://github.com/apache/druid/blob/master/sql/src/main/java/org/apache/druid/sql/calcite/rel/DruidRel.java#L62) assumes that if a query can be explained without exploding, that it is a 'valid' druid query. [However, `DruidOuterQueryRel`](https://github.com/apache/druid/blob/master/sql/src/main/java/org/apache/druid/sql/calcite/rel/DruidOuterQueryRel.java#L117) doesn't actually explain the subquery, so cannot actually be treated as if the query is valid because full validation is deferred until the subquery is actually transformed (which is where the failure occurs). But because the query is "valid", the transforms happen and then explodes ,which is not caught by any of the rules which call this method. We would have to handle every call to `isValidDruidQuery` similarly with a try catch (wh
ich already has it's own) if we don't fix the contract of the method itself I think.
The change in this PR just makes `DruidOuterQueryRel` eagerly 'validate' the subquery by explaining it, so that the exception happens early and the 'is valid' check catches and returns false, which allows the planner to proceed to other plans. An alternative fix would be to change the valid check to transform to an actual druid query instead of using the explain method to validate, which is perhaps better if we think there are other rels that cannot be considered valid by just explaining, at least with their current explain implementations.
--
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
[GitHub] [druid] somu-imply commented on pull request #12418: Handling planning with alias for time for group by and order by
Posted by GitBox <gi...@apache.org>.
somu-imply commented on PR #12418:
URL: https://github.com/apache/druid/pull/12418#issuecomment-1094470313
@abhishekagarwal87 thanks for your comments. The scan query allows order by only on the __time column and the column name also needs to be `__time` as in https://github.com/apache/druid/blob/8edea5a82dfdb34c3177c0efd7a79bae54751ab8/sql/src/main/java/org/apache/druid/sql/calcite/rel/DruidQuery.java#L1205.
Your suggestion works great. It is unfortunate that the planner reaches a scan query (may be due to the exploratory nature) where it should have been contained to an outer sort and an inner group by query. The difference between the two approaches are ignoring a faulty plan vs correcting a faulty plan (as the faulty plan uses the alias name instead of __time). I am not that familiar with calcite, can we guarantee that the faulty plan if corrected will never generate a plan with a better cost ?
--
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
[GitHub] [druid] abhishekagarwal87 commented on a diff in pull request #12418: Handling planning with alias for time for group by and order by
Posted by GitBox <gi...@apache.org>.
abhishekagarwal87 commented on code in PR #12418:
URL: https://github.com/apache/druid/pull/12418#discussion_r851051077
##########
sql/src/test/java/org/apache/druid/sql/calcite/CalciteQueryTest.java:
##########
@@ -7002,7 +7008,7 @@ public void testExplainExactCountDistinctOfSemiJoinResult() throws Exception
+ " )\n"
+ ")";
final String legacyExplanation =
- "DruidOuterQueryRel(query=[{\"queryType\":\"timeseries\",\"dataSource\":{\"type\":\"table\",\"name\":\"__subquery__\"},\"intervals\":{\"type\":\"intervals\",\"intervals\":[\"-146136543-09-08T08:23:32.096Z/146140482-04-24T15:36:27.903Z\"]},\"descending\":false,\"virtualColumns\":[],\"filter\":null,\"granularity\":{\"type\":\"all\"},\"aggregations\":[{\"type\":\"count\",\"name\":\"a0\"}],\"postAggregations\":[],\"limit\":2147483647,\"context\":{\"defaultTimeout\":300000,\"maxScatterGatherBytes\":9223372036854775807,\"sqlCurrentTimestamp\":\"2000-01-01T00:00:00Z\",\"sqlQueryId\":\"dummy\",\"vectorize\":\"false\",\"vectorizeVirtualColumns\":\"false\"}}], signature=[{a0:LONG}])\n"
Review Comment:
Ah right. That makes sense.
--
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
[GitHub] [druid] abhishekagarwal87 commented on a diff in pull request #12418: Handling planning with alias for time for group by and order by
Posted by GitBox <gi...@apache.org>.
abhishekagarwal87 commented on code in PR #12418:
URL: https://github.com/apache/druid/pull/12418#discussion_r851058217
##########
sql/src/main/java/org/apache/druid/sql/calcite/rel/DruidQuery.java:
##########
@@ -1199,14 +1199,25 @@ private ScanQuery toScanQuery(final QueryFeatureInspector queryFeatureInspector)
orderByColumns = Collections.emptyList();
}
- if (!queryFeatureInspector.feature(QueryFeature.SCAN_CAN_ORDER_BY_NON_TIME)
- && (orderByColumns.size() > 1
- || orderByColumns.stream()
- .anyMatch(orderBy -> !orderBy.getColumnName().equals(ColumnHolder.TIME_COLUMN_NAME)))) {
- // Cannot handle this ordering.
- // Scan cannot ORDER BY non-time columns.
- plannerContext.setPlanningError("SQL query requires order by non-time column %s that is not supported.", orderByColumns);
- return null;
+ if (!queryFeatureInspector.feature(QueryFeature.SCAN_CAN_ORDER_BY_NON_TIME) && !orderByColumns.isEmpty()) {
+ if (orderByColumns.size() > 1 || !ColumnHolder.TIME_COLUMN_NAME.equals(orderByColumns.get(0).getColumnName())) {
+ // Cannot handle this ordering.
+ // Scan cannot ORDER BY non-time columns.
+ plannerContext.setPlanningError(
+ "SQL query requires order by non-time column %s that is not supported.",
+ orderByColumns
+ );
+ return null;
+ }
+ if (!dataSource.isConcrete()) {
+ // Cannot handle this ordering.
+ // Scan cannot ORDER BY non-time columns.
+ plannerContext.setPlanningError(
+ "SQL query requires order by non-time column on a datasource[%s], which is not supported.",
+ dataSource
Review Comment:
I realized that this message is incorrect. You already checked that the column being ordered on is a time column.
```suggestion
plannerContext.setPlanningError(
"SQL query is a scan and requires order by on a datasource of type[%s], which is not supported.",
dataSource.getType()
```
--
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
[GitHub] [druid] abhishekagarwal87 commented on a diff in pull request #12418: Handling planning with alias for time for group by and order by
Posted by GitBox <gi...@apache.org>.
abhishekagarwal87 commented on code in PR #12418:
URL: https://github.com/apache/druid/pull/12418#discussion_r850160942
##########
sql/src/test/java/org/apache/druid/sql/calcite/CalciteQueryTest.java:
##########
@@ -6719,6 +6719,12 @@ public void testMinMaxAvgDailyCountWithLimit() throws Exception
)
)
.setInterval(querySegmentSpec(Filtration.eternity()))
+ .setLimitSpec(
Review Comment:
I am surprised that `explanation` didn't need to change here. The object should also have the same limit spec as this native plan.
##########
sql/src/test/java/org/apache/druid/sql/calcite/CalciteQueryTest.java:
##########
@@ -7002,7 +7008,7 @@ public void testExplainExactCountDistinctOfSemiJoinResult() throws Exception
+ " )\n"
+ ")";
final String legacyExplanation =
- "DruidOuterQueryRel(query=[{\"queryType\":\"timeseries\",\"dataSource\":{\"type\":\"table\",\"name\":\"__subquery__\"},\"intervals\":{\"type\":\"intervals\",\"intervals\":[\"-146136543-09-08T08:23:32.096Z/146140482-04-24T15:36:27.903Z\"]},\"descending\":false,\"virtualColumns\":[],\"filter\":null,\"granularity\":{\"type\":\"all\"},\"aggregations\":[{\"type\":\"count\",\"name\":\"a0\"}],\"postAggregations\":[],\"limit\":2147483647,\"context\":{\"defaultTimeout\":300000,\"maxScatterGatherBytes\":9223372036854775807,\"sqlCurrentTimestamp\":\"2000-01-01T00:00:00Z\",\"sqlQueryId\":\"dummy\",\"vectorize\":\"false\",\"vectorizeVirtualColumns\":\"false\"}}], signature=[{a0:LONG}])\n"
Review Comment:
not sure why this changed but the query still runs as before since the final native query is still the same.
--
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