alamb commented on code in PR #26000:
URL: https://github.com/apache/datafusion/pull/26000#discussion_r4573646302
##########
datafusion/optimizer/src/single_distinct_to_groupby.rs:
##########
@@ -110,7 +111,58 @@ impl CountRollup {
}
}
-/// Check whether all aggregate exprs are distinct on a single field.
+/// Returns `true` for fixed-width primitive types where single-phase distinct
+/// accumulation avoids heap allocation churn **and** where bit-identity equals
+/// value-equality, so that skipping the GROUP BY rewrite cannot silently
change
+/// query results.
+///
+/// Float types are deliberately excluded: IEEE 754 defines `-0.0 == 0.0`, so a
+/// `GROUP BY` on floats merges those two bit-patterns into one group. The
+/// native distinct accumulators hash raw bits and therefore count them
+/// separately. Keeping floats in this list would let
`single_distinct_prefer_native`
+/// skip the rewrite for a float column, producing a different answer than the
+/// non-rewritten path — a silent correctness regression.
+///
+/// `Duration` is also excluded: `avg` has no native DISTINCT accumulator for
duration
+/// types, so skipping the rewrite would cause a runtime error.
+fn is_fixed_width_type(dt: &DataType) -> bool {
+ matches!(
+ dt,
+ DataType::Boolean
+ | DataType::Int8
+ | DataType::Int16
+ | DataType::Int32
+ | DataType::Int64
+ | DataType::UInt8
+ | DataType::UInt16
+ | DataType::UInt32
+ | DataType::UInt64
+ | DataType::Date32
+ | DataType::Date64
+ | DataType::Time32(_)
+ | DataType::Time64(_)
+ | DataType::Timestamp(_, _)
+ | DataType::Interval(_)
+ | DataType::Decimal128(_, _)
+ | DataType::Decimal256(_, _)
+ )
+}
+
+/// Check whether all aggregate exprs are eligible for the
single-distinct-to-group-by rewrite.
+///
+/// With `use_native_grouped_distinct` enabled (which requires one target
partition,
Review Comment:
can you please summarieze this paragraph -- it has too much irrelevant
detail (eg. restating what `use_native_grouped_distinct` does in English)
##########
datafusion/common/src/config.rs:
##########
@@ -1950,16 +1940,25 @@ config_namespace! {
/// projections or aliases.
pub enable_unions_to_filter: bool, default = false
- /// Prefer native grouped DISTINCT aggregation when target_partitions
is 1
- /// and every aggregate explicitly supports a specialized
GroupsAccumulator
- /// for its DISTINCT argument types. Defaults to false, preserving the
- /// two-phase GROUP BY rewrite.
+ /// When `true` and `target_partitions` is 1, the
`SingleDistinctToGroupBy`
Review Comment:
Can we please rephrase this so it starts with the impact of this knob -- I
think it is "queries with a single DISTINCT aggregate will take less memory but
also run more slowly"?
i think all the background of the tradeoff is way too much detail for the
config docs -- it probably belongs on the `SingleDistinctToGroupBy` docs, with
perhaps a link from the config
##########
datafusion/optimizer/src/single_distinct_to_groupby.rs:
##########
@@ -1366,4 +1483,182 @@ mod tests {
assert_eq!(plan.schema().as_ref(), optimized_plan.schema().as_ref());
Ok(())
}
+
+ fn unsupported_distinct(expr: Expr) -> Expr {
Review Comment:
If these can be .slt tests, I think we should rewrite them to be slt -- the
rust is far more verbose
--
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]
