You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "chenkovsky (via GitHub)" <gi...@apache.org> on 2025/04/26 09:20:33 UTC
[PR] feat: simplify count distinct logical plan [datafusion]
chenkovsky opened a new pull request, #15867:
URL: https://github.com/apache/datafusion/pull/15867
## Which issue does this PR close?
- Closes #15850.
## Rationale for this change
select count(distinct ..) query doesn't go to the specialized distinct accumulator
## What changes are included in this PR?
disable single_distinct_to_groupby optimzation for count distinct.
## Are these changes tested?
UT
## Are there any user-facing changes?
No
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: simplify count distinct logical plan [datafusion]
Posted by "chenkovsky (via GitHub)" <gi...@apache.org>.
chenkovsky closed pull request #15867: feat: simplify count distinct logical plan
URL: https://github.com/apache/datafusion/pull/15867
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: simplify count distinct logical plan [datafusion]
Posted by "chenkovsky (via GitHub)" <gi...@apache.org>.
chenkovsky commented on PR #15867:
URL: https://github.com/apache/datafusion/pull/15867#issuecomment-2832901033
> clickbench Q42 fail
>
> ```
> SELECT "URLHash", "EventDate", COUNT(*) AS PageViews FROM hits WHERE "CounterID" = 62 AND "EventDate" >= '2013-07-01' AND "EventDate" <= '2013-07-31' AND "IsRefresh" = 0 AND "TraficSourceID" IN (-1, 6) AND "RefererHash" = 3594120000172545465 GROUP BY "URLHash", "EventDate" ORDER BY PageViews DESC LIMIT 10 OFFSET 100;
> ```
this query is in clickbench.slt, and sqllogictest doesn't fail. I cannot reproduce it in my env.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: simplify count distinct logical plan [datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on PR #15867:
URL: https://github.com/apache/datafusion/pull/15867#issuecomment-2832935755
```
┏━━━━━━━━━━━━━━┳━━━━━━━━━━━┳━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┳━━━━━━━━━━━━━━━┓
┃ Query ┃ main ┃ feature_count_distinct_accumulator ┃ Change ┃
┡━━━━━━━━━━━━━━╇━━━━━━━━━━━╇━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━╇━━━━━━━━━━━━━━━┩
│ QQuery 0 │ 0.18ms │ 0.23ms │ 1.31x slower │
│ QQuery 1 │ 28.20ms │ 26.98ms │ no change │
│ QQuery 2 │ 54.85ms │ 54.72ms │ no change │
│ QQuery 3 │ 50.61ms │ 44.70ms │ +1.13x faster │
│ QQuery 4 │ 321.17ms │ 596.53ms │ 1.86x slower │
│ QQuery 5 │ 480.40ms │ 700.37ms │ 1.46x slower │
│ QQuery 6 │ 0.21ms │ 0.18ms │ +1.16x faster │
│ QQuery 7 │ 33.49ms │ 29.93ms │ +1.12x faster │
│ QQuery 8 │ 411.45ms │ 415.03ms │ no change │
│ QQuery 9 │ 537.09ms │ 520.49ms │ no change │
│ QQuery 10 │ 148.01ms │ 144.94ms │ no change │
│ QQuery 11 │ 161.14ms │ 159.35ms │ no change │
│ QQuery 12 │ 516.02ms │ 458.13ms │ +1.13x faster │
│ QQuery 13 │ 675.36ms │ 1523.54ms │ 2.26x slower │
│ QQuery 14 │ 484.44ms │ 454.57ms │ +1.07x faster │
│ QQuery 15 │ 393.40ms │ 386.47ms │ no change │
│ QQuery 16 │ 919.67ms │ 867.38ms │ +1.06x faster │
│ QQuery 17 │ 816.58ms │ 783.79ms │ no change │
│ QQuery 18 │ 2365.83ms │ 2443.65ms │ no change │
│ QQuery 19 │ 49.46ms │ 43.93ms │ +1.13x faster │
│ QQuery 20 │ 689.76ms │ 659.51ms │ no change │
│ QQuery 21 │ 907.96ms │ 865.63ms │ no change │
│ QQuery 22 │ 1729.82ms │ 1882.47ms │ 1.09x slower │
│ QQuery 23 │ 5449.95ms │ 5430.83ms │ no change │
│ QQuery 24 │ 305.67ms │ 285.31ms │ +1.07x faster │
│ QQuery 25 │ 297.98ms │ 285.56ms │ no change │
│ QQuery 26 │ 336.26ms │ 322.70ms │ no change │
│ QQuery 27 │ 1093.64ms │ 1065.44ms │ no change │
│ QQuery 28 │ 8143.92ms │ 8515.65ms │ no change │
│ QQuery 29 │ 399.70ms │ 359.68ms │ +1.11x faster │
│ QQuery 30 │ 674.77ms │ 426.97ms │ +1.58x faster │
│ QQuery 31 │ 597.66ms │ 442.16ms │ +1.35x faster │
│ QQuery 32 │ 2636.13ms │ 2211.93ms │ +1.19x faster │
│ QQuery 33 │ 3176.46ms │ 2929.34ms │ +1.08x faster │
│ QQuery 34 │ 4208.19ms │ 3916.81ms │ +1.07x faster │
│ QQuery 35 │ 715.89ms │ 603.21ms │ +1.19x faster │
│ QQuery 36 │ 84.79ms │ 84.43ms │ no change │
│ QQuery 37 │ 62.08ms │ 52.38ms │ +1.19x faster │
│ QQuery 38 │ 92.32ms │ 84.77ms │ +1.09x faster │
│ QQuery 39 │ 153.67ms │ 136.62ms │ +1.12x faster │
│ QQuery 40 │ 92.64ms │ 34.58ms │ +2.68x faster │
│ QQuery 41 │ 120.72ms │ 29.36ms │ +4.11x faster │
│ QQuery 42 │ 35.37ms │ 27.37ms │ +1.29x faster │
└──────────────┴───────────┴────────────────────────────────────┴───────────────┘
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: simplify count distinct logical plan [datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on PR #15867:
URL: https://github.com/apache/datafusion/pull/15867#issuecomment-2832928771
Q14
```
SELECT "SearchPhrase", COUNT(DISTINCT "UserID") AS u FROM hits WHERE "SearchPhrase" <> '' GROUP BY "SearchPhrase" ORDER BY u DESC LIMIT 10;
```
This query is 2x slower, we need to find the reason and optimize it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: simplify count distinct logical plan [datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on PR #15867:
URL: https://github.com/apache/datafusion/pull/15867#issuecomment-2832904890
My bad, it seems like the casting error is allowed and the log is removed in #15858
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: simplify count distinct logical plan [datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on PR #15867:
URL: https://github.com/apache/datafusion/pull/15867#issuecomment-2837279738
I found we actually didn't have distinct count group accumulator
https://github.com/apache/datafusion/pull/15888
I try one and the performance is much better now, but still slightly left behind the main branch.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: simplify count distinct logical plan [datafusion]
Posted by "chenkovsky (via GitHub)" <gi...@apache.org>.
chenkovsky commented on PR #15867:
URL: https://github.com/apache/datafusion/pull/15867#issuecomment-2833360269
BTW, if we want to run count distinct in big data scenario, we have to use two-step process. so I think we have to add an configure to toggle this optimization.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: simplify count distinct logical plan [datafusion]
Posted by "chenkovsky (via GitHub)" <gi...@apache.org>.
chenkovsky commented on PR #15867:
URL: https://github.com/apache/datafusion/pull/15867#issuecomment-2833162562
@jayzhan211 after profiling, I found that most time is costed in hashset operator. for primative type, hashset may not a good option.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: simplify count distinct logical plan [datafusion]
Posted by "chenkovsky (via GitHub)" <gi...@apache.org>.
chenkovsky commented on PR #15867:
URL: https://github.com/apache/datafusion/pull/15867#issuecomment-2837256602
@jayzhan211 I think the root cause of poor performance is that, orginal plan can count parallelly, but current plan is actually blocked on final aggregation. I think I' trying to modify physical plan to make count distinct more efficient.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: simplify count distinct logical plan [datafusion]
Posted by "chenkovsky (via GitHub)" <gi...@apache.org>.
chenkovsky commented on PR #15867:
URL: https://github.com/apache/datafusion/pull/15867#issuecomment-2834018098
I tested shared concurrent hashset(DashSet) to avoid clone, but no performance gain.
something like
```rust
static global_values: LazyLock<DashSet<i64, std::hash::RandomState>> = LazyLock::new(|| DashSet::new());
#[derive(Debug)]
pub struct PrimitiveDistinctCountAccumulatorI64
{
data_type: DataType,
values: &'static DashSet<i64, std::hash::RandomState>
}
impl PrimitiveDistinctCountAccumulatorI64
{
pub fn new(data_type: &DataType) -> Self {
Self {
values: &global_values,
data_type: data_type.clone(),
}
}
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: simplify count distinct logical plan [datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on code in PR #15867:
URL: https://github.com/apache/datafusion/pull/15867#discussion_r2061907296
##########
datafusion/optimizer/src/single_distinct_to_groupby.rs:
##########
@@ -84,9 +87,11 @@ fn is_single_distinct_agg(aggr_expr: &[Expr]) -> Result<bool> {
}
aggregate_count += 1;
if *distinct {
+ distinct_count += 1;
for e in args {
fields_set.insert(e);
}
+ distinct_func = Some(Arc::clone(func));
Review Comment:
we can record the name instead of the whole func
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: simplify count distinct logical plan [datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on PR #15867:
URL: https://github.com/apache/datafusion/pull/15867#issuecomment-2832860997
clickbench Q42 fail
```
SELECT "URLHash", "EventDate", COUNT(*) AS PageViews FROM hits WHERE "CounterID" = 62 AND "EventDate" >= '2013-07-01' AND "EventDate" <= '2013-07-31' AND "IsRefresh" = 0 AND "TraficSourceID" IN (-1, 6) AND "RefererHash" = 3594120000172545465 GROUP BY "URLHash", "EventDate" ORDER BY PageViews DESC LIMIT 10 OFFSET 100;
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: simplify count distinct logical plan [datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on PR #15867:
URL: https://github.com/apache/datafusion/pull/15867#issuecomment-2833713281
>
BTW, if we want to run count distinct in big data scenario, we have to use two-step process. so I think we have to add an configure to toggle this optimization.
We can control the skip partial aggregation ratio, the lower the ration the more likely the partial aggregation is skipped, so we probably don't need another switch to control two-step or single step.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: simplify count distinct logical plan [datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on PR #15867:
URL: https://github.com/apache/datafusion/pull/15867#issuecomment-2833182081
```rust
fn state(&mut self) -> Result<Vec<ScalarValue>> {
let scalars = self.values.iter().cloned().collect::<Vec<_>>();
let arr =
ScalarValue::new_list_nullable(scalars.as_slice(), &self.state_data_type);
Ok(vec![ScalarValue::List(arr)])
}
```
We clone the hashset from partial aggregation and convert to List for final aggregation. In high cardinality case, where most of the values are different we do aggregation twice + additional clone.
I can think of 2 possible solution.
1. Use single aggregation but somehow aggregation parallelly
we can try convert it single aggregation and see whether it is fast enough than the current version
2. Find out a way to avoid cloning hashset to list array and initilize the accumulator with the hashset in final aggregation state. This is probably not trivial to ensure we have zero copy all the way down to final aggregation.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: simplify count distinct logical plan [datafusion]
Posted by "chenkovsky (via GitHub)" <gi...@apache.org>.
chenkovsky commented on PR #15867:
URL: https://github.com/apache/datafusion/pull/15867#issuecomment-2835208591
I tried to do single thread distinct, but no performance gain.
something like:
```
impl<T> Accumulator for PrimitiveDistinctCountAccumulator<T> {
fn update_batch(&mut self, values: &[ArrayRef]) -> datafusion_common::Result<()> {
// don't dedup
}
fn merge_batch(&mut self, states: &[ArrayRef]) -> datafusion_common::Result<()> {
// don't dedup
}
fn evaluate(&mut self) -> datafusion_common::Result<ScalarValue> {
// do dedup
}
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org