Enable Estimated Stats on Query Plan for Instrumentation Monitoring#18682
Enable Estimated Stats on Query Plan for Instrumentation Monitoring#18682yingsu00 merged 3 commits intoprestodb:masterfrom
Conversation
5fa2734 to
d713726
Compare
d713726 to
53bb9fc
Compare
|
@aaneja could you please help give a first pass on this PR? Thanks! |
53bb9fc to
c12d0f3
Compare
|
@bot kick off tests |
c12d0f3 to
ab6bdcb
Compare
aaneja
left a comment
There was a problem hiding this comment.
Some minor comments, but LGTM otherwise
presto-tests/src/main/java/com/facebook/presto/tests/AbstractTestQueries.java
Outdated
Show resolved
Hide resolved
presto-main/src/main/java/com/facebook/presto/sql/planner/planPrinter/JsonRenderer.java
Show resolved
Hide resolved
presto-tests/src/test/java/com/facebook/presto/execution/TestEventListener.java
Show resolved
Hide resolved
presto-main/src/main/java/com/facebook/presto/util/GraphvizPrinter.java
Outdated
Show resolved
Hide resolved
presto-tests/src/test/java/com/facebook/presto/execution/TestEventListener.java
Show resolved
Hide resolved
93ac48d to
7db18ba
Compare
769bfad to
bf82e3b
Compare
presto-main/src/main/java/com/facebook/presto/sql/planner/planPrinter/JsonRenderer.java
Outdated
Show resolved
Hide resolved
presto-main/src/main/java/com/facebook/presto/sql/planner/planPrinter/PlanPrinter.java
Outdated
Show resolved
Hide resolved
e6eea28 to
9ac09ab
Compare
presto-main/src/main/java/com/facebook/presto/sql/analyzer/QueryExplainer.java
Outdated
Show resolved
Hide resolved
presto-main/src/main/java/com/facebook/presto/sql/planner/planPrinter/PlanPrinter.java
Outdated
Show resolved
Hide resolved
presto-main/src/main/java/com/facebook/presto/cost/PlanNodeStatsEstimate.java
Outdated
Show resolved
Hide resolved
presto-main/src/main/java/com/facebook/presto/sql/Serialization.java
Outdated
Show resolved
Hide resolved
presto-main/src/main/java/com/facebook/presto/sql/Serialization.java
Outdated
Show resolved
Hide resolved
presto-main/src/test/java/com/facebook/presto/sql/planner/planPrinter/TestJsonRenderer.java
Outdated
Show resolved
Hide resolved
presto-main/src/test/java/com/facebook/presto/sql/planner/planPrinter/TestJsonRenderer.java
Outdated
Show resolved
Hide resolved
presto-main/src/test/java/com/facebook/presto/sql/planner/planPrinter/TestJsonRenderer.java
Outdated
Show resolved
Hide resolved
presto-main/src/test/java/com/facebook/presto/sql/planner/planPrinter/TestJsonRenderer.java
Outdated
Show resolved
Hide resolved
presto-main/src/test/java/com/facebook/presto/sql/planner/planPrinter/TestJsonRenderer.java
Outdated
Show resolved
Hide resolved
9ac09ab to
30b4d4a
Compare
|
@yingsu00 thank you for the review. I have updated the before/after change. All comments are addressed. Could you please help take a look again? |
presto-main/src/main/java/com/facebook/presto/sql/planner/planPrinter/PlanPrinter.java
Outdated
Show resolved
Hide resolved
presto-main/src/main/java/com/facebook/presto/cost/PlanNodeStatsEstimate.java
Outdated
Show resolved
Hide resolved
presto-main/src/main/java/com/facebook/presto/sql/planner/planPrinter/PlanPrinter.java
Outdated
Show resolved
Hide resolved
presto-main/src/main/java/com/facebook/presto/sql/planner/planPrinter/PlanPrinter.java
Outdated
Show resolved
Hide resolved
presto-main/src/main/java/com/facebook/presto/sql/planner/planPrinter/PlanPrinter.java
Outdated
Show resolved
Hide resolved
presto-main/src/main/java/com/facebook/presto/sql/planner/planPrinter/PlanPrinter.java
Outdated
Show resolved
Hide resolved
presto-main/src/main/java/com/facebook/presto/util/GraphvizPrinter.java
Outdated
Show resolved
Hide resolved
presto-main/src/test/java/com/facebook/presto/sql/planner/planPrinter/TestJsonRenderer.java
Outdated
Show resolved
Hide resolved
885e8e4 to
3f4328c
Compare
3f4328c to
061ea67
Compare
presto-main/src/main/java/com/facebook/presto/sql/Serialization.java
Outdated
Show resolved
Hide resolved
presto-main/src/main/java/com/facebook/presto/sql/planner/BasePlanFragmenter.java
Outdated
Show resolved
Hide resolved
presto-main/src/main/java/com/facebook/presto/sql/planner/LocalExecutionPlanner.java
Outdated
Show resolved
Hide resolved
presto-main/src/test/java/com/facebook/presto/util/TestGraphvizPrinter.java
Outdated
Show resolved
Hide resolved
c05c968 to
d03908d
Compare
|
Thank you @yingsu00 for helping review the PR. All comments are addressed. could you please help give another pass? Thanks! |
d03908d to
b06bdf1
Compare
|
@prestodb/committers ping! |
b06bdf1 to
11ed45e
Compare
56c65e1 to
df3d362
Compare
presto-main/src/main/java/com/facebook/presto/sql/Serialization.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Is this paragraph still valid?
There was a problem hiding this comment.
The fix to avoid using createTestFunctionAndTypeManager in Serialization main code path is to create a json mapper provider using the SerDe function for VariableReferenceExpression and use this custom json codec for serialization. This does not need any special no-args handling on the testing code path, e.g. TestExplainVerification, the planCodec is bind with JsonObjectMapperProvider that uses VariableReferenceExpressionDeserializer.
df3d362 to
c488562
Compare
c488562 to
da88672
Compare
Test plan - (Please fill in how you tested your changes)
Enforce a testcase to verify the plan entirety with stats info using graphviz format and json format
Before Change:
After Change: