You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "buraksenn (via GitHub)" <gi...@apache.org> on 2026/04/09 17:42:38 UTC
[PR] feat: extend interval analysis support for temporal types [datafusion]
buraksenn opened a new pull request, #21520:
URL: https://github.com/apache/datafusion/pull/21520
## Which issue does this PR close?
- Closes #21111
related with #21109 and its PR https://github.com/apache/datafusion/pull/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
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: extend interval analysis support for temporal types [datafusion]
Posted by "berkaysynnada (via GitHub)" <gi...@apache.org>.
berkaysynnada commented on code in PR #21520:
URL: https://github.com/apache/datafusion/pull/21520#discussion_r3097270784
##########
datafusion/common/src/scalar/mod.rs:
##########
@@ -2526,6 +2526,23 @@ impl ScalarValue {
(Self::Float64(Some(l)), Self::Float64(Some(r))) => {
Some((l - r).abs().round() as _)
}
+ (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 _)
Review 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.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: extend interval analysis support for temporal types [datafusion]
Posted by "buraksenn (via GitHub)" <gi...@apache.org>.
buraksenn commented on code in PR #21520:
URL: https://github.com/apache/datafusion/pull/21520#discussion_r3098169916
##########
datafusion/expr-common/src/interval_arithmetic.rs:
##########
@@ -929,7 +929,7 @@ impl Interval {
/// 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() {
Review Comment:
I've missed that yes it became unintentionally broad and return None as you've said. For this I've changed it to use `matches` macro for the supported types
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: extend interval analysis support for temporal types [datafusion]
Posted by "buraksenn (via GitHub)" <gi...@apache.org>.
buraksenn commented on code in PR #21520:
URL: https://github.com/apache/datafusion/pull/21520#discussion_r3098161993
##########
datafusion/common/src/scalar/mod.rs:
##########
@@ -2526,6 +2526,23 @@ impl ScalarValue {
(Self::Float64(Some(l)), Self::Float64(Some(r))) => {
Some((l - r).abs().round() as _)
}
+ (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 _)
Review Comment:
I've verified that arrow stores timestamps in epoch always. Thus timezone does not affect this distance calculation. I'll add a comment it makes sense thanks
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: extend interval analysis support for temporal types [datafusion]
Posted by "berkaysynnada (via GitHub)" <gi...@apache.org>.
berkaysynnada merged PR #21520:
URL: https://github.com/apache/datafusion/pull/21520
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: extend interval analysis support for temporal types [datafusion]
Posted by "berkaysynnada (via GitHub)" <gi...@apache.org>.
berkaysynnada commented on code in PR #21520:
URL: https://github.com/apache/datafusion/pull/21520#discussion_r3097264342
##########
datafusion/expr-common/src/interval_arithmetic.rs:
##########
@@ -929,7 +929,7 @@ impl Interval {
/// 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() {
Review 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
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org