mkleen commented on code in PR #27064:
URL: https://github.com/apache/datafusion/pull/27064#discussion_r4530278479
##########
datafusion/functions-aggregate/src/approx_distinct.rs:
##########
@@ -913,75 +926,69 @@ fn is_fixed_domain_type(data_type: &DataType) -> bool {
)
}
+/// Returns true if `approx_distinct` can run on this data type
fn is_supported_type(data_type: &DataType) -> bool {
- let value_type = encoded_value_type(data_type);
- matches!(value_type, DataType::Null)
- || is_fixed_domain_type(value_type)
- || is_hll_groups_type(value_type)
-}
+ if has_float_leaf(data_type) {
+ return false;
+ }
-fn encoded_value_type(data_type: &DataType) -> &DataType {
- let mut value_type = data_type;
- loop {
- match value_type {
- DataType::Dictionary(_, inner) => value_type = inner,
- DataType::RunEndEncoded(_, values) => value_type =
values.data_type(),
- _ => return value_type,
- }
+ match data_type {
+ DataType::Dictionary(_, values) => is_supported_type(values),
+ DataType::RunEndEncoded(_, values) =>
is_supported_type(values.data_type()),
+ DataType::Null
+ | DataType::Boolean
+ | DataType::UInt8
+ | DataType::Int8
+ | DataType::UInt16
+ | DataType::Int16
+ | DataType::UInt32
+ | DataType::UInt64
+ | DataType::Int32
+ | DataType::Int64
+ | DataType::Date32
+ | DataType::Date64
+ | DataType::Time32(TimeUnit::Second)
+ | DataType::Time32(TimeUnit::Millisecond)
+ | DataType::Time64(TimeUnit::Microsecond)
+ | DataType::Time64(TimeUnit::Nanosecond)
+ | DataType::Timestamp(TimeUnit::Second, _)
+ | DataType::Timestamp(TimeUnit::Millisecond, _)
+ | DataType::Timestamp(TimeUnit::Microsecond, _)
+ | DataType::Timestamp(TimeUnit::Nanosecond, _)
+ | DataType::Interval(IntervalUnit::YearMonth)
+ | DataType::Interval(IntervalUnit::DayTime)
+ | DataType::Interval(IntervalUnit::MonthDayNano)
+ | DataType::Decimal32(_, _)
+ | DataType::Decimal64(_, _)
+ | DataType::Decimal128(_, _)
+ | DataType::Decimal256(_, _)
+ | DataType::Duration(_)
+ | DataType::Utf8
+ | DataType::LargeUtf8
+ | DataType::Utf8View
+ | DataType::Binary
+ | DataType::BinaryView
+ | DataType::FixedSizeBinary(_)
+ | DataType::LargeBinary
+ | DataType::List(_)
+ | DataType::LargeList(_)
+ | DataType::FixedSizeList(_, _)
+ | DataType::ListView(_)
+ | DataType::LargeListView(_)
+ | DataType::Map(_, _)
+ | DataType::Struct(_)
+ | DataType::Union(_, _) => true,
+ _ => false,
}
}
/// Returns true for the data types backed by the HyperLogLog
/// [`HllGroupsAccumulator`]. The fixed-domain types (booleans / small ints)
and
/// `Null` fall back to the per-group [`Accumulator`] path.
fn is_hll_groups_type(data_type: &DataType) -> bool {
- if matches!(
Review Comment:
Could we keep this as on main? Leave the type list in is_hll_groups_type
and add on top:
```
if has_float_leaf(data_type) {
return false;
}
```
--
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]
