You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "jonahgao (via GitHub)" <gi...@apache.org> on 2024/10/21 07:57:09 UTC
[PR] feat: support arbitrary expressions in `LIMIT` plan [datafusion]
jonahgao opened a new pull request, #13028:
URL: https://github.com/apache/datafusion/pull/13028
## Which issue does this PR close?
<!--
We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. For example `Closes #123` indicates that this PR will close issue #123.
-->
Closes #9821.
## Rationale for this change
`SKIP` and `FETCH` in the `LIMIT` plan can support arbitrary expressions.
```rust
pub struct Limit {
pub skip: Option<Box<Expr>>,
pub fetch: Option<Box<Expr>>,
pub input: Arc<LogicalPlan>,
}
```
This olny extends the `LogicalPlan` of `LIMIT`. It relies on `SimplifyExpressions` to fold expressions into constants; if it can't, it will skip executing certain optimization rules, and report an error when creating the physical plan.
## What changes are included in this PR?
<!--
There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes in this PR.
-->
## Are these changes tested?
Yes
<!--
We typically require tests for all PRs in order to:
1. Prevent the code from being accidentally broken by subsequent changes
2. Serve as another way to document the expected behavior of the code
If tests are not included in your PR, please explain why (for example, are they covered by existing tests)?
-->
## Are there any user-facing changes?
Yes.
<!--
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] feat: support arbitrary expressions in `LIMIT` plan [datafusion]
Posted by "findepi (via GitHub)" <gi...@apache.org>.
findepi commented on PR #13028:
URL: https://github.com/apache/datafusion/pull/13028#issuecomment-2429111288
> Postgres's behavior is: if `OFFSET` is not integer for example 0.5, 0th row won't be included, if `LIMIT` is 0.5, then 0th row would be included:
@2010YOUY01 good point!
do you maybe know what is the use-case for allowing fractional values for LIMIT or OFFSET?
this looks like a pandora box. strictly speaking, casting them to integral values isn't exactly what we can do: `OFFSET 1.5 LIMIT 1.5` should return 3rd row, but not the 4th row.
--
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] feat: support arbitrary expressions in `LIMIT` plan [datafusion]
Posted by "findepi (via GitHub)" <gi...@apache.org>.
findepi commented on PR #13028:
URL: https://github.com/apache/datafusion/pull/13028#issuecomment-2431142121
> Not really, allowing fractional values looks quite bizarre to me, however, nowadays many SQL are auto-generated so we can't make too many assumptions
If i were generating queries, i wouldn't ask a database to do `LIMIT 1+2`, I'd generate `LIMIT 3`.
Do you maybe know what class of software generates `LIMIT <expr>` or `LIMIT <fractional>`?
--
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] feat: support arbitrary expressions in `LIMIT` plan [datafusion]
Posted by "findepi (via GitHub)" <gi...@apache.org>.
findepi commented on code in PR #13028:
URL: https://github.com/apache/datafusion/pull/13028#discussion_r1812061636
##########
datafusion/expr/src/logical_plan/plan.rs:
##########
@@ -2799,14 +2817,71 @@ impl PartialOrd for Extension {
#[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Hash)]
pub struct Limit {
/// Number of rows to skip before fetch
- pub skip: usize,
+ pub skip: Option<Box<Expr>>,
/// Maximum number of rows to fetch,
/// None means fetching all rows
- pub fetch: Option<usize>,
+ pub fetch: Option<Box<Expr>>,
/// The logical plan
pub input: Arc<LogicalPlan>,
}
+/// Different types of skip expression in Limit plan.
+pub enum SkipType {
+ /// The skip expression is a literal value.
+ Literal(usize),
+ /// Currently only supports expressions that can be folded into constants.
+ UnsupportedExpr,
Review Comment:
@jonahgao good point. I also thought about this, but ignored, assuming we don't plan to support this.
We could have `Limit` node as-is _for now_ (and do trivial constant folding when building the plan), and introduce expressions in the plan when we add support for `limit (<uncorrelated subquery>)`. WDYT?
--
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] feat: support arbitrary expressions in `LIMIT` plan [datafusion]
Posted by "jonahgao (via GitHub)" <gi...@apache.org>.
jonahgao commented on code in PR #13028:
URL: https://github.com/apache/datafusion/pull/13028#discussion_r1808293093
##########
datafusion/substrait/src/logical_plan/consumer.rs:
##########
@@ -623,8 +623,8 @@ pub async fn from_substrait_rel(
from_substrait_rel(ctx, input, extensions).await?,
);
let offset = fetch.offset as usize;
- // Since protobuf can't directly distinguish `None` vs `0` `None` is encoded as `MAX`
- let count = if fetch.count as usize == usize::MAX {
+ // -1 means that ALL records should be returned
Review Comment:
According to the substrait [spec](https://github.com/substrait-io/substrait/blob/683f4179a058c2c99c04501b920a48ff372356ff/proto/substrait/algebra.proto#L273), we should use `-1` 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] feat: support arbitrary expressions in `LIMIT` plan [datafusion]
Posted by "jonahgao (via GitHub)" <gi...@apache.org>.
jonahgao commented on code in PR #13028:
URL: https://github.com/apache/datafusion/pull/13028#discussion_r1808305148
##########
datafusion/expr/src/logical_plan/plan.rs:
##########
@@ -2799,14 +2817,71 @@ impl PartialOrd for Extension {
#[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Hash)]
pub struct Limit {
/// Number of rows to skip before fetch
- pub skip: usize,
+ pub skip: Option<Box<Expr>>,
Review Comment:
Use `Box` here to prevent increasing the size of `LogicalPlan`; otherwise it will cause a stack overflow in one of the array_ndims [test](https://github.com/apache/datafusion/blob/b42d9b81caddb5b53800f8eed32f9af4a9e3a01d/datafusion/sqllogictest/test_files/array.slt#L5152)
--
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] feat: support arbitrary expressions in `LIMIT` plan [datafusion]
Posted by "goldmedal (via GitHub)" <gi...@apache.org>.
goldmedal commented on PR #13028:
URL: https://github.com/apache/datafusion/pull/13028#issuecomment-2429558649
> Disallow non-integer types in [e6d1297](https://github.com/apache/datafusion/commit/e6d1297cb9045f99cab21690335740853fd91507) . Fractional values are more complicated than I expected, let's handle them later. @2010YOUY01 @findepi
I think the floating value is caused by the DataFusion casting behavior is different from others. I tried to cast a float to an integer.
### DataFusion
```
DataFusion CLI v42.0.0
> select 1.5::int;
+--------------+
| Float64(1.5) |
+--------------+
| 1 |
+--------------+
1 row(s) fetched.
Elapsed 0.055 seconds.
> select 1.4::int;
+--------------+
| Float64(1.4) |
+--------------+
| 1 |
+--------------+
1 row(s) fetched.
Elapsed 0.001 seconds.
```
### Postgres
```
test=# select 1.5::int;
int4
------
2
(1 row)
test=# select 1.4::int;
int4
------
1
(1 row)
```
### DuckDB
```
D select 1.5::int;
┌──────────────────────┐
│ CAST(1.5 AS INTEGER) │
│ int32 │
├──────────────────────┤
│ 2 │
└──────────────────────┘
D select 1.4::int;
┌──────────────────────┐
│ CAST(1.4 AS INTEGER) │
│ int32 │
├──────────────────────┤
│ 1 │
└─────
```
Postgres and DuckDB are rounding but DataFusion is round down.
So, that's why `limit 1.5` only returns 1 row in DataFusion but returns 2 rows in Postgres.
--
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] feat: support arbitrary expressions in `LIMIT` plan [datafusion]
Posted by "2010YOUY01 (via GitHub)" <gi...@apache.org>.
2010YOUY01 commented on PR #13028:
URL: https://github.com/apache/datafusion/pull/13028#issuecomment-2428913421
Thank you. I have some suggestions for `LIMIT`'s behavior:
Before this PR, only int literals are accepted
```sh
DataFusion CLI v42.0.0
> create table t1(v1 int);
0 row(s) fetched.
Elapsed 0.052 seconds.
> insert into t1 values (1),(2),(3);
+-------+
| count |
+-------+
| 3 |
+-------+
1 row(s) fetched.
Elapsed 0.086 seconds.
> select * from t1 limit 1.5;
Error during planning: Unexpected expression in LIMIT clause
> select * from t1 limit true;
Error during planning: Unexpected expression in LIMIT clause
```
This PR:
```sh
> select * from t1 limit 1.5;
+----+
| v1 |
+----+
| 1 |
+----+
1 row(s) fetched.
Elapsed 0.006 seconds.
> select * from t1 offset 0.5 limit 10;
+----+
| v1 |
+----+
| 1 |
| 2 |
| 3 |
+----+
3 row(s) fetched.
Elapsed 0.004 seconds.
> select * from t1 limit false;
+----+
| v1 |
+----+
+----+
0 row(s) fetched.
Elapsed 0.009 seconds.
```
Postgres's behavior is: if `OFFSET` is not integer for example 0.5, 0th row won't be included, if `LIMIT` is 0.5, then 0th row would be included:
```sh
postgres=# select * from t1 limit 1.5;
v1
----
1
2
(2 rows)
postgres=# select * from t1 offset 0.5 limit 10;
v1
----
2
3
(2 rows)
postgres=# select * from t1 limit false;
ERROR: argument of LIMIT must be type bigint, not type boolean
LINE 1: select * from t1 limit false;
```
1. I think it's good to follow Postgres' behavior for non-integer arguments
2. Don't support non-numeric arguments like bool, I think they are allowed to cast to integer, but not meaningful in SQL query (maybe there are other types that have a similar issue I'm not aware of)
--
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] feat: support arbitrary expressions in `LIMIT` plan [datafusion]
Posted by "goldmedal (via GitHub)" <gi...@apache.org>.
goldmedal commented on PR #13028:
URL: https://github.com/apache/datafusion/pull/13028#issuecomment-2429583495
> > Disallow non-integer types in [e6d1297](https://github.com/apache/datafusion/commit/e6d1297cb9045f99cab21690335740853fd91507) . Fractional values are more complicated than I expected, let's handle them later. @2010YOUY01 @findepi
>
> I think the floating value is caused by the DataFusion casting behavior is different from others. I tried to cast a float to an integer.
Maybe related to https://github.com/apache/datafusion/issues/8302 and https://github.com/apache/datafusion/issues/11886
--
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] feat: support arbitrary expressions in `LIMIT` plan [datafusion]
Posted by "jonahgao (via GitHub)" <gi...@apache.org>.
jonahgao commented on code in PR #13028:
URL: https://github.com/apache/datafusion/pull/13028#discussion_r1809146268
##########
datafusion/optimizer/src/eliminate_limit.rs:
##########
@@ -63,17 +63,23 @@ impl OptimizerRule for EliminateLimit {
> {
match plan {
LogicalPlan::Limit(limit) => {
- if let Some(fetch) = limit.fetch {
- if fetch == 0 {
+ // Only supports rewriting for literal fetch
+ let FetchType::Literal(fetch) = limit.get_fetch_type()? else {
+ return Ok(Transformed::no(LogicalPlan::Limit(limit)));
+ };
+
+ if let Some(v) = fetch {
+ if v == 0 {
return Ok(Transformed::yes(LogicalPlan::EmptyRelation(
EmptyRelation {
produce_one_row: false,
schema: Arc::clone(limit.input.schema()),
},
)));
}
- } else if limit.skip == 0 {
- // input also can be Limit, so we should apply again.
+ } else if matches!(limit.get_skip_type()?, SkipType::Literal(0)) {
+ // If fetch is `None` and skip is 0, then Limit takes no effect and
+ // we can remove it. Its input also can be Limit, so we should apply again.
return Ok(self
.rewrite(Arc::unwrap_or_clone(limit.input), _config)
.unwrap());
Review Comment:
Done. Thank you @goldmedal
--
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] feat: support arbitrary expressions in `LIMIT` plan [datafusion]
Posted by "jonahgao (via GitHub)" <gi...@apache.org>.
jonahgao commented on PR #13028:
URL: https://github.com/apache/datafusion/pull/13028#issuecomment-2428950514
> 1. I think it's good to follow Postgres' behavior for non-integer arguments
> 2. Don't support non-numeric arguments like bool, I think they are allowed to cast to integer, but not meaningful in SQL query (maybe there are other types that have a similar issue I'm not aware of)
DuckDB supports them, but I think we can follow PG for now until someone requests these features.
--
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] feat: support arbitrary expressions in `LIMIT` plan [datafusion]
Posted by "findepi (via GitHub)" <gi...@apache.org>.
findepi commented on code in PR #13028:
URL: https://github.com/apache/datafusion/pull/13028#discussion_r1810451330
##########
datafusion/expr/src/logical_plan/plan.rs:
##########
@@ -2799,14 +2817,71 @@ impl PartialOrd for Extension {
#[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Hash)]
pub struct Limit {
/// Number of rows to skip before fetch
- pub skip: usize,
+ pub skip: Option<Box<Expr>>,
Review Comment:
What are the constraints on the expression that can be used here?
For example, can it have any column references?
##########
datafusion/expr/src/logical_plan/plan.rs:
##########
@@ -2799,14 +2817,71 @@ impl PartialOrd for Extension {
#[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Hash)]
pub struct Limit {
/// Number of rows to skip before fetch
- pub skip: usize,
+ pub skip: Option<Box<Expr>>,
/// Maximum number of rows to fetch,
/// None means fetching all rows
- pub fetch: Option<usize>,
+ pub fetch: Option<Box<Expr>>,
/// The logical plan
pub input: Arc<LogicalPlan>,
}
+/// Different types of skip expression in Limit plan.
+pub enum SkipType {
+ /// The skip expression is a literal value.
+ Literal(usize),
+ /// Currently only supports expressions that can be folded into constants.
+ UnsupportedExpr,
Review Comment:
Limit is a relational operator, so this will always need to be constant-foldable.
--
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] feat: support arbitrary expressions in `LIMIT` plan [datafusion]
Posted by "jonahgao (via GitHub)" <gi...@apache.org>.
jonahgao commented on PR #13028:
URL: https://github.com/apache/datafusion/pull/13028#issuecomment-2430627797
> I think the floating value is caused by the DataFusion casting behavior is different from others. I tried to cast a float to an integer.
>
> Maybe related to #8302 and #11886
Maybe we can add support for floats after these issues are resolved🤔.
--
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] feat: support arbitrary expressions in `LIMIT` plan [datafusion]
Posted by "jonahgao (via GitHub)" <gi...@apache.org>.
jonahgao commented on code in PR #13028:
URL: https://github.com/apache/datafusion/pull/13028#discussion_r1808305148
##########
datafusion/expr/src/logical_plan/plan.rs:
##########
@@ -2799,14 +2817,71 @@ impl PartialOrd for Extension {
#[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Hash)]
pub struct Limit {
/// Number of rows to skip before fetch
- pub skip: usize,
+ pub skip: Option<Box<Expr>>,
Review Comment:
Use `Box` here to prevent increasing the size of `LogicalPlan`, that is`std::mem::size_of::<LogicalPlan>()`; otherwise it will cause a stack overflow in one of the array_ndims [test](https://github.com/apache/datafusion/blob/b42d9b81caddb5b53800f8eed32f9af4a9e3a01d/datafusion/sqllogictest/test_files/array.slt#L5152)
--
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] feat: support arbitrary expressions in `LIMIT` plan [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #13028:
URL: https://github.com/apache/datafusion/pull/13028#discussion_r1813910274
##########
datafusion/core/src/physical_planner.rs:
##########
@@ -796,8 +795,20 @@ impl DefaultPhysicalPlanner {
}
LogicalPlan::Subquery(_) => todo!(),
LogicalPlan::SubqueryAlias(_) => children.one()?,
- LogicalPlan::Limit(Limit { skip, fetch, .. }) => {
+ LogicalPlan::Limit(limit) => {
let input = children.one()?;
+ let SkipType::Literal(skip) = limit.get_skip_type()? else {
Review Comment:
👍
--
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] feat: support arbitrary expressions in `LIMIT` plan [datafusion]
Posted by "jonahgao (via GitHub)" <gi...@apache.org>.
jonahgao merged PR #13028:
URL: https://github.com/apache/datafusion/pull/13028
--
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] feat: support arbitrary expressions in `LIMIT` plan [datafusion]
Posted by "jonahgao (via GitHub)" <gi...@apache.org>.
jonahgao commented on code in PR #13028:
URL: https://github.com/apache/datafusion/pull/13028#discussion_r1810490001
##########
datafusion/expr/src/logical_plan/plan.rs:
##########
@@ -2799,14 +2817,71 @@ impl PartialOrd for Extension {
#[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Hash)]
pub struct Limit {
/// Number of rows to skip before fetch
- pub skip: usize,
+ pub skip: Option<Box<Expr>>,
Review Comment:
> What are the constraints on the expression that can be used here? For example, can it have any column references?
I think it can be any integer expression, and can also contain column references. Both PostgreSQL and DuckDB support `select 1 limit (select max(col0) from t)`.
```sh
v1.1.1-dev319 af39bd0dcf
D create table t as values(1);
D select 1 limit (select max(col0) from t);
┌───────┐
│ 1 │
│ int32 │
├───────┤
│ 1 │
└───────┘
```
--
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] feat: support arbitrary expressions in `LIMIT` plan [datafusion]
Posted by "jonahgao (via GitHub)" <gi...@apache.org>.
jonahgao commented on code in PR #13028:
URL: https://github.com/apache/datafusion/pull/13028#discussion_r1808297266
##########
datafusion/expr/src/logical_plan/plan.rs:
##########
@@ -1930,16 +1944,20 @@ impl LogicalPlan {
)
}
},
- LogicalPlan::Limit(Limit {
- ref skip,
- ref fetch,
- ..
- }) => {
+ LogicalPlan::Limit(limit) => {
+ // Attempt to display `skip` and `fetch` as literals if possible, otherwise as expressions.
Review Comment:
Display literals as before to avoid breaking too many tests. Maybe we could display them in the expr-style through a follow-up PR. For example, `1` -> `Int64(1)`.
--
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] feat: support arbitrary expressions in `LIMIT` plan [datafusion]
Posted by "jonahgao (via GitHub)" <gi...@apache.org>.
jonahgao commented on PR #13028:
URL: https://github.com/apache/datafusion/pull/13028#issuecomment-2429395372
Disallow non-integer types in https://github.com/apache/datafusion/pull/13028/commits/e6d1297cb9045f99cab21690335740853fd91507 . Fractional values are more complicated than I expected, let's handle them later. @2010YOUY01 @findepi
--
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] feat: support arbitrary expressions in `LIMIT` plan [datafusion]
Posted by "jonahgao (via GitHub)" <gi...@apache.org>.
jonahgao commented on code in PR #13028:
URL: https://github.com/apache/datafusion/pull/13028#discussion_r1810815912
##########
datafusion/expr/src/logical_plan/plan.rs:
##########
@@ -2799,14 +2817,71 @@ impl PartialOrd for Extension {
#[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Hash)]
pub struct Limit {
/// Number of rows to skip before fetch
- pub skip: usize,
+ pub skip: Option<Box<Expr>>,
/// Maximum number of rows to fetch,
/// None means fetching all rows
- pub fetch: Option<usize>,
+ pub fetch: Option<Box<Expr>>,
/// The logical plan
pub input: Arc<LogicalPlan>,
}
+/// Different types of skip expression in Limit plan.
+pub enum SkipType {
+ /// The skip expression is a literal value.
+ Literal(usize),
+ /// Currently only supports expressions that can be folded into constants.
+ UnsupportedExpr,
Review Comment:
Yes, but some expressions like `limit (select count(*) from t)` can't be folded at the planning stage , so we need to keep them for conversion into physical expressions later.
--
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] feat: support arbitrary expressions in `LIMIT` plan [datafusion]
Posted by "2010YOUY01 (via GitHub)" <gi...@apache.org>.
2010YOUY01 commented on PR #13028:
URL: https://github.com/apache/datafusion/pull/13028#issuecomment-2429157042
> @2010YOUY01 good point! do you maybe know what is the use-case for allowing fractional values for LIMIT or OFFSET?
Not really, allowing fractional values looks quite bizarre to me, however, nowadays many SQL are auto-generated so we can't make too many assumptions
> this looks like a pandora box. strictly speaking, casting them to integral values isn't exactly what we can do: `OFFSET 1.5 LIMIT 1.5` should return 3rd row, but not the 4th row.
This way it requires lots of effort to match PostgreSQL's behavior 🤔 Maybe it should be left to a later task
--
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] feat: support arbitrary expressions in `LIMIT` plan [datafusion]
Posted by "goldmedal (via GitHub)" <gi...@apache.org>.
goldmedal commented on code in PR #13028:
URL: https://github.com/apache/datafusion/pull/13028#discussion_r1809054261
##########
datafusion/optimizer/src/eliminate_limit.rs:
##########
@@ -63,17 +63,23 @@ impl OptimizerRule for EliminateLimit {
> {
match plan {
LogicalPlan::Limit(limit) => {
- if let Some(fetch) = limit.fetch {
- if fetch == 0 {
+ // Only supports rewriting for literal fetch
+ let FetchType::Literal(fetch) = limit.get_fetch_type()? else {
+ return Ok(Transformed::no(LogicalPlan::Limit(limit)));
+ };
+
+ if let Some(v) = fetch {
+ if v == 0 {
return Ok(Transformed::yes(LogicalPlan::EmptyRelation(
EmptyRelation {
produce_one_row: false,
schema: Arc::clone(limit.input.schema()),
},
)));
}
- } else if limit.skip == 0 {
- // input also can be Limit, so we should apply again.
+ } else if matches!(limit.get_skip_type()?, SkipType::Literal(0)) {
+ // If fetch is `None` and skip is 0, then Limit takes no effect and
+ // we can remove it. Its input also can be Limit, so we should apply again.
return Ok(self
.rewrite(Arc::unwrap_or_clone(limit.input), _config)
.unwrap());
Review Comment:
```suggestion
.rewrite(Arc::unwrap_or_clone(limit.input), _config)?);
```
Just noticed this point. It's better to return an error instead of a panic. Could you help to improve it?
##########
datafusion/expr/src/expr_rewriter/mod.rs:
##########
@@ -308,7 +308,10 @@ impl NamePreserver {
Self {
// The schema of Filter and Join nodes comes from their inputs rather than their output expressions,
Review Comment:
```suggestion
// The schema of Filter, Join, and Limit nodes comes from their inputs rather than their output expressions,
```
--
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] feat: support arbitrary expressions in `LIMIT` plan [datafusion]
Posted by "findepi (via GitHub)" <gi...@apache.org>.
findepi commented on code in PR #13028:
URL: https://github.com/apache/datafusion/pull/13028#discussion_r1810458179
##########
datafusion/expr/src/logical_plan/plan.rs:
##########
@@ -2799,14 +2817,71 @@ impl PartialOrd for Extension {
#[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Hash)]
pub struct Limit {
/// Number of rows to skip before fetch
- pub skip: usize,
+ pub skip: Option<Box<Expr>>,
Review Comment:
Would it be possible to do the constant folding when _building_ Limit node, so that logical plan structure remains intact? See also https://github.com/apache/datafusion/issues/12723
--
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] feat: support arbitrary expressions in `LIMIT` plan [datafusion]
Posted by "jonahgao (via GitHub)" <gi...@apache.org>.
jonahgao commented on code in PR #13028:
URL: https://github.com/apache/datafusion/pull/13028#discussion_r1812109755
##########
datafusion/expr/src/logical_plan/plan.rs:
##########
@@ -2799,14 +2817,71 @@ impl PartialOrd for Extension {
#[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Hash)]
pub struct Limit {
/// Number of rows to skip before fetch
- pub skip: usize,
+ pub skip: Option<Box<Expr>>,
/// Maximum number of rows to fetch,
/// None means fetching all rows
- pub fetch: Option<usize>,
+ pub fetch: Option<Box<Expr>>,
/// The logical plan
pub input: Arc<LogicalPlan>,
}
+/// Different types of skip expression in Limit plan.
+pub enum SkipType {
+ /// The skip expression is a literal value.
+ Literal(usize),
+ /// Currently only supports expressions that can be folded into constants.
+ UnsupportedExpr,
Review Comment:
Constant folding requires the `TypeCoercion` and `SimplifyExpressions` rule. We can't directly invoke them during planning for now and need to defer them to optimizer.
Another reason the Limit node needs to contain expressions is to support Prepare statements. Issue #12294 requires the Limit node to use `Expr::Placeholder`.
--
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] feat: support arbitrary expressions in `LIMIT` plan [datafusion]
Posted by "jonahgao (via GitHub)" <gi...@apache.org>.
jonahgao commented on code in PR #13028:
URL: https://github.com/apache/datafusion/pull/13028#discussion_r1812109755
##########
datafusion/expr/src/logical_plan/plan.rs:
##########
@@ -2799,14 +2817,71 @@ impl PartialOrd for Extension {
#[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Hash)]
pub struct Limit {
/// Number of rows to skip before fetch
- pub skip: usize,
+ pub skip: Option<Box<Expr>>,
/// Maximum number of rows to fetch,
/// None means fetching all rows
- pub fetch: Option<usize>,
+ pub fetch: Option<Box<Expr>>,
/// The logical plan
pub input: Arc<LogicalPlan>,
}
+/// Different types of skip expression in Limit plan.
+pub enum SkipType {
+ /// The skip expression is a literal value.
+ Literal(usize),
+ /// Currently only supports expressions that can be folded into constants.
+ UnsupportedExpr,
Review Comment:
Constant folding requires the `TypeCoercion` and `SimplifyExpressions` rule. We can't directly invoke them during building for now and need to defer them to optimizer.
Another reason the Limit node needs to contain expressions is to support Prepare statements. Issue #12294 requires the Limit node to use `Expr::Placeholder`.
--
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] feat: support arbitrary expressions in `LIMIT` plan [datafusion]
Posted by "jonahgao (via GitHub)" <gi...@apache.org>.
jonahgao commented on PR #13028:
URL: https://github.com/apache/datafusion/pull/13028#issuecomment-2434114066
Thanks @goldmedal @findepi @2010YOUY01 @alamb for the review.
--
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