Fix DistinctCount for timestamps with time zone #10043
Conversation
joroKr21
force-pushed
the
bugfix/count-distinct-ts
branch
from
4f749bd
to
28d6e2d
Compare
Preserve the original data type in the aggregation state
joroKr21
force-pushed
the
bugfix/count-distinct-ts
branch
from
28d6e2d
to
cb59fbd
Compare
| dt @ Decimal256(_, _) => Box::new( | ||
| PrimitiveDistinctCountAccumulator::<Decimal256Type>::new() | ||
| .with_data_type(dt.clone()), | ||
| ), |
There was a problem hiding this comment.
Can you add the test for decimal too?
There was a problem hiding this comment.
I expect the test that shows the change is needed. If this change is removed, it failed.
There was a problem hiding this comment.
It is clear for me the change for timestamp with timezone, but I'm not sure about the decimal one.
There was a problem hiding this comment.
I can show that for Decimal128 (not sure why it doesn't trigger for Decimal256). But it's better to be consistent, losing the data type might lead to unexpected consequences.
joroKr21
force-pushed
the
bugfix/count-distinct-ts
branch
from
224ea2b
to
8e7d302
Compare
joroKr21
force-pushed
the
bugfix/count-distinct-ts
branch
from
8e7d302
to
ce7843e
Compare
| Timestamp(Second, _) => { | ||
| Box::new(PrimitiveDistinctCountAccumulator::<TimestampSecondType>::new()) | ||
| } | ||
| dt @ Timestamp(Microsecond, _) => Box::new( |
There was a problem hiding this comment.
I wonder if we should defensively always set with_data_type to PrimitiveDistinctCountAccumulator 🤔
I tried to come up with other DataType that doesn't have a 1:1 mapping to its ArrowPrimitiveType and I don't think there is one, but always supplying the data_type might be a more defensive pattern
There was a problem hiding this comment.
I was wondering the same. Let me know what you prefer?
There was a problem hiding this comment.
How about we do it as a follow on PR? I would like to merge this one asap as it fixes a regression
* Fix DistinctCount for timestamps with time zone Preserve the original data type in the aggregation state * Add tests for decimal count distinct
* Fix DistinctCount for timestamps with time zone (apache#10043) * Fix DistinctCount for timestamps with time zone Preserve the original data type in the aggregation state * Add tests for decimal count distinct * Fix with_new_children for EmptyExec
* Fix DistinctCount for timestamps with time zone Preserve the original data type in the aggregation state * Add tests for decimal count distinct
* Fix DistinctCount for timestamps with time zone Preserve the original data type in the aggregation state * Add tests for decimal count distinct
Which issue does this PR close?
Closes #10042
Rationale for this change
Fixing a regression.
What changes are included in this PR?
Preserve the original data type in the aggregation state.
Are these changes tested?
Tested in SLT.
Are there any user-facing changes?
Internal changes only.