tjnangosha opened a new pull request, #26821: URL: https://github.com/apache/datafusion/pull/26821
## Which issue does this PR close? - Closes #26820. ## Rationale for this change `COUNT(DISTINCT)` on a Decimal32 or Decimal64 column is slower than the same query on a Decimal128 column, even though the values are smaller. On `main`, counting the distinct values in a batch of 8,192 Decimal32 values takes about 870 µs, compared with about 110 µs for Decimal128. The cause is in `get_count_accumulator` in `datafusion/functions-aggregate/src/count.rs`. Decimal128 and Decimal256 use `PrimitiveDistinctCountAccumulator`, which works on the raw values directly. Decimal32 and Decimal64 have no arm there, so they fall through to the generic `DistinctCountAccumulator`, which converts every row into a `ScalarValue` before inserting it into a hash set. This looks like a spot that was missed when Decimal32/64 support was added in #17501 (issue #17489). ## What changes are included in this PR? - `count.rs`: add `Decimal32` and `Decimal64` arms to `get_count_accumulator`, using `PrimitiveDistinctCountAccumulator` like Decimal128 and Decimal256 do. - `benches/count_distinct.rs`: add `count_distinct` cases for Decimal32, Decimal64 and Decimal128 (as a reference) at 80% and 99% distinct. - `aggregate.slt`: add `COUNT(DISTINCT)` tests on Decimal32 and Decimal64 columns. ## Benchmark results `COUNT(DISTINCT)` accumulator, one batch of 8,192 values (`datafusion/functions-aggregate/benches/count_distinct.rs`). | case | main | this PR | change | speedup | |---|---|---|---|---| | decimal32, 80% distinct | 867.4 µs | 99.0 µs | −88.6% | 8.8× | | decimal32, 99% distinct | 910.5 µs | 100.0 µs | −89.1% | 9.1× | | decimal64, 80% distinct | 911.5 µs | 98.4 µs | −89.3% | 9.3× | | decimal64, 99% distinct | 918.6 µs | 99.8 µs | −89.0% | 9.2× | | decimal128, 80% distinct (unchanged path) | 108.6 µs | 109.7 µs | no change (p = 0.81) | – | | decimal128, 99% distinct (unchanged path) | 113.2 µs | 112.0 µs | no change (p = 0.31) | – | Decimal32 and Decimal64 now run at about the same speed as Int32/Int64, since they use the same accumulator. ## What is the testing strategy for this PR? The change doesn't affect results, only which accumulator is used. The new sqllogictest cases in `aggregate.slt` run `COUNT(DISTINCT)` on Decimal32 and Decimal64 columns containing duplicates and a NULL, with and without `GROUP BY`. Each query counts two different columns, so the single-distinct rewrite (`single_distinct_to_groupby`) doesn't turn it into a `GROUP BY` and the query goes through the accumulator changed here. The new bench cases cover the performance side. ## Are there any user-facing changes? No. Query results are unchanged, and `COUNT(DISTINCT)` on Decimal32 and Decimal64 columns is faster. There are no public API changes. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
