-
Notifications
You must be signed in to change notification settings - Fork 3.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add more statement attributes to explain plan result. #14391
Add more statement attributes to explain plan result. #14391
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice 🐻
sql/src/main/java/org/apache/druid/sql/calcite/planner/ExplainAttributes.java
Outdated
Show resolved
Hide resolved
sql/src/main/java/org/apache/druid/sql/calcite/planner/ExplainAttributes.java
Outdated
Show resolved
Hide resolved
sql/src/main/java/org/apache/druid/sql/calcite/planner/ExplainAttributes.java
Outdated
Show resolved
Hide resolved
sql/src/test/java/org/apache/druid/sql/avatica/DruidAvaticaHandlerTest.java
Outdated
Show resolved
Hide resolved
sql/src/test/java/org/apache/druid/sql/calcite/CalciteReplaceDmlTest.java
Show resolved
Hide resolved
Co-authored-by: Kashif Faraz <kashif.faraz@gmail.com>
Co-authored-by: Kashif Faraz <kashif.faraz@gmail.com>
…Attributes.java Co-authored-by: Kashif Faraz <kashif.faraz@gmail.com>
…krb19/incubator-druid into explain_plan_explicit_attributes
Thanks for the reviews! Since we're officially documenting attributes for the next release, I also added a new unit test to test the serialization part, so we don't accidentally change how things are serialized moving forward. |
@@ -107,6 +112,7 @@ public String getClusteredBy() | |||
*/ | |||
@Nullable | |||
@JsonProperty | |||
@JsonInclude(JsonInclude.Include.NON_NULL) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it work to have this annotation just once at the class level instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it seems like we do a mix of method-level and class-level annotations in the codebase. Since we already do @Nullable
consistently for all the methods in this class, I just stuck with the former.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🚀
This PR adds the following to the
ATTRIBUTES
column in the explain plan output:This PR leverages the work done in #14074, which added a new column
ATTRIBUTES
to encapsulate all the statement-related attributes.Release note
EXPLAIN PLAN
result includes a new columnATTRIBUTES
that describes the attributes of a query. Seedocs/querying/sql-translation.md
for more information and examples.Key changed/added classes in this PR
ExplainAttributes.java
ExplainAttributesTest.java
This PR has: