feat: extend interval analysis support for temporal types#21520
feat: extend interval analysis support for temporal types#21520buraksenn wants to merge 3 commits intoapache:mainfrom
Conversation
github-actions
bot
added
logical-expr
| /// when the calculated cardinality does not fit in an `u64`. | ||
| pub fn cardinality(&self) -> Option<u64> { | ||
| let data_type = self.data_type(); | ||
| if data_type.is_integer() { |
There was a problem hiding this comment.
Minor notice: perhaps it would be cleaner to gate on the concrete types which distance actually handles (Date & Timestamp) now rather than the broader is_temporal() check. Your call though. Once the remaining temporal types (Time*, Duration*, Interval*) also get distance arms, this line won't need to change. Just a bit confusing right now since the gate can be read as "all temporal types supported" but in practice distance returns None for some of them
| (Self::Date32(Some(l)), Self::Date32(Some(r))) => Some(l.abs_diff(*r) as _), | ||
| (Self::Date64(Some(l)), Self::Date64(Some(r))) => Some(l.abs_diff(*r) as _), | ||
| (Self::TimestampSecond(Some(l), _), Self::TimestampSecond(Some(r), _)) => { | ||
| Some(l.abs_diff(*r) as _) |
There was a problem hiding this comment.
The pattern (Self::TimestampSecond(Some(l), _), Self::TimestampSecond(Some(r), _)) ignores the timezone field on both sides. Is that intentional? I want to make sure two timestamps with different tz strings produce a meaningful distance. If so, a short comment on one of the arms explaining the reasoning would be helpful.
berkaysynnada
left a comment
berkaysynnada
left a comment
There was a problem hiding this comment.
LGTM, just two minor issues
2 participants
Which issue does this PR close?
related with #21109 and its PR #21473
Rationale for this change
Temporal types can actually support internal arithmetics and with this change internal arithmetics can now narrow down column boundaries and selectivity instead of falling back to default selectivity of 1.0
What changes are included in this PR?
Extend internal_arithmetic types with date and duration types
Are these changes tested?
Yes adjusted tests
Are there any user-facing changes?
no