Uh oh!
There was an error while loading. Please reload this page.
Support API for "pre-image" for pruning predicate evaluation - #19722
Conversation
alamb
left a comment
There was a problem hiding this comment.
Thank you @sdf-jkl -- reviewed this PR carefully this morning and it looks great (thank you for splitting up the work), I found it well commented and well designed and a joy to read
I do think we need to add unit tests tests to for this feature, which I know you have lined up in #18789 but I think writing the unit tests in for the rewrite will make it easiest to validate.
I also have some questions about the rewrite for = (aka the boundary conditions)
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
f308662 to
5ffb704Comparealamb
commented
Jan 20, 2026
I think this PR needs two more things:
(I am trying to keep my review context under control, so trying to focus on getting stuff through before starting more) |
sdf-jkl
commented
Jan 20, 2026
Both done. Re-requested a review. |
alamb
left a comment
There was a problem hiding this comment.
Thank you very much @sdf-jkl
This looks good to me. I would like to change the signature to use Interval rather than Box<Interval> and there are a few other small comments, but we can also do this as a follow on PR (or I can push some commits to this PR)
Thank you for hanging with this one
FYI @colinmarc -- once we get this in, I think @sdf-jkl plans to implement preimage for date_part. Perhaps you are interested in something similar for date_trunc
Also, FYI @jonahgao and @xudong963 / @zhuqi-lucas in case you are interested in this PR (the primary usecase is improving the handling of date/timestamp predicates)
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
| let Expr::ScalarFunction(ScalarFunction { func, args }) = left_expr else { | ||
| return Ok((None, None)); | ||
| }; | ||
| if !is_literal_or_literal_cast(right_expr) { |
There was a problem hiding this comment.
I think this is still an open question, but it is ok to handle as a follow on PR (aka widen the expressions)
| if !is_literal_or_literal_cast(right_expr) { | ||
| return Ok(PreimageResult::None); | ||
| } | ||
| if func.signature().volatility != Volatility::Immutable { |
There was a problem hiding this comment.
Also for a follow on PR, I think it would be safe to rewrite stable functions (whose values don't change during the statement)
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
xudong963
commented
Jan 21, 2026
I'll have a look at the PR today |
| /// [preimage]: https://en.wikipedia.org/wiki/Image_(mathematics)#Inverse_image | ||
| /// | ||
| pub(super) fn rewrite_with_preimage( | ||
| _info: &SimplifyContext, |
There was a problem hiding this comment.
@alamb mentioned that we should keep it in #18789 (comment), but it was a while ago.
There was a problem hiding this comment.
I think it is important to pass to ScalarUDFImpl::preimage but probably it can be removed from this method call
Since I want to merge this PR up from main anyways before merge, I'll clean it up too
Uh oh!
There was an error while loading. Please reload this page.
Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>
alamb
commented
Jan 22, 2026
Thank you @sdf-jkl and @xudong963 -- I took a final look through this and it looks really nice. Thank you. I merged up from main and I'll plan to merge when the tests pass. Let's get this one in to keep things moving forward |
alamb
commented
Jan 22, 2026
FYI @rcurtin |
alamb
commented
Jan 22, 2026
Thanks again @sdf-jkl and @xudong963 |
Uh oh!
There was an error while loading. Please reload this page.
rcurtin
commented
Jan 22, 2026
Thank you for keeping me in the loop! 👍 Glad to see this. |
…19722) ## Which issue does this PR close? - closesapache#18320 ## Rationale for this change Splitting the PR to make it more readable. ## What changes are included in this PR? Adding the udf_preimage logic without date_part implementation. ## Are these changes tested? Added unit tests for a test specific function ## Are there any user-facing changes? No --------- Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>
Which issue does this PR close?
Rationale for this change
Splitting the PR to make it more readable.
What changes are included in this PR?
Adding the udf_preimage logic without date_part implementation.
Are these changes tested?
Added unit tests for a test specific function
Are there any user-facing changes?
No