Use precomputed stats/cost estimates in distributed explain plans#11511
Use precomputed stats/cost estimates in distributed explain plans#11511rschlussel merged 6 commits intoprestodb:masterfrom
Conversation
arhimondr
left a comment
arhimondr
left a comment
There was a problem hiding this comment.
Made a first pass. Let me see it again once you address the comments.
There was a problem hiding this comment.
this.symbolStatistics = HashTreePMap.from(requireNonNull(symbolStatistics, "symbolStatistics is null"));
There was a problem hiding this comment.
I'm not going to inline because i want to follow the style of the rest of the class.
There was a problem hiding this comment.
This style is more verbose and more error prone. You can fix it in a separate commit, and than have your fields inlined. But that's up to you.
There was a problem hiding this comment.
Inline createPlan, or move it here.
There was a problem hiding this comment.
Inline all the requireNonNulls in a separate commit
There was a problem hiding this comment.
Remove functionRegistry and statsCalculator
There was a problem hiding this comment.
function registry ultimately gets used in PlanPrinter.formatFragment
presto-main/src/main/java/com/facebook/presto/sql/analyzer/QueryExplainer.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Why this change? Maybe extract it to a separate commit with an explanation?
There was a problem hiding this comment.
unintentional. I'll remove
There was a problem hiding this comment.
Pass the stats and costs maps instead
There was a problem hiding this comment.
Ultimately we don't want to have any references to StatsProvider or CostProvider in this class.
There was a problem hiding this comment.
All IS_NULL_OVERHEAD multiplications seems to deserve a separate commit
|
I love this change |
There was a problem hiding this comment.
or AstUtils.preOrder(node).forEach
There was a problem hiding this comment.
the (Plan's) constructor is not the best place to do this, especially since calculating stats may involve a lot, including network communication
There was a problem hiding this comment.
we use just stats very often (predominantly? i don't know)
There was a problem hiding this comment.
I don;t quite like this being part of the plan -- would it be possible to keep it separate and pass further down separately?
There was a problem hiding this comment.
Yeah, we can have a separate class StatsAndCosts that contains both stats and costs maps, and we can pass it all around as a single object. I think it will be even nicer.
rschlussel
force-pushed
the
explain-stats
branch
2 times, most recently
from
4e94be0 to
e5f1b46
Compare
There was a problem hiding this comment.
function registry ultimately gets used in PlanPrinter.formatFragment
There was a problem hiding this comment.
true. it's there for future support of groupid nodes, but seeing as there aren't any immediate plans to expand that out, I think it's fair to add it later when it's needed. I'll remove.
There was a problem hiding this comment.
good catch, thanks
There was a problem hiding this comment.
I see both, and I thought we prefer things that are in Java rather than guava when they're available. but happy to switch it.
There was a problem hiding this comment.
we use them interchangeably throughout the code. I think they are pretty equivalent as names.
presto-main/src/main/java/com/facebook/presto/sql/analyzer/QueryExplainer.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
moved to StatsAndCosts.Builder
arhimondr
left a comment
arhimondr
left a comment
There was a problem hiding this comment.
Make stats/cost estimates serializable
LGTM % nit
There was a problem hiding this comment.
nit: Just do this(outputRowCount, HashTreePMap.from(requireNonNull(symbolStatistics, "symbolStatistics is null"))) to avoud copy pasting the argument checks
arhimondr
left a comment
arhimondr
left a comment
There was a problem hiding this comment.
Pass pre-computed stats/costs through to plan fragments
Some comments
There was a problem hiding this comment.
No need to store UNKNOWN stats.
There was a problem hiding this comment.
No need to store UKNOWN costs
There was a problem hiding this comment.
nit: Move getters after the constructor
There was a problem hiding this comment.
Replace it with StatsAndCosts.empty()
There was a problem hiding this comment.
Replace it with StatsAndCosts.empty()
There was a problem hiding this comment.
Replace it with StatsAndCosts.empty()
There was a problem hiding this comment.
Once you remove the statsAndCosts field from the Plan you can simply pass StatsAndCosts.empty() here. Also don't forget to remove CostProvider costProvider = node -> UNKNOWN_COST;, as it will be not needed.
There was a problem hiding this comment.
I'll leave stats and cost with the plan, but the costProvider is already not needed. I took it out, but somehow it got added back while I was resolving conflicts during rebase. I'll remove :).
arhimondr
left a comment
arhimondr
left a comment
There was a problem hiding this comment.
Use precomputed stats/costs in distributed explains
Some comments
There was a problem hiding this comment.
It is just unused. An might be unused for a very long time.
There was a problem hiding this comment.
If you decided to change the name don't forget to fix the message
There was a problem hiding this comment.
I'm trying to distinguish it from the execution stats.
presto-main/src/main/java/com/facebook/presto/sql/planner/planPrinter/PlanPrinter.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
I would refactor this part to be like this
private void printPlanNodesStatsAndCost(int indent, PlanNode... nodes)
{
List<String> statsAndCosts = Arrays.stream(nodes)
.map(this::formatPlanNodeStatsAndCost)
.filter(Optional::isPresent)
.map(Optional::get)
.collect(toImmutableList());
if (!statsAndCosts.isEmpty()) {
print(indent, "Cost: %s", Joiner.on("/").join(statsAndCosts));
}
}
private Optional<String> formatPlanNodeStatsAndCost(PlanNode node)
{
PlanNodeStatsEstimate stats = estimatedStatsAndCosts.getStats().get(node.getId());
PlanNodeCostEstimate cost = estimatedStatsAndCosts.getCosts().get(node.getId());
if (stats == null || cost == null) {
return Optional.empty();
}
return Optional.of(String.format("{rows: %s (%s), cpu: %s, memory: %s, network: %s}",
formatAsLong(stats.getOutputRowCount()),
formatEstimateAsDataSize(stats.getOutputSizeInBytes(node.getOutputSymbols(), types)),
formatDouble(cost.getCpuCost()),
formatDouble(cost.getMemoryCost()),
formatDouble(cost.getNetworkCost())));
}
Also consider not printing stats if stats for some node is missing. If, for example, for ScanFilterAndProject, the stats are missing for Filter - the output may be confusing.
There was a problem hiding this comment.
I like that. I'm doing that with a slight modification that I'll still check for unknown because it's still possible for the Stats/Cost maps to contain UNKNOWN_STATS and I don't think we gain anything from enforcing that in StatsAndCosts itself.
arhimondr
left a comment
arhimondr
left a comment
There was a problem hiding this comment.
Inline requireNonNull in Plan and LogicalPlanner
LGTM. Thanks for doing this.
arhimondr
left a comment
arhimondr
left a comment
There was a problem hiding this comment.
Remove lookup from CostCalculator interface
LGTM, thanks for doing that
There was a problem hiding this comment.
Why is this segmented per fragment? The costs are per node, so they should be associated with the overall query plan.
There was a problem hiding this comment.
We had to attach the stats to the Fragment, so it is easier to pass it to the QueryMonitor and PlanPrinter.
Otherwise you need to toss stats around across many classes. SqlQueryExecution#analyzeQuery -> PlanRoot -> SqlQueryExecution#planDistribution -> SqlQueryScheduler -> StageInfo -> QueryStateMachine-> QueryInfo -> QueryMonitor
It can be simplified somehow, by taking the stats in the SqlQueryExecution#buildQueryInfo from the SqlQueryExecution#queryPlan, and storing the stats directly into the QueryInfo. But than the stats object has to be tossed around in the PlanPrinter.
There was a problem hiding this comment.
they are associated with the overall plan, but once you have a list of fragments, you need to still be able to get the stats and costs
There was a problem hiding this comment.
I talked to @martint offline, and we agreed that storing StatsAndCosts in PlanFragment is good for now.
There was a problem hiding this comment.
Why do these need to be json serializable? At what point do they leave the coordinator?
There was a problem hiding this comment.
It was done just so it can be included into the PlanFragment.
There was a problem hiding this comment.
Ok, so see my other comment about why are these being attached to the fragments vs the overall plan (since they are not a per-fragment entity)
There was a problem hiding this comment.
Even if we decided to do not attach this to PlanFragment, it still has to be serializable, since it hass to be passed to the QueryMonitor as part of the QueryInfo object, which is serizalizable.
There was a problem hiding this comment.
they don't, but fragments get serialized and deserialized somehow before explain. Honestly, I didn't look into why that happens, I just saw the serialization errors when I tried running EXPLAIN (TYPE DISTRIBUTED) and fixed them.
rschlussel
force-pushed
the
explain-stats
branch
from
e5f1b46 to
864f227
Compare
There was a problem hiding this comment.
nit: add a space before to separate statistic constants from the object fields
rschlussel
force-pushed
the
explain-stats
branch
from
864f227 to
3a20164
Compare
|
Thanks @arhimondr for the review. addressed comments, and i'll merge after the release. |
|
@rschlussel Could you please also change the @dain is working on a patch that auto-closes the transaction in case when query got canceled. He is getting failures in the |
Updated thanks |
|
@rschlussel Rebecca, would you update the PR description to show the new output of the |
|
@rschlussel |
@mbasmanova explain plans look basically the same. The difference is that plans from the QueryMonitor now also have stats. |
|
@rschlussel Event listeners would also have complete stats, correct? |
In order to pass stats/cost estimates to plan fragments, they need to be serializable.
This will be used to print estimated stats/costs in explains
Computing stats on the fly couldn't be used from the QueryMonitor because that gets called after the transaction completes.
It doesn't get used, and there aren't immediate plans to add support. Simplify things for now and when it becomes useful we can add it back.
This way stats computation remains within the transaction.
rschlussel
force-pushed
the
explain-stats
branch
from
db7a1eb to
5ab34ff
Compare
|
@rschlussel, were you able to update TestTpchDistributedStats also? |
|
@dain Yes, those are the metric comparator changes. |
Stats calculator fails on assertions for complex queries, thus it is not production ready yet. The prestodb#11511 changes the place where the stats are computed to be displayed in the final plan. Before this patch, the stats were computed in QueryMonitor, when printing a final plan. Before if the stats couldn't be computed for whatever reason, only the text plan generation would fail. Currently, when the stats calculator is invoked for every query during the initial planning, queries may be failing, even when the CBO is not used. This change disables stats calculator by default. It can be enabled back with a session property on per query basis.
Stats calculator fails on assertions for complex queries, thus it is not production ready yet. The #11511 changes the place where the stats are computed to be displayed in the final plan. Before this patch, the stats were computed in QueryMonitor, when printing a final plan. Before if the stats couldn't be computed for whatever reason, only the text plan generation would fail. Currently, when the stats calculator is invoked for every query during the initial planning, queries may be failing, even when the CBO is not used. This change disables stats calculator by default. It can be enabled back with a session property on per query basis.
8 participants
No description provided.