feat(proto): support expressions in LIMIT skip and fetch - #24159
Open
karpenkoag wants to merge 1 commit into
Open
feat(proto): support expressions in LIMIT skip and fetch#24159karpenkoag wants to merge 1 commit into
karpenkoag wants to merge 1 commit into
Conversation
github-actions
Bot
added
logical-expr
karpenkoag
force-pushed
the
a.karpenko/allow-evaluated-expr-in-logical-plan
branch
from
August 7, 2026 09:50
acd6517 to
1420aed
Compare
karpenkoag
marked this pull request as draft
August 7, 2026 09:54
Serialize and deserialize LIMIT expressions in logical plan protobufs, and simplify them before physical planning so evaluable expressions can be used.
karpenkoag
force-pushed
the
a.karpenko/allow-evaluated-expr-in-logical-plan
branch
from
August 7, 2026 10:27
1420aed to
7d464ab
Compare
karpenkoag
marked this pull request as ready for review
August 7, 2026 10:27
Contributor
|
@karpenkoag Please reflect in the PR description that it extends support for limit expressions. Currently, it seems that this patch only touches the proto, while there are also changes in the planner. |
askalt
reviewed
Aug 9, 2026
| } | ||
| LogicalPlan::SubqueryAlias(_) => children.one()?, | ||
| LogicalPlan::Limit(limit) => { | ||
| // Try to evaluate skip and fetch expressions. |
Contributor
There was a problem hiding this comment.
Let's add SLT to test these changes.
Comment on lines
+1226
to
+1234
| let skip = match &limit.skip { | ||
| Some(expr) => Some(Box::new(simplifier.simplify(*expr.clone())?)), | ||
| None => None, | ||
| }; | ||
|
|
||
| let fetch = match &limit.fetch { | ||
| Some(expr) => Some(Box::new(simplifier.simplify(*expr.clone())?)), | ||
| None => None, | ||
| }; |
Contributor
There was a problem hiding this comment.
Can we avoid code duplication here? e.g. create a closure.
| Literal(Option<usize>), | ||
| /// Currently only supports expressions that can be folded into constants. | ||
| //// Currently supports all expressions that can be evaluated. | ||
| /// UnsupportedExpr means that the expression is not considered by the analyzer/optimizer. |
Contributor
There was a problem hiding this comment.
Suggested change
| /// UnsupportedExpr means that the expression is not considered by the analyzer/optimizer. | |
| /// UnsupportedExpr means that the expression is not considered by the logical analyzer/optimizer. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Serialize and deserialize LIMIT expressions in logical plan protobufs, and simplify them before physical planning so evaluable expressions can be used.