Skip to content

Conversation

@pepijnve
Copy link
Contributor

Which issue does this PR close?

Rationale for this change

In #17813, the when expressions are rewritten using GuaranteesRewrite before evaluating their bounds. This PR avoids the rewrite cost by inlining the special case of GuaranteesRewrite with a single Null guarantee into the PredicateBounds::evaluate_bounds logic.

What changes are included in this PR?

Inlines guarantee rewriting into predicate bounds evaluation.

Are these changes tested?

Covered by existing tests

Are there any user-facing changes?

No

Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @pepijnve -- is there anything else to do for this PR or is it good to go from your perspective? (the PR is marked as a draft so I wanted to double check)

@pepijnve
Copy link
Contributor Author

pepijnve commented Nov 21, 2025

I marked it as draft to see if it was worth it or not first. No further code changes needed as far as I know.

Wdyt? Should we go for this?

@alamb
Copy link
Contributor

alamb commented Nov 21, 2025

I marked it as draft to see if it was worth it or not first. No further code changes needed as far as I know.

Wdyt? Should we go for this?

Yes I think we should do it -- if for no other reasons it makes the code simpler

@pepijnve
Copy link
Contributor Author

Yes I think we should do it -- if for no other reasons it makes the code simpler

Should I revert the GuaranteeRewrite move as well then since that's no longer necessary with this change.

@alamb
Copy link
Contributor

alamb commented Nov 21, 2025

Yes I think we should do it -- if for no other reasons it makes the code simpler

Should I revert the GuaranteeRewrite move as well then since that's no longer necessary with this change.

I think it is a good improvement in general (and it fixed some bugs and added tests).

So I think we should keep it

@pepijnve pepijnve marked this pull request as ready for review November 21, 2025 16:19
@alamb alamb added this pull request to the merge queue Nov 23, 2025
Merged via the queue into apache:main with commit 7d87976 Nov 23, 2025
32 checks passed
@alamb
Copy link
Contributor

alamb commented Nov 23, 2025

🚀 -- thanks again @pepijnve

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

logical-expr Logical plan and expressions

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants