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 2021/11/11 07:29:40 UTC
[GitHub] [druid] LakshSingla opened a new pull request #11908: Improve the output of SQL explain message
LakshSingla opened a new pull request #11908:
URL: https://github.com/apache/druid/pull/11908
### Description
Currently, when we try to do `EXPLAIN PLAN FOR`, it returns the structure of the SQL parsed (via Calcite's internal planner util), which is verbose (since it tries to explain about the nodes in the SQL, instead of the Druid Query), and not representative of the native Druid query which will get executed on the broker side.
For example:
// Example here
This PR aims to change the format when user tries to `EXPLAIN PLAN FOR` for queries which are executed by converting them into Druid's native queries (i.e. not `sys` schemas).
The explanation now will be a list of columns with information about `resources` (no change) and `explanation`.
Explanation is a string representing an array of queries & their signatures (in JSON format). Final shape of the `explanation` will look like:
```
[
{
"query" : <native_druid_query>,
"signature": <signature of the query>
},
{
"query" : <native_druid_query>,
"signature": <signature of the query>
}
]
```
<!-- Describe the goal of this PR, what problem are you fixing. If there is a corresponding issue (referenced above), it's not necessary to repeat the description here, however, you may choose to keep one summary sentence. -->
<!-- Describe your patch: what did you change in code? How did you fix the problem? -->
<!-- If there are several relatively logically separate changes in this PR, create a mini-section for each of them. For example: -->
<!-- It's good to describe an alternative design (or mention an alternative name) for every design (or naming) decision point and compare the alternatives with the designs that you've implemented (or the names you've chosen) to highlight the advantages of the chosen designs and names. -->
<!-- Some of the aspects mentioned above may be omitted for simple and small changes. -->
<hr>
##### Key changed/added classes in this PR
* Modified implementation of `DruidPlanner#planExplanation`.
<hr>
<!-- Check the items by putting "x" in the brackets for the done things. Not all of these items apply to every PR. Remove the items which are not done or not relevant to the PR. None of the items from the checklist below are strictly necessary, but it would be very helpful if you at least self-review the PR. -->
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] clintropolis commented on pull request #11908: Improve the output of SQL explain message
Posted by GitBox <gi...@apache.org>.
clintropolis commented on pull request #11908:
URL: https://github.com/apache/druid/pull/11908#issuecomment-971816547
This looks nice :+1:
Since this changes the output of a query, i'm +1 for adding a feature flag to allow the previous results to be returned. `PlannerConfig` would probably be the most appropriate place, maybe something like `druid.sql.useLegacyDruidExplain`?
Since this output seems totally better I think it is ok to default it to the new stuff. It might be nice to also add a context parameter so that it could be overridden per query to make it easier for developers to migrate their apps to the new output while debugging without having to set it for the entire 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] LakshSingla edited a comment on pull request #11908: Improve the output of SQL explain message
Posted by GitBox <gi...@apache.org>.
LakshSingla edited a comment on pull request #11908:
URL: https://github.com/apache/druid/pull/11908#issuecomment-972839405
In accordance with the above suggestions, I have added a config option in the `PlannerConfig` which will allow the user to switch between the explain plan outputs. It can also be overridden on a per query basis.
By default the newer output would be visible. This default behavior can be changed by setting `druid.sql.planner.useLegacyDruidExplain = true` (default is false)
Irrespective of the default behavior set in the properties, the explain plan output can also be modified on a per query basis by setting the `useLegacyDruidExplain` to true or false in the query's context.
--
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 a change in pull request #11908: Improve the output of SQL explain message
Posted by GitBox <gi...@apache.org>.
clintropolis commented on a change in pull request #11908:
URL: https://github.com/apache/druid/pull/11908#discussion_r756402548
##########
File path: docs/configuration/index.md
##########
@@ -1827,7 +1827,7 @@ The Druid SQL server is configured through the following properties on the Broke
|`druid.sql.planner.metadataSegmentCacheEnable`|Whether to keep a cache of published segments in broker. If true, broker polls coordinator in background to get segments from metadata store and maintains a local cache. If false, coordinator's REST API will be invoked when broker needs published segments info.|false|
|`druid.sql.planner.metadataSegmentPollPeriod`|How often to poll coordinator for published segments list if `druid.sql.planner.metadataSegmentCacheEnable` is set to true. Poll period is in milliseconds. |60000|
|`druid.sql.planner.authorizeSystemTablesDirectly`|If true, Druid authorizes queries against any of the system schema tables (`sys` in SQL) as `SYSTEM_TABLE` resources which require `READ` access, in addition to permissions based content filtering.|false|
-|`druid.sql.planner.useLegacyDruidExplain`|If true, `EXPLAIN PLAN FOR` will return the explain plan in legacy format, else it will return a JSON representation of the native queries the given SQL statement translates to. It can be overridden per query with `useLegacyDruidExplain` context key.|false|
+|`druid.sql.planner.useLegacyDruidExplain`|If true, `EXPLAIN PLAN FOR` will return the explain plan in legacy format, else it will return a JSON representation of the native queries the given SQL statement translates to. It can be overridden per query with `useLegacyDruidExplain` context key.|true|
Review comment:
since the default has been flipped, I wonder if we should invert this setting and call something like `useJsonStringExplain" or something similar that defaults to false. (I just did a similar thing when swapping the default to keep old behavior in #11184, swapping a "use legacy" to "use new thing", which I think maybe is better since is a bit odd for "use legacy" to be the default of something)
--
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] LakshSingla commented on pull request #11908: Improve the output of SQL explain message
Posted by GitBox <gi...@apache.org>.
LakshSingla commented on pull request #11908:
URL: https://github.com/apache/druid/pull/11908#issuecomment-978202370
Just merged in the changes!
--
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] LakshSingla commented on pull request #11908: Improve the output of SQL explain message
Posted by GitBox <gi...@apache.org>.
LakshSingla commented on pull request #11908:
URL: https://github.com/apache/druid/pull/11908#issuecomment-975466328
Updated the description with pros and cons of the newer approach.
--
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 change in pull request #11908: Improve the output of SQL explain message
Posted by GitBox <gi...@apache.org>.
abhishekagarwal87 commented on a change in pull request #11908:
URL: https://github.com/apache/druid/pull/11908#discussion_r752349624
##########
File path: sql/src/main/java/org/apache/druid/sql/calcite/planner/DruidPlanner.java
##########
@@ -384,6 +399,46 @@ private PlannerResult planExplanation(
return new PlannerResult(resultsSupplier, getExplainStructType(rel.getCluster().getTypeFactory()));
}
+ /**
+ * This method doesn't utilize the Calcite's internal {@link RelOptUtil#dumpPlan} since that tends to be verbose
+ * and not indicative of the native Druid Queries which will get executed
+ * This method assumes that the Planner has converted the RelNodes to DruidRels, and thereby we can implictly cast it
+ *
+ * @param rel Instance of the root {@link DruidRel} which is formed by running the planner transformations on it
+ * @return A string representing an array of native queries that correspond to the given SQL query, in JSON format
+ * @throws JsonProcessingException
+ */
+ private String explainSqlPlanAsNativeQueries(DruidRel<?> rel) throws JsonProcessingException
+ {
+ // Only if rel is an instance of DruidUnionRel, do we run multiple native queries corresponding to single SQL query
+ // Also, DruidUnionRel can only be a top level node, so we don't need to check for this condition in the subsequent
+ // child nodes
+ List<DruidQuery> druidQueryList;
+ if (rel instanceof DruidUnionRel) {
+ druidQueryList = rel.getInputs().stream().map(childRel -> (DruidRel<?>) childRel).map(childRel -> {
+ if (childRel instanceof DruidUnionRel) {
+ log.error("DruidUnionRel can only be the outermost RelNode. This error shouldn't be encountered");
+ // TODO: Throw an error here
Review comment:
`ISE` makes sense to me.
--
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 #11908: Improve the output of SQL explain message
Posted by GitBox <gi...@apache.org>.
abhishekagarwal87 commented on pull request #11908:
URL: https://github.com/apache/druid/pull/11908#issuecomment-972967645
> Should the property `druid.sql.planner.useLegacyDruidExplain` and the overriding context key be documented somewhere?
Yes. You can put it in `docs/querying/query-context.md`
--
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 change in pull request #11908: Improve the output of SQL explain message
Posted by GitBox <gi...@apache.org>.
abhishekagarwal87 commented on a change in pull request #11908:
URL: https://github.com/apache/druid/pull/11908#discussion_r756007722
##########
File path: sql/src/main/java/org/apache/druid/sql/calcite/planner/DruidPlanner.java
##########
@@ -366,24 +374,76 @@ public void cleanup(EnumeratorIterator<Object[]> iterFromMake)
*/
private PlannerResult planExplanation(
final RelNode rel,
- final SqlExplain explain
+ final SqlExplain explain,
+ final boolean isDruidConventionExplanation
)
{
- final String explanation = RelOptUtil.dumpPlan("", rel, explain.getFormat(), explain.getDetailLevel());
+ String explanation = RelOptUtil.dumpPlan("", rel, explain.getFormat(), explain.getDetailLevel());
String resources;
try {
+ if (isDruidConventionExplanation && rel instanceof DruidRel) {
+ // Show the native queries instead of Calcite's explain if the legacy flag is turned off
+ if (!plannerContext.getPlannerConfig().isUseLegacyDruidExplain()) {
+ DruidRel<?> druidRel = (DruidRel<?>) rel;
+ explanation = explainSqlPlanAsNativeQueries(druidRel);
+ }
+ }
resources = jsonMapper.writeValueAsString(plannerContext.getResources());
}
catch (JsonProcessingException jpe) {
// this should never happen, we create the Resources here, not a user
log.error(jpe, "Encountered exception while serializing Resources for explain output");
resources = null;
}
+ catch (ISE ise) {
+ log.error(ise, "Unable to translate to a native Druid query. Resorting to legacy Druid explain plan");
+ resources = null;
+ }
final Supplier<Sequence<Object[]>> resultsSupplier = Suppliers.ofInstance(
Sequences.simple(ImmutableList.of(new Object[]{explanation, resources})));
return new PlannerResult(resultsSupplier, getExplainStructType(rel.getCluster().getTypeFactory()));
}
+ /**
+ * This method doesn't utilize the Calcite's internal {@link RelOptUtil#dumpPlan} since that tends to be verbose
+ * and not indicative of the native Druid Queries which will get executed
+ * This method assumes that the Planner has converted the RelNodes to DruidRels, and thereby we can implictly cast it
+ *
+ * @param rel Instance of the root {@link DruidRel} which is formed by running the planner transformations on it
+ * @return A string representing an array of native queries that correspond to the given SQL query, in JSON format
+ * @throws JsonProcessingException
+ */
+ private String explainSqlPlanAsNativeQueries(DruidRel<?> rel) throws JsonProcessingException
+ {
+ // Only if rel is an instance of DruidUnionRel, do we run multiple native queries corresponding to single SQL query
+ // Also, DruidUnionRel can only be a top level node, so we don't need to check for this condition in the subsequent
+ // child nodes
+ List<DruidQuery> druidQueryList;
+ if (rel instanceof DruidUnionRel) {
+ druidQueryList = rel.getInputs().stream().map(childRel -> (DruidRel<?>) childRel).map(childRel -> {
+ if (childRel instanceof DruidUnionRel) {
+ log.error("DruidUnionRel can only be the outermost RelNode. This error shouldn't be encountered");
+ throw new ISE("DruidUnionRel is only supported at the outermost RelNode.");
+ }
+ return childRel.toDruidQuery(false);
+ }).collect(Collectors.toList());
+ } else {
+ druidQueryList = ImmutableList.of(rel.toDruidQuery(false));
+ }
+
+ ArrayNode nativeQueriesArrayNode = jsonMapper.createArrayNode();
+
+ for (DruidQuery druidQuery : druidQueryList) {
+ Query<?> nativeQuery = druidQuery.getQuery();
+ ObjectNode objectNode = jsonMapper.createObjectNode();
+ objectNode.put("query", jsonMapper.convertValue(nativeQuery, ObjectNode.class));
+ objectNode.put("signature", druidQuery.getOutputRowSignature().toString());
Review comment:
should this be changed as well?
--
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] dbardbar commented on pull request #11908: Improve the output of SQL explain message
Posted by GitBox <gi...@apache.org>.
dbardbar commented on pull request #11908:
URL: https://github.com/apache/druid/pull/11908#issuecomment-971576221
@abhishekagarwal87 - for our use-case we want to extract from the native query on the part related to the 'WHERE' clause, so anyway we'll need some basic handling, even with the new code. With the new code, the extraction will be a bit easier.
--
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] LakshSingla commented on a change in pull request #11908: Improve the output of SQL explain message
Posted by GitBox <gi...@apache.org>.
LakshSingla commented on a change in pull request #11908:
URL: https://github.com/apache/druid/pull/11908#discussion_r747506946
##########
File path: sql/src/main/java/org/apache/druid/sql/calcite/planner/DruidPlanner.java
##########
@@ -384,6 +399,72 @@ private PlannerResult planExplanation(
return new PlannerResult(resultsSupplier, getExplainStructType(rel.getCluster().getTypeFactory()));
}
+ /**
+ * Construct a {@link PlannerResult} for an 'explain' query from a {@link RelNode}
+ */
+ private PlannerResult planDruidExplanation(
+ final DruidRel<?> rel
+ )
+ {
+ String resources = null;
+ String explanation = null;
+ try {
+ explanation = dumpDruidPlan(rel);
+ resources = jsonMapper.writeValueAsString(plannerContext.getResources());
+ }
+ catch (JsonProcessingException jpe) {
+ // this should never happen, we create the Resources here, not a user
+ log.error(jpe, "Encountered exception while serializing Resources for explain output");
+ }
+ final Supplier<Sequence<Object[]>> resultsSupplier = Suppliers.ofInstance(
+ Sequences.simple(ImmutableList.of(new Object[]{explanation, resources})));
+ return new PlannerResult(resultsSupplier, getExplainStructType(rel.getCluster().getTypeFactory()));
+ }
+
+ /**
+ * This method doesn't utilize the Calcite's internal {@link RelOptUtil#dumpPlan} since that tends to be verbose
+ * and not indicative of the native Druid Queries which will get executed
+ * This method assumes that the Planner has converted the RelNodes to DruidRels, and thereby we can implictly cast it
+ *
+ * @param rel Instance of the root {@link DruidRel} which is formed by running the planner transformations on it
+ * @return A string representing an array of native queries that correspond to the given SQL query, in JSON format
+ * @throws JsonProcessingException
+ */
+ public static String dumpDruidPlan(DruidRel<?> rel) throws JsonProcessingException
+ {
+ List<Map<String, Object>> nativeQueries;
+
+ // Only if rel is an instance of DruidUnionRel, do we run multiple native queries corresponding to single SQL query
Review comment:
Yes, AFAIK only the outermost union query is run as multiple native queries (and the results are simply concatenated), and rest all are implemented as a single native query. Reference: https://github.com/apache/druid/blob/223c5692a8292a210a770dc0702f75b4484ae347/sql/src/main/java/org/apache/druid/sql/calcite/rel/DruidUnionRel.java#L44.
The Query that you mentioned is getting translated to a scan query on a join dataSource with left as foo, and right as a GroupBy on the foo4 dataSource.
```
[
{
"query": {
"queryType": "scan",
"dataSource": {
"type": "join",
"left": {
"type": "table",
"name": "foo"
},
"right": {
"type": "query",
"query": {
"queryType": "groupBy",
"dataSource": {
"type": "table",
"name": "foo4"
},
"intervals": {
"type": "intervals",
"intervals": [
"-146136543-09-08T08:23:32.096Z/146140482-04-24T15:36:27.903Z"
]
},
"virtualColumns": [],
"filter": null,
"granularity": {
"type": "all"
},
"dimensions": [
{
"type": "default",
"dimension": "dim1",
"outputName": "d0",
"outputType": "STRING"
}
],
"aggregations": [],
"postAggregations": [],
"having": null,
"limitSpec": {
"type": "NoopLimitSpec"
},
"context": {
"defaultTimeout": 300000,
"maxScatterGatherBytes": 9223372036854776000,
"sqlCurrentTimestamp": "2000-01-01T00:00:00Z",
"sqlQueryId": "dummy",
"vectorize": "false",
"vectorizeVirtualColumns": "false"
},
"descending": false
}
},
"rightPrefix": "j0.",
"condition": "(\"dim1\" == \"j0.d0\")",
"joinType": "INNER",
"leftFilter": null
},
...
},
"signature": "{__time:LONG, cnt:LONG, dim1:STRING, dim2:STRING, dim3:STRING, m1:FLOAT, m2:DOUBLE, unique_dim1:COMPLEX<hyperUnique>}"
}
]
```
--
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] LakshSingla commented on a change in pull request #11908: Improve the output of SQL explain message
Posted by GitBox <gi...@apache.org>.
LakshSingla commented on a change in pull request #11908:
URL: https://github.com/apache/druid/pull/11908#discussion_r747505855
##########
File path: sql/src/test/java/org/apache/druid/sql/calcite/CalciteQueryTest.java
##########
@@ -8955,6 +8955,28 @@ public void testMaxSubqueryRows() throws Exception
);
}
+ @Test
+ public void testExplainUnionAllOfQueries() throws Exception
+ {
+ skipVectorize(); // Required otherwise results of the EXPLAIN change
+ String query = "EXPLAIN PLAN FOR (SELECT dim1 FROM druid.foo UNION ALL SELECT dim1 from druid.foo2)";
+ String resources = "[{\"name\":\"foo2\",\"type\":\"DATASOURCE\"},{\"name\":\"foo\",\"type\":\"DATASOURCE\"}]";
+ String explanation = "[{\"query\":{\"dataSource\":{\"type\":\"union\",\"dataSources\":[{\"type\":\"table\",\"name\":\"foo\"},{\"type\":\"table\",\"name\":\"foo2\"}]},\"intervals\":{\"type\":\"intervals\",\"intervals\":[\"-146136543-09-08T08:23:32.096Z/146140482-04-24T15:36:27.903Z\"]},\"virtualColumns\":[],\"resultFormat\":\"compactedList\",\"batchSize\":20480,\"order\":\"none\",\"filter\":null,\"columns\":[\"dim1\"],\"legacy\":false,\"context\":{\"defaultTimeout\":300000,\"maxScatterGatherBytes\":9223372036854775807,\"sqlCurrentTimestamp\":\"2000-01-01T00:00:00Z\",\"sqlQueryId\":\"dummy\",\"vectorize\":\"false\",\"vectorizeVirtualColumns\":\"false\"},\"descending\":false,\"granularity\":{\"type\":\"all\"}},\"signature\":\"{dim1:STRING}\"}]";
+ testQuery(query, ImmutableList.of(), ImmutableList.of(new Object[]{explanation, resources}));
+ }
+
+ @Test
+ public void testExplainUnionOfMultipleNativeQueries() throws Exception
+ {
+ skipVectorize();
+ String query = "EXPLAIN PLAN FOR ((SELECT dim1 FROM druid.foo) UNION ALL (SELECT dim1 FROM druid.foo4 where dim1 in (SELECT dim1 FROM druid.foo)))";
+ String explanation = "["
+ + "{\"query\":{\"dataSource\":{\"type\":\"table\",\"name\":\"foo\"},\"intervals\":{\"type\":\"intervals\",\"intervals\":[\"-146136543-09-08T08:23:32.096Z/146140482-04-24T15:36:27.903Z\"]},\"virtualColumns\":[],\"resultFormat\":\"compactedList\",\"batchSize\":20480,\"order\":\"none\",\"filter\":null,\"columns\":[\"dim1\"],\"legacy\":false,\"context\":{\"defaultTimeout\":300000,\"maxScatterGatherBytes\":9223372036854775807,\"sqlCurrentTimestamp\":\"2000-01-01T00:00:00Z\",\"sqlQueryId\":\"dummy\",\"vectorize\":\"false\",\"vectorizeVirtualColumns\":\"false\"},\"descending\":false,\"granularity\":{\"type\":\"all\"}},\"signature\":\"{dim1:STRING}\"}"
+ + ",{\"query\":{\"dataSource\":{\"type\":\"join\",\"left\":{\"type\":\"table\",\"name\":\"foo4\"},\"right\":{\"type\":\"query\",\"query\":{\"queryType\":\"groupBy\",\"dataSource\":{\"type\":\"table\",\"name\":\"foo\"},\"intervals\":{\"type\":\"intervals\",\"intervals\":[\"-146136543-09-08T08:23:32.096Z/146140482-04-24T15:36:27.903Z\"]},\"virtualColumns\":[],\"filter\":null,\"granularity\":{\"type\":\"all\"},\"dimensions\":[{\"type\":\"default\",\"dimension\":\"dim1\",\"outputName\":\"d0\",\"outputType\":\"STRING\"}],\"aggregations\":[],\"postAggregations\":[],\"having\":null,\"limitSpec\":{\"type\":\"NoopLimitSpec\"},\"context\":{\"defaultTimeout\":300000,\"maxScatterGatherBytes\":9223372036854775807,\"sqlCurrentTimestamp\":\"2000-01-01T00:00:00Z\",\"sqlQueryId\":\"dummy\",\"vectorize\":\"false\",\"vectorizeVirtualColumns\":\"false\"},\"descending\":false}},\"rightPrefix\":\"j0.\",\"condition\":\"(\\\"dim1\\\" == \\\"j0.d0\\\")\",\"joinType\":\"INNER\",\"lef
tFilter\":null},\"intervals\":{\"type\":\"intervals\",\"intervals\":[\"-146136543-09-08T08:23:32.096Z/146140482-04-24T15:36:27.903Z\"]},\"virtualColumns\":[],\"resultFormat\":\"compactedList\",\"batchSize\":20480,\"order\":\"none\",\"filter\":null,\"columns\":[\"dim1\"],\"legacy\":false,\"context\":{\"defaultTimeout\":300000,\"maxScatterGatherBytes\":9223372036854775807,\"sqlCurrentTimestamp\":\"2000-01-01T00:00:00Z\",\"sqlQueryId\":\"dummy\",\"vectorize\":\"false\",\"vectorizeVirtualColumns\":\"false\"},\"descending\":false,\"granularity\":{\"type\":\"all\"}},\"signature\":\"{dim1:STRING}\"}]";
+ String resources = "[{\"name\":\"foo4\",\"type\":\"DATASOURCE\"},{\"name\":\"foo\",\"type\":\"DATASOURCE\"}]";
+ testQuery(query, ImmutableList.of(), ImmutableList.of(new Object[]{explanation, resources}));
+ }
+
@Test
public void testExplainExactCountDistinctOfSemiJoinResult() throws Exception
{
Review comment:
I haven't yet edited the current test cases. I wanted an opinion on the output before modifying the existing test cases.
--
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] dbardbar commented on pull request #11908: Improve the output of SQL explain message
Posted by GitBox <gi...@apache.org>.
dbardbar commented on pull request #11908:
URL: https://github.com/apache/druid/pull/11908#issuecomment-969914136
@LakshSingla - just a quick question - will this be backward-compatible?
I have a use-case that we use the EXPLAIN to convert SQL to native queries automatically, so I'm wondering if I'll need to adapt my code to the new proposed format.
--
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 #11908: Improve the output of SQL explain message
Posted by GitBox <gi...@apache.org>.
abhishekagarwal87 commented on pull request #11908:
URL: https://github.com/apache/druid/pull/11908#issuecomment-970041741
hmm. I guess you would still want to rid of the extra fields such as row signature etc
--
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] LakshSingla commented on a change in pull request #11908: Improve the output of SQL explain message
Posted by GitBox <gi...@apache.org>.
LakshSingla commented on a change in pull request #11908:
URL: https://github.com/apache/druid/pull/11908#discussion_r756406517
##########
File path: docs/configuration/index.md
##########
@@ -1827,7 +1827,7 @@ The Druid SQL server is configured through the following properties on the Broke
|`druid.sql.planner.metadataSegmentCacheEnable`|Whether to keep a cache of published segments in broker. If true, broker polls coordinator in background to get segments from metadata store and maintains a local cache. If false, coordinator's REST API will be invoked when broker needs published segments info.|false|
|`druid.sql.planner.metadataSegmentPollPeriod`|How often to poll coordinator for published segments list if `druid.sql.planner.metadataSegmentCacheEnable` is set to true. Poll period is in milliseconds. |60000|
|`druid.sql.planner.authorizeSystemTablesDirectly`|If true, Druid authorizes queries against any of the system schema tables (`sys` in SQL) as `SYSTEM_TABLE` resources which require `READ` access, in addition to permissions based content filtering.|false|
-|`druid.sql.planner.useLegacyDruidExplain`|If true, `EXPLAIN PLAN FOR` will return the explain plan in legacy format, else it will return a JSON representation of the native queries the given SQL statement translates to. It can be overridden per query with `useLegacyDruidExplain` context key.|false|
+|`druid.sql.planner.useLegacyDruidExplain`|If true, `EXPLAIN PLAN FOR` will return the explain plan in legacy format, else it will return a JSON representation of the native queries the given SQL statement translates to. It can be overridden per query with `useLegacyDruidExplain` context key.|true|
Review comment:
If in the future, the newer explain plan is to be used, then I think `useLegacyDruidExplain` would work better, since we won't have to change the feature flag then. But it does look weird right now, since it hasn't been deprecated yet. I am fine with keeping it either way.
--
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] vogievetsky commented on pull request #11908: Improve the output of SQL explain message
Posted by GitBox <gi...@apache.org>.
vogievetsky commented on pull request #11908:
URL: https://github.com/apache/druid/pull/11908#issuecomment-977557250
Very excited for this change! I got 2 questions:
(1) is it possible to provide the signature as JSON and not as a string?
instead of `"signature": "{d0:STRING, a0:LONG}"`
provide it as `"signature": [{name: "d0", "type":"STRING"}, {"name":"a0", "type":"LONG"}]`
the signature strings are really annoying to parse and these signatures are really important (sometimes more so than then query)
(2) you included screenshots of the old format in the console query view, but how does it work with the explain dialog? Does it need to be updated as part of this PR or right afterwards?
List to be clear I am talking about this dialog:
As you can see the current dialog parses the old format thus relying on it. I would LOVE nothing more than to switch to this new format (providing pt.1) but does this PR break this dialog?
--
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] LakshSingla commented on pull request #11908: Improve the output of SQL explain message
Posted by GitBox <gi...@apache.org>.
LakshSingla commented on pull request #11908:
URL: https://github.com/apache/druid/pull/11908#issuecomment-977877937
Thanks for the comment @vogievetsky
1. As mentioned by @clintropolis, Gian's PR should make the RowSignature serializable. However, I have added a temporary `JsonValue` method till then which will help the Jackson serialize it, till those changes get merged. I have kept the output of my change is similar to Gian's change, so ideally no testcases need to updated, and the `DruidPlanner's` code also works without any tweaks. (This would introduce some merge conflicts in `RowSignature` and `RowSignatureTest` in https://github.com/apache/druid/pull/11959, but resolving it should be simple - discard the older changes for the new ones.
2. I haven't tested it out, but it should break the console output. I have changed the default behavior to show the __legacy__ output (i.e. the original behaviour), and therefore, no breaking changes are introduced in the PR. There is no requirement of updating the UI code immediately.
--
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] LakshSingla commented on a change in pull request #11908: Improve the output of SQL explain message
Posted by GitBox <gi...@apache.org>.
LakshSingla commented on a change in pull request #11908:
URL: https://github.com/apache/druid/pull/11908#discussion_r747506946
##########
File path: sql/src/main/java/org/apache/druid/sql/calcite/planner/DruidPlanner.java
##########
@@ -384,6 +399,72 @@ private PlannerResult planExplanation(
return new PlannerResult(resultsSupplier, getExplainStructType(rel.getCluster().getTypeFactory()));
}
+ /**
+ * Construct a {@link PlannerResult} for an 'explain' query from a {@link RelNode}
+ */
+ private PlannerResult planDruidExplanation(
+ final DruidRel<?> rel
+ )
+ {
+ String resources = null;
+ String explanation = null;
+ try {
+ explanation = dumpDruidPlan(rel);
+ resources = jsonMapper.writeValueAsString(plannerContext.getResources());
+ }
+ catch (JsonProcessingException jpe) {
+ // this should never happen, we create the Resources here, not a user
+ log.error(jpe, "Encountered exception while serializing Resources for explain output");
+ }
+ final Supplier<Sequence<Object[]>> resultsSupplier = Suppliers.ofInstance(
+ Sequences.simple(ImmutableList.of(new Object[]{explanation, resources})));
+ return new PlannerResult(resultsSupplier, getExplainStructType(rel.getCluster().getTypeFactory()));
+ }
+
+ /**
+ * This method doesn't utilize the Calcite's internal {@link RelOptUtil#dumpPlan} since that tends to be verbose
+ * and not indicative of the native Druid Queries which will get executed
+ * This method assumes that the Planner has converted the RelNodes to DruidRels, and thereby we can implictly cast it
+ *
+ * @param rel Instance of the root {@link DruidRel} which is formed by running the planner transformations on it
+ * @return A string representing an array of native queries that correspond to the given SQL query, in JSON format
+ * @throws JsonProcessingException
+ */
+ public static String dumpDruidPlan(DruidRel<?> rel) throws JsonProcessingException
+ {
+ List<Map<String, Object>> nativeQueries;
+
+ // Only if rel is an instance of DruidUnionRel, do we run multiple native queries corresponding to single SQL query
Review comment:
Yes, AFAIK only the outermost union query is run as multiple native queries (and the results are simply concatenated), and rest all are implemented as a single native query. Reference: https://github.com/apache/druid/blob/223c5692a8292a210a770dc0702f75b4484ae347/sql/src/main/java/org/apache/druid/sql/calcite/rel/DruidUnionRel.java#L44 There seems to be no processing/manipulation of the results in the SQL layer, apart from concatenation in the `DruidUnionRel` referenced, so it makes sense that for a most of the given SQL queries, we don't have multiple native queries.
The Query that you mentioned is getting translated to a scan query on a join dataSource with left as foo, and right as a GroupBy on the foo4 dataSource.
```
[
{
"query": {
"queryType": "scan",
"dataSource": {
"type": "join",
"left": {
"type": "table",
"name": "foo"
},
"right": {
"type": "query",
"query": {
"queryType": "groupBy",
"dataSource": {
"type": "table",
"name": "foo4"
},
"intervals": {
"type": "intervals",
"intervals": [
"-146136543-09-08T08:23:32.096Z/146140482-04-24T15:36:27.903Z"
]
},
"virtualColumns": [],
"filter": null,
"granularity": {
"type": "all"
},
"dimensions": [
{
"type": "default",
"dimension": "dim1",
"outputName": "d0",
"outputType": "STRING"
}
],
"aggregations": [],
"postAggregations": [],
"having": null,
"limitSpec": {
"type": "NoopLimitSpec"
},
"context": {
"defaultTimeout": 300000,
"maxScatterGatherBytes": 9223372036854776000,
"sqlCurrentTimestamp": "2000-01-01T00:00:00Z",
"sqlQueryId": "dummy",
"vectorize": "false",
"vectorizeVirtualColumns": "false"
},
"descending": false
}
},
"rightPrefix": "j0.",
"condition": "(\"dim1\" == \"j0.d0\")",
"joinType": "INNER",
"leftFilter": null
},
...
},
"signature": "{__time:LONG, cnt:LONG, dim1:STRING, dim2:STRING, dim3:STRING, m1:FLOAT, m2:DOUBLE, unique_dim1:COMPLEX<hyperUnique>}"
}
]
```
--
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] LakshSingla commented on a change in pull request #11908: Improve the output of SQL explain message
Posted by GitBox <gi...@apache.org>.
LakshSingla commented on a change in pull request #11908:
URL: https://github.com/apache/druid/pull/11908#discussion_r747798694
##########
File path: sql/src/main/java/org/apache/druid/sql/calcite/planner/DruidPlanner.java
##########
@@ -384,6 +399,72 @@ private PlannerResult planExplanation(
return new PlannerResult(resultsSupplier, getExplainStructType(rel.getCluster().getTypeFactory()));
}
+ /**
+ * Construct a {@link PlannerResult} for an 'explain' query from a {@link RelNode}
+ */
+ private PlannerResult planDruidExplanation(
+ final DruidRel<?> rel
+ )
+ {
+ String resources = null;
+ String explanation = null;
+ try {
+ explanation = dumpDruidPlan(rel);
+ resources = jsonMapper.writeValueAsString(plannerContext.getResources());
+ }
+ catch (JsonProcessingException jpe) {
+ // this should never happen, we create the Resources here, not a user
+ log.error(jpe, "Encountered exception while serializing Resources for explain output");
+ }
+ final Supplier<Sequence<Object[]>> resultsSupplier = Suppliers.ofInstance(
+ Sequences.simple(ImmutableList.of(new Object[]{explanation, resources})));
+ return new PlannerResult(resultsSupplier, getExplainStructType(rel.getCluster().getTypeFactory()));
+ }
+
+ /**
+ * This method doesn't utilize the Calcite's internal {@link RelOptUtil#dumpPlan} since that tends to be verbose
+ * and not indicative of the native Druid Queries which will get executed
+ * This method assumes that the Planner has converted the RelNodes to DruidRels, and thereby we can implictly cast it
+ *
+ * @param rel Instance of the root {@link DruidRel} which is formed by running the planner transformations on it
+ * @return A string representing an array of native queries that correspond to the given SQL query, in JSON format
+ * @throws JsonProcessingException
+ */
+ public static String dumpDruidPlan(DruidRel<?> rel) throws JsonProcessingException
Review comment:
Refactored the code, to reuse the original methods wherever possible.
As per the JSON Mapper used, I was trying to keep the code as similar to what was used in `explainTerms`. Makes sense to use the JSON Mapper given to the DruidPlanner.
--
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] LakshSingla commented on a change in pull request #11908: Improve the output of SQL explain message
Posted by GitBox <gi...@apache.org>.
LakshSingla commented on a change in pull request #11908:
URL: https://github.com/apache/druid/pull/11908#discussion_r756037412
##########
File path: sql/src/main/java/org/apache/druid/sql/calcite/planner/DruidPlanner.java
##########
@@ -366,24 +374,76 @@ public void cleanup(EnumeratorIterator<Object[]> iterFromMake)
*/
private PlannerResult planExplanation(
final RelNode rel,
- final SqlExplain explain
+ final SqlExplain explain,
+ final boolean isDruidConventionExplanation
)
{
- final String explanation = RelOptUtil.dumpPlan("", rel, explain.getFormat(), explain.getDetailLevel());
+ String explanation = RelOptUtil.dumpPlan("", rel, explain.getFormat(), explain.getDetailLevel());
String resources;
try {
+ if (isDruidConventionExplanation && rel instanceof DruidRel) {
+ // Show the native queries instead of Calcite's explain if the legacy flag is turned off
+ if (!plannerContext.getPlannerConfig().isUseLegacyDruidExplain()) {
+ DruidRel<?> druidRel = (DruidRel<?>) rel;
+ explanation = explainSqlPlanAsNativeQueries(druidRel);
+ }
+ }
resources = jsonMapper.writeValueAsString(plannerContext.getResources());
}
catch (JsonProcessingException jpe) {
// this should never happen, we create the Resources here, not a user
log.error(jpe, "Encountered exception while serializing Resources for explain output");
resources = null;
}
+ catch (ISE ise) {
+ log.error(ise, "Unable to translate to a native Druid query. Resorting to legacy Druid explain plan");
+ resources = null;
+ }
final Supplier<Sequence<Object[]>> resultsSupplier = Suppliers.ofInstance(
Sequences.simple(ImmutableList.of(new Object[]{explanation, resources})));
return new PlannerResult(resultsSupplier, getExplainStructType(rel.getCluster().getTypeFactory()));
}
+ /**
+ * This method doesn't utilize the Calcite's internal {@link RelOptUtil#dumpPlan} since that tends to be verbose
+ * and not indicative of the native Druid Queries which will get executed
+ * This method assumes that the Planner has converted the RelNodes to DruidRels, and thereby we can implictly cast it
+ *
+ * @param rel Instance of the root {@link DruidRel} which is formed by running the planner transformations on it
+ * @return A string representing an array of native queries that correspond to the given SQL query, in JSON format
+ * @throws JsonProcessingException
+ */
+ private String explainSqlPlanAsNativeQueries(DruidRel<?> rel) throws JsonProcessingException
+ {
+ // Only if rel is an instance of DruidUnionRel, do we run multiple native queries corresponding to single SQL query
+ // Also, DruidUnionRel can only be a top level node, so we don't need to check for this condition in the subsequent
+ // child nodes
+ List<DruidQuery> druidQueryList;
+ if (rel instanceof DruidUnionRel) {
+ druidQueryList = rel.getInputs().stream().map(childRel -> (DruidRel<?>) childRel).map(childRel -> {
+ if (childRel instanceof DruidUnionRel) {
+ log.error("DruidUnionRel can only be the outermost RelNode. This error shouldn't be encountered");
+ throw new ISE("DruidUnionRel is only supported at the outermost RelNode.");
+ }
+ return childRel.toDruidQuery(false);
+ }).collect(Collectors.toList());
+ } else {
+ druidQueryList = ImmutableList.of(rel.toDruidQuery(false));
+ }
+
+ ArrayNode nativeQueriesArrayNode = jsonMapper.createArrayNode();
+
+ for (DruidQuery druidQuery : druidQueryList) {
+ Query<?> nativeQuery = druidQuery.getQuery();
+ ObjectNode objectNode = jsonMapper.createObjectNode();
+ objectNode.put("query", jsonMapper.convertValue(nativeQuery, ObjectNode.class));
+ objectNode.put("signature", druidQuery.getOutputRowSignature().toString());
Review comment:
Added the JSON representation of the row signature. Output of the signature is similar to https://github.com/apache/druid/pull/11959/files#diff-baa9c67c8599846d87cd9205a5b18e6d407aad799c8457d542216129f22c07e2R90, so the testcases needn't be changed once Gian's changes gets merged.
--
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 change in pull request #11908: Improve the output of SQL explain message
Posted by GitBox <gi...@apache.org>.
abhishekagarwal87 commented on a change in pull request #11908:
URL: https://github.com/apache/druid/pull/11908#discussion_r754954301
##########
File path: sql/src/main/java/org/apache/druid/sql/calcite/planner/DruidPlanner.java
##########
@@ -366,24 +374,76 @@ public void cleanup(EnumeratorIterator<Object[]> iterFromMake)
*/
private PlannerResult planExplanation(
final RelNode rel,
- final SqlExplain explain
+ final SqlExplain explain,
+ final boolean isDruidConventionExplanation
)
{
- final String explanation = RelOptUtil.dumpPlan("", rel, explain.getFormat(), explain.getDetailLevel());
+ String explanation = RelOptUtil.dumpPlan("", rel, explain.getFormat(), explain.getDetailLevel());
String resources;
try {
+ if (isDruidConventionExplanation && rel instanceof DruidRel) {
+ // Show the native queries instead of Calcite's explain if the legacy flag is turned off
+ if (!plannerContext.getPlannerConfig().isUseLegacyDruidExplain()) {
+ DruidRel<?> druidRel = (DruidRel<?>) rel;
+ explanation = explainSqlPlanAsNativeQueries(druidRel);
+ }
+ }
resources = jsonMapper.writeValueAsString(plannerContext.getResources());
}
catch (JsonProcessingException jpe) {
// this should never happen, we create the Resources here, not a user
log.error(jpe, "Encountered exception while serializing Resources for explain output");
resources = null;
}
+ catch (ISE ise) {
+ log.error("Unable to translate to a native Druid query. Resorting to legacy Druid explain plan");
Review comment:
```suggestion
log.error(ise, "Unable to translate to a native Druid query. Resorting to legacy Druid explain plan");
```
--
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] LakshSingla edited a comment on pull request #11908: Improve the output of SQL explain message
Posted by GitBox <gi...@apache.org>.
LakshSingla edited a comment on pull request #11908:
URL: https://github.com/apache/druid/pull/11908#issuecomment-966326496
We can support both the formats simultaneously in Calcite's syntax using `SqlExplainLevel` and `SqlExplain.Depth`. From the Calcite's docs.
```
explain:
EXPLAIN PLAN
[ WITH TYPE | WITH IMPLEMENTATION | WITHOUT IMPLEMENTATION ]
[ EXCLUDING ATTRIBUTES | INCLUDING [ ALL ] ATTRIBUTES ]
[ AS JSON | AS XML | AS DOT ]
FOR { query | insert | update | merge | delete }
```
So potentially,
`EXPLAIN PLAN FOR {query}` can give older output and say `EXPLAIN PLAN EXCLUDING ATTRIBUTES {query}` can give the newer one. (Can be altered to whatever makes more sense semantically).
--
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] LakshSingla commented on a change in pull request #11908: Improve the output of SQL explain message
Posted by GitBox <gi...@apache.org>.
LakshSingla commented on a change in pull request #11908:
URL: https://github.com/apache/druid/pull/11908#discussion_r747508320
##########
File path: sql/src/main/java/org/apache/druid/sql/calcite/planner/DruidPlanner.java
##########
@@ -384,6 +399,72 @@ private PlannerResult planExplanation(
return new PlannerResult(resultsSupplier, getExplainStructType(rel.getCluster().getTypeFactory()));
}
+ /**
+ * Construct a {@link PlannerResult} for an 'explain' query from a {@link RelNode}
+ */
+ private PlannerResult planDruidExplanation(
+ final DruidRel<?> rel
+ )
+ {
+ String resources = null;
+ String explanation = null;
+ try {
+ explanation = dumpDruidPlan(rel);
+ resources = jsonMapper.writeValueAsString(plannerContext.getResources());
+ }
+ catch (JsonProcessingException jpe) {
+ // this should never happen, we create the Resources here, not a user
+ log.error(jpe, "Encountered exception while serializing Resources for explain output");
+ }
+ final Supplier<Sequence<Object[]>> resultsSupplier = Suppliers.ofInstance(
+ Sequences.simple(ImmutableList.of(new Object[]{explanation, resources})));
+ return new PlannerResult(resultsSupplier, getExplainStructType(rel.getCluster().getTypeFactory()));
+ }
+
+ /**
+ * This method doesn't utilize the Calcite's internal {@link RelOptUtil#dumpPlan} since that tends to be verbose
+ * and not indicative of the native Druid Queries which will get executed
+ * This method assumes that the Planner has converted the RelNodes to DruidRels, and thereby we can implictly cast it
+ *
+ * @param rel Instance of the root {@link DruidRel} which is formed by running the planner transformations on it
+ * @return A string representing an array of native queries that correspond to the given SQL query, in JSON format
+ * @throws JsonProcessingException
+ */
+ public static String dumpDruidPlan(DruidRel<?> rel) throws JsonProcessingException
+ {
+ List<Map<String, Object>> nativeQueries;
+
+ // Only if rel is an instance of DruidUnionRel, do we run multiple native queries corresponding to single SQL query
+ // Also, DruidUnionRel can only be a top level node, so we don't need to check for this condition in the subsequent
+ // child nodes
+ if (rel instanceof DruidUnionRel) {
+ nativeQueries = new ArrayList<>();
+ for (RelNode childRel : rel.getInputs()) {
+ DruidRel<?> druidChildRel = (DruidRel<?>) childRel;
+ if (childRel instanceof DruidUnionRel) { // Failsafe block, shouldn't be encountered
+ log.error("DruidUnionRel can only be the outermost Rel. This error shouldn't be encountered");
+ // TODO: Throw an error here
+ }
+ DruidQuery query1 = druidChildRel.toDruidQuery(false); // Finalize aggregations
+ nativeQueries.add(ImmutableMap.of(
+ "query",
+ query1.getQuery(),
+ "signature",
Review comment:
Yes, it is getting populated inside individual rel's `explainTerms` function. https://github.com/apache/druid/blob/223c5692a8292a210a770dc0702f75b4484ae347/sql/src/main/java/org/apache/druid/sql/calcite/rel/DruidQueryRel.java#L199
If not required this can be removed.
--
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] LakshSingla commented on a change in pull request #11908: Improve the output of SQL explain message
Posted by GitBox <gi...@apache.org>.
LakshSingla commented on a change in pull request #11908:
URL: https://github.com/apache/druid/pull/11908#discussion_r747508320
##########
File path: sql/src/main/java/org/apache/druid/sql/calcite/planner/DruidPlanner.java
##########
@@ -384,6 +399,72 @@ private PlannerResult planExplanation(
return new PlannerResult(resultsSupplier, getExplainStructType(rel.getCluster().getTypeFactory()));
}
+ /**
+ * Construct a {@link PlannerResult} for an 'explain' query from a {@link RelNode}
+ */
+ private PlannerResult planDruidExplanation(
+ final DruidRel<?> rel
+ )
+ {
+ String resources = null;
+ String explanation = null;
+ try {
+ explanation = dumpDruidPlan(rel);
+ resources = jsonMapper.writeValueAsString(plannerContext.getResources());
+ }
+ catch (JsonProcessingException jpe) {
+ // this should never happen, we create the Resources here, not a user
+ log.error(jpe, "Encountered exception while serializing Resources for explain output");
+ }
+ final Supplier<Sequence<Object[]>> resultsSupplier = Suppliers.ofInstance(
+ Sequences.simple(ImmutableList.of(new Object[]{explanation, resources})));
+ return new PlannerResult(resultsSupplier, getExplainStructType(rel.getCluster().getTypeFactory()));
+ }
+
+ /**
+ * This method doesn't utilize the Calcite's internal {@link RelOptUtil#dumpPlan} since that tends to be verbose
+ * and not indicative of the native Druid Queries which will get executed
+ * This method assumes that the Planner has converted the RelNodes to DruidRels, and thereby we can implictly cast it
+ *
+ * @param rel Instance of the root {@link DruidRel} which is formed by running the planner transformations on it
+ * @return A string representing an array of native queries that correspond to the given SQL query, in JSON format
+ * @throws JsonProcessingException
+ */
+ public static String dumpDruidPlan(DruidRel<?> rel) throws JsonProcessingException
+ {
+ List<Map<String, Object>> nativeQueries;
+
+ // Only if rel is an instance of DruidUnionRel, do we run multiple native queries corresponding to single SQL query
+ // Also, DruidUnionRel can only be a top level node, so we don't need to check for this condition in the subsequent
+ // child nodes
+ if (rel instanceof DruidUnionRel) {
+ nativeQueries = new ArrayList<>();
+ for (RelNode childRel : rel.getInputs()) {
+ DruidRel<?> druidChildRel = (DruidRel<?>) childRel;
+ if (childRel instanceof DruidUnionRel) { // Failsafe block, shouldn't be encountered
+ log.error("DruidUnionRel can only be the outermost Rel. This error shouldn't be encountered");
+ // TODO: Throw an error here
+ }
+ DruidQuery query1 = druidChildRel.toDruidQuery(false); // Finalize aggregations
+ nativeQueries.add(ImmutableMap.of(
+ "query",
+ query1.getQuery(),
+ "signature",
Review comment:
Yes, it is getting populated inside individual rel's `explainTerms` function. https://github.com/apache/druid/blob/223c5692a8292a210a770dc0702f75b4484ae347/sql/src/main/java/org/apache/druid/sql/calcite/rel/DruidQueryRel.java#L199
--
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 #11908: Improve the output of SQL explain message
Posted by GitBox <gi...@apache.org>.
abhishekagarwal87 merged pull request #11908:
URL: https://github.com/apache/druid/pull/11908
--
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] LakshSingla commented on a change in pull request #11908: Improve the output of SQL explain message
Posted by GitBox <gi...@apache.org>.
LakshSingla commented on a change in pull request #11908:
URL: https://github.com/apache/druid/pull/11908#discussion_r747506946
##########
File path: sql/src/main/java/org/apache/druid/sql/calcite/planner/DruidPlanner.java
##########
@@ -384,6 +399,72 @@ private PlannerResult planExplanation(
return new PlannerResult(resultsSupplier, getExplainStructType(rel.getCluster().getTypeFactory()));
}
+ /**
+ * Construct a {@link PlannerResult} for an 'explain' query from a {@link RelNode}
+ */
+ private PlannerResult planDruidExplanation(
+ final DruidRel<?> rel
+ )
+ {
+ String resources = null;
+ String explanation = null;
+ try {
+ explanation = dumpDruidPlan(rel);
+ resources = jsonMapper.writeValueAsString(plannerContext.getResources());
+ }
+ catch (JsonProcessingException jpe) {
+ // this should never happen, we create the Resources here, not a user
+ log.error(jpe, "Encountered exception while serializing Resources for explain output");
+ }
+ final Supplier<Sequence<Object[]>> resultsSupplier = Suppliers.ofInstance(
+ Sequences.simple(ImmutableList.of(new Object[]{explanation, resources})));
+ return new PlannerResult(resultsSupplier, getExplainStructType(rel.getCluster().getTypeFactory()));
+ }
+
+ /**
+ * This method doesn't utilize the Calcite's internal {@link RelOptUtil#dumpPlan} since that tends to be verbose
+ * and not indicative of the native Druid Queries which will get executed
+ * This method assumes that the Planner has converted the RelNodes to DruidRels, and thereby we can implictly cast it
+ *
+ * @param rel Instance of the root {@link DruidRel} which is formed by running the planner transformations on it
+ * @return A string representing an array of native queries that correspond to the given SQL query, in JSON format
+ * @throws JsonProcessingException
+ */
+ public static String dumpDruidPlan(DruidRel<?> rel) throws JsonProcessingException
+ {
+ List<Map<String, Object>> nativeQueries;
+
+ // Only if rel is an instance of DruidUnionRel, do we run multiple native queries corresponding to single SQL query
Review comment:
Yes, AFAIK only the outermost union query is run as multiple native queries (and the results are simply appended), and rest all are implemented as a single native query. Reference: https://github.com/apache/druid/blob/223c5692a8292a210a770dc0702f75b4484ae347/sql/src/main/java/org/apache/druid/sql/calcite/rel/DruidUnionRel.java#L44
--
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] LakshSingla commented on pull request #11908: Improve the output of SQL explain message
Posted by GitBox <gi...@apache.org>.
LakshSingla commented on pull request #11908:
URL: https://github.com/apache/druid/pull/11908#issuecomment-966892025
Commented out the `explanation` of the original tests in case we decide to follow up with the above suggestion. Will remove it if not required.
--
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] vogievetsky edited a comment on pull request #11908: Improve the output of SQL explain message
Posted by GitBox <gi...@apache.org>.
vogievetsky edited a comment on pull request #11908:
URL: https://github.com/apache/druid/pull/11908#issuecomment-977557250
Very excited for this change! I got 2 questions:
(1) is it possible to provide the signature as JSON and not as a string?
instead of `"signature": "{d0:STRING, a0:LONG}"`
provide it as `"signature": [{name: "d0", "type":"STRING"}, {"name":"a0", "type":"LONG"}]`
the signature strings are really annoying to parse and these signatures are really important (sometimes more so than the query)
(2) you included screenshots of the old format in the console query view, but how does it work with the explain dialog? Does it need to be updated as part of this PR or right afterwards?
Just to be clear I am talking about this dialog:
As you can see the current dialog parses the old format thus relying on it. I would LOVE nothing more than to switch to this new format (providing pt.1) but does this PR break this dialog? Ideally there would be a context flag to trigger the new format but the old format would be the default (at least for a few releases).
--
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 #11908: Improve the output of SQL explain message
Posted by GitBox <gi...@apache.org>.
clintropolis commented on pull request #11908:
URL: https://github.com/apache/druid/pull/11908#issuecomment-977562380
it looks like `RowSignature` is made JSON friendly in https://github.com/apache/druid/pull/11959/files#diff-efdda11ff1dc815218691ec3a5c8d487bd01cd5c297de95690b7b83cea1eb5b8R82 so might want to do that in a follow-up after #11959 goes in.
--
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 change in pull request #11908: Improve the output of SQL explain message
Posted by GitBox <gi...@apache.org>.
abhishekagarwal87 commented on a change in pull request #11908:
URL: https://github.com/apache/druid/pull/11908#discussion_r756561038
##########
File path: docs/configuration/index.md
##########
@@ -1827,7 +1827,7 @@ The Druid SQL server is configured through the following properties on the Broke
|`druid.sql.planner.metadataSegmentCacheEnable`|Whether to keep a cache of published segments in broker. If true, broker polls coordinator in background to get segments from metadata store and maintains a local cache. If false, coordinator's REST API will be invoked when broker needs published segments info.|false|
|`druid.sql.planner.metadataSegmentPollPeriod`|How often to poll coordinator for published segments list if `druid.sql.planner.metadataSegmentCacheEnable` is set to true. Poll period is in milliseconds. |60000|
|`druid.sql.planner.authorizeSystemTablesDirectly`|If true, Druid authorizes queries against any of the system schema tables (`sys` in SQL) as `SYSTEM_TABLE` resources which require `READ` access, in addition to permissions based content filtering.|false|
-|`druid.sql.planner.useLegacyDruidExplain`|If true, `EXPLAIN PLAN FOR` will return the explain plan in legacy format, else it will return a JSON representation of the native queries the given SQL statement translates to. It can be overridden per query with `useLegacyDruidExplain` context key.|false|
+|`druid.sql.planner.useLegacyDruidExplain`|If true, `EXPLAIN PLAN FOR` will return the explain plan in legacy format, else it will return a JSON representation of the native queries the given SQL statement translates to. It can be overridden per query with `useLegacyDruidExplain` context key.|true|
Review comment:
I agree. The choice is between legacy vs new plan so from that the flag name does sounds good to me.
##########
File path: docs/configuration/index.md
##########
@@ -1827,7 +1827,7 @@ The Druid SQL server is configured through the following properties on the Broke
|`druid.sql.planner.metadataSegmentCacheEnable`|Whether to keep a cache of published segments in broker. If true, broker polls coordinator in background to get segments from metadata store and maintains a local cache. If false, coordinator's REST API will be invoked when broker needs published segments info.|false|
|`druid.sql.planner.metadataSegmentPollPeriod`|How often to poll coordinator for published segments list if `druid.sql.planner.metadataSegmentCacheEnable` is set to true. Poll period is in milliseconds. |60000|
|`druid.sql.planner.authorizeSystemTablesDirectly`|If true, Druid authorizes queries against any of the system schema tables (`sys` in SQL) as `SYSTEM_TABLE` resources which require `READ` access, in addition to permissions based content filtering.|false|
-|`druid.sql.planner.useLegacyDruidExplain`|If true, `EXPLAIN PLAN FOR` will return the explain plan in legacy format, else it will return a JSON representation of the native queries the given SQL statement translates to. It can be overridden per query with `useLegacyDruidExplain` context key.|false|
+|`druid.sql.planner.useLegacyDruidExplain`|If true, `EXPLAIN PLAN FOR` will return the explain plan in legacy format, else it will return a JSON representation of the native queries the given SQL statement translates to. It can be overridden per query with `useLegacyDruidExplain` context key.|true|
Review comment:
I agree. The choice is between legacy vs new plan so from that the flag name does sound good to me.
--
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] LakshSingla commented on a change in pull request #11908: Improve the output of SQL explain message
Posted by GitBox <gi...@apache.org>.
LakshSingla commented on a change in pull request #11908:
URL: https://github.com/apache/druid/pull/11908#discussion_r756037412
##########
File path: sql/src/main/java/org/apache/druid/sql/calcite/planner/DruidPlanner.java
##########
@@ -366,24 +374,76 @@ public void cleanup(EnumeratorIterator<Object[]> iterFromMake)
*/
private PlannerResult planExplanation(
final RelNode rel,
- final SqlExplain explain
+ final SqlExplain explain,
+ final boolean isDruidConventionExplanation
)
{
- final String explanation = RelOptUtil.dumpPlan("", rel, explain.getFormat(), explain.getDetailLevel());
+ String explanation = RelOptUtil.dumpPlan("", rel, explain.getFormat(), explain.getDetailLevel());
String resources;
try {
+ if (isDruidConventionExplanation && rel instanceof DruidRel) {
+ // Show the native queries instead of Calcite's explain if the legacy flag is turned off
+ if (!plannerContext.getPlannerConfig().isUseLegacyDruidExplain()) {
+ DruidRel<?> druidRel = (DruidRel<?>) rel;
+ explanation = explainSqlPlanAsNativeQueries(druidRel);
+ }
+ }
resources = jsonMapper.writeValueAsString(plannerContext.getResources());
}
catch (JsonProcessingException jpe) {
// this should never happen, we create the Resources here, not a user
log.error(jpe, "Encountered exception while serializing Resources for explain output");
resources = null;
}
+ catch (ISE ise) {
+ log.error(ise, "Unable to translate to a native Druid query. Resorting to legacy Druid explain plan");
+ resources = null;
+ }
final Supplier<Sequence<Object[]>> resultsSupplier = Suppliers.ofInstance(
Sequences.simple(ImmutableList.of(new Object[]{explanation, resources})));
return new PlannerResult(resultsSupplier, getExplainStructType(rel.getCluster().getTypeFactory()));
}
+ /**
+ * This method doesn't utilize the Calcite's internal {@link RelOptUtil#dumpPlan} since that tends to be verbose
+ * and not indicative of the native Druid Queries which will get executed
+ * This method assumes that the Planner has converted the RelNodes to DruidRels, and thereby we can implictly cast it
+ *
+ * @param rel Instance of the root {@link DruidRel} which is formed by running the planner transformations on it
+ * @return A string representing an array of native queries that correspond to the given SQL query, in JSON format
+ * @throws JsonProcessingException
+ */
+ private String explainSqlPlanAsNativeQueries(DruidRel<?> rel) throws JsonProcessingException
+ {
+ // Only if rel is an instance of DruidUnionRel, do we run multiple native queries corresponding to single SQL query
+ // Also, DruidUnionRel can only be a top level node, so we don't need to check for this condition in the subsequent
+ // child nodes
+ List<DruidQuery> druidQueryList;
+ if (rel instanceof DruidUnionRel) {
+ druidQueryList = rel.getInputs().stream().map(childRel -> (DruidRel<?>) childRel).map(childRel -> {
+ if (childRel instanceof DruidUnionRel) {
+ log.error("DruidUnionRel can only be the outermost RelNode. This error shouldn't be encountered");
+ throw new ISE("DruidUnionRel is only supported at the outermost RelNode.");
+ }
+ return childRel.toDruidQuery(false);
+ }).collect(Collectors.toList());
+ } else {
+ druidQueryList = ImmutableList.of(rel.toDruidQuery(false));
+ }
+
+ ArrayNode nativeQueriesArrayNode = jsonMapper.createArrayNode();
+
+ for (DruidQuery druidQuery : druidQueryList) {
+ Query<?> nativeQuery = druidQuery.getQuery();
+ ObjectNode objectNode = jsonMapper.createObjectNode();
+ objectNode.put("query", jsonMapper.convertValue(nativeQuery, ObjectNode.class));
+ objectNode.put("signature", druidQuery.getOutputRowSignature().toString());
Review comment:
Added the JSON representation of the row signature. Output of the signature is similar to https://github.com/apache/druid/pull/11959/files#diff-baa9c67c8599846d87cd9205a5b18e6d407aad799c8457d542216129f22c07e2R90https://github.com/apache/druid/pull/11959/files#diff-baa9c67c8599846d87cd9205a5b18e6d407aad799c8457d542216129f22c07e2R90, so the testcases needn't be changed once Gian's changes gets merged.
--
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] LakshSingla commented on a change in pull request #11908: Improve the output of SQL explain message
Posted by GitBox <gi...@apache.org>.
LakshSingla commented on a change in pull request #11908:
URL: https://github.com/apache/druid/pull/11908#discussion_r756037412
##########
File path: sql/src/main/java/org/apache/druid/sql/calcite/planner/DruidPlanner.java
##########
@@ -366,24 +374,76 @@ public void cleanup(EnumeratorIterator<Object[]> iterFromMake)
*/
private PlannerResult planExplanation(
final RelNode rel,
- final SqlExplain explain
+ final SqlExplain explain,
+ final boolean isDruidConventionExplanation
)
{
- final String explanation = RelOptUtil.dumpPlan("", rel, explain.getFormat(), explain.getDetailLevel());
+ String explanation = RelOptUtil.dumpPlan("", rel, explain.getFormat(), explain.getDetailLevel());
String resources;
try {
+ if (isDruidConventionExplanation && rel instanceof DruidRel) {
+ // Show the native queries instead of Calcite's explain if the legacy flag is turned off
+ if (!plannerContext.getPlannerConfig().isUseLegacyDruidExplain()) {
+ DruidRel<?> druidRel = (DruidRel<?>) rel;
+ explanation = explainSqlPlanAsNativeQueries(druidRel);
+ }
+ }
resources = jsonMapper.writeValueAsString(plannerContext.getResources());
}
catch (JsonProcessingException jpe) {
// this should never happen, we create the Resources here, not a user
log.error(jpe, "Encountered exception while serializing Resources for explain output");
resources = null;
}
+ catch (ISE ise) {
+ log.error(ise, "Unable to translate to a native Druid query. Resorting to legacy Druid explain plan");
+ resources = null;
+ }
final Supplier<Sequence<Object[]>> resultsSupplier = Suppliers.ofInstance(
Sequences.simple(ImmutableList.of(new Object[]{explanation, resources})));
return new PlannerResult(resultsSupplier, getExplainStructType(rel.getCluster().getTypeFactory()));
}
+ /**
+ * This method doesn't utilize the Calcite's internal {@link RelOptUtil#dumpPlan} since that tends to be verbose
+ * and not indicative of the native Druid Queries which will get executed
+ * This method assumes that the Planner has converted the RelNodes to DruidRels, and thereby we can implictly cast it
+ *
+ * @param rel Instance of the root {@link DruidRel} which is formed by running the planner transformations on it
+ * @return A string representing an array of native queries that correspond to the given SQL query, in JSON format
+ * @throws JsonProcessingException
+ */
+ private String explainSqlPlanAsNativeQueries(DruidRel<?> rel) throws JsonProcessingException
+ {
+ // Only if rel is an instance of DruidUnionRel, do we run multiple native queries corresponding to single SQL query
+ // Also, DruidUnionRel can only be a top level node, so we don't need to check for this condition in the subsequent
+ // child nodes
+ List<DruidQuery> druidQueryList;
+ if (rel instanceof DruidUnionRel) {
+ druidQueryList = rel.getInputs().stream().map(childRel -> (DruidRel<?>) childRel).map(childRel -> {
+ if (childRel instanceof DruidUnionRel) {
+ log.error("DruidUnionRel can only be the outermost RelNode. This error shouldn't be encountered");
+ throw new ISE("DruidUnionRel is only supported at the outermost RelNode.");
+ }
+ return childRel.toDruidQuery(false);
+ }).collect(Collectors.toList());
+ } else {
+ druidQueryList = ImmutableList.of(rel.toDruidQuery(false));
+ }
+
+ ArrayNode nativeQueriesArrayNode = jsonMapper.createArrayNode();
+
+ for (DruidQuery druidQuery : druidQueryList) {
+ Query<?> nativeQuery = druidQuery.getQuery();
+ ObjectNode objectNode = jsonMapper.createObjectNode();
+ objectNode.put("query", jsonMapper.convertValue(nativeQuery, ObjectNode.class));
+ objectNode.put("signature", druidQuery.getOutputRowSignature().toString());
Review comment:
Added the JSON representation of the row signature. Output of the signature is similar to [test](https://github.com/apache/druid/pull/11959/files#diff-baa9c67c8599846d87cd9205a5b18e6d407aad799c8457d542216129f22c07e2R90) so the testcases needn't be changed once Gian's changes gets merged.
--
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] LakshSingla commented on pull request #11908: Improve the output of SQL explain message
Posted by GitBox <gi...@apache.org>.
LakshSingla commented on pull request #11908:
URL: https://github.com/apache/druid/pull/11908#issuecomment-972956504
Should the property `druid.sql.planner.useLegacyDruidExplain` and the overriding context key be documented somewhere?
--
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] kfaraz commented on a change in pull request #11908: Improve the output of SQL explain message
Posted by GitBox <gi...@apache.org>.
kfaraz commented on a change in pull request #11908:
URL: https://github.com/apache/druid/pull/11908#discussion_r747380495
##########
File path: sql/src/main/java/org/apache/druid/sql/calcite/planner/DruidPlanner.java
##########
@@ -384,6 +399,72 @@ private PlannerResult planExplanation(
return new PlannerResult(resultsSupplier, getExplainStructType(rel.getCluster().getTypeFactory()));
}
+ /**
+ * Construct a {@link PlannerResult} for an 'explain' query from a {@link RelNode}
+ */
+ private PlannerResult planDruidExplanation(
+ final DruidRel<?> rel
+ )
+ {
+ String resources = null;
+ String explanation = null;
+ try {
+ explanation = dumpDruidPlan(rel);
+ resources = jsonMapper.writeValueAsString(plannerContext.getResources());
+ }
+ catch (JsonProcessingException jpe) {
+ // this should never happen, we create the Resources here, not a user
+ log.error(jpe, "Encountered exception while serializing Resources for explain output");
+ }
+ final Supplier<Sequence<Object[]>> resultsSupplier = Suppliers.ofInstance(
+ Sequences.simple(ImmutableList.of(new Object[]{explanation, resources})));
+ return new PlannerResult(resultsSupplier, getExplainStructType(rel.getCluster().getTypeFactory()));
+ }
+
+ /**
+ * This method doesn't utilize the Calcite's internal {@link RelOptUtil#dumpPlan} since that tends to be verbose
+ * and not indicative of the native Druid Queries which will get executed
+ * This method assumes that the Planner has converted the RelNodes to DruidRels, and thereby we can implictly cast it
+ *
+ * @param rel Instance of the root {@link DruidRel} which is formed by running the planner transformations on it
+ * @return A string representing an array of native queries that correspond to the given SQL query, in JSON format
+ * @throws JsonProcessingException
+ */
+ public static String dumpDruidPlan(DruidRel<?> rel) throws JsonProcessingException
+ {
+ List<Map<String, Object>> nativeQueries;
+
+ // Only if rel is an instance of DruidUnionRel, do we run multiple native queries corresponding to single SQL query
Review comment:
What about sub-selects?
e.g. `SELECT * FROM abc WHERE col IN (SELECT col1 FROM xyz)`
Does this also run as a single native query?
##########
File path: sql/src/main/java/org/apache/druid/sql/calcite/planner/DruidPlanner.java
##########
@@ -384,6 +399,72 @@ private PlannerResult planExplanation(
return new PlannerResult(resultsSupplier, getExplainStructType(rel.getCluster().getTypeFactory()));
}
+ /**
+ * Construct a {@link PlannerResult} for an 'explain' query from a {@link RelNode}
+ */
+ private PlannerResult planDruidExplanation(
+ final DruidRel<?> rel
+ )
+ {
+ String resources = null;
+ String explanation = null;
+ try {
+ explanation = dumpDruidPlan(rel);
+ resources = jsonMapper.writeValueAsString(plannerContext.getResources());
+ }
+ catch (JsonProcessingException jpe) {
+ // this should never happen, we create the Resources here, not a user
+ log.error(jpe, "Encountered exception while serializing Resources for explain output");
+ }
+ final Supplier<Sequence<Object[]>> resultsSupplier = Suppliers.ofInstance(
+ Sequences.simple(ImmutableList.of(new Object[]{explanation, resources})));
+ return new PlannerResult(resultsSupplier, getExplainStructType(rel.getCluster().getTypeFactory()));
+ }
+
+ /**
+ * This method doesn't utilize the Calcite's internal {@link RelOptUtil#dumpPlan} since that tends to be verbose
+ * and not indicative of the native Druid Queries which will get executed
+ * This method assumes that the Planner has converted the RelNodes to DruidRels, and thereby we can implictly cast it
+ *
+ * @param rel Instance of the root {@link DruidRel} which is formed by running the planner transformations on it
+ * @return A string representing an array of native queries that correspond to the given SQL query, in JSON format
+ * @throws JsonProcessingException
+ */
+ public static String dumpDruidPlan(DruidRel<?> rel) throws JsonProcessingException
Review comment:
I think it would be better if this method returned the List of native queries instead of serializing and then returning it.
You are already doing serialization of the resources in method `planDruidExplanation`, it makes sense to move the serialization of the native queries as well to that method.
Also rename this method accordingly.
##########
File path: sql/src/main/java/org/apache/druid/sql/calcite/planner/DruidPlanner.java
##########
@@ -384,6 +399,72 @@ private PlannerResult planExplanation(
return new PlannerResult(resultsSupplier, getExplainStructType(rel.getCluster().getTypeFactory()));
}
+ /**
+ * Construct a {@link PlannerResult} for an 'explain' query from a {@link RelNode}
+ */
+ private PlannerResult planDruidExplanation(
+ final DruidRel<?> rel
+ )
+ {
+ String resources = null;
+ String explanation = null;
+ try {
+ explanation = dumpDruidPlan(rel);
+ resources = jsonMapper.writeValueAsString(plannerContext.getResources());
+ }
+ catch (JsonProcessingException jpe) {
+ // this should never happen, we create the Resources here, not a user
+ log.error(jpe, "Encountered exception while serializing Resources for explain output");
+ }
+ final Supplier<Sequence<Object[]>> resultsSupplier = Suppliers.ofInstance(
+ Sequences.simple(ImmutableList.of(new Object[]{explanation, resources})));
+ return new PlannerResult(resultsSupplier, getExplainStructType(rel.getCluster().getTypeFactory()));
+ }
+
+ /**
+ * This method doesn't utilize the Calcite's internal {@link RelOptUtil#dumpPlan} since that tends to be verbose
+ * and not indicative of the native Druid Queries which will get executed
+ * This method assumes that the Planner has converted the RelNodes to DruidRels, and thereby we can implictly cast it
+ *
+ * @param rel Instance of the root {@link DruidRel} which is formed by running the planner transformations on it
+ * @return A string representing an array of native queries that correspond to the given SQL query, in JSON format
+ * @throws JsonProcessingException
+ */
+ public static String dumpDruidPlan(DruidRel<?> rel) throws JsonProcessingException
Review comment:
I see that the only difference is that to serialize the native queries, you are using a different json mapper `rel.getQueryMaker().getJsonMapper()`.
Can we not use the same `jsonMapper` as is being used in `planDruidExplanation`?
##########
File path: sql/src/main/java/org/apache/druid/sql/calcite/planner/DruidPlanner.java
##########
@@ -384,6 +399,72 @@ private PlannerResult planExplanation(
return new PlannerResult(resultsSupplier, getExplainStructType(rel.getCluster().getTypeFactory()));
}
+ /**
+ * Construct a {@link PlannerResult} for an 'explain' query from a {@link RelNode}
+ */
+ private PlannerResult planDruidExplanation(
+ final DruidRel<?> rel
+ )
+ {
+ String resources = null;
+ String explanation = null;
+ try {
+ explanation = dumpDruidPlan(rel);
+ resources = jsonMapper.writeValueAsString(plannerContext.getResources());
+ }
+ catch (JsonProcessingException jpe) {
+ // this should never happen, we create the Resources here, not a user
+ log.error(jpe, "Encountered exception while serializing Resources for explain output");
+ }
+ final Supplier<Sequence<Object[]>> resultsSupplier = Suppliers.ofInstance(
+ Sequences.simple(ImmutableList.of(new Object[]{explanation, resources})));
+ return new PlannerResult(resultsSupplier, getExplainStructType(rel.getCluster().getTypeFactory()));
+ }
+
+ /**
+ * This method doesn't utilize the Calcite's internal {@link RelOptUtil#dumpPlan} since that tends to be verbose
+ * and not indicative of the native Druid Queries which will get executed
+ * This method assumes that the Planner has converted the RelNodes to DruidRels, and thereby we can implictly cast it
+ *
+ * @param rel Instance of the root {@link DruidRel} which is formed by running the planner transformations on it
+ * @return A string representing an array of native queries that correspond to the given SQL query, in JSON format
+ * @throws JsonProcessingException
+ */
+ public static String dumpDruidPlan(DruidRel<?> rel) throws JsonProcessingException
+ {
+ List<Map<String, Object>> nativeQueries;
+
+ // Only if rel is an instance of DruidUnionRel, do we run multiple native queries corresponding to single SQL query
+ // Also, DruidUnionRel can only be a top level node, so we don't need to check for this condition in the subsequent
+ // child nodes
+ if (rel instanceof DruidUnionRel) {
+ nativeQueries = new ArrayList<>();
+ for (RelNode childRel : rel.getInputs()) {
+ DruidRel<?> druidChildRel = (DruidRel<?>) childRel;
+ if (childRel instanceof DruidUnionRel) { // Failsafe block, shouldn't be encountered
+ log.error("DruidUnionRel can only be the outermost Rel. This error shouldn't be encountered");
+ // TODO: Throw an error here
+ }
+ DruidQuery query1 = druidChildRel.toDruidQuery(false); // Finalize aggregations
Review comment:
Nit: Rename this variable to `childQuery`.
##########
File path: sql/src/main/java/org/apache/druid/sql/calcite/planner/DruidPlanner.java
##########
@@ -384,6 +399,72 @@ private PlannerResult planExplanation(
return new PlannerResult(resultsSupplier, getExplainStructType(rel.getCluster().getTypeFactory()));
}
+ /**
+ * Construct a {@link PlannerResult} for an 'explain' query from a {@link RelNode}
+ */
+ private PlannerResult planDruidExplanation(
+ final DruidRel<?> rel
+ )
+ {
+ String resources = null;
+ String explanation = null;
+ try {
+ explanation = dumpDruidPlan(rel);
+ resources = jsonMapper.writeValueAsString(plannerContext.getResources());
+ }
+ catch (JsonProcessingException jpe) {
+ // this should never happen, we create the Resources here, not a user
+ log.error(jpe, "Encountered exception while serializing Resources for explain output");
+ }
+ final Supplier<Sequence<Object[]>> resultsSupplier = Suppliers.ofInstance(
+ Sequences.simple(ImmutableList.of(new Object[]{explanation, resources})));
+ return new PlannerResult(resultsSupplier, getExplainStructType(rel.getCluster().getTypeFactory()));
+ }
+
+ /**
+ * This method doesn't utilize the Calcite's internal {@link RelOptUtil#dumpPlan} since that tends to be verbose
+ * and not indicative of the native Druid Queries which will get executed
+ * This method assumes that the Planner has converted the RelNodes to DruidRels, and thereby we can implictly cast it
+ *
+ * @param rel Instance of the root {@link DruidRel} which is formed by running the planner transformations on it
+ * @return A string representing an array of native queries that correspond to the given SQL query, in JSON format
+ * @throws JsonProcessingException
+ */
+ public static String dumpDruidPlan(DruidRel<?> rel) throws JsonProcessingException
+ {
+ List<Map<String, Object>> nativeQueries;
+
+ // Only if rel is an instance of DruidUnionRel, do we run multiple native queries corresponding to single SQL query
+ // Also, DruidUnionRel can only be a top level node, so we don't need to check for this condition in the subsequent
+ // child nodes
+ if (rel instanceof DruidUnionRel) {
+ nativeQueries = new ArrayList<>();
+ for (RelNode childRel : rel.getInputs()) {
+ DruidRel<?> druidChildRel = (DruidRel<?>) childRel;
+ if (childRel instanceof DruidUnionRel) { // Failsafe block, shouldn't be encountered
+ log.error("DruidUnionRel can only be the outermost Rel. This error shouldn't be encountered");
+ // TODO: Throw an error here
+ }
+ DruidQuery query1 = druidChildRel.toDruidQuery(false); // Finalize aggregations
+ nativeQueries.add(ImmutableMap.of(
+ "query",
+ query1.getQuery(),
+ "signature",
Review comment:
I am not sure why we need the `signature` field. Is it also present in the existing EXPLAIN PLAN output?
##########
File path: sql/src/test/java/org/apache/druid/sql/calcite/CalciteQueryTest.java
##########
@@ -8955,6 +8955,28 @@ public void testMaxSubqueryRows() throws Exception
);
}
+ @Test
+ public void testExplainUnionAllOfQueries() throws Exception
+ {
+ skipVectorize(); // Required otherwise results of the EXPLAIN change
+ String query = "EXPLAIN PLAN FOR (SELECT dim1 FROM druid.foo UNION ALL SELECT dim1 from druid.foo2)";
+ String resources = "[{\"name\":\"foo2\",\"type\":\"DATASOURCE\"},{\"name\":\"foo\",\"type\":\"DATASOURCE\"}]";
+ String explanation = "[{\"query\":{\"dataSource\":{\"type\":\"union\",\"dataSources\":[{\"type\":\"table\",\"name\":\"foo\"},{\"type\":\"table\",\"name\":\"foo2\"}]},\"intervals\":{\"type\":\"intervals\",\"intervals\":[\"-146136543-09-08T08:23:32.096Z/146140482-04-24T15:36:27.903Z\"]},\"virtualColumns\":[],\"resultFormat\":\"compactedList\",\"batchSize\":20480,\"order\":\"none\",\"filter\":null,\"columns\":[\"dim1\"],\"legacy\":false,\"context\":{\"defaultTimeout\":300000,\"maxScatterGatherBytes\":9223372036854775807,\"sqlCurrentTimestamp\":\"2000-01-01T00:00:00Z\",\"sqlQueryId\":\"dummy\",\"vectorize\":\"false\",\"vectorizeVirtualColumns\":\"false\"},\"descending\":false,\"granularity\":{\"type\":\"all\"}},\"signature\":\"{dim1:STRING}\"}]";
+ testQuery(query, ImmutableList.of(), ImmutableList.of(new Object[]{explanation, resources}));
+ }
+
+ @Test
+ public void testExplainUnionOfMultipleNativeQueries() throws Exception
+ {
+ skipVectorize();
+ String query = "EXPLAIN PLAN FOR ((SELECT dim1 FROM druid.foo) UNION ALL (SELECT dim1 FROM druid.foo4 where dim1 in (SELECT dim1 FROM druid.foo)))";
+ String explanation = "["
+ + "{\"query\":{\"dataSource\":{\"type\":\"table\",\"name\":\"foo\"},\"intervals\":{\"type\":\"intervals\",\"intervals\":[\"-146136543-09-08T08:23:32.096Z/146140482-04-24T15:36:27.903Z\"]},\"virtualColumns\":[],\"resultFormat\":\"compactedList\",\"batchSize\":20480,\"order\":\"none\",\"filter\":null,\"columns\":[\"dim1\"],\"legacy\":false,\"context\":{\"defaultTimeout\":300000,\"maxScatterGatherBytes\":9223372036854775807,\"sqlCurrentTimestamp\":\"2000-01-01T00:00:00Z\",\"sqlQueryId\":\"dummy\",\"vectorize\":\"false\",\"vectorizeVirtualColumns\":\"false\"},\"descending\":false,\"granularity\":{\"type\":\"all\"}},\"signature\":\"{dim1:STRING}\"}"
+ + ",{\"query\":{\"dataSource\":{\"type\":\"join\",\"left\":{\"type\":\"table\",\"name\":\"foo4\"},\"right\":{\"type\":\"query\",\"query\":{\"queryType\":\"groupBy\",\"dataSource\":{\"type\":\"table\",\"name\":\"foo\"},\"intervals\":{\"type\":\"intervals\",\"intervals\":[\"-146136543-09-08T08:23:32.096Z/146140482-04-24T15:36:27.903Z\"]},\"virtualColumns\":[],\"filter\":null,\"granularity\":{\"type\":\"all\"},\"dimensions\":[{\"type\":\"default\",\"dimension\":\"dim1\",\"outputName\":\"d0\",\"outputType\":\"STRING\"}],\"aggregations\":[],\"postAggregations\":[],\"having\":null,\"limitSpec\":{\"type\":\"NoopLimitSpec\"},\"context\":{\"defaultTimeout\":300000,\"maxScatterGatherBytes\":9223372036854775807,\"sqlCurrentTimestamp\":\"2000-01-01T00:00:00Z\",\"sqlQueryId\":\"dummy\",\"vectorize\":\"false\",\"vectorizeVirtualColumns\":\"false\"},\"descending\":false}},\"rightPrefix\":\"j0.\",\"condition\":\"(\\\"dim1\\\" == \\\"j0.d0\\\")\",\"joinType\":\"INNER\",\"lef
tFilter\":null},\"intervals\":{\"type\":\"intervals\",\"intervals\":[\"-146136543-09-08T08:23:32.096Z/146140482-04-24T15:36:27.903Z\"]},\"virtualColumns\":[],\"resultFormat\":\"compactedList\",\"batchSize\":20480,\"order\":\"none\",\"filter\":null,\"columns\":[\"dim1\"],\"legacy\":false,\"context\":{\"defaultTimeout\":300000,\"maxScatterGatherBytes\":9223372036854775807,\"sqlCurrentTimestamp\":\"2000-01-01T00:00:00Z\",\"sqlQueryId\":\"dummy\",\"vectorize\":\"false\",\"vectorizeVirtualColumns\":\"false\"},\"descending\":false,\"granularity\":{\"type\":\"all\"}},\"signature\":\"{dim1:STRING}\"}]";
+ String resources = "[{\"name\":\"foo4\",\"type\":\"DATASOURCE\"},{\"name\":\"foo\",\"type\":\"DATASOURCE\"}]";
+ testQuery(query, ImmutableList.of(), ImmutableList.of(new Object[]{explanation, resources}));
+ }
+
@Test
public void testExplainExactCountDistinctOfSemiJoinResult() throws Exception
{
Review comment:
As the other tests have not changed, I assume this means that the output of EXPLAIN PLAN for most use cases will remain 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
[GitHub] [druid] LakshSingla commented on pull request #11908: Improve the output of SQL explain message
Posted by GitBox <gi...@apache.org>.
LakshSingla commented on pull request #11908:
URL: https://github.com/apache/druid/pull/11908#issuecomment-966326496
We can support both the formats simultaneously in Calcite's syntax using `SqlExplainLevel` and `SqlExplain.Depth`. I have added.
```
explain:
EXPLAIN PLAN
[ WITH TYPE | WITH IMPLEMENTATION | WITHOUT IMPLEMENTATION ]
[ EXCLUDING ATTRIBUTES | INCLUDING [ ALL ] ATTRIBUTES ]
[ AS JSON | AS XML | AS DOT ]
FOR { query | insert | update | merge | delete }
```
So potentially,
`EXPLAIN PLAN FOR {query}` can give older output and say `EXPLAIN PLAN EXCLUDING ATTRIBUTES {query}` can give the newer one. (Whichever makes mores sense semantically)
--
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] LakshSingla commented on a change in pull request #11908: Improve the output of SQL explain message
Posted by GitBox <gi...@apache.org>.
LakshSingla commented on a change in pull request #11908:
URL: https://github.com/apache/druid/pull/11908#discussion_r747506946
##########
File path: sql/src/main/java/org/apache/druid/sql/calcite/planner/DruidPlanner.java
##########
@@ -384,6 +399,72 @@ private PlannerResult planExplanation(
return new PlannerResult(resultsSupplier, getExplainStructType(rel.getCluster().getTypeFactory()));
}
+ /**
+ * Construct a {@link PlannerResult} for an 'explain' query from a {@link RelNode}
+ */
+ private PlannerResult planDruidExplanation(
+ final DruidRel<?> rel
+ )
+ {
+ String resources = null;
+ String explanation = null;
+ try {
+ explanation = dumpDruidPlan(rel);
+ resources = jsonMapper.writeValueAsString(plannerContext.getResources());
+ }
+ catch (JsonProcessingException jpe) {
+ // this should never happen, we create the Resources here, not a user
+ log.error(jpe, "Encountered exception while serializing Resources for explain output");
+ }
+ final Supplier<Sequence<Object[]>> resultsSupplier = Suppliers.ofInstance(
+ Sequences.simple(ImmutableList.of(new Object[]{explanation, resources})));
+ return new PlannerResult(resultsSupplier, getExplainStructType(rel.getCluster().getTypeFactory()));
+ }
+
+ /**
+ * This method doesn't utilize the Calcite's internal {@link RelOptUtil#dumpPlan} since that tends to be verbose
+ * and not indicative of the native Druid Queries which will get executed
+ * This method assumes that the Planner has converted the RelNodes to DruidRels, and thereby we can implictly cast it
+ *
+ * @param rel Instance of the root {@link DruidRel} which is formed by running the planner transformations on it
+ * @return A string representing an array of native queries that correspond to the given SQL query, in JSON format
+ * @throws JsonProcessingException
+ */
+ public static String dumpDruidPlan(DruidRel<?> rel) throws JsonProcessingException
+ {
+ List<Map<String, Object>> nativeQueries;
+
+ // Only if rel is an instance of DruidUnionRel, do we run multiple native queries corresponding to single SQL query
Review comment:
Yes, AFAIK only the outermost union query is run as multiple native queries (and the results are simply concatenated), and rest all are implemented as a single native query. Reference: https://github.com/apache/druid/blob/223c5692a8292a210a770dc0702f75b4484ae347/sql/src/main/java/org/apache/druid/sql/calcite/rel/DruidUnionRel.java#L44. There seems to be no processing/manipulation of the results in the SQL layer, apart from concatenation in the `DruidUnionRel` referenced, so it makes sense that for a most of the given SQL queries, we don't have multiple native queries.
The Query that you mentioned is getting translated to a scan query on a join dataSource with left as foo, and right as a GroupBy on the foo4 dataSource.
```
[
{
"query": {
"queryType": "scan",
"dataSource": {
"type": "join",
"left": {
"type": "table",
"name": "foo"
},
"right": {
"type": "query",
"query": {
"queryType": "groupBy",
"dataSource": {
"type": "table",
"name": "foo4"
},
"intervals": {
"type": "intervals",
"intervals": [
"-146136543-09-08T08:23:32.096Z/146140482-04-24T15:36:27.903Z"
]
},
"virtualColumns": [],
"filter": null,
"granularity": {
"type": "all"
},
"dimensions": [
{
"type": "default",
"dimension": "dim1",
"outputName": "d0",
"outputType": "STRING"
}
],
"aggregations": [],
"postAggregations": [],
"having": null,
"limitSpec": {
"type": "NoopLimitSpec"
},
"context": {
"defaultTimeout": 300000,
"maxScatterGatherBytes": 9223372036854776000,
"sqlCurrentTimestamp": "2000-01-01T00:00:00Z",
"sqlQueryId": "dummy",
"vectorize": "false",
"vectorizeVirtualColumns": "false"
},
"descending": false
}
},
"rightPrefix": "j0.",
"condition": "(\"dim1\" == \"j0.d0\")",
"joinType": "INNER",
"leftFilter": null
},
...
},
"signature": "{__time:LONG, cnt:LONG, dim1:STRING, dim2:STRING, dim3:STRING, m1:FLOAT, m2:DOUBLE, unique_dim1:COMPLEX<hyperUnique>}"
}
]
```
--
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 #11908: Improve the output of SQL explain message
Posted by GitBox <gi...@apache.org>.
abhishekagarwal87 commented on pull request #11908:
URL: https://github.com/apache/druid/pull/11908#issuecomment-969992324
@dbardbar - will you still need that custom code once this change is merged? This change is trying to solve the same problem of not being able to see the final 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.
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] dbardbar commented on pull request #11908: Improve the output of SQL explain message
Posted by GitBox <gi...@apache.org>.
dbardbar commented on pull request #11908:
URL: https://github.com/apache/druid/pull/11908#issuecomment-970059080
@LakshSingla - the native query can be extracted from the response today, but it does require some logic to extract it. Not the prettiest piece of code, but not that complicated.
If I understand correctly your proposal, it seems like it make our lives better, and will simplify our parser, but making a breaking change does have it's drawbacks.
If your new code will return the new response only based on some new flag (global, or on the request), then that would be great.
--
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 change in pull request #11908: Improve the output of SQL explain message
Posted by GitBox <gi...@apache.org>.
abhishekagarwal87 commented on a change in pull request #11908:
URL: https://github.com/apache/druid/pull/11908#discussion_r755888531
##########
File path: docs/configuration/index.md
##########
@@ -1827,6 +1827,7 @@ The Druid SQL server is configured through the following properties on the Broke
|`druid.sql.planner.metadataSegmentCacheEnable`|Whether to keep a cache of published segments in broker. If true, broker polls coordinator in background to get segments from metadata store and maintains a local cache. If false, coordinator's REST API will be invoked when broker needs published segments info.|false|
|`druid.sql.planner.metadataSegmentPollPeriod`|How often to poll coordinator for published segments list if `druid.sql.planner.metadataSegmentCacheEnable` is set to true. Poll period is in milliseconds. |60000|
|`druid.sql.planner.authorizeSystemTablesDirectly`|If true, Druid authorizes queries against any of the system schema tables (`sys` in SQL) as `SYSTEM_TABLE` resources which require `READ` access, in addition to permissions based content filtering.|false|
+|`druid.sql.planner.useLegacyDruidExplain`|If true, `EXPLAIN PLAN FOR` will return the explain plan in legacy format, else it will return a JSON representation of the native queries the given SQL statement translates to. It can be overridden per query with `useLegacyDruidExplain` context key.|false|
Review comment:
```suggestion
|`druid.sql.planner.useLegacyDruidExplain`|If true, `EXPLAIN PLAN FOR` will return the explain plan in legacy format, else it will return a JSON representation of the native queries the given SQL statement translates to. It can be overridden per query with `useLegacyDruidExplain` context key.|true|
```
--
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] LakshSingla commented on a change in pull request #11908: Improve the output of SQL explain message
Posted by GitBox <gi...@apache.org>.
LakshSingla commented on a change in pull request #11908:
URL: https://github.com/apache/druid/pull/11908#discussion_r756008795
##########
File path: sql/src/main/java/org/apache/druid/sql/calcite/planner/DruidPlanner.java
##########
@@ -366,24 +374,76 @@ public void cleanup(EnumeratorIterator<Object[]> iterFromMake)
*/
private PlannerResult planExplanation(
final RelNode rel,
- final SqlExplain explain
+ final SqlExplain explain,
+ final boolean isDruidConventionExplanation
)
{
- final String explanation = RelOptUtil.dumpPlan("", rel, explain.getFormat(), explain.getDetailLevel());
+ String explanation = RelOptUtil.dumpPlan("", rel, explain.getFormat(), explain.getDetailLevel());
String resources;
try {
+ if (isDruidConventionExplanation && rel instanceof DruidRel) {
+ // Show the native queries instead of Calcite's explain if the legacy flag is turned off
+ if (!plannerContext.getPlannerConfig().isUseLegacyDruidExplain()) {
+ DruidRel<?> druidRel = (DruidRel<?>) rel;
+ explanation = explainSqlPlanAsNativeQueries(druidRel);
+ }
+ }
resources = jsonMapper.writeValueAsString(plannerContext.getResources());
}
catch (JsonProcessingException jpe) {
// this should never happen, we create the Resources here, not a user
log.error(jpe, "Encountered exception while serializing Resources for explain output");
resources = null;
}
+ catch (ISE ise) {
+ log.error(ise, "Unable to translate to a native Druid query. Resorting to legacy Druid explain plan");
+ resources = null;
+ }
final Supplier<Sequence<Object[]>> resultsSupplier = Suppliers.ofInstance(
Sequences.simple(ImmutableList.of(new Object[]{explanation, resources})));
return new PlannerResult(resultsSupplier, getExplainStructType(rel.getCluster().getTypeFactory()));
}
+ /**
+ * This method doesn't utilize the Calcite's internal {@link RelOptUtil#dumpPlan} since that tends to be verbose
+ * and not indicative of the native Druid Queries which will get executed
+ * This method assumes that the Planner has converted the RelNodes to DruidRels, and thereby we can implictly cast it
+ *
+ * @param rel Instance of the root {@link DruidRel} which is formed by running the planner transformations on it
+ * @return A string representing an array of native queries that correspond to the given SQL query, in JSON format
+ * @throws JsonProcessingException
+ */
+ private String explainSqlPlanAsNativeQueries(DruidRel<?> rel) throws JsonProcessingException
+ {
+ // Only if rel is an instance of DruidUnionRel, do we run multiple native queries corresponding to single SQL query
+ // Also, DruidUnionRel can only be a top level node, so we don't need to check for this condition in the subsequent
+ // child nodes
+ List<DruidQuery> druidQueryList;
+ if (rel instanceof DruidUnionRel) {
+ druidQueryList = rel.getInputs().stream().map(childRel -> (DruidRel<?>) childRel).map(childRel -> {
+ if (childRel instanceof DruidUnionRel) {
+ log.error("DruidUnionRel can only be the outermost RelNode. This error shouldn't be encountered");
+ throw new ISE("DruidUnionRel is only supported at the outermost RelNode.");
+ }
+ return childRel.toDruidQuery(false);
+ }).collect(Collectors.toList());
+ } else {
+ druidQueryList = ImmutableList.of(rel.toDruidQuery(false));
+ }
+
+ ArrayNode nativeQueriesArrayNode = jsonMapper.createArrayNode();
+
+ for (DruidQuery druidQuery : druidQueryList) {
+ Query<?> nativeQuery = druidQuery.getQuery();
+ ObjectNode objectNode = jsonMapper.createObjectNode();
+ objectNode.put("query", jsonMapper.convertValue(nativeQuery, ObjectNode.class));
+ objectNode.put("signature", druidQuery.getOutputRowSignature().toString());
Review comment:
Yes, updating this along with the testcases.
--
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] LakshSingla commented on pull request #11908: Improve the output of SQL explain message
Posted by GitBox <gi...@apache.org>.
LakshSingla commented on pull request #11908:
URL: https://github.com/apache/druid/pull/11908#issuecomment-979071831
Minor comment:
Currently, deserializing the list of queries in JSON is done by converting it into instance of ArrayNode, since a simple List<Map<String, Object>> was losing the information about `queryType` [referenced code](https://github.com/apache/druid/pull/11908/files#diff-48bb0966fc1146b9b29e18a06ad3f0c38ba62fd2603905d33ecda842d06d98ddR453), [stackoverflow](https://stackoverflow.com/questions/34193177/why-does-jackson-polymorphic-serialization-not-work-in-lists) I have recently found a way to not use these by creating an inner class
```
private static class ExplainOutputNode
{
@JsonProperty
Query query;
@JsonProperty
RowSignature signature;
public ExplainOutputNode(RowSignature signature, Query query) {
this.signature = signature;
this.query = query;
}
}
String outputString = jsonMapper.writerFor(new TypeReference<List<ExplainOutputNode>>()
{
}).writeValueAsString(queryList);
```
which frees the referenced code from usage of `ArrayNode` and `ObjectNode`.
Should that be a preferred approach to the current one?
--
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 #11908: Improve the output of SQL explain message
Posted by GitBox <gi...@apache.org>.
abhishekagarwal87 commented on pull request #11908:
URL: https://github.com/apache/druid/pull/11908#issuecomment-979310785
Thank you @LakshSingla. I have merged your change.
--
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 #11908: Improve the output of SQL explain message
Posted by GitBox <gi...@apache.org>.
abhishekagarwal87 commented on pull request #11908:
URL: https://github.com/apache/druid/pull/11908#issuecomment-979080833
> Minor comment: Currently, deserializing the list of queries in JSON is done by converting it into instance of ArrayNode, since a simple List<Map<String, Object>> was losing the information about `queryType` [referenced code](https://github.com/apache/druid/pull/11908/files#diff-48bb0966fc1146b9b29e18a06ad3f0c38ba62fd2603905d33ecda842d06d98ddR453), [stackoverflow](https://stackoverflow.com/questions/34193177/why-does-jackson-polymorphic-serialization-not-work-in-lists) I have recently found a way to not use these by creating an inner class
>
> ```
> private static class ExplainOutputNode
> {
> @JsonProperty
> Query query;
>
> @JsonProperty
> RowSignature signature;
>
> public ExplainOutputNode(RowSignature signature, Query query) {
> this.signature = signature;
> this.query = query;
> }
> }
>
> String outputString = jsonMapper.writerFor(new TypeReference<List<ExplainOutputNode>>()
> {
> }).writeValueAsString(queryList);
> ```
>
> which frees the referenced code from usage of `ArrayNode` and `ObjectNode`. Should that be a preferred approach to the current one?
I think that the current approach is fine since the new approach means creating a new type. If in future, we do similar deserialization in other places, we can use a custom type specifically for explain output.
--
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] LakshSingla commented on a change in pull request #11908: Improve the output of SQL explain message
Posted by GitBox <gi...@apache.org>.
LakshSingla commented on a change in pull request #11908:
URL: https://github.com/apache/druid/pull/11908#discussion_r752332641
##########
File path: sql/src/main/java/org/apache/druid/sql/calcite/planner/DruidPlanner.java
##########
@@ -384,6 +399,46 @@ private PlannerResult planExplanation(
return new PlannerResult(resultsSupplier, getExplainStructType(rel.getCluster().getTypeFactory()));
}
+ /**
+ * This method doesn't utilize the Calcite's internal {@link RelOptUtil#dumpPlan} since that tends to be verbose
+ * and not indicative of the native Druid Queries which will get executed
+ * This method assumes that the Planner has converted the RelNodes to DruidRels, and thereby we can implictly cast it
+ *
+ * @param rel Instance of the root {@link DruidRel} which is formed by running the planner transformations on it
+ * @return A string representing an array of native queries that correspond to the given SQL query, in JSON format
+ * @throws JsonProcessingException
+ */
+ private String explainSqlPlanAsNativeQueries(DruidRel<?> rel) throws JsonProcessingException
+ {
+ // Only if rel is an instance of DruidUnionRel, do we run multiple native queries corresponding to single SQL query
+ // Also, DruidUnionRel can only be a top level node, so we don't need to check for this condition in the subsequent
+ // child nodes
+ List<DruidQuery> druidQueryList;
+ if (rel instanceof DruidUnionRel) {
+ druidQueryList = rel.getInputs().stream().map(childRel -> (DruidRel<?>) childRel).map(childRel -> {
+ if (childRel instanceof DruidUnionRel) {
+ log.error("DruidUnionRel can only be the outermost RelNode. This error shouldn't be encountered");
+ // TODO: Throw an error here
Review comment:
Thanks for pointing this out. I am unsure as to what error could be thrown here, since we do not plan to encounter it. `UOE` or `ISE` seem like viable options.
--
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 change in pull request #11908: Improve the output of SQL explain message
Posted by GitBox <gi...@apache.org>.
abhishekagarwal87 commented on a change in pull request #11908:
URL: https://github.com/apache/druid/pull/11908#discussion_r752253808
##########
File path: sql/src/main/java/org/apache/druid/sql/calcite/planner/DruidPlanner.java
##########
@@ -384,6 +399,46 @@ private PlannerResult planExplanation(
return new PlannerResult(resultsSupplier, getExplainStructType(rel.getCluster().getTypeFactory()));
}
+ /**
+ * This method doesn't utilize the Calcite's internal {@link RelOptUtil#dumpPlan} since that tends to be verbose
+ * and not indicative of the native Druid Queries which will get executed
+ * This method assumes that the Planner has converted the RelNodes to DruidRels, and thereby we can implictly cast it
+ *
+ * @param rel Instance of the root {@link DruidRel} which is formed by running the planner transformations on it
+ * @return A string representing an array of native queries that correspond to the given SQL query, in JSON format
+ * @throws JsonProcessingException
+ */
+ private String explainSqlPlanAsNativeQueries(DruidRel<?> rel) throws JsonProcessingException
+ {
+ // Only if rel is an instance of DruidUnionRel, do we run multiple native queries corresponding to single SQL query
+ // Also, DruidUnionRel can only be a top level node, so we don't need to check for this condition in the subsequent
+ // child nodes
+ List<DruidQuery> druidQueryList;
+ if (rel instanceof DruidUnionRel) {
+ druidQueryList = rel.getInputs().stream().map(childRel -> (DruidRel<?>) childRel).map(childRel -> {
+ if (childRel instanceof DruidUnionRel) {
+ log.error("DruidUnionRel can only be the outermost RelNode. This error shouldn't be encountered");
+ // TODO: Throw an error here
Review comment:
should you throw an error here?
--
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] LakshSingla commented on pull request #11908: Improve the output of SQL explain message
Posted by GitBox <gi...@apache.org>.
LakshSingla commented on pull request #11908:
URL: https://github.com/apache/druid/pull/11908#issuecomment-972839405
In accordance with the above suggestions, I have added a config option in the `PlannerConfig` which will allow the user to switch between the explain plan outputs. It can also be overridden on a per query basis.
By default the newer output would be visible. This default behavior can be changed by setting `druid.sql.planner.useLegacyDruidExplain = true` (by default this is false now)
Irrespective of the default behavior set in the properties, the explain plan output can also be modified on a per query basis by setting the `useLegacyDruidExplain` to true or false in the query's context.
--
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