You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2022/05/26 16:08:23 UTC

[GitHub] [arrow-datafusion] ming535 opened a new issue, #2624: limit_push_down is not working properly with OFFSET

ming535 opened a new issue, #2624:
URL: https://github.com/apache/arrow-datafusion/issues/2624

   **Describe the bug**
   `limit_push_down` tries to push a limit to TableScan when `OFFSET` is without `LIMIT` which makes all the result disappeared.
   `OFFSET` without `LIMIT` is allowed in Postgres.
   
   **To Reproduce**
   in `limit_push_down.rs`, add this test code:
   `#[test]
       fn limit_pushdown_should_not_pushdown_limit_with_offset_only() -> Result<()> {
   
           let table_scan = test_table_scan()?;
           let plan = LogicalPlanBuilder::from(table_scan)
               .offset(10)?
               .build()?;
   
           // Should not push any limit down to table provider
           // When it has a select
           let expected = "Offset: 10\
           \n  TableScan: test projection=None";
   
           assert_optimized_plan_eq(&plan, expected);
   
           Ok(())
       }
   `
   
   The problem is here in the code, which adds a limit when `OFFSET` doesnt have one:
   <img width="1368" alt="Screen Shot 2022-05-27 at 00 05 17" src="https://user-images.githubusercontent.com/7576663/170527953-36a7412e-ea4e-4146-ae86-b8f35c7cbe58.png">
   
   
   **Expected behavior**
   The test case should pass. No limit should pushed down to TableScan.
   
   **Additional context**
   I was trying to implement the `OFFSET` physical plan.
   


-- 
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@arrow.apache.org.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow-datafusion] ming535 commented on issue #2624: limit_push_down is not working properly with OFFSET

Posted by GitBox <gi...@apache.org>.
ming535 commented on issue #2624:
URL: https://github.com/apache/arrow-datafusion/issues/2624#issuecomment-1138768411

   How about add another LogicalPlan `LimitWithOffset`. So that the three LogicalPlan used in different cases:
   1. `Limit`: without offset
   2. `Offset`: without limit
   3. `LimitWithOffset`: offset + limit
   
   Then in the `limit_push_down`, we need to handle two cases: `Limit` and `LimitWithOffset`.
   For `Limit`, the push down logic is roughly the same as before; for `LimitWithOffset`, we adjust the limit pushed down.


-- 
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@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow-datafusion] alamb commented on issue #2624: limit_push_down is not working properly with OFFSET

Posted by GitBox <gi...@apache.org>.
alamb commented on issue #2624:
URL: https://github.com/apache/arrow-datafusion/issues/2624#issuecomment-1139509273

   What about consolidating Limit and Offset into `LimitWithOffset` (both logical and physical plans), with optional limit or offset:
   
   ```rust
     limit: Option<usize>,
     offset: Option<usize>
   ```
   
   Then when a limit / offset is pushed down, it can be transformed from `Limit(limit, offset)` to `Limit(limit+offset, 0)` ?


-- 
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@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow-datafusion] ming535 commented on issue #2624: limit_push_down is not working properly with OFFSET

Posted by GitBox <gi...@apache.org>.
ming535 commented on issue #2624:
URL: https://github.com/apache/arrow-datafusion/issues/2624#issuecomment-1138750473

   Any idea on how to handle this? I was thinking adding another boolean attribute `has_limit` inside `LogicalPlan::OffsetPlan`, and handle the case when there is no limit in `limit_push_down`.


-- 
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@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow-datafusion] alamb closed issue #2624: limit_push_down is not working properly with OFFSET

Posted by GitBox <gi...@apache.org>.
alamb closed issue #2624: limit_push_down is not working properly with OFFSET
URL: https://github.com/apache/arrow-datafusion/issues/2624


-- 
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@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org