You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "ShashidharM0118 (via GitHub)" <gi...@apache.org> on 2025/11/17 11:22:38 UTC
[PR] feat: Make GenericDistinctBuffer generic over both Hashable and native types [datafusion]
ShashidharM0118 opened a new pull request, #18763:
URL: https://github.com/apache/datafusion/pull/18763
## Which issue does this PR close?
- Closes #18670
## Rationale for this change
As described in #18670, `GenericDistinctBuffer` previously forced all types through the `Hashable<T>` wrapper, even for natively hashable types like integers. This was necessary to handle floats (which don't implement `Hash` natively), but it meant that types with efficient native `Hash` implementations (like `i32`, `u64`, etc.) were unnecessarily wrapped, potentially impacting performance.
This PR introduces a trait-based solution that allows `GenericDistinctBuffer` to work generically with both native hashable types and types requiring the `Hashable` wrapper, without code duplication.
## What changes are included in this PR?
1. **Introduced `DistinctStorage` trait**: A new trait that abstracts over different storage strategies for distinct value tracking. It provides two implementations:
- `HashSet<T, RandomState>` for natively hashable types (integers, decimals, dates, timestamps)
- `HashSet<Hashable<T>, RandomState>` for types requiring special hashing (floats)
2. **Made `GenericDistinctBuffer` generic over storage**: Changed from `GenericDistinctBuffer<T>` to `GenericDistinctBuffer<T, S>` where `S: DistinctStorage<Native = T::Native>`. This allows the buffer to use the appropriate storage strategy based on the type.
3. **Unified distinct count accumulators**: Merged `PrimitiveDistinctCountAccumulator` and `FloatDistinctCountAccumulator` into a single generic implementation `PrimitiveDistinctCountAccumulator<T, S>`. The former `FloatDistinctCountAccumulator` is now a type alias using `Hashable` storage.
4. **Updated all usages**: Modified all call sites in `count.rs`, `sum_distinct`, `median`, and `percentile_cont` to specify the appropriate storage type (native `HashSet<T>` for integers/dates/timestamps, `HashSet<Hashable<T>>` for floats).
5. **Added `Send + Sync` bounds**: Added necessary trait bounds for proper thread-safe trait implementations in DataFusion's parallel execution environment.
## Are these changes tested?
Yes
## 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: Make GenericDistinctBuffer generic over both Hashable and native types [datafusion]
Posted by "ShashidharM0118 (via GitHub)" <gi...@apache.org>.
ShashidharM0118 commented on PR #18763:
URL: https://github.com/apache/datafusion/pull/18763#issuecomment-3556198695
@Jefffrey is there any changes required in this pr?
--
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: Make GenericDistinctBuffer generic over both Hashable and native types [datafusion]
Posted by "ShashidharM0118 (via GitHub)" <gi...@apache.org>.
ShashidharM0118 commented on PR #18763:
URL: https://github.com/apache/datafusion/pull/18763#issuecomment-3592590067
I added a `distinct_ops` benchmark (`count_distinct_int64`, `count_distinct_float64`, `median_distinct_float64`, `percentile_cont_distinct_float64`) and ran it several times on both `main` and this branch.
Results vary between runs (normal micro-benchmark noise on my machine), so I’m not confident claiming a clear regression or improvement from my local data alone.
If possible, could you:
- run `cargo bench -p datafusion-functions-aggregate --bench distinct_ops` on your side, and/or
- suggest any specific benchmark setup you’d like (target time, sample count, etc.)?
I’m happy to adjust the implementation or benches based on your feedback.
--
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: Make GenericDistinctBuffer generic over both Hashable and native types [datafusion]
Posted by "ShashidharM0118 (via GitHub)" <gi...@apache.org>.
ShashidharM0118 commented on PR #18763:
URL: https://github.com/apache/datafusion/pull/18763#issuecomment-3633649258
> Apologies I still haven't been able to take a look at this; however before I dive into the PR it would be good to see some of the benchmarks I mention above just so we know this can be a valid approach that doesn't regress performance
I added the requested micro-benchmarks (count/sum distinct int64/float64 and median/percentile_cont int64/float64) and ran them against `main` vs this branch, no regressions observed. Please take a look when you have time—thanks!
--
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: Make GenericDistinctBuffer generic over both Hashable and native types [datafusion]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] closed pull request #18763: feat: Make GenericDistinctBuffer generic over both Hashable and native types
URL: https://github.com/apache/datafusion/pull/18763
--
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: Make GenericDistinctBuffer generic over both Hashable and native types [datafusion]
Posted by "ShashidharM0118 (via GitHub)" <gi...@apache.org>.
ShashidharM0118 commented on code in PR #18763:
URL: https://github.com/apache/datafusion/pull/18763#discussion_r2658722434
##########
benchmarks/src/bin/dfbench.rs:
##########
@@ -34,17 +34,20 @@ static ALLOC: snmalloc_rs::SnMalloc = snmalloc_rs::SnMalloc;
static ALLOC: mimalloc::MiMalloc = mimalloc::MiMalloc;
use datafusion_benchmarks::{
- cancellation, clickbench, h2o, hj, imdb, nlj, smj, sort_tpch, tpcds, tpch,
+ cancellation, clickbench, distinct, h2o, hj, imdb, nlj, quantiles, smj, sort_tpch,
+ tpcds, tpch,
};
#[derive(Debug, StructOpt)]
#[structopt(about = "benchmark command")]
enum Options {
Cancellation(cancellation::RunOpt),
Clickbench(clickbench::RunOpt),
+ Distinct(distinct::RunOpt),
Review Comment:
Thanks! I actually realized this just yesterday 😅
I'll update the PR shortly and share the benchmark results.
--
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: Make GenericDistinctBuffer generic over both Hashable and native types [datafusion]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #18763:
URL: https://github.com/apache/datafusion/pull/18763#issuecomment-4001636283
Thank you for your contribution. Unfortunately, this pull request is stale because it has been open 60 days with no activity. Please remove the stale label or comment or this will be closed in 7 days.
--
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: Make GenericDistinctBuffer generic over both Hashable and native types [datafusion]
Posted by "ShashidharM0118 (via GitHub)" <gi...@apache.org>.
ShashidharM0118 commented on PR #18763:
URL: https://github.com/apache/datafusion/pull/18763#issuecomment-3541346954
This PR introduces significant refactoring by adding the `DistinctStorage` trait to unify accumulators. Please let me know if there are any concerns with the approach, implementation details, or coding style. Happy to make adjustments based on feedback!
--
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: Make GenericDistinctBuffer generic over both Hashable and native types [datafusion]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey commented on PR #18763:
URL: https://github.com/apache/datafusion/pull/18763#issuecomment-3587640505
Apologies I still haven't been able to take a look at this; however before I dive into the PR it would be good to see some of the benchmarks I mention above just so we know this can be a valid approach that doesn't regress performance
--
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: Make GenericDistinctBuffer generic over both Hashable and native types [datafusion]
Posted by "ShashidharM0118 (via GitHub)" <gi...@apache.org>.
ShashidharM0118 commented on PR #18763:
URL: https://github.com/apache/datafusion/pull/18763#issuecomment-3556205527
@Jefffrey Are there any changes required in this PR?
--
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: Make GenericDistinctBuffer generic over both Hashable and native types [datafusion]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey commented on PR #18763:
URL: https://github.com/apache/datafusion/pull/18763#issuecomment-3561074665
I'll try review this when I have time; but it would be nice if we have some benchmarks to verify this doesn't regress performance. Off the top of my head we'd need a benchmark for count(distinct) where type is not float (e.g. int64), one where it is float, and a benchmark for median or percentile_cont. Reason being, in current main:
- count(distinct) non-float -> uses `PrimitiveDistinctCountAccumulator`
- count(distinct) float -> uses `FloatDistinctCountAccumulator`
- percentile_cont/median -> uses `GenericDistinctBuffer` (which I believe `FloatDistinctCountAccumulator` uses too?)
So they should be a representative sample of affected functions
--
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: Make GenericDistinctBuffer generic over both Hashable and native types [datafusion]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey commented on code in PR #18763:
URL: https://github.com/apache/datafusion/pull/18763#discussion_r2658717474
##########
benchmarks/src/bin/dfbench.rs:
##########
@@ -34,17 +34,20 @@ static ALLOC: snmalloc_rs::SnMalloc = snmalloc_rs::SnMalloc;
static ALLOC: mimalloc::MiMalloc = mimalloc::MiMalloc;
use datafusion_benchmarks::{
- cancellation, clickbench, h2o, hj, imdb, nlj, smj, sort_tpch, tpcds, tpch,
+ cancellation, clickbench, distinct, h2o, hj, imdb, nlj, quantiles, smj, sort_tpch,
+ tpcds, tpch,
};
#[derive(Debug, StructOpt)]
#[structopt(about = "benchmark command")]
enum Options {
Cancellation(cancellation::RunOpt),
Clickbench(clickbench::RunOpt),
+ Distinct(distinct::RunOpt),
Review Comment:
I don't think we want to add benchmarks here; they would be more appropriate as microbenchmarks here: https://github.com/apache/datafusion/tree/main/datafusion/functions-aggregate/benches
--
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