You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "alamb (via GitHub)" <gi...@apache.org> on 2026/01/26 13:07:22 UTC
[PR] Improve documentation for ScalarUDFImpl::preimage [datafusion]
alamb opened a new pull request, #20008:
URL: https://github.com/apache/datafusion/pull/20008
## Which issue does this PR close?
- Related to https://github.com/apache/datafusion/issues/18319#issuecomment-3798031798
## Rationale for this change
As @sdf-jkl @drin @2010YOUY01 and I have been discussing in https://github.com/apache/datafusion/issues/18319#issuecomment-3798031798, the new `preimage` API is somewhat subtle.
@sdf-jkl did a great job with the initial documentation but let's try and reduce the cognative load slightly
## What changes are included in this PR?
Rework the documentation to try and make it easier to understand
## Are these changes tested?
Checked by doc CI, and I rendered it locally:
<img width="1060" height="858" alt="Screenshot 2026-01-26 at 8 06 57 AM" src="https://github.com/user-attachments/assets/27bcaea8-833e-4c0c-9bc6-c9656e15641d" />
## Are there any user-facing changes?
<!--
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] Improve documentation for ScalarUDFImpl::preimage [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #20008:
URL: https://github.com/apache/datafusion/pull/20008#issuecomment-3819500118
Thanks again everyone for the reviews
--
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] Improve documentation for ScalarUDFImpl::preimage [datafusion]
Posted by "sdf-jkl (via GitHub)" <gi...@apache.org>.
sdf-jkl commented on code in PR #20008:
URL: https://github.com/apache/datafusion/pull/20008#discussion_r2732339346
##########
datafusion/expr/src/udf.rs:
##########
@@ -709,20 +709,49 @@ pub trait ScalarUDFImpl: Debug + DynEq + DynHash + Send + Sync {
Ok(ExprSimplifyResult::Original(args))
}
- /// Returns the [preimage] for this function and the specified scalar value, if any.
+ /// Returns a single contiguous preimage for this function and the specified
+ /// scalar expression, if any.
///
- /// A preimage is a single contiguous [`Interval`] of values where the function
- /// will always return `lit_value`
+ /// # Return Value
///
- /// Implementations should return intervals with an inclusive lower bound and
- /// exclusive upper bound.
+ /// Implementations should return a half-open interval: inclusive lower
+ /// bound and exclusive upper bound. Note that this is slightly different
+ /// from normal [`Interval`] semantics where the upper bound is closed. The
+ /// upper endpoint should be adjusted to the next value.
///
- /// This rewrite is described in the [ClickHouse Paper] and is particularly
- /// useful for simplifying expressions `date_part` or equivalent functions. The
- /// idea is that if you have an expression like `date_part(YEAR, k) = 2024` and you
- /// can find a [preimage] for `date_part(YEAR, k)`, which is the range of dates
- /// covering the entire year of 2024. Thus, you can rewrite the expression to `k
- /// >= '2024-01-01' AND k < '2025-01-01' which is often more optimizable.
+ /// # Background
+ ///
+ /// A [preimage] here is a single contiguous [`Interval`] of the function's
+ /// argument(s) where the function will return a single literal (constant)
+ /// value. This can also be thought of as a form of interval containment.
+ ///
+ /// Using a preimage to rewrite predicates is described in the [ClickHouse
+ /// Paper]:
+ ///
+ /// > some functions can compute the preimage of a given function result.
+ /// > This is used to replace comparisons of constants with function calls
+ /// > on the key columns by comparing the key column value with the preimage.
+ /// > For example, `toYear(k) = 2024` can be replaced by
+ /// > `k >= 2024-01-01 && k < 2025-01-01`
+ ///
+ /// As mentioned above, the preimage can be used to simplify certain types of
+ /// expressions such as `date_part` into a form that is more optimizable.
+ ///
+ /// For example, given an expression like
+ /// ```sql
+ /// date_part(YEAR, k) = 2024
+ /// ```
+ ///
+ /// There is a single preimage [`2024-01-01`, `2025-01-01`), which is the
+ /// range of dates covering the entire year of 2024 for which
+ /// `date_part(YEAR, k)` evaluates to `2024`. Using this preimage the
+ /// expression can be rewritten to
Review Comment:
@paleolimbot
--
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] Improve documentation for ScalarUDFImpl::preimage [datafusion]
Posted by "drin (via GitHub)" <gi...@apache.org>.
drin commented on code in PR #20008:
URL: https://github.com/apache/datafusion/pull/20008#discussion_r2729394863
##########
datafusion/expr/src/udf.rs:
##########
@@ -709,20 +709,49 @@ pub trait ScalarUDFImpl: Debug + DynEq + DynHash + Send + Sync {
Ok(ExprSimplifyResult::Original(args))
}
- /// Returns the [preimage] for this function and the specified scalar value, if any.
+ /// Returns a single contiguous preimage for this function and the specified
+ /// scalar expression, if any.
///
- /// A preimage is a single contiguous [`Interval`] of values where the function
- /// will always return `lit_value`
+ /// # Return Value
///
- /// Implementations should return intervals with an inclusive lower bound and
- /// exclusive upper bound.
+ /// Implementations should return a half-open interval: inclusive lower
+ /// bound and exclusive upper bound. Note that this is slightly different
+ /// from normal [`Interval`] semantics where the upper bound is closed. The
+ /// upper endpoint should be adjusted to the next value.
///
- /// This rewrite is described in the [ClickHouse Paper] and is particularly
- /// useful for simplifying expressions `date_part` or equivalent functions. The
- /// idea is that if you have an expression like `date_part(YEAR, k) = 2024` and you
- /// can find a [preimage] for `date_part(YEAR, k)`, which is the range of dates
- /// covering the entire year of 2024. Thus, you can rewrite the expression to `k
- /// >= '2024-01-01' AND k < '2025-01-01' which is often more optimizable.
+ /// # Background
+ ///
+ /// A [preimage] here is a single contiguous [`Interval`] of the function's
+ /// argument(s) where the function will return a single literal (constant)
+ /// value. This can also be thought of as a form of interval containment.
+ ///
+ /// Using a preimage to rewrite predicates is described in the [ClickHouse
+ /// Paper]:
Review Comment:
```suggestion
/// Inspired by the [ClickHouse Paper], a "preimage rewrite" transforms
/// a predicate containing a function call into a predicate containing an
/// equivalent set of input literal (constant) values. The resulting predicate
/// is then in a form that can be further optimized by other rewrites.
/// From the paper:
```
--
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] Improve documentation for ScalarUDFImpl::preimage [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #20008:
URL: https://github.com/apache/datafusion/pull/20008#issuecomment-3807654020
Now that we have nice documentation for this API, we need to actually go and make use of it 😆
--
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] Improve documentation for ScalarUDFImpl::preimage [datafusion]
Posted by "drin (via GitHub)" <gi...@apache.org>.
drin commented on code in PR #20008:
URL: https://github.com/apache/datafusion/pull/20008#discussion_r2729394863
##########
datafusion/expr/src/udf.rs:
##########
@@ -709,20 +709,49 @@ pub trait ScalarUDFImpl: Debug + DynEq + DynHash + Send + Sync {
Ok(ExprSimplifyResult::Original(args))
}
- /// Returns the [preimage] for this function and the specified scalar value, if any.
+ /// Returns a single contiguous preimage for this function and the specified
+ /// scalar expression, if any.
///
- /// A preimage is a single contiguous [`Interval`] of values where the function
- /// will always return `lit_value`
+ /// # Return Value
///
- /// Implementations should return intervals with an inclusive lower bound and
- /// exclusive upper bound.
+ /// Implementations should return a half-open interval: inclusive lower
+ /// bound and exclusive upper bound. Note that this is slightly different
+ /// from normal [`Interval`] semantics where the upper bound is closed. The
+ /// upper endpoint should be adjusted to the next value.
///
- /// This rewrite is described in the [ClickHouse Paper] and is particularly
- /// useful for simplifying expressions `date_part` or equivalent functions. The
- /// idea is that if you have an expression like `date_part(YEAR, k) = 2024` and you
- /// can find a [preimage] for `date_part(YEAR, k)`, which is the range of dates
- /// covering the entire year of 2024. Thus, you can rewrite the expression to `k
- /// >= '2024-01-01' AND k < '2025-01-01' which is often more optimizable.
+ /// # Background
+ ///
+ /// A [preimage] here is a single contiguous [`Interval`] of the function's
+ /// argument(s) where the function will return a single literal (constant)
+ /// value. This can also be thought of as a form of interval containment.
+ ///
+ /// Using a preimage to rewrite predicates is described in the [ClickHouse
+ /// Paper]:
Review Comment:
```suggestion
/// Inspired by the [ClickHouse Paper], a "preimage rewrite" transforms
/// a predicate containing a function call with a predicate containing an
/// equivalent set of input literal (constant) values. From the paper:
```
--
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] Improve documentation for ScalarUDFImpl::preimage [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #20008:
URL: https://github.com/apache/datafusion/pull/20008#discussion_r2734007623
##########
datafusion/expr/src/udf.rs:
##########
@@ -709,20 +709,49 @@ pub trait ScalarUDFImpl: Debug + DynEq + DynHash + Send + Sync {
Ok(ExprSimplifyResult::Original(args))
}
- /// Returns the [preimage] for this function and the specified scalar value, if any.
+ /// Returns a single contiguous preimage for this function and the specified
+ /// scalar expression, if any.
///
- /// A preimage is a single contiguous [`Interval`] of values where the function
- /// will always return `lit_value`
+ /// # Return Value
///
- /// Implementations should return intervals with an inclusive lower bound and
- /// exclusive upper bound.
+ /// Implementations should return a half-open interval: inclusive lower
+ /// bound and exclusive upper bound. Note that this is slightly different
Review Comment:
I added this in the "# Half-Open Intervals" section
--
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] Improve documentation for ScalarUDFImpl::preimage [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #20008:
URL: https://github.com/apache/datafusion/pull/20008#discussion_r2728566147
##########
datafusion/expr/src/udf.rs:
##########
@@ -709,20 +709,49 @@ pub trait ScalarUDFImpl: Debug + DynEq + DynHash + Send + Sync {
Ok(ExprSimplifyResult::Original(args))
}
- /// Returns the [preimage] for this function and the specified scalar value, if any.
+ /// Returns a single contiguous preimage for this function and the specified
+ /// scalar expression, if any.
///
- /// A preimage is a single contiguous [`Interval`] of values where the function
- /// will always return `lit_value`
+ /// # Return Value
///
- /// Implementations should return intervals with an inclusive lower bound and
- /// exclusive upper bound.
+ /// Implementations should return a half-open interval: inclusive lower
+ /// bound and exclusive upper bound. Note that this is slightly different
Review Comment:
As in I was thinking explaining the rationale for the use of exclusive upper bounds would be valuable (as otherwise it may just appear to be an inconsistency due to oversight)
--
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] Improve documentation for ScalarUDFImpl::preimage [datafusion]
Posted by "drin (via GitHub)" <gi...@apache.org>.
drin commented on code in PR #20008:
URL: https://github.com/apache/datafusion/pull/20008#discussion_r2729411211
##########
datafusion/expr/src/udf.rs:
##########
@@ -709,20 +709,49 @@ pub trait ScalarUDFImpl: Debug + DynEq + DynHash + Send + Sync {
Ok(ExprSimplifyResult::Original(args))
}
- /// Returns the [preimage] for this function and the specified scalar value, if any.
+ /// Returns a single contiguous preimage for this function and the specified
+ /// scalar expression, if any.
///
- /// A preimage is a single contiguous [`Interval`] of values where the function
- /// will always return `lit_value`
+ /// # Return Value
///
- /// Implementations should return intervals with an inclusive lower bound and
- /// exclusive upper bound.
+ /// Implementations should return a half-open interval: inclusive lower
+ /// bound and exclusive upper bound. Note that this is slightly different
+ /// from normal [`Interval`] semantics where the upper bound is closed. The
+ /// upper endpoint should be adjusted to the next value.
///
- /// This rewrite is described in the [ClickHouse Paper] and is particularly
- /// useful for simplifying expressions `date_part` or equivalent functions. The
- /// idea is that if you have an expression like `date_part(YEAR, k) = 2024` and you
- /// can find a [preimage] for `date_part(YEAR, k)`, which is the range of dates
- /// covering the entire year of 2024. Thus, you can rewrite the expression to `k
- /// >= '2024-01-01' AND k < '2025-01-01' which is often more optimizable.
+ /// # Background
+ ///
+ /// A [preimage] here is a single contiguous [`Interval`] of the function's
+ /// argument(s) where the function will return a single literal (constant)
+ /// value. This can also be thought of as a form of interval containment.
+ ///
+ /// Using a preimage to rewrite predicates is described in the [ClickHouse
+ /// Paper]:
+ ///
+ /// > some functions can compute the preimage of a given function result.
+ /// > This is used to replace comparisons of constants with function calls
+ /// > on the key columns by comparing the key column value with the preimage.
+ /// > For example, `toYear(k) = 2024` can be replaced by
+ /// > `k >= 2024-01-01 && k < 2025-01-01`
+ ///
+ /// As mentioned above, the preimage can be used to simplify certain types of
+ /// expressions such as `date_part` into a form that is more optimizable.
Review Comment:
```suggestion
```
--
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] Improve documentation for ScalarUDFImpl::preimage [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #20008:
URL: https://github.com/apache/datafusion/pull/20008
--
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] Improve documentation for ScalarUDFImpl::preimage [datafusion]
Posted by "drin (via GitHub)" <gi...@apache.org>.
drin commented on code in PR #20008:
URL: https://github.com/apache/datafusion/pull/20008#discussion_r2729320568
##########
datafusion/expr/src/udf.rs:
##########
@@ -709,20 +709,49 @@ pub trait ScalarUDFImpl: Debug + DynEq + DynHash + Send + Sync {
Ok(ExprSimplifyResult::Original(args))
}
- /// Returns the [preimage] for this function and the specified scalar value, if any.
+ /// Returns a single contiguous preimage for this function and the specified
+ /// scalar expression, if any.
///
- /// A preimage is a single contiguous [`Interval`] of values where the function
- /// will always return `lit_value`
+ /// # Return Value
///
- /// Implementations should return intervals with an inclusive lower bound and
- /// exclusive upper bound.
+ /// Implementations should return a half-open interval: inclusive lower
+ /// bound and exclusive upper bound. Note that this is slightly different
+ /// from normal [`Interval`] semantics where the upper bound is closed. The
+ /// upper endpoint should be adjusted to the next value.
///
- /// This rewrite is described in the [ClickHouse Paper] and is particularly
- /// useful for simplifying expressions `date_part` or equivalent functions. The
- /// idea is that if you have an expression like `date_part(YEAR, k) = 2024` and you
- /// can find a [preimage] for `date_part(YEAR, k)`, which is the range of dates
- /// covering the entire year of 2024. Thus, you can rewrite the expression to `k
- /// >= '2024-01-01' AND k < '2025-01-01' which is often more optimizable.
+ /// # Background
+ ///
+ /// A [preimage] here is a single contiguous [`Interval`] of the function's
+ /// argument(s) where the function will return a single literal (constant)
+ /// value. This can also be thought of as a form of interval containment.
+ ///
+ /// Using a preimage to rewrite predicates is described in the [ClickHouse
+ /// Paper]:
+ ///
+ /// > some functions can compute the preimage of a given function result.
+ /// > This is used to replace comparisons of constants with function calls
+ /// > on the key columns by comparing the key column value with the preimage.
+ /// > For example, `toYear(k) = 2024` can be replaced by
+ /// > `k >= 2024-01-01 && k < 2025-01-01`
+ ///
+ /// As mentioned above, the preimage can be used to simplify certain types of
+ /// expressions such as `date_part` into a form that is more optimizable.
+ ///
+ /// For example, given an expression like
+ /// ```sql
+ /// date_part(YEAR, k) = 2024
+ /// ```
+ ///
+ /// There is a single preimage [`2024-01-01`, `2025-01-01`), which is the
+ /// range of dates covering the entire year of 2024 for which
+ /// `date_part(YEAR, k)` evaluates to `2024`. Using this preimage the
+ /// expression can be rewritten to
Review Comment:
This is an attempt to more clearly use the [image of an element](https://en.wikipedia.org/wiki/Image_(mathematics)#Image_of_an_element) definition rather than the [image of a function definition](https://en.wikipedia.org/wiki/Image_(mathematics)#Image_of_a_function).
In this specific example, `2024` is a value of interest output by `date_part(YEAR, k)` and so we are concerned with it as an element for which we want to know preimage values. Whereas the image of `date_part(YEAR, k)` is any year (which is any value in the set of integers).
This is probably pedantic, but as long as we are using `image` and `preimage` terminology at all, I think it makes sense to be properly specific. And, according to wikipedia:
> In these definitions, f : X → Y is a function from the set X to the set Y.
Thus, either `date_part` is the function and x is `(YEAR, k)` or `date_part(YEAR)` is the function and x is `k`.
I think all of the terminology is cleanest if we think about the function being `date_part(YEAR)` and the input being a scalar date timestamp and the output being a scalar date timestamp and set X being a set of timestamps and the set Y being a set of timestamps (which has a cardinality of 1 for this function).
--
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] Improve documentation for ScalarUDFImpl::preimage [datafusion]
Posted by "sdf-jkl (via GitHub)" <gi...@apache.org>.
sdf-jkl commented on code in PR #20008:
URL: https://github.com/apache/datafusion/pull/20008#discussion_r2728255986
##########
datafusion/expr/src/udf.rs:
##########
@@ -709,20 +709,49 @@ pub trait ScalarUDFImpl: Debug + DynEq + DynHash + Send + Sync {
Ok(ExprSimplifyResult::Original(args))
}
- /// Returns the [preimage] for this function and the specified scalar value, if any.
+ /// Returns a single contiguous preimage for this function and the specified
+ /// scalar expression, if any.
///
- /// A preimage is a single contiguous [`Interval`] of values where the function
- /// will always return `lit_value`
+ /// # Return Value
///
- /// Implementations should return intervals with an inclusive lower bound and
- /// exclusive upper bound.
+ /// Implementations should return a half-open interval: inclusive lower
+ /// bound and exclusive upper bound. Note that this is slightly different
Review Comment:
It's extra complexity for calculating the upper bound in a preimage impl.
For example for date_part we would just subtract one time unit from each value.
Something like:
```rust
fn date_to_scalar(date: NaiveDate, target_type: &DataType) -> Option<ScalarValue> {
// --snip--
Some(match target_type {
Date32 => ScalarValue::Date32(Some(days as i32 - 1)),
Date64 => ScalarValue::Date64(Some(days * MILLISECONDS_IN_DAY - 1))
// utc timestamps are easy too
// still figuring out how to make tz timestamps work
```
Half open interval relies on logic and would make it more simple to implement a preimage function without adding extra calculations and less error-prone.
I also think it looks more clear when using exclusive upper range:
`<'01-01-2025T00:00:00'` vs `<='12-31-2024T23:59:999' `in sql `explain`.
Timestamps would still look the same though.
--
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] Improve documentation for ScalarUDFImpl::preimage [datafusion]
Posted by "drin (via GitHub)" <gi...@apache.org>.
drin commented on code in PR #20008:
URL: https://github.com/apache/datafusion/pull/20008#discussion_r2729423939
##########
datafusion/expr/src/udf.rs:
##########
@@ -709,20 +709,49 @@ pub trait ScalarUDFImpl: Debug + DynEq + DynHash + Send + Sync {
Ok(ExprSimplifyResult::Original(args))
}
- /// Returns the [preimage] for this function and the specified scalar value, if any.
+ /// Returns a single contiguous preimage for this function and the specified
+ /// scalar expression, if any.
///
- /// A preimage is a single contiguous [`Interval`] of values where the function
- /// will always return `lit_value`
+ /// # Return Value
///
- /// Implementations should return intervals with an inclusive lower bound and
- /// exclusive upper bound.
+ /// Implementations should return a half-open interval: inclusive lower
+ /// bound and exclusive upper bound. Note that this is slightly different
+ /// from normal [`Interval`] semantics where the upper bound is closed. The
+ /// upper endpoint should be adjusted to the next value.
///
- /// This rewrite is described in the [ClickHouse Paper] and is particularly
- /// useful for simplifying expressions `date_part` or equivalent functions. The
- /// idea is that if you have an expression like `date_part(YEAR, k) = 2024` and you
- /// can find a [preimage] for `date_part(YEAR, k)`, which is the range of dates
- /// covering the entire year of 2024. Thus, you can rewrite the expression to `k
- /// >= '2024-01-01' AND k < '2025-01-01' which is often more optimizable.
+ /// # Background
+ ///
+ /// A [preimage] here is a single contiguous [`Interval`] of the function's
+ /// argument(s) where the function will return a single literal (constant)
+ /// value. This can also be thought of as a form of interval containment.
+ ///
+ /// Using a preimage to rewrite predicates is described in the [ClickHouse
+ /// Paper]:
+ ///
+ /// > some functions can compute the preimage of a given function result.
+ /// > This is used to replace comparisons of constants with function calls
+ /// > on the key columns by comparing the key column value with the preimage.
+ /// > For example, `toYear(k) = 2024` can be replaced by
+ /// > `k >= 2024-01-01 && k < 2025-01-01`
+ ///
+ /// As mentioned above, the preimage can be used to simplify certain types of
+ /// expressions such as `date_part` into a form that is more optimizable.
+ ///
+ /// For example, given an expression like
+ /// ```sql
+ /// date_part(YEAR, k) = 2024
+ /// ```
+ ///
+ /// There is a single preimage [`2024-01-01`, `2025-01-01`), which is the
+ /// range of dates covering the entire year of 2024 for which
+ /// `date_part(YEAR, k)` evaluates to `2024`. Using this preimage the
+ /// expression can be rewritten to
+ ///
+ /// ```sql
+ /// k >= '2024-01-01' AND k < '2025-01-01'
+ /// ```
+ ///
+ /// which is often more optimizable, such as being used in min/max pruning.
///
/// [ClickHouse Paper]: https://www.vldb.org/pvldb/vol17/p3731-schulze.pdf
/// [preimage]: https://en.wikipedia.org/wiki/Image_(mathematics)#Inverse_image
Review Comment:
```suggestion
/// [image]: https://en.wikipedia.org/wiki/Image_(mathematics)#Image_of_an_element
/// [preimage]: https://en.wikipedia.org/wiki/Image_(mathematics)#Inverse_image
```
Should add which definition of image we're discussing, because "preimage" is just the inverse of whatever "image" means
--
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] Improve documentation for ScalarUDFImpl::preimage [datafusion]
Posted by "drin (via GitHub)" <gi...@apache.org>.
drin commented on code in PR #20008:
URL: https://github.com/apache/datafusion/pull/20008#discussion_r2729450819
##########
datafusion/expr/src/udf.rs:
##########
@@ -709,20 +709,49 @@ pub trait ScalarUDFImpl: Debug + DynEq + DynHash + Send + Sync {
Ok(ExprSimplifyResult::Original(args))
}
- /// Returns the [preimage] for this function and the specified scalar value, if any.
+ /// Returns a single contiguous preimage for this function and the specified
+ /// scalar expression, if any.
///
- /// A preimage is a single contiguous [`Interval`] of values where the function
- /// will always return `lit_value`
+ /// # Return Value
///
- /// Implementations should return intervals with an inclusive lower bound and
- /// exclusive upper bound.
+ /// Implementations should return a half-open interval: inclusive lower
+ /// bound and exclusive upper bound. Note that this is slightly different
+ /// from normal [`Interval`] semantics where the upper bound is closed. The
+ /// upper endpoint should be adjusted to the next value.
///
- /// This rewrite is described in the [ClickHouse Paper] and is particularly
- /// useful for simplifying expressions `date_part` or equivalent functions. The
- /// idea is that if you have an expression like `date_part(YEAR, k) = 2024` and you
- /// can find a [preimage] for `date_part(YEAR, k)`, which is the range of dates
- /// covering the entire year of 2024. Thus, you can rewrite the expression to `k
- /// >= '2024-01-01' AND k < '2025-01-01' which is often more optimizable.
+ /// # Background
+ ///
+ /// A [preimage] here is a single contiguous [`Interval`] of the function's
+ /// argument(s) where the function will return a single literal (constant)
+ /// value. This can also be thought of as a form of interval containment.
+ ///
+ /// Using a preimage to rewrite predicates is described in the [ClickHouse
+ /// Paper]:
+ ///
+ /// > some functions can compute the preimage of a given function result.
+ /// > This is used to replace comparisons of constants with function calls
+ /// > on the key columns by comparing the key column value with the preimage.
+ /// > For example, `toYear(k) = 2024` can be replaced by
+ /// > `k >= 2024-01-01 && k < 2025-01-01`
+ ///
+ /// As mentioned above, the preimage can be used to simplify certain types of
+ /// expressions such as `date_part` into a form that is more optimizable.
+ ///
+ /// For example, given an expression like
+ /// ```sql
+ /// date_part(YEAR, k) = 2024
+ /// ```
+ ///
+ /// There is a single preimage [`2024-01-01`, `2025-01-01`), which is the
+ /// range of dates covering the entire year of 2024 for which
+ /// `date_part(YEAR, k)` evaluates to `2024`. Using this preimage the
+ /// expression can be rewritten to
Review Comment:
Also, if we want the API to be extended beyond time intervals we probably don't need the documentation to specifically mention contiguous intervals.
Maybe geospatial use cases would be a good example for extending the API beyond just date intervals. Something like [map projections](https://en.wikipedia.org/wiki/Map_projection#Projections_by_preservation_of_a_metric_property). Maybe ping geodatafusion people if there's something interesting for them here?
--
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] Improve documentation for ScalarUDFImpl::preimage [datafusion]
Posted by "sdf-jkl (via GitHub)" <gi...@apache.org>.
sdf-jkl commented on code in PR #20008:
URL: https://github.com/apache/datafusion/pull/20008#discussion_r2728530316
##########
datafusion/expr/src/udf.rs:
##########
@@ -709,20 +709,49 @@ pub trait ScalarUDFImpl: Debug + DynEq + DynHash + Send + Sync {
Ok(ExprSimplifyResult::Original(args))
}
- /// Returns the [preimage] for this function and the specified scalar value, if any.
+ /// Returns a single contiguous preimage for this function and the specified
+ /// scalar expression, if any.
///
- /// A preimage is a single contiguous [`Interval`] of values where the function
- /// will always return `lit_value`
+ /// # Return Value
///
- /// Implementations should return intervals with an inclusive lower bound and
- /// exclusive upper bound.
+ /// Implementations should return a half-open interval: inclusive lower
+ /// bound and exclusive upper bound. Note that this is slightly different
Review Comment:
Only if we decide to make the upper bound inclusive
--
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] Improve documentation for ScalarUDFImpl::preimage [datafusion]
Posted by "drin (via GitHub)" <gi...@apache.org>.
drin commented on code in PR #20008:
URL: https://github.com/apache/datafusion/pull/20008#discussion_r2729394863
##########
datafusion/expr/src/udf.rs:
##########
@@ -709,20 +709,49 @@ pub trait ScalarUDFImpl: Debug + DynEq + DynHash + Send + Sync {
Ok(ExprSimplifyResult::Original(args))
}
- /// Returns the [preimage] for this function and the specified scalar value, if any.
+ /// Returns a single contiguous preimage for this function and the specified
+ /// scalar expression, if any.
///
- /// A preimage is a single contiguous [`Interval`] of values where the function
- /// will always return `lit_value`
+ /// # Return Value
///
- /// Implementations should return intervals with an inclusive lower bound and
- /// exclusive upper bound.
+ /// Implementations should return a half-open interval: inclusive lower
+ /// bound and exclusive upper bound. Note that this is slightly different
+ /// from normal [`Interval`] semantics where the upper bound is closed. The
+ /// upper endpoint should be adjusted to the next value.
///
- /// This rewrite is described in the [ClickHouse Paper] and is particularly
- /// useful for simplifying expressions `date_part` or equivalent functions. The
- /// idea is that if you have an expression like `date_part(YEAR, k) = 2024` and you
- /// can find a [preimage] for `date_part(YEAR, k)`, which is the range of dates
- /// covering the entire year of 2024. Thus, you can rewrite the expression to `k
- /// >= '2024-01-01' AND k < '2025-01-01' which is often more optimizable.
+ /// # Background
+ ///
+ /// A [preimage] here is a single contiguous [`Interval`] of the function's
+ /// argument(s) where the function will return a single literal (constant)
+ /// value. This can also be thought of as a form of interval containment.
+ ///
+ /// Using a preimage to rewrite predicates is described in the [ClickHouse
+ /// Paper]:
Review Comment:
```suggestion
/// Inspired by the [ClickHouse Paper], a "preimage rewrite" transforms
/// a predicate containing a function call with a predicate containing an
/// equivalent set of input literal (constant) values.
```
--
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] Improve documentation for ScalarUDFImpl::preimage [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #20008:
URL: https://github.com/apache/datafusion/pull/20008#discussion_r2733915208
##########
datafusion/expr/src/udf.rs:
##########
@@ -709,20 +709,49 @@ pub trait ScalarUDFImpl: Debug + DynEq + DynHash + Send + Sync {
Ok(ExprSimplifyResult::Original(args))
}
- /// Returns the [preimage] for this function and the specified scalar value, if any.
+ /// Returns a single contiguous preimage for this function and the specified
+ /// scalar expression, if any.
///
- /// A preimage is a single contiguous [`Interval`] of values where the function
- /// will always return `lit_value`
+ /// # Return Value
///
- /// Implementations should return intervals with an inclusive lower bound and
- /// exclusive upper bound.
+ /// Implementations should return a half-open interval: inclusive lower
+ /// bound and exclusive upper bound. Note that this is slightly different
+ /// from normal [`Interval`] semantics where the upper bound is closed. The
+ /// upper endpoint should be adjusted to the next value.
///
- /// This rewrite is described in the [ClickHouse Paper] and is particularly
- /// useful for simplifying expressions `date_part` or equivalent functions. The
- /// idea is that if you have an expression like `date_part(YEAR, k) = 2024` and you
- /// can find a [preimage] for `date_part(YEAR, k)`, which is the range of dates
- /// covering the entire year of 2024. Thus, you can rewrite the expression to `k
- /// >= '2024-01-01' AND k < '2025-01-01' which is often more optimizable.
+ /// # Background
+ ///
+ /// A [preimage] here is a single contiguous [`Interval`] of the function's
+ /// argument(s) where the function will return a single literal (constant)
+ /// value. This can also be thought of as a form of interval containment.
+ ///
+ /// Using a preimage to rewrite predicates is described in the [ClickHouse
+ /// Paper]:
+ ///
+ /// > some functions can compute the preimage of a given function result.
+ /// > This is used to replace comparisons of constants with function calls
+ /// > on the key columns by comparing the key column value with the preimage.
+ /// > For example, `toYear(k) = 2024` can be replaced by
+ /// > `k >= 2024-01-01 && k < 2025-01-01`
+ ///
+ /// As mentioned above, the preimage can be used to simplify certain types of
+ /// expressions such as `date_part` into a form that is more optimizable.
+ ///
+ /// For example, given an expression like
+ /// ```sql
+ /// date_part(YEAR, k) = 2024
+ /// ```
+ ///
+ /// There is a single preimage [`2024-01-01`, `2025-01-01`), which is the
+ /// range of dates covering the entire year of 2024 for which
+ /// `date_part(YEAR, k)` evaluates to `2024`. Using this preimage the
+ /// expression can be rewritten to
Review Comment:
As I understand it, the rewrite as currently implemented makes the assumption that the interval must be contiguous, even though in theory this same approach could be applied to non-contiguous ranges
--
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] Improve documentation for ScalarUDFImpl::preimage [datafusion]
Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot commented on code in PR #20008:
URL: https://github.com/apache/datafusion/pull/20008#discussion_r2733567918
##########
datafusion/expr/src/udf.rs:
##########
@@ -709,20 +709,49 @@ pub trait ScalarUDFImpl: Debug + DynEq + DynHash + Send + Sync {
Ok(ExprSimplifyResult::Original(args))
}
- /// Returns the [preimage] for this function and the specified scalar value, if any.
+ /// Returns a single contiguous preimage for this function and the specified
+ /// scalar expression, if any.
///
- /// A preimage is a single contiguous [`Interval`] of values where the function
- /// will always return `lit_value`
+ /// # Return Value
///
- /// Implementations should return intervals with an inclusive lower bound and
- /// exclusive upper bound.
+ /// Implementations should return a half-open interval: inclusive lower
+ /// bound and exclusive upper bound. Note that this is slightly different
+ /// from normal [`Interval`] semantics where the upper bound is closed. The
+ /// upper endpoint should be adjusted to the next value.
///
- /// This rewrite is described in the [ClickHouse Paper] and is particularly
- /// useful for simplifying expressions `date_part` or equivalent functions. The
- /// idea is that if you have an expression like `date_part(YEAR, k) = 2024` and you
- /// can find a [preimage] for `date_part(YEAR, k)`, which is the range of dates
- /// covering the entire year of 2024. Thus, you can rewrite the expression to `k
- /// >= '2024-01-01' AND k < '2025-01-01' which is often more optimizable.
+ /// # Background
+ ///
+ /// A [preimage] here is a single contiguous [`Interval`] of the function's
+ /// argument(s) where the function will return a single literal (constant)
+ /// value. This can also be thought of as a form of interval containment.
+ ///
+ /// Using a preimage to rewrite predicates is described in the [ClickHouse
+ /// Paper]:
+ ///
+ /// > some functions can compute the preimage of a given function result.
+ /// > This is used to replace comparisons of constants with function calls
+ /// > on the key columns by comparing the key column value with the preimage.
+ /// > For example, `toYear(k) = 2024` can be replaced by
+ /// > `k >= 2024-01-01 && k < 2025-01-01`
+ ///
+ /// As mentioned above, the preimage can be used to simplify certain types of
+ /// expressions such as `date_part` into a form that is more optimizable.
+ ///
+ /// For example, given an expression like
+ /// ```sql
+ /// date_part(YEAR, k) = 2024
+ /// ```
+ ///
+ /// There is a single preimage [`2024-01-01`, `2025-01-01`), which is the
+ /// range of dates covering the entire year of 2024 for which
+ /// `date_part(YEAR, k)` evaluates to `2024`. Using this preimage the
+ /// expression can be rewritten to
Review Comment:
Thank you for the ping! I think it is more likely that SedonaDB will provide its own logical optimization rules to cover spatial expressions. Both GeoDataFusion and SedonaDB store CRS (map projection) as type-level extension metadata propagated via `return_field()`.
--
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] Improve documentation for ScalarUDFImpl::preimage [datafusion]
Posted by "2010YOUY01 (via GitHub)" <gi...@apache.org>.
2010YOUY01 commented on code in PR #20008:
URL: https://github.com/apache/datafusion/pull/20008#discussion_r2730273058
##########
datafusion/expr/src/udf.rs:
##########
@@ -709,20 +709,49 @@ pub trait ScalarUDFImpl: Debug + DynEq + DynHash + Send + Sync {
Ok(ExprSimplifyResult::Original(args))
}
- /// Returns the [preimage] for this function and the specified scalar value, if any.
+ /// Returns a single contiguous preimage for this function and the specified
+ /// scalar expression, if any.
///
- /// A preimage is a single contiguous [`Interval`] of values where the function
- /// will always return `lit_value`
+ /// # Return Value
///
- /// Implementations should return intervals with an inclusive lower bound and
- /// exclusive upper bound.
+ /// Implementations should return a half-open interval: inclusive lower
+ /// bound and exclusive upper bound. Note that this is slightly different
+ /// from normal [`Interval`] semantics where the upper bound is closed. The
+ /// upper endpoint should be adjusted to the next value.
///
- /// This rewrite is described in the [ClickHouse Paper] and is particularly
- /// useful for simplifying expressions `date_part` or equivalent functions. The
- /// idea is that if you have an expression like `date_part(YEAR, k) = 2024` and you
- /// can find a [preimage] for `date_part(YEAR, k)`, which is the range of dates
- /// covering the entire year of 2024. Thus, you can rewrite the expression to `k
- /// >= '2024-01-01' AND k < '2025-01-01' which is often more optimizable.
+ /// # Background
+ ///
+ /// A [preimage] here is a single contiguous [`Interval`] of the function's
+ /// argument(s) where the function will return a single literal (constant)
+ /// value. This can also be thought of as a form of interval containment.
+ ///
+ /// Using a preimage to rewrite predicates is described in the [ClickHouse
+ /// Paper]:
+ ///
+ /// > some functions can compute the preimage of a given function result.
+ /// > This is used to replace comparisons of constants with function calls
+ /// > on the key columns by comparing the key column value with the preimage.
+ /// > For example, `toYear(k) = 2024` can be replaced by
+ /// > `k >= 2024-01-01 && k < 2025-01-01`
+ ///
+ /// As mentioned above, the preimage can be used to simplify certain types of
+ /// expressions such as `date_part` into a form that is more optimizable.
+ ///
+ /// For example, given an expression like
+ /// ```sql
+ /// date_part(YEAR, k) = 2024
Review Comment:
```suggestion
/// date_part('YEAR', k) = 2024
```
A nit sql syntax issue
##########
datafusion/expr/src/udf.rs:
##########
@@ -709,20 +709,49 @@ pub trait ScalarUDFImpl: Debug + DynEq + DynHash + Send + Sync {
Ok(ExprSimplifyResult::Original(args))
}
- /// Returns the [preimage] for this function and the specified scalar value, if any.
+ /// Returns a single contiguous preimage for this function and the specified
+ /// scalar expression, if any.
///
- /// A preimage is a single contiguous [`Interval`] of values where the function
- /// will always return `lit_value`
+ /// # Return Value
///
- /// Implementations should return intervals with an inclusive lower bound and
- /// exclusive upper bound.
+ /// Implementations should return a half-open interval: inclusive lower
+ /// bound and exclusive upper bound. Note that this is slightly different
+ /// from normal [`Interval`] semantics where the upper bound is closed. The
+ /// upper endpoint should be adjusted to the next value.
///
- /// This rewrite is described in the [ClickHouse Paper] and is particularly
- /// useful for simplifying expressions `date_part` or equivalent functions. The
- /// idea is that if you have an expression like `date_part(YEAR, k) = 2024` and you
- /// can find a [preimage] for `date_part(YEAR, k)`, which is the range of dates
- /// covering the entire year of 2024. Thus, you can rewrite the expression to `k
- /// >= '2024-01-01' AND k < '2025-01-01' which is often more optimizable.
+ /// # Background
+ ///
+ /// A [preimage] here is a single contiguous [`Interval`] of the function's
+ /// argument(s) where the function will return a single literal (constant)
+ /// value. This can also be thought of as a form of interval containment.
+ ///
+ /// Using a preimage to rewrite predicates is described in the [ClickHouse
+ /// Paper]:
+ ///
+ /// > some functions can compute the preimage of a given function result.
+ /// > This is used to replace comparisons of constants with function calls
+ /// > on the key columns by comparing the key column value with the preimage.
+ /// > For example, `toYear(k) = 2024` can be replaced by
+ /// > `k >= 2024-01-01 && k < 2025-01-01`
+ ///
+ /// As mentioned above, the preimage can be used to simplify certain types of
+ /// expressions such as `date_part` into a form that is more optimizable.
+ ///
+ /// For example, given an expression like
+ /// ```sql
+ /// date_part(YEAR, k) = 2024
+ /// ```
+ ///
+ /// There is a single preimage [`2024-01-01`, `2025-01-01`), which is the
+ /// range of dates covering the entire year of 2024 for which
+ /// `date_part(YEAR, k)` evaluates to `2024`. Using this preimage the
+ /// expression can be rewritten to
+ ///
+ /// ```sql
+ /// k >= '2024-01-01' AND k < '2025-01-01'
+ /// ```
+ ///
+ /// which is often more optimizable, such as being used in min/max pruning.
///
Review Comment:
```suggestion
/// which is often more optimizable: the predicate is rewritten into a simpler
/// and more canonical form, making it easier for different optimizer passes
/// to recognize and apply further transformations. For example:
///
/// Case 1:
///
/// Original:
/// ```sql
/// date_part('YEAR', k) = 2024 AND k >= '2024-06-01'
/// ```
///
/// After preimage rewrite:
/// ```sql
/// k >= '2024-01-01' AND k < '2025-01-01' AND k >= '2024-06-01'
/// ```
///
/// Since this form is much simpler, the optimizer can combine and simplify
/// sub-expressions further into:
/// ```sql
/// k >= '2024-06-01' AND k < '2025-01-01'
/// ```
///
/// Case 2:
///
/// For min/max pruning, simpler predicates such as:
/// ```sql
/// k >= '2024-01-01' AND k < '2025-01-01'
/// ```
/// are much easier for the pruner to reason about. See [PruningPredicate]
/// for the backgrounds of predicate pruning.
///
/// The trade-off is that evaluating the preimage form can be slightly more
/// expensive than evaluating the original expression. In practice, this cost
/// is usually outweighed by the more aggressive optimization opportunities it
/// enables.
///
/// [PruningPredicate]: https://docs.rs/datafusion/latest/datafusion/physical_optimizer/pruning/struct.PruningPredicate.html
```
I think we can should explain what is 'more optimizable'.
I haven't verified if the `Case 1` would actually get triggered, but it should be doable technically.
##########
datafusion/expr/src/udf.rs:
##########
@@ -709,20 +709,49 @@ pub trait ScalarUDFImpl: Debug + DynEq + DynHash + Send + Sync {
Ok(ExprSimplifyResult::Original(args))
}
- /// Returns the [preimage] for this function and the specified scalar value, if any.
+ /// Returns a single contiguous preimage for this function and the specified
+ /// scalar expression, if any.
///
- /// A preimage is a single contiguous [`Interval`] of values where the function
- /// will always return `lit_value`
+ /// # Return Value
///
- /// Implementations should return intervals with an inclusive lower bound and
- /// exclusive upper bound.
+ /// Implementations should return a half-open interval: inclusive lower
+ /// bound and exclusive upper bound. Note that this is slightly different
+ /// from normal [`Interval`] semantics where the upper bound is closed. The
+ /// upper endpoint should be adjusted to the next value.
///
- /// This rewrite is described in the [ClickHouse Paper] and is particularly
- /// useful for simplifying expressions `date_part` or equivalent functions. The
- /// idea is that if you have an expression like `date_part(YEAR, k) = 2024` and you
- /// can find a [preimage] for `date_part(YEAR, k)`, which is the range of dates
- /// covering the entire year of 2024. Thus, you can rewrite the expression to `k
- /// >= '2024-01-01' AND k < '2025-01-01' which is often more optimizable.
+ /// # Background
+ ///
+ /// A [preimage] here is a single contiguous [`Interval`] of the function's
+ /// argument(s) where the function will return a single literal (constant)
+ /// value. This can also be thought of as a form of interval containment.
+ ///
+ /// Using a preimage to rewrite predicates is described in the [ClickHouse
+ /// Paper]:
+ ///
+ /// > some functions can compute the preimage of a given function result.
+ /// > This is used to replace comparisons of constants with function calls
+ /// > on the key columns by comparing the key column value with the preimage.
+ /// > For example, `toYear(k) = 2024` can be replaced by
+ /// > `k >= 2024-01-01 && k < 2025-01-01`
+ ///
+ /// As mentioned above, the preimage can be used to simplify certain types of
+ /// expressions such as `date_part` into a form that is more optimizable.
+ ///
+ /// For example, given an expression like
+ /// ```sql
+ /// date_part(YEAR, k) = 2024
+ /// ```
+ ///
+ /// There is a single preimage [`2024-01-01`, `2025-01-01`), which is the
+ /// range of dates covering the entire year of 2024 for which
+ /// `date_part(YEAR, k)` evaluates to `2024`. Using this preimage the
Review Comment:
```suggestion
/// `date_part('YEAR', k)` evaluates to `2024`. Using this preimage the
```
--
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] Improve documentation for ScalarUDFImpl::preimage [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #20008:
URL: https://github.com/apache/datafusion/pull/20008#discussion_r2727547648
##########
datafusion/expr/src/udf.rs:
##########
@@ -709,20 +709,49 @@ pub trait ScalarUDFImpl: Debug + DynEq + DynHash + Send + Sync {
Ok(ExprSimplifyResult::Original(args))
}
- /// Returns the [preimage] for this function and the specified scalar value, if any.
+ /// Returns a single contiguous preimage for this function and the specified
+ /// scalar expression, if any.
///
- /// A preimage is a single contiguous [`Interval`] of values where the function
- /// will always return `lit_value`
+ /// # Return Value
Review Comment:
Moved discussion of return value to its own section and clearly separated the background from the API details
##########
datafusion/expr/src/udf.rs:
##########
@@ -709,20 +709,49 @@ pub trait ScalarUDFImpl: Debug + DynEq + DynHash + Send + Sync {
Ok(ExprSimplifyResult::Original(args))
}
- /// Returns the [preimage] for this function and the specified scalar value, if any.
+ /// Returns a single contiguous preimage for this function and the specified
+ /// scalar expression, if any.
///
- /// A preimage is a single contiguous [`Interval`] of values where the function
- /// will always return `lit_value`
+ /// # Return Value
///
- /// Implementations should return intervals with an inclusive lower bound and
- /// exclusive upper bound.
+ /// Implementations should return a half-open interval: inclusive lower
+ /// bound and exclusive upper bound. Note that this is slightly different
Review Comment:
one thing I think would make this API simpler would be to adjust the semantics so that the returned Interval used the same semantics as normal Intervals (entirely closed, so including the endpoints)
Not sure what you think about this @sdf-jkl
--
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] Improve documentation for ScalarUDFImpl::preimage [datafusion]
Posted by "drin (via GitHub)" <gi...@apache.org>.
drin commented on code in PR #20008:
URL: https://github.com/apache/datafusion/pull/20008#discussion_r2729282358
##########
datafusion/expr/src/udf.rs:
##########
@@ -709,20 +709,49 @@ pub trait ScalarUDFImpl: Debug + DynEq + DynHash + Send + Sync {
Ok(ExprSimplifyResult::Original(args))
}
- /// Returns the [preimage] for this function and the specified scalar value, if any.
+ /// Returns a single contiguous preimage for this function and the specified
+ /// scalar expression, if any.
///
- /// A preimage is a single contiguous [`Interval`] of values where the function
- /// will always return `lit_value`
+ /// # Return Value
///
- /// Implementations should return intervals with an inclusive lower bound and
- /// exclusive upper bound.
+ /// Implementations should return a half-open interval: inclusive lower
+ /// bound and exclusive upper bound. Note that this is slightly different
+ /// from normal [`Interval`] semantics where the upper bound is closed. The
+ /// upper endpoint should be adjusted to the next value.
///
- /// This rewrite is described in the [ClickHouse Paper] and is particularly
- /// useful for simplifying expressions `date_part` or equivalent functions. The
- /// idea is that if you have an expression like `date_part(YEAR, k) = 2024` and you
- /// can find a [preimage] for `date_part(YEAR, k)`, which is the range of dates
- /// covering the entire year of 2024. Thus, you can rewrite the expression to `k
- /// >= '2024-01-01' AND k < '2025-01-01' which is often more optimizable.
+ /// # Background
+ ///
+ /// A [preimage] here is a single contiguous [`Interval`] of the function's
+ /// argument(s) where the function will return a single literal (constant)
+ /// value. This can also be thought of as a form of interval containment.
+ ///
+ /// Using a preimage to rewrite predicates is described in the [ClickHouse
+ /// Paper]:
+ ///
+ /// > some functions can compute the preimage of a given function result.
+ /// > This is used to replace comparisons of constants with function calls
+ /// > on the key columns by comparing the key column value with the preimage.
+ /// > For example, `toYear(k) = 2024` can be replaced by
+ /// > `k >= 2024-01-01 && k < 2025-01-01`
+ ///
+ /// As mentioned above, the preimage can be used to simplify certain types of
+ /// expressions such as `date_part` into a form that is more optimizable.
+ ///
+ /// For example, given an expression like
+ /// ```sql
+ /// date_part(YEAR, k) = 2024
+ /// ```
+ ///
+ /// There is a single preimage [`2024-01-01`, `2025-01-01`), which is the
+ /// range of dates covering the entire year of 2024 for which
+ /// `date_part(YEAR, k)` evaluates to `2024`. Using this preimage the
+ /// expression can be rewritten to
Review Comment:
```suggestion
/// The interval [`2024-01-01`, `2025-01-01`) contains all possible
/// input values (preimage values) for which the function `date_part`
/// produces the output value `2024` (image value). Using this interval,
/// the expression can be rewritten to
```
--
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] Improve documentation for ScalarUDFImpl::preimage [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #20008:
URL: https://github.com/apache/datafusion/pull/20008#discussion_r2728509304
##########
datafusion/expr/src/udf.rs:
##########
@@ -709,20 +709,49 @@ pub trait ScalarUDFImpl: Debug + DynEq + DynHash + Send + Sync {
Ok(ExprSimplifyResult::Original(args))
}
- /// Returns the [preimage] for this function and the specified scalar value, if any.
+ /// Returns a single contiguous preimage for this function and the specified
+ /// scalar expression, if any.
///
- /// A preimage is a single contiguous [`Interval`] of values where the function
- /// will always return `lit_value`
+ /// # Return Value
///
- /// Implementations should return intervals with an inclusive lower bound and
- /// exclusive upper bound.
+ /// Implementations should return a half-open interval: inclusive lower
+ /// bound and exclusive upper bound. Note that this is slightly different
Review Comment:
Ah, I forgot about the subtracting the unit. Maybe I can find some way to include that in the explanation 🤔
--
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] Improve documentation for ScalarUDFImpl::preimage [datafusion]
Posted by "drin (via GitHub)" <gi...@apache.org>.
drin commented on code in PR #20008:
URL: https://github.com/apache/datafusion/pull/20008#discussion_r2729282358
##########
datafusion/expr/src/udf.rs:
##########
@@ -709,20 +709,49 @@ pub trait ScalarUDFImpl: Debug + DynEq + DynHash + Send + Sync {
Ok(ExprSimplifyResult::Original(args))
}
- /// Returns the [preimage] for this function and the specified scalar value, if any.
+ /// Returns a single contiguous preimage for this function and the specified
+ /// scalar expression, if any.
///
- /// A preimage is a single contiguous [`Interval`] of values where the function
- /// will always return `lit_value`
+ /// # Return Value
///
- /// Implementations should return intervals with an inclusive lower bound and
- /// exclusive upper bound.
+ /// Implementations should return a half-open interval: inclusive lower
+ /// bound and exclusive upper bound. Note that this is slightly different
+ /// from normal [`Interval`] semantics where the upper bound is closed. The
+ /// upper endpoint should be adjusted to the next value.
///
- /// This rewrite is described in the [ClickHouse Paper] and is particularly
- /// useful for simplifying expressions `date_part` or equivalent functions. The
- /// idea is that if you have an expression like `date_part(YEAR, k) = 2024` and you
- /// can find a [preimage] for `date_part(YEAR, k)`, which is the range of dates
- /// covering the entire year of 2024. Thus, you can rewrite the expression to `k
- /// >= '2024-01-01' AND k < '2025-01-01' which is often more optimizable.
+ /// # Background
+ ///
+ /// A [preimage] here is a single contiguous [`Interval`] of the function's
+ /// argument(s) where the function will return a single literal (constant)
+ /// value. This can also be thought of as a form of interval containment.
+ ///
+ /// Using a preimage to rewrite predicates is described in the [ClickHouse
+ /// Paper]:
+ ///
+ /// > some functions can compute the preimage of a given function result.
+ /// > This is used to replace comparisons of constants with function calls
+ /// > on the key columns by comparing the key column value with the preimage.
+ /// > For example, `toYear(k) = 2024` can be replaced by
+ /// > `k >= 2024-01-01 && k < 2025-01-01`
+ ///
+ /// As mentioned above, the preimage can be used to simplify certain types of
+ /// expressions such as `date_part` into a form that is more optimizable.
+ ///
+ /// For example, given an expression like
+ /// ```sql
+ /// date_part(YEAR, k) = 2024
+ /// ```
+ ///
+ /// There is a single preimage [`2024-01-01`, `2025-01-01`), which is the
+ /// range of dates covering the entire year of 2024 for which
+ /// `date_part(YEAR, k)` evaluates to `2024`. Using this preimage the
+ /// expression can be rewritten to
Review Comment:
```suggestion
/// The interval [`2024-01-01`, `2025-01-01`) contains all possible
/// input values (preimage values) for which the function `date_part(YEAR, k)`
/// produces the output value `2024` (image value). Using this interval,
/// the expression can be rewritten to
```
--
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] Improve documentation for ScalarUDFImpl::preimage [datafusion]
Posted by "drin (via GitHub)" <gi...@apache.org>.
drin commented on code in PR #20008:
URL: https://github.com/apache/datafusion/pull/20008#discussion_r2729394863
##########
datafusion/expr/src/udf.rs:
##########
@@ -709,20 +709,49 @@ pub trait ScalarUDFImpl: Debug + DynEq + DynHash + Send + Sync {
Ok(ExprSimplifyResult::Original(args))
}
- /// Returns the [preimage] for this function and the specified scalar value, if any.
+ /// Returns a single contiguous preimage for this function and the specified
+ /// scalar expression, if any.
///
- /// A preimage is a single contiguous [`Interval`] of values where the function
- /// will always return `lit_value`
+ /// # Return Value
///
- /// Implementations should return intervals with an inclusive lower bound and
- /// exclusive upper bound.
+ /// Implementations should return a half-open interval: inclusive lower
+ /// bound and exclusive upper bound. Note that this is slightly different
+ /// from normal [`Interval`] semantics where the upper bound is closed. The
+ /// upper endpoint should be adjusted to the next value.
///
- /// This rewrite is described in the [ClickHouse Paper] and is particularly
- /// useful for simplifying expressions `date_part` or equivalent functions. The
- /// idea is that if you have an expression like `date_part(YEAR, k) = 2024` and you
- /// can find a [preimage] for `date_part(YEAR, k)`, which is the range of dates
- /// covering the entire year of 2024. Thus, you can rewrite the expression to `k
- /// >= '2024-01-01' AND k < '2025-01-01' which is often more optimizable.
+ /// # Background
+ ///
+ /// A [preimage] here is a single contiguous [`Interval`] of the function's
+ /// argument(s) where the function will return a single literal (constant)
+ /// value. This can also be thought of as a form of interval containment.
+ ///
+ /// Using a preimage to rewrite predicates is described in the [ClickHouse
+ /// Paper]:
Review Comment:
```suggestion
/// Inspired by the [ClickHouse Paper], a "preimage rewrite" transforms
/// a predicate containing a function call into a predicate containing an
/// equivalent set of input literal (constant) values. From the paper:
```
--
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] Improve documentation for ScalarUDFImpl::preimage [datafusion]
Posted by "sdf-jkl (via GitHub)" <gi...@apache.org>.
sdf-jkl commented on code in PR #20008:
URL: https://github.com/apache/datafusion/pull/20008#discussion_r2733925382
##########
datafusion/expr/src/udf.rs:
##########
@@ -709,20 +709,49 @@ pub trait ScalarUDFImpl: Debug + DynEq + DynHash + Send + Sync {
Ok(ExprSimplifyResult::Original(args))
}
- /// Returns the [preimage] for this function and the specified scalar value, if any.
+ /// Returns a single contiguous preimage for this function and the specified
+ /// scalar expression, if any.
///
Review Comment:
```suggestion
///
/// Currently only applies to `=, !=, >, >=, <, <=, is distinct from, is not distinct from` predicates
```
--
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] Improve documentation for ScalarUDFImpl::preimage [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #20008:
URL: https://github.com/apache/datafusion/pull/20008#issuecomment-3814029111
I'll plan to merge this tomorrow unless there are any more comments
--
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