Provide estimates for partial and final topN in TopNStatsRule #15428
Conversation
raunaqmorarka
force-pushed
the
topn-stats
branch
from
293d496 to
f0eda09
Compare
| @@ -47,8 +47,13 @@ protected Optional<PlanNodeStatsEstimate> doCalculate(TopNNode node, StatsProvid | |||
| PlanNodeStatsEstimate sourceStats = statsProvider.getStats(node.getSource()); | |||
| double rowCount = sourceStats.getOutputRowCount(); | |||
|
|
|||
| if (node.getStep() != TopNNode.Step.SINGLE) { | |||
| return Optional.empty(); | |||
| /* CreatePartialTopN rule runs after ReorderJoins but before DetermineJoinDistributionType and DetermineSemiJoinDistributionType. | |||
There was a problem hiding this comment.
In my understanding, providing an estimate is a more flexible solution than moving rules around so that unestimated things are added to the plan after CBO rules.
We gain estimates in the output of EXPLAIN query and avoid adding another ordering requirement in PlanOptimizers by providing an estimate here.
| * We use the conservative estimate of sourceStats for partial topN so that final topN can be accurately estimated. | ||
| */ | ||
| if (node.getStep() == TopNNode.Step.PARTIAL) { | ||
| return Optional.of(sourceStats); |
There was a problem hiding this comment.
We could assume some minimal reasonable split size.
For example, "TOP 5" is going to reduce data heavily (except for a pretty uncommon situation where splits return up to 5 rows), and "TOP 1000000" is not going to reduce data much (splits are often less than million rows).
BTW, the comment here seems to assume partial TopN is applied directly in the source stage, which may or may not be the same.
Does it matter? Can we improve the comment?
There was a problem hiding this comment.
Yeah, I don't think partial TopN will always be in source stage
There was a problem hiding this comment.
I've updated the comment here to clarify that we're talking about an example scenario, not implying that partialTopN can only be in source stage.
For HASH stages maybe it could be estimated as (input / (hash partitions count * task concurrency)) * N, but I don't think we know the stage partitioning at this point.
I've updated the partial topN estimate calculation to assume 1 million rows input.
|
could you rebase please? |
raunaqmorarka
force-pushed
the
topn-stats
branch
2 times, most recently
from
3aa3c01 to
7ff3142
Compare
raunaqmorarka
force-pushed
the
topn-stats
branch
from
7ff3142 to
de46ef9
Compare
| @@ -47,8 +48,16 @@ protected Optional<PlanNodeStatsEstimate> doCalculate(TopNNode node, StatsProvid | |||
| PlanNodeStatsEstimate sourceStats = statsProvider.getStats(node.getSource()); | |||
| double rowCount = sourceStats.getOutputRowCount(); | |||
|
|
|||
| if (node.getStep() != TopNNode.Step.SINGLE) { | |||
| return Optional.empty(); | |||
There was a problem hiding this comment.
nit: I think this comment will be obsolete pretty quickly.
| @@ -28,6 +28,7 @@ | |||
| public class TopNStatsRule | |||
There was a problem hiding this comment.
Does this PR fix any existing cost-based use-cases?
There was a problem hiding this comment.
It solves the problem reported in https://trinodb.slack.com/archives/C0305TQ05KL/p1671020850646689?thread_ts=1669977747.493179&cid=C0305TQ05KL
Description
Provide estimates for partial and final topN in TopNStatsRule
Additional context and related issues
https://trinodb.slack.com/archives/C0305TQ05KL/p1671020850646689?thread_ts=1669977747.493179&cid=C0305TQ05KL
Release notes
( ) This is not user-visible or docs only and no release notes are required.
( ) Release notes are required, please propose a release note for me.
(x) Release notes are required, with the following suggested text: