Conversation
…rcuit after passing the threshold instead of continuing to scan the entire dataset
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #25390 +/- ##
==========================================
- Coverage 81.93% 81.93% -0.01%
==========================================
Files 1136 1136
Lines 428779 429216 +437
Branches 428779 429216 +437
==========================================
+ Hits 351319 351674 +355
- Misses 56446 56485 +39
- Partials 21014 21057 +43 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Thank you. This is a legitimate optimization, but I'm not sure the pattern is common enough to justify maintaining an additional, less intuitive optimizer rule. The same goal can already be achieved with a small application-side workaround, using the existing optimization that pushes -- To check whether NDV > 1, run:
SELECT DISTINCT v1 % 100
FROM generate_series(10000000000) AS t1(v1)
LIMIT 10;
-- Then check whether the result contains more than one row.So I'm leaning toward avoiding this rule. DataFusion is increasingly constrained by implementation complexity, and the optimizer is already one of the trickiest parts to maintain. I'd reconsider if we can confirm that this is a sufficiently common query pattern to justify the additional complexity. |
|
@2010YOUY01 Thanks for the quick review. |
Which issue does this PR close?
Rationale for this change
Distinct accumulates all the values and then counts them, even if the
threshold is met it continues through the rest of the dataset.
Right now
count(distinct)doesn't go to the distinct accumulator, DF puts twoaggregates on top of each other (a group by and then a count of the groups).
Measured on a 50M-row memory table (same binary, toggled via
SET datafusion.optimizer.enable_distinct_aggregation_soft_limit):count(DISTINCT col) > 1, 5 distinct valuescount(DISTINCT col) > 1, 50M distinct valuesWhat changes are included in this PR?
SELECT count(DISTINCT col) > 1 FROM tabletype queries are when this coderuns. Take the constant value + 2, one extra for the NULL case and another for
clearing the threshold. When we are counting the groups we track the count
until n + 2, if we hit that number we short circuit. This short circuit is
checked after each batch in Partial.
HAVING was left out for simplicity.
Both the existing
LIMITpushdown and the new comparison patterndo the same thing so they share the flag
(
enable_distinct_aggregation_soft_limit).What is the testing strategy for this PR?
Yes.
Are there any user-facing changes?
No user facing changes