You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "ch-sc (via GitHub)" <gi...@apache.org> on 2025/02/06 11:36:35 UTC
[PR] Support bounds evaluation for temporal data types [datafusion]
ch-sc opened a new pull request, #14523:
URL: https://github.com/apache/datafusion/pull/14523
## Which issue does this PR close?
As discussed in [14237](https://github.com/apache/datafusion/issues/14237) temporal data types should be supported in bounds evaluation.
<!--
We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. For example `Closes #123` indicates that this PR will close issue #123.
-->
- Closes #14237.
## Rationale for this change
<!--
Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed.
Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes.
-->
We want to extend the number of expressions that can evaluate bounds to calculate statistics and enable better/more query plan optimisations.
## What changes are included in this PR?
<!--
There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes in this PR.
-->
- Additional data types are supported: `Timestamp`, `Date`, `Time`, `Duration`, `Interval`
- Additional binary operators are supported: `NotEq`, `IsDistinctFrom`, `IsNotDistinctFrom`
- Whether an expression supports bounds evaluation or not is now part of the `PhysicalExpr` trait and therefore decided by every expression type itself. This will also enables UDFs to implement bounds evaluation.
## Are these changes tested?
yes
<!--
We typically require tests for all PRs in order to:
1. Prevent the code from being accidentally broken by subsequent changes
2. Serve as another way to document the expected behavior of the code
If tests are not included in your PR, please explain why (for example, are they covered by existing tests)?
-->
## Are there any user-facing changes?
`PhysicalExpr` and `ScalarUDFImpl` change, but both provide default implementations for additional functions.
<!--
If there are user-facing changes then we may require documentation to be updated before approving the PR.
-->
<!--
If there are any breaking changes to public APIs, please add the `api change` label.
-->
--
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] Support bounds evaluation for temporal data types [datafusion]
Posted by "ch-sc (via GitHub)" <gi...@apache.org>.
ch-sc commented on PR #14523:
URL: https://github.com/apache/datafusion/pull/14523#issuecomment-2728490058
Hi @berkaysynnada, I should be able to spend some time on this at the end of this week.
--
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] Support bounds evaluation for temporal data types [datafusion]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #14523:
URL: https://github.com/apache/datafusion/pull/14523#issuecomment-2978700370
Thank you for your contribution. Unfortunately, this pull request is stale because it has been open 60 days with no activity. Please remove the stale label or comment or this will be closed in 7 days.
--
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] Support bounds evaluation for temporal data types [datafusion]
Posted by "ch-sc (via GitHub)" <gi...@apache.org>.
ch-sc commented on code in PR #14523:
URL: https://github.com/apache/datafusion/pull/14523#discussion_r1967699042
##########
datafusion/expr/src/udf.rs:
##########
@@ -717,9 +722,18 @@ pub trait ScalarUDFImpl: Debug + Send + Sync {
///
/// If the function is `ABS(a)`, and the input interval is `a: [-3, 2]`,
/// then the output interval would be `[0, 3]`.
- fn evaluate_bounds(&self, _input: &[&Interval]) -> Result<Interval> {
- // We cannot assume the input datatype is the same of output type.
- Interval::make_unbounded(&DataType::Null)
+ fn evaluate_bounds(&self, input: &[&Interval]) -> Result<Interval> {
Review Comment:
This should work similar to what we do in PhysicalExpr. Be default, this is not supported, but every UDF can implement its own support.
`evaluate_bounds` in ScalarUDFImpl returns an unbounded Interval which is not helpful for statistics. This is rather a leftover of my attempt to make use of known bounds as much as possible and ignore null bounds. We should probably throw a not implemented error here as well.
--
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] Support bounds evaluation for temporal data types [datafusion]
Posted by "berkaysynnada (via GitHub)" <gi...@apache.org>.
berkaysynnada commented on code in PR #14523:
URL: https://github.com/apache/datafusion/pull/14523#discussion_r1966851842
##########
datafusion/common/src/scalar/mod.rs:
##########
@@ -1583,6 +1583,17 @@ impl ScalarValue {
}
}
+ /// Returns negation for a boolean scalar value
+ pub fn boolean_negate(&self) -> Result<Self> {
Review Comment:
Maybe we can extend arithmetic_negate() with booleans, wdyt?
##########
datafusion/expr-common/src/interval_arithmetic.rs:
##########
@@ -424,6 +416,14 @@ impl Interval {
)
}
+ pub fn is_null(&self) -> bool {
Review Comment:
"null interval" term might mislead, maybe we can rename it as is_unbounded or is_infinite ?
##########
datafusion/physical-expr/src/expressions/in_list.rs:
##########
@@ -398,6 +399,51 @@ impl PhysicalExpr for InListExpr {
self.static_filter.clone(),
)))
}
+
+ /// The output interval is computed by checking if the list item intervals are
+ /// a subset of, overlap, or are disjoint with the input expression's interval.
+ ///
+ /// If [InListExpr::negated] is true, the output interval gets negated.
+ ///
+ /// # Example:
+ /// If the input expression's interval is a superset of the
+ /// conjunction of the list items intervals, the output
+ /// interval is [`Interval::CERTAINLY_TRUE`].
+ ///
+ /// ```text
+ /// interval of expr: ....---------------------....
+ /// Some list items: ..........|..|.....|.|.......
+ ///
+ /// output interval: [`true`, `true`]
+ /// ```
+ fn evaluate_bounds(&self, children: &[&Interval]) -> Result<Interval> {
+ let expr_bounds = children[0];
+
+ debug_assert!(
+ children.len() >= 2,
+ "InListExpr requires at least one list item"
+ );
+
+ // conjunction of list item intervals
+ let list_bounds = children
+ .iter()
+ .skip(2)
+ .try_fold(children[1].clone(), |acc, item| acc.union(*item))?;
+
+ if self.negated {
+ expr_bounds.contains(list_bounds)?.boolean_negate()
+ } else {
+ expr_bounds.contains(list_bounds)
Review Comment:
Let's say expr_bounds is [3,5], and list bounds are [[0,2], [6,9]]. As the union logic cannot yet support interval sets, the union result would be [0, 9], and this contains check will give certainly_true; however, the actual result is certainly_false. I think that's not what you prefer
Even if we can support interval sets, let's consider this example
expr_bounds is [0,10] & list_bounds: [[2,6], [3,5], [6,9]]. Do we want to return certainly_true? expr could be 5, and the list might include [3,4,7]. So, in_list() is false, even if we say its bounds are certainly_true. Am I missing something?
##########
datafusion/expr/src/udf.rs:
##########
@@ -717,9 +722,18 @@ pub trait ScalarUDFImpl: Debug + Send + Sync {
///
/// If the function is `ABS(a)`, and the input interval is `a: [-3, 2]`,
/// then the output interval would be `[0, 3]`.
- fn evaluate_bounds(&self, _input: &[&Interval]) -> Result<Interval> {
- // We cannot assume the input datatype is the same of output type.
- Interval::make_unbounded(&DataType::Null)
+ fn evaluate_bounds(&self, input: &[&Interval]) -> Result<Interval> {
Review Comment:
I really like getting rid of Null bounds and looking up the result type, but I have a question:
You declared supports_bounds_evaluation() as false. So, do the "evaluate" invokers even call this function after seeing supports_bounds_evaluation() as false? I think supports_bounds_evaluation() shouldn't say false
##########
datafusion/expr-common/src/operator.rs:
##########
@@ -164,6 +164,25 @@ impl Operator {
)
}
+ /// Indicates whether this operator supports interval arithmetic
+ pub fn supports_bounds_evaluation(&self) -> bool {
Review Comment:
supports_interval_evaluation or supports_range_evaluation, wdyt?
##########
datafusion/expr-common/src/interval_arithmetic.rs:
##########
@@ -880,6 +925,19 @@ impl Interval {
upper: self.lower().clone().arithmetic_negate()?,
})
}
+
+ pub fn boolean_negate(self) -> Result<Self> {
+ if self.data_type() != DataType::Boolean {
Review Comment:
likewise here, would it be too inclusive to use "arithmetic" keyword for booleans?
##########
datafusion/expr-common/src/interval_arithmetic.rs:
##########
@@ -902,6 +960,15 @@ pub fn apply_operator(op: &Operator, lhs: &Interval, rhs: &Interval) -> Result<I
Operator::Minus => lhs.sub(rhs),
Operator::Multiply => lhs.mul(rhs),
Operator::Divide => lhs.div(rhs),
+ Operator::IsDistinctFrom | Operator::IsNotDistinctFrom => {
+ NullableInterval::from(lhs)
+ .apply_operator(op, &rhs.into())
+ .and_then(|x| {
Review Comment:
We can avoid cloning here actually introducing a new taker API to NullableInterval
##########
datafusion/expr-common/src/interval_arithmetic.rs:
##########
@@ -306,23 +306,15 @@ impl Interval {
// Standardize floating-point endpoints:
DataType::Float32 => handle_float_intervals!(Float32, f32, lower, upper),
DataType::Float64 => handle_float_intervals!(Float64, f64, lower, upper),
- // Unsigned null values for lower bounds are set to zero:
- DataType::UInt8 if lower.is_null() => Self {
- lower: ScalarValue::UInt8(Some(0)),
- upper,
- },
- DataType::UInt16 if lower.is_null() => Self {
- lower: ScalarValue::UInt16(Some(0)),
- upper,
- },
- DataType::UInt32 if lower.is_null() => Self {
- lower: ScalarValue::UInt32(Some(0)),
- upper,
- },
- DataType::UInt64 if lower.is_null() => Self {
- lower: ScalarValue::UInt64(Some(0)),
- upper,
- },
+ // Lower bounds of unsigned integer null values are set to zero:
Review Comment:
👍🏻
--
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] Support bounds evaluation for temporal data types [datafusion]
Posted by "ch-sc (via GitHub)" <gi...@apache.org>.
ch-sc commented on code in PR #14523:
URL: https://github.com/apache/datafusion/pull/14523#discussion_r1968018999
##########
datafusion/physical-expr/src/expressions/in_list.rs:
##########
@@ -398,6 +399,51 @@ impl PhysicalExpr for InListExpr {
self.static_filter.clone(),
)))
}
+
+ /// The output interval is computed by checking if the list item intervals are
+ /// a subset of, overlap, or are disjoint with the input expression's interval.
+ ///
+ /// If [InListExpr::negated] is true, the output interval gets negated.
+ ///
+ /// # Example:
+ /// If the input expression's interval is a superset of the
+ /// conjunction of the list items intervals, the output
+ /// interval is [`Interval::CERTAINLY_TRUE`].
+ ///
+ /// ```text
+ /// interval of expr: ....---------------------....
+ /// Some list items: ..........|..|.....|.|.......
+ ///
+ /// output interval: [`true`, `true`]
+ /// ```
+ fn evaluate_bounds(&self, children: &[&Interval]) -> Result<Interval> {
+ let expr_bounds = children[0];
+
+ debug_assert!(
+ children.len() >= 2,
+ "InListExpr requires at least one list item"
+ );
+
+ // conjunction of list item intervals
+ let list_bounds = children
+ .iter()
+ .skip(2)
+ .try_fold(children[1].clone(), |acc, item| acc.union(*item))?;
+
+ if self.negated {
+ expr_bounds.contains(list_bounds)?.boolean_negate()
+ } else {
+ expr_bounds.contains(list_bounds)
Review Comment:
`contains` for `[3,5]` and `[0,9]` should return `UNCERTAIN`. I added a test with your example in in_list.rs.
`Certainly_true` should only be returned, if expr_bounds is a superset of the union of the list bounds.
--
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] Support bounds evaluation for temporal data types [datafusion]
Posted by "berkaysynnada (via GitHub)" <gi...@apache.org>.
berkaysynnada commented on PR #14523:
URL: https://github.com/apache/datafusion/pull/14523#issuecomment-2812071975
Thank you @ch-sc. I will review it today
--
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] Support bounds evaluation for temporal data types [datafusion]
Posted by "ch-sc (via GitHub)" <gi...@apache.org>.
ch-sc commented on PR #14523:
URL: https://github.com/apache/datafusion/pull/14523#issuecomment-2665828516
Looks like I confused the meaning of nulls in intervals. I switched to the union logic @berkaysynnada suggested.
--
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] Support bounds evaluation for temporal data types [datafusion]
Posted by "ch-sc (via GitHub)" <gi...@apache.org>.
ch-sc commented on code in PR #14523:
URL: https://github.com/apache/datafusion/pull/14523#discussion_r1967699042
##########
datafusion/expr/src/udf.rs:
##########
@@ -717,9 +722,18 @@ pub trait ScalarUDFImpl: Debug + Send + Sync {
///
/// If the function is `ABS(a)`, and the input interval is `a: [-3, 2]`,
/// then the output interval would be `[0, 3]`.
- fn evaluate_bounds(&self, _input: &[&Interval]) -> Result<Interval> {
- // We cannot assume the input datatype is the same of output type.
- Interval::make_unbounded(&DataType::Null)
+ fn evaluate_bounds(&self, input: &[&Interval]) -> Result<Interval> {
Review Comment:
This should work similar to what we do in PhysicalExpr. By default, this is not supported, but every UDF can implement its own version of bounds evaluation.
`evaluate_bounds` in ScalarUDFImpl returns an unbounded Interval which is not helpful for statistics. This is rather a leftover of my attempt to make use of known bounds as much as possible and ignore null bounds. We should probably throw a not implemented error here as well.
--
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] Support bounds evaluation for temporal data types [datafusion]
Posted by "ch-sc (via GitHub)" <gi...@apache.org>.
ch-sc commented on PR #14523:
URL: https://github.com/apache/datafusion/pull/14523#issuecomment-2653779520
@berkaysynnada do you have time to take another look? :)
`NotEq` leads to the removal of the sort operator.
I debugged into this and noticed that the `EnforceSorting` optimiser removes `SortExec`. [enforce_sorting/mod.rs:415][1]
However, prior to that the order requirement somehow is lost and I don't understand why that happens.
I removed `NotEq` again from the supported binary operators to move this forward. We might want to look into this in a follow-up PR. WDYT?
[1]: https://github.com/apache/datafusion/blob/02cc22eebd7e422cbba6788971f2ffae103180e8/datafusion/physical-optimizer/src/enforce_sorting/mod.rs#L415
--
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] Support bounds evaluation for temporal data types [datafusion]
Posted by "ch-sc (via GitHub)" <gi...@apache.org>.
ch-sc commented on PR #14523:
URL: https://github.com/apache/datafusion/pull/14523#issuecomment-2642837400
Thanks @berkaysynnada! Yeah I'd like to get a review.
There is still a bug, though, that I don't understand yet. Sometimes the sort operator gets removed as can be seen in the `repartition_scan.slt` test.
--
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] Support bounds evaluation for temporal data types [datafusion]
Posted by "berkaysynnada (via GitHub)" <gi...@apache.org>.
berkaysynnada commented on PR #14523:
URL: https://github.com/apache/datafusion/pull/14523#issuecomment-2642846047
> Sometimes the sort operator gets removed as can be seen in the `repartition_scan.slt` test.
Seems like you're losing the order requirement somehow
--
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] Support bounds evaluation for temporal data types [datafusion]
Posted by "ch-sc (via GitHub)" <gi...@apache.org>.
ch-sc commented on code in PR #14523:
URL: https://github.com/apache/datafusion/pull/14523#discussion_r1968018999
##########
datafusion/physical-expr/src/expressions/in_list.rs:
##########
@@ -398,6 +399,51 @@ impl PhysicalExpr for InListExpr {
self.static_filter.clone(),
)))
}
+
+ /// The output interval is computed by checking if the list item intervals are
+ /// a subset of, overlap, or are disjoint with the input expression's interval.
+ ///
+ /// If [InListExpr::negated] is true, the output interval gets negated.
+ ///
+ /// # Example:
+ /// If the input expression's interval is a superset of the
+ /// conjunction of the list items intervals, the output
+ /// interval is [`Interval::CERTAINLY_TRUE`].
+ ///
+ /// ```text
+ /// interval of expr: ....---------------------....
+ /// Some list items: ..........|..|.....|.|.......
+ ///
+ /// output interval: [`true`, `true`]
+ /// ```
+ fn evaluate_bounds(&self, children: &[&Interval]) -> Result<Interval> {
+ let expr_bounds = children[0];
+
+ debug_assert!(
+ children.len() >= 2,
+ "InListExpr requires at least one list item"
+ );
+
+ // conjunction of list item intervals
+ let list_bounds = children
+ .iter()
+ .skip(2)
+ .try_fold(children[1].clone(), |acc, item| acc.union(*item))?;
+
+ if self.negated {
+ expr_bounds.contains(list_bounds)?.boolean_negate()
+ } else {
+ expr_bounds.contains(list_bounds)
Review Comment:
`contains` for `[3,5]` and `[0,9]` should return `UNCERTAIN`. I added a test with your example in in_list.rs.
`CERTAINLY_TRUE` should only be returned if expr_bounds is a superset of the union of the list bounds.
--
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] Support bounds evaluation for temporal data types [datafusion]
Posted by "ch-sc (via GitHub)" <gi...@apache.org>.
ch-sc commented on code in PR #14523:
URL: https://github.com/apache/datafusion/pull/14523#discussion_r1967570230
##########
datafusion/common/src/scalar/mod.rs:
##########
@@ -1583,6 +1583,17 @@ impl ScalarValue {
}
}
+ /// Returns negation for a boolean scalar value
+ pub fn boolean_negate(&self) -> Result<Self> {
Review Comment:
We can do that, but I think we should then rename `arithmetic_negate` to just `negate` to cause no ambiguity. WDYT?
--
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] Support bounds evaluation for temporal data types [datafusion]
Posted by "berkaysynnada (via GitHub)" <gi...@apache.org>.
berkaysynnada commented on PR #14523:
URL: https://github.com/apache/datafusion/pull/14523#issuecomment-2792466034
> Hi @berkaysynnada, can you take another look to move this forward? :)
I will do but some tests are failing. I'll have some time probably in the evening or tomorrow morning
--
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] Support bounds evaluation for temporal data types [datafusion]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] closed pull request #14523: Support bounds evaluation for temporal data types
URL: https://github.com/apache/datafusion/pull/14523
--
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] Support bounds evaluation for temporal data types [datafusion]
Posted by "berkaysynnada (via GitHub)" <gi...@apache.org>.
berkaysynnada commented on code in PR #14523:
URL: https://github.com/apache/datafusion/pull/14523#discussion_r2049071746
##########
datafusion/expr-common/src/interval_arithmetic.rs:
##########
@@ -963,6 +961,23 @@ pub fn apply_operator(op: &Operator, lhs: &Interval, rhs: &Interval) -> Result<I
Operator::Minus => lhs.sub(rhs),
Operator::Multiply => lhs.mul(rhs),
Operator::Divide => lhs.div(rhs),
+ Operator::IsDistinctFrom | Operator::IsNotDistinctFrom => {
Review Comment:
can apply_operator() return NullableInterval::Null or NullableInterval::MaybeNull here? isn't it strange for isDistinct and isNotDistinct?
##########
datafusion/expr-common/src/interval_arithmetic.rs:
##########
@@ -1690,6 +1705,24 @@ impl Display for NullableInterval {
}
}
+impl From<&Interval> for NullableInterval {
Review Comment:
This conversion is dangerous. Null means different things in Interval and NullableInterval. Interval's can be converted to NotNull types only in the current behavior. Where do we need such a conversion, and why? can you elaborate a bit
##########
datafusion/expr-common/src/interval_arithmetic.rs:
##########
@@ -631,13 +623,19 @@ impl Interval {
/// to an error.
pub fn intersect<T: Borrow<Self>>(&self, other: T) -> Result<Option<Self>> {
let rhs = other.borrow();
+
if self.data_type().ne(&rhs.data_type()) {
- return internal_err!(
- "Only intervals with the same data type are intersectable, lhs:{}, rhs:{}",
- self.data_type(),
- rhs.data_type()
- );
- };
+ BinaryTypeCoercer::new(&self.data_type(), &Operator::Plus, &rhs.data_type()).get_result_type()
Review Comment:
should we check the result type is also supported by evaluate_bounds? WDYT
##########
datafusion/physical-expr-common/src/physical_expr.rs:
##########
@@ -126,6 +126,25 @@ pub trait PhysicalExpr: Send + Sync + Display + Debug + DynEq + DynHash {
not_impl_err!("Not implemented for {self}")
}
+ /// Checks support of bounds evaluation for this expression, before evaluating bounds.
+ /// Returns None if bounds evaluation is not supported.
+ fn evaluate_bounds_checked(
Review Comment:
We have supports_bounds_evaluation() there already. I think no need to introduce another API in PhysicalExpr. If we see lots of repetition of "first supports_bounds_evaluation, then evaluate_bounds" pattern, then we can make a standalone function, IMO
##########
datafusion/physical-expr/src/analysis.rs:
##########
@@ -94,17 +98,10 @@ pub struct ExprBoundaries {
impl ExprBoundaries {
/// Create a new `ExprBoundaries` object from column level statistics.
pub fn try_from_column(
- schema: &Schema,
+ field: &Field,
col_stats: &ColumnStatistics,
col_index: usize,
) -> Result<Self> {
- let field = schema.fields().get(col_index).ok_or_else(|| {
Review Comment:
Why do we spread this check over all caller side code?
##########
datafusion/physical-expr/src/expressions/in_list.rs:
##########
@@ -414,6 +415,51 @@ impl PhysicalExpr for InListExpr {
}
write!(f, ")")
}
+
+ /// The output interval is computed by checking if the list item intervals are
+ /// a subset of, overlap, or are disjoint with the input expression's interval.
+ ///
+ /// If [InListExpr::negated] is true, the output interval gets negated.
+ ///
+ /// # Example:
+ /// If the input expression's interval is a superset of the
+ /// conjunction of the list items intervals, the output
+ /// interval is [`Interval::CERTAINLY_TRUE`].
+ ///
+ /// ```text
+ /// interval of expr: ....---------------------....
+ /// Some list items: ..........|..|.....|.|.......
+ ///
+ /// output interval: [`true`, `true`]
+ /// ```
+ fn evaluate_bounds(&self, children: &[&Interval]) -> Result<Interval> {
Review Comment:
My previous concern still applies. I think there should be 3 different paths of this logic:
1) expr_bounds is singleton:
check the inList expression **separately**. If any of them is also singleton, and has the same value with expr_bounds, then the result is CertainlyTrue
2) expr_bounds is not a singleton => intersect expr_bounds with all inList expressions.
a. if any of inList expressions intersect with expr_bounds, then the result is Uncertain
b. if any of inList does not intersect with expr_bounds, then the result is CertainlyFalse
--
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] Support bounds evaluation for temporal data types [datafusion]
Posted by "berkaysynnada (via GitHub)" <gi...@apache.org>.
berkaysynnada commented on PR #14523:
URL: https://github.com/apache/datafusion/pull/14523#issuecomment-2642167448
Thank you @ch-sc for working on this. When you need a review, I can do that if you ping me.
--
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] Support bounds evaluation for temporal data types [datafusion]
Posted by "ch-sc (via GitHub)" <gi...@apache.org>.
ch-sc commented on PR #14523:
URL: https://github.com/apache/datafusion/pull/14523#issuecomment-2656384285
> I try to add support for NotEq. If I find a solution, I can fix it within this PR itself—if that's okay with you.
Sure, feel free to make any adjustment as you see fit.
--
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] Support bounds evaluation for temporal data types [datafusion]
Posted by "ch-sc (via GitHub)" <gi...@apache.org>.
ch-sc commented on PR #14523:
URL: https://github.com/apache/datafusion/pull/14523#issuecomment-2789351136
Hi @berkaysynnada, can you take another look to move this forward? :)
--
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] Support bounds evaluation for temporal data types [datafusion]
Posted by "berkaysynnada (via GitHub)" <gi...@apache.org>.
berkaysynnada commented on PR #14523:
URL: https://github.com/apache/datafusion/pull/14523#issuecomment-2796267763
Hi @ch-sc. Reviewing while tests are failing is not very efficient. Are you dealing with 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
Re: [PR] Support bounds evaluation for temporal data types [datafusion]
Posted by "berkaysynnada (via GitHub)" <gi...@apache.org>.
berkaysynnada commented on PR #14523:
URL: https://github.com/apache/datafusion/pull/14523#issuecomment-2653891174
> @berkaysynnada do you have time to take another look? :)
Thanks again for driving this forward :) I'm a bit busy these days, but I'd love to go over it. If it's not urgent for you, would you mind waiting until the weekend?
> I removed `NotEq` again from the supported binary operators to move this forward. We might want to look into this in a follow-up PR. WDYT?
Thanks for the heads-up. I try to add support for NotEq. If I find a solution, I can fix it within this PR itself—if that's okay with you.
I took a quick look, and you can use the union logic there:
https://github.com/Fly-Style/datafusion/blob/fee3023a63227f0a22ac2da1d040d373cc028c34/datafusion/expr-common/src/interval_arithmetic.rs#L667
It seems to follow a better style, IMO.
--
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] Support bounds evaluation for temporal data types [datafusion]
Posted by "ch-sc (via GitHub)" <gi...@apache.org>.
ch-sc commented on PR #14523:
URL: https://github.com/apache/datafusion/pull/14523#issuecomment-2660141218
> I took a quick look, and you can use the union logic there
I thought a bit more about the union logic and came up with another solution.
Sometimes intervals are not overlapping and sometimes have no bound in one direction (`NULL`). Typically the union of non-overlapping intervals would result in a set of intervals, but there is no support of this.
Here are some examples of what I think should be correct, without interval sets support:
```
[1, 2] ∪ [3,4] = [1,4]
[1, NULL] ∪ [NULL, NULL] = [1, NULL]
[NULL, 1] ∪ [NULL, NULL] = [NULL, 1]
[3, NULL] ∪ [NULL, 1] = [NULL, NULL]
[5, NULL] ∪ [1, 2] = [1, NULL]
```
I adapted the union logic accordingly and added tests. Happy for your feedback!
--
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] Support bounds evaluation for temporal data types [datafusion]
Posted by "berkaysynnada (via GitHub)" <gi...@apache.org>.
berkaysynnada commented on PR #14523:
URL: https://github.com/apache/datafusion/pull/14523#issuecomment-2727459728
Hi @ch-sc. Do you plan to complete the work here? (as it's so almost)
--
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] Support bounds evaluation for temporal data types [datafusion]
Posted by "ch-sc (via GitHub)" <gi...@apache.org>.
ch-sc commented on code in PR #14523:
URL: https://github.com/apache/datafusion/pull/14523#discussion_r2014120634
##########
datafusion/expr-common/src/interval_arithmetic.rs:
##########
@@ -902,6 +960,15 @@ pub fn apply_operator(op: &Operator, lhs: &Interval, rhs: &Interval) -> Result<I
Operator::Minus => lhs.sub(rhs),
Operator::Multiply => lhs.mul(rhs),
Operator::Divide => lhs.div(rhs),
+ Operator::IsDistinctFrom | Operator::IsNotDistinctFrom => {
+ NullableInterval::from(lhs)
+ .apply_operator(op, &rhs.into())
+ .and_then(|x| {
Review Comment:
Not sure how you would do that.
--
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] Support bounds evaluation for temporal data types [datafusion]
Posted by "ch-sc (via GitHub)" <gi...@apache.org>.
ch-sc commented on PR #14523:
URL: https://github.com/apache/datafusion/pull/14523#issuecomment-2682042295
@berkaysynnada thank you for your review 🙂
> Rather than introducing new supports_() API's, should we force the users to infer the support by the evaluate API?
I agree, something like that would be ideal. I'll look into it today
--
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] Support bounds evaluation for temporal data types [datafusion]
Posted by "ch-sc (via GitHub)" <gi...@apache.org>.
ch-sc commented on PR #14523:
URL: https://github.com/apache/datafusion/pull/14523#issuecomment-2797103457
Sorry @berkaysynnada, I got side-tracked from this yesterday. The test is fixed.
--
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