Uh oh!
There was an error while loading. Please reload this page.
[Minor] propagate distinct_count as inexact through unions - #20846
Conversation
| right: &ColumnStatistics, | ||
| ) -> ColumnStatistics { | ||
| left.distinct_count = Precision::Absent; | ||
| left.distinct_count = left.distinct_count.add(&right.distinct_count).to_inexact(); |
There was a problem hiding this comment.
This doesn't account for any overlap between the ranges.
I had previously taken a look at trino's implementation for this. What we want to do is account for the range of the left and right statistics
The formula trino uses is:
// for unioning A + B
// calculate A overlap with B using min/max statistics
overlap_a = percent of overlap that A has with B
overlap_b = percent of overlap that B has with A
new_distinct_count = max(overlap_a * NDV_a, overlap_b * NDV_b) // find interesect
+ (1 - overlap_a) * NDV_a // overlap for just a
+ (1 - overlap_b) * NDV_b // overlap for just b
It is quite intuitive
There was a problem hiding this comment.
@asolimando Do you think this would be a better alternative when merging parquet row groups instead of max? (this is unrelated to this pr)
There was a problem hiding this comment.
@jonathanc-n, when min/max are available we can indeed do better by using that formula for updating NDV, max being just the degenerate case where you assume 100% overlap across merged relations (be it partitions or union'ed relations).
This would directly address @xudong963's concern about disjoint domains: min/max ranges would show near-zero overlap, so the formula naturally approaches sum instead of max.
The formula assumes uniform distribution within the min/max range, but that's a classic assumption when working with scalar statistics, and with just min/max/NDV we can't easily do better. (Richer stats like in StatisticsV2 could help, but let's reason within the current statistics framing for now).
I will add a comment in #19957 to make sure we capture this discussion, thanks for the ping.
EDIT: added here
There was a problem hiding this comment.
I've applied the formula. Thanks @jonathanc-n for review I was under the assumption of we can simply add to have it as an upper bound value
| assert_eq!(result, expected); | ||
| } | ||
| #[tokio::test] |
There was a problem hiding this comment.
thanks changed to simple test
| right: &ColumnStatistics, | ||
| ) -> ColumnStatistics { | ||
| left.distinct_count = Precision::Absent; | ||
| left.distinct_count = left.distinct_count.add(&right.distinct_count).to_inexact(); |
There was a problem hiding this comment.
add comment saying why it should be inexact just for to help out developers
| max_right | ||
| }; | ||
| if overlap_min > overlap_max { |
There was a problem hiding this comment.
Nit: the formula naturally degrades to sum when there's no overlap (overlap_range = 0 gives overlap_left = overlap_right = 0, so the result is ndv_left + ndv_right), so this is a short-circuit optimization, we might want to add that as a comment.
There was a problem hiding this comment.
That's why I've returned actually but as you've said comment makes sense here. Thanks for heads up I've added a comment
| /// overlap_a = fraction of A's range that overlaps with B | ||
| /// overlap_b = fraction of B's range that overlaps with A | ||
| /// NDV = max(overlap_a * NDV_a, overlap_b * NDV_b) \[intersection\] | ||
| /// + (1 - overlap_a) * NDV_a [only in A] | ||
| /// + (1 - overlap_b) * NDV_b [only in B] |
There was a problem hiding this comment.
Could you please add some comments about why the formula is reasonable?
There was a problem hiding this comment.
Would my high-level description in #19957 (comment) be what you'd expect as a comment?
In case, @buraksenn feel free to use it.
There was a problem hiding this comment.
Thanks @asolimando , with that comment and also some llm input I've add comments hope it is ok now
Uh oh!
There was an error while loading. Please reload this page.
Replace the max(left, right) heuristic in try_merge with overlap-based NDV estimation, which accounts for range overlap between partitions instead of blindly picking the larger value. Move estimate_ndv_with_overlap from union.rs (apache#20846) to stats.rs as a pub function and reuse it in try_merge for row group/partition merging. The merge loop is reordered so distinct_count is computed before min/max updates (needs pre-merge ranges for the overlap formula). Fallback to max(left, right) when min/max are absent or distance is unsupported.
) ## Which issue does this PR close? Does not close but part of apache#20766 ## Rationale for this change As @jonathanc-n describes here is the Trino's formula about inexact formula: ``` // for unioning A + B // calculate A overlap with B using min/max statistics overlap_a = percent of overlap that A has with B overlap_b = percent of overlap that B has with A new_distinct_count = max(overlap_a * NDV_a, overlap_b * NDV_b) // find interesect + (1 - overlap_a) * NDV_a // overlap for just a + (1 - overlap_b) * NDV_b // overlap for just b ``` ## What changes are included in this PR? Instead of absent set `distinct_count` with inexact precision depending on overlaps and distinct counts ## Are these changes tested? I've added unit tests ## Are there any user-facing changes? No
Which issue does this PR close?
Does not close but part of #20766
Rationale for this change
As @jonathanc-n describes here is the Trino's formula about inexact formula:
What changes are included in this PR?
Instead of absent set
distinct_countwith inexact precision depending on overlaps and distinct countsAre these changes tested?
I've added unit tests
Are there any user-facing changes?
No