Add columnMappings to explain plan output#14187
Conversation
* add tests
abhishekrb19
left a comment
There was a problem hiding this comment.
LGTM overall. A few questions and comments:
- Will the new
columnMappingsbe backwards compatible with existing clients? Currently, this info is added as part ofPLANcolumn along withqueriesandsignature-- I think this should be ok as it's a new map entry, but would like to double check. - Can we update the
EXPLAIN PLANsample output in the followingREADMEto include the new info: https://github.com/apache/druid/blob/master/docs/querying/sql-translation.md#interpreting-explain-plan-output? - Since
columnMappingsinfo is only available in the native explain query mode (and not in the legacy Calcite mode), it'd be worth clarifying that in the PR summary or title, so it's evident in the next release notes.
.github/scripts/unit_tests_script.sh
Outdated
| --type jacoco \ | ||
| --line-coverage 50 \ | ||
| --branch-coverage 50 \ | ||
| --line-coverage 1 \ |
There was a problem hiding this comment.
Is this change intentional?
There was a problem hiding this comment.
Please do revert this back to 50, 50.
There was a problem hiding this comment.
yeah was intentional to get around the code coverage blocking the ITs from running, I will revert back for sure.
There was a problem hiding this comment.
If this change is to get around the coverage failure and run everything, I believe you could achieve that by running workflows directly from the source repo's branch, i.e., your fork
sql/src/main/java/org/apache/druid/sql/calcite/planner/QueryHandler.java
Show resolved
Hide resolved
sql/src/main/java/org/apache/druid/sql/calcite/planner/QueryUtils.java
Outdated
Show resolved
Hide resolved
sql/src/main/java/org/apache/druid/sql/calcite/planner/QueryUtils.java
Outdated
Show resolved
Hide resolved
gianm
left a comment
There was a problem hiding this comment.
The basic idea LGTM. I generally agree with @abhishekrb19's review comments.
.github/scripts/unit_tests_script.sh
Outdated
| --type jacoco \ | ||
| --line-coverage 50 \ | ||
| --branch-coverage 50 \ | ||
| --line-coverage 1 \ |
There was a problem hiding this comment.
Please do revert this back to 50, 50.
thanks @abhishekrb19 , I addressed your concerns here. Adding a new property to existing returned object should not break existing clients, unless they are bad clients 👿 |
|
dont merge until I revert the code coverage threshold. Will do so when all tests pass. |
This change adds
columnMappingsto the native explain query mode explain plan output. This field is a list containing the mapping of queryColumn name to outputColumn name.As an example for this rollup query (as given as an example in the MSQ docs)
The plan with added
columnMappingsfield is given as:This PR has: