Design for `LIMIT` in pagination. #1752

Yury-Fridlyand · 2023-06-17T00:14:26Z

Description

This PR is design review for supporting LIMIT in pagination. It includes:

doc describing changes need for this feature
MVP PoC code

TODOs:

Close cursor when limit reached
Optimize push down operations - don't push down page size if page_size > limit
Test MergeFilterAndFilter and PushFilterUnderSort optimizer rules (probably by combining WHERE and HAVING clauses or with PPL query)

Issues Resolved

N/A

Check List

Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Signed-off-by: Yury-Fridlyand <[email protected]>

codecov · 2023-06-17T00:34:51Z

Codecov Report

Merging #1752 (195de33) into feature/pagination/limit (94d5479) will increase coverage by 0.74%.
The diff coverage is 64.86%.

@@                      Coverage Diff                       @@
##             feature/pagination/limit    #1752      +/-   ##
==============================================================
+ Coverage                       97.33%   98.08%   +0.74%     
+ Complexity                       4408     3436     -972     
==============================================================
  Files                             388      292      -96     
  Lines                           10938     8364    -2574     
  Branches                          773      573     -200     
==============================================================
- Hits                            10647     8204    -2443     
+ Misses                            284      157     -127     
+ Partials                            7        3       -4

Flag	Coverage Δ
sql-engine	`98.08% <64.86%> (+0.74%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...opensearch/sql/planner/physical/LimitOperator.java	`55.17% <27.77%> (-44.83%)`	⬇️
...ch/sql/executor/pagination/CanPaginateVisitor.java	`100.00% <100.00%> (ø)`
...ch/sql/planner/optimizer/LogicalPlanOptimizer.java	`100.00% <100.00%> (ø)`
...planner/optimizer/rule/read/TableScanPushDown.java	`100.00% <100.00%> (ø)`
...logical/PrometheusLogicalPlanOptimizerFactory.java	`100.00% <100.00%> (ø)`

... and 96 files with indirect coverage changes

Signed-off-by: Yury-Fridlyand <[email protected]>

forestmvey · 2023-06-20T15:02:46Z

docs/dev/limit-in-pagination.md

+
+## Solution
+
+Don't do push down for `LIMIT`. `LimitOperator` Physical Plan Tree node will cut off yielding search results with minimal overhead.


If we utilize the limit operator how will this work with any other operator that needs to do post-processing. This also asks the question of how we will continue to add operators and how will they work with each other. We may need some way to chain operators if we don't want to have the limitation of one operator for post-processing per query.

Rather than have an operator to limit the output with post processing why not include the functionality as part of the base class. We can have an optional limit if push down isn't available that can be performed in the PhysicalPlan base class prior to the inheriting class's next() call. Another option would be to have the limit post-processing performed in the ResourceMonitorPlan prior to calling the delegate next().

acarbonetto · 2023-06-19T16:20:14Z

core/src/main/java/org/opensearch/sql/planner/optimizer/LogicalPlanOptimizer.java

  }

  /**
   * Optimize {@link LogicalPlan}.
   */
  public LogicalPlan optimize(LogicalPlan plan) {
-    LogicalPlan optimized = internalOptimize(plan);
+    var node = plan;


maybe just rename the argument?

acarbonetto · 2023-06-21T06:20:02Z

docs/dev/limit-in-pagination.md

+
+## Problem statement
+
+`LIMIT` clause is being converted to `size` by SQL plugin during push down operation.


We need to properly define what we want to accomplish, but discuss what is currently the problem with the code. I would change this to talk about LIMIT and size conflict, but also include the use cases:

When LIMIT > size

When size < LIMIT

When size == LIMIT (this is kind of naive case, but maybe just mention it anyways)
Then discuss exactly how we expect the system to behave (which does it return a cursor, when does it NOT return a cursor, where does it break (max_window_size), and does this override the default fetch size.
If there is anything to add in terms of how the JDBC driver will behave, then include it here.

acarbonetto · 2023-06-21T06:21:00Z

docs/dev/limit-in-pagination.md

+
+## Solution
+
+Don't do push down for `LIMIT`. `LimitOperator` Physical Plan Tree node will cut off yielding search results with minimal overhead.


I don't think this is the solution we talked about in the walk-through... I think you just need to reverse the business logic to execute all the rules on nodes before proceeding to the next node...

Seems like this section is similar to Fix below

acarbonetto · 2023-06-21T06:21:37Z

docs/dev/limit-in-pagination.md

+CreateTableScanBuilder
+PushDownPageSize
+...
+PUSH_DOWN_LIMIT


why is this ordering important?

acarbonetto · 2023-06-21T06:22:25Z

docs/dev/limit-in-pagination.md

+...
+```
+
+This gives us warranty that `pushDownLimit` operation would be rejected if `pushPageSize` called before. Then, not optimized Logical Plan Tree node `LogicalLimit` will be converted to `LimitOperator` Physical Plan tree node.


Suggested change

This gives us warranty that `pushDownLimit` operation would be rejected if `pushPageSize` called before. Then, not optimized Logical Plan Tree node `LogicalLimit` will be converted to `LimitOperator` Physical Plan tree node.

This gives us guarantee that `pushDownLimit` operation will be rejected if `pushPageSize` already called. In that case, the not-optimized Logical Plan Tree node `LogicalLimit` will be converted to `LimitOperator` Physical Plan tree node.

