vikrantmehta123 commented on code in PR #27064:
URL: https://github.com/apache/datafusion/pull/27064#discussion_r4535376653
##########
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:
Any specific reason for doing this change? I felt that since the check is
already present in the is_supported_type method we don't need to duplicate it
here. And also, I felt that when reading the code, the reader would have to pay
extra attention to realize that the list in is_supported_type is different than
the list in is_hll_groups_type. Here, by reusing the check, we make it a bit
more explicit.
Please suggest if I still need to make the change.
--
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]
