Add support for constant expression evaluation in limit #9790
Conversation
| /// * `Result<i64>` - An `Ok` variant containing the constant result if evaluation is successful, | ||
| /// or an `Err` variant containing an error message if evaluation fails. | ||
| fn get_constant_result(expr: &Expr, arg_name: &str) -> Result<i64> { | ||
| match expr { |
There was a problem hiding this comment.
We could use ConstEvaluator and see if it is evaluated to a constant value
There was a problem hiding this comment.
To use this helper. I have imported datafusion-optimizer to datafusion-sql crate. This dependency caused an error in test_deps test. Hence I am retracting these changes for now.
There was a problem hiding this comment.
Could we change the Limit logical plan to support arbitrary expressions?
pub struct Limit {
pub skip: Expr,
pub fetch: Option<Expr>,
pub input: Arc<LogicalPlan>,
}The SimplifyExpressions rule can automatically optimize them into constants. Some optimization rules such as PushDownLimit only run when the limit expression is a constant. We may need to add a cast for the limit expression when planning, only checking if it is a constant of type u64.
When creating the LimitExec physical plan, convert the limit expression into PhysicalExpr and evaluate it.
There was a problem hiding this comment.
Could we change the Limit logical plan to support arbitrary expressions?
I agree this would be the "correct" way to support arbitrarily simplifiable expressions in the limit clause
However, I suspect it might be a major change
So I guess it comes down to "how important is the + / - case -- if it is really important then we can proceed with this PR, but if the real use case is arbitrary expressions, it probably makes sense to do the more substantial change as suggested by @jonahgao
There was a problem hiding this comment.
I have experimented with the keeping Expr in the limit state. However, it is not a trivial change. Also, it is not clear when to do error checking, when to ignore errors while converting skip and fetch to their corresponding u64 versions. I suggest, we first merge this PR. Then, we can have a support for arbitrary expressions in another PR (Also I think, we can postpone this feature until a use case hits.).
This reverts commit 99b7a55.
There was a problem hiding this comment.
Thank you @mustafasrepo
I agree this PR makes the situation better (error message is better and more cases are supported). Thank you very much @mustafasrepo
We can improve the situation more in the future and I filed #9821 to track that . Thank you for the suggestions @jonahgao and @Dandandan
🚀
Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>
* Add support for constant expression evaluation in limit * Use existing const evaluator * Revert "Use existing const evaluator" This reverts commit 99b7a55. * Update datafusion/sql/src/query.rs Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org> * Add negative tests --------- Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>
Which issue does this PR close?
Closes #9506.
Rationale for this change
Add support for queries in following kind:
Unfortunately, I couldn't apply the suggestion because of the library dependencies.
What changes are included in this PR?
Are these changes tested?
Yes
Are there any user-facing changes?