acarbonetto · 2023-06-21T06:22:59Z

docs/dev/limit-in-pagination.md

+3. Make `LimitOperator` properly serialized and deserialized.
+4. Make `OpenSearchIndexScanBuilder::pushDownLimit` return `false` if `pushDownPageSize` was called before.
+5. (Optional) Groom `Optimizer` to reduce amount of unchecked casts and uses raw classes.
+6. (Optional) Rework `Optimizer` to make it a tree visitor.


Out of scope work. Please raise a follow-up ticket to do these later. You can label this as maintenance ticket.

acarbonetto · 2023-06-21T06:23:58Z

...ava/org/opensearch/sql/prometheus/planner/logical/PrometheusLogicalPlanOptimizerFactory.java

        new MergeFilterAndRelation(),
        new MergeAggAndIndexScan(),
        new MergeAggAndRelation()
-    ));
+    ).map(r -> (Rule<LogicalPlan>)r).collect(Collectors.toList()));


this is messy, and we can update the Rules classes to extend LogicalPlan instead.

Yury-Fridlyand · 2023-06-23T19:22:02Z

Design review meeting notes:

Approach is correct, we should proceed in that direction
Split implementation into parts:
1. Optimizer rework
2. Reorder rules and update push down operations
3. Add support for limit in paginated queries

Design doc for LIMIT in pagination.

9cb88a2

Signed-off-by: Yury-Fridlyand <[email protected]>

Yury-Fridlyand added the pagination Pagination feature, ref #656 label Jun 17, 2023

Yury-Fridlyand requested review from pjfitzgibbons, ps48, kavithacm, derek-ho, joshuali925, dai-chen, YANG-DB, rupal-bq, mengweieric, vamsi-amazon, Swiddis, penghuo, seankao-az, MaxKsyunz, anirudha, forestmvey, acarbonetto and GumpacG as code owners June 17, 2023 00:14

Yury-Fridlyand marked this pull request as draft June 17, 2023 00:14

Code PoC MVP.

195de33

Signed-off-by: Yury-Fridlyand <[email protected]>

forestmvey reviewed Jun 20, 2023

View reviewed changes

Yury-Fridlyand mentioned this pull request Jun 20, 2023

[FEATURE] Implement pagination in V2 #1759

Open

18 tasks

acarbonetto requested changes Jun 21, 2023

View reviewed changes

Yury-Fridlyand mentioned this pull request Jun 22, 2023

[BUG] LIMIT works incorrectly with NESTED #1764

Open

This was referenced Jun 24, 2023

[BUG] aggregation works incorrectly with LIMIT #1774

Open

[BUG] Nested Function With Sort Doesn't Sort Inner Hits #1788

Open

Yury-Fridlyand mentioned this pull request Jun 27, 2023

[BUG] PPL query with head and sort can not properly rewrite as DSL. #494

Open

Yury-Fridlyand mentioned this pull request Jul 15, 2023

[RFC] Logical Plan Optimizer rework #1872

Open

5 tasks

sudssf mentioned this pull request Oct 21, 2023

[BUG] unsupported operator: if when using sql API #2320

Open

YANG-DB mentioned this pull request Jul 3, 2024

[META]Add Missing PPL commands - campaign opensearch-project/opensearch-spark#408

Open

35 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Design for `LIMIT` in pagination. #1752

Design for `LIMIT` in pagination. #1752

Yury-Fridlyand commented Jun 17, 2023 •

edited

Loading

codecov bot commented Jun 17, 2023 •

edited

Loading

forestmvey Jun 20, 2023 •

edited

Loading

acarbonetto Jun 19, 2023

acarbonetto Jun 21, 2023

acarbonetto Jun 21, 2023

acarbonetto Jun 21, 2023

acarbonetto Jun 21, 2023

acarbonetto Jun 21, 2023

acarbonetto Jun 21, 2023

acarbonetto Jun 21, 2023

Yury-Fridlyand commented Jun 23, 2023


		## Solution

		Don't do push down for `LIMIT`. `LimitOperator` Physical Plan Tree node will cut off yielding search results with minimal overhead.


		## Problem statement

		`LIMIT` clause is being converted to `size` by SQL plugin during push down operation.

	This gives us warranty that `pushDownLimit` operation would be rejected if `pushPageSize` called before. Then, not optimized Logical Plan Tree node `LogicalLimit` will be converted to `LimitOperator` Physical Plan tree node.
	This gives us guarantee that `pushDownLimit` operation will be rejected if `pushPageSize` already called. In that case, the not-optimized Logical Plan Tree node `LogicalLimit` will be converted to `LimitOperator` Physical Plan tree node.

Design for LIMIT in pagination. #1752

Are you sure you want to change the base?

Design for LIMIT in pagination. #1752

Conversation

Yury-Fridlyand commented Jun 17, 2023 • edited Loading

Description

TODOs:

Issues Resolved

Check List

codecov bot commented Jun 17, 2023 • edited Loading

Codecov Report

forestmvey Jun 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Yury-Fridlyand commented Jun 23, 2023

Design for `LIMIT` in pagination. #1752

Design for `LIMIT` in pagination. #1752

Yury-Fridlyand commented Jun 17, 2023 •

edited

Loading

codecov bot commented Jun 17, 2023 •

edited

Loading

forestmvey Jun 20, 2023 •

edited

Loading