Support pagination in V2 engine, phase 1

[Yury-Fridlyand]

Description

https://github.com/Bit-Quill/opensearch-project-sql/blob/61767f2b200b2a7f8c8b2df32de209b3c30caa61/docs/dev/Pagination-v2.md

All credits to @MaxKsyunz.

You can use attached script for testing as well. Command line: ./cursor_test.sh <table> <page size>. Requires jq. Rename it before use.
cursor_test.sh.txt

Scroll API usage doc:
https://www.elastic.co/guide/en/elasticsearch/client/java-api/current/java-search-scrolling.html

TODOs

• Move _explain handlers to one place (see 74b1e94) - fixed in 304616d
• Remove of fix commented code of scroll cleaning (see 9db17c3) - clean scroll on the last page only - fixed in 1b5ab7e
• Make scroll timeout configurable (see 7b508db#diff-bf04623db606aec89ecb2e337e7cdcb4c2525b25a437dba60113f86717f7eff5)
• Complete TODOs left in code and remove them; open tickets for further changes
• Add more integration tests - Fallback to v1 engine for pagination #245
• Add unit tests for full code coverage - done in Pagination, phase 1: Add unit tests for :opensearch module with coverage. #233, Pagination, phase 1: Add unit tests for :core module with coverage. #230, Pagination, phase 1: Add unit tests for SQL module with coverage. #239
• Record a demo - https://user-images.githubusercontent.com/88679692/224208630-8d38d833-abf8-4035-8d15-d5fb4382deca.mp4
• Update docs/doctests : cursor
• Update docs : limitations (https://github.com/opensearch-project/sql/blob/main/docs/user/limitations/limitations.rst)
• Add a developer's guide for pagination implementation with schema/workflow
• Fix: total in jdbc response shows current page size (TotalHits aren't accounted) - fixed in f4ea4ad
• Test (add ITs) for a case when data is removed/added while scrolling an index

Follow-up TODOs

Decided to do out of scope of this PR:

• Remove duplicating entities SqlRequest and SQLQueryRequest or at least fix processing json content (if cursor detected, content isn't kept)
• Rework cursor reading/writing:
  • Convert cursor parsing to deserialization to make it scalable
  • Move cursor serialization and deserialization to one place (see 1a83941)
  • Rework cursor serialization - binary objects are being converted to string twice - in DefaultExpressionSerializer and in compress function; keep cursor as a binary object.
  • Optional: Encode/encrypt cursor to avoid it being modified/hacked
• Should we return cursor automatically for big tables?

curl -s -XPOST http://localhost:9200/_plugins/_sql -H 'Content-Type: application/json' -d '{"query": "SELECT * FROM online" }' | jq .total,.cursor

Issues Resolved

opensearch-project#656

Check List

• New functionality includes testing.
  • All tests pass, including unit test, integration test and doctest
• New functionality has been documented.
  • New functionality has javadoc added
  • New functionality has user manual doc added
• 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.

[penghuo]

why get TableScanOperator in StorageEngine?

[Yury-Fridlyand]

It is used in cursor deserialization/parsing, this happens in :core module. All implementation of StorageEngine are defined outside of :core and unavailable and that point.

[MaxKsyunz]

@penghuo does this design make sense? Would love to know what you think.

[penghuo]

difference is CreatePagingTableScanBuilder()?

[Yury-Fridlyand]

PushPageSize and CreatePagingTableScanBuilder instead of CreateTableScanBuilder

[acarbonetto]

There's a lot of overlap here. You could create a common function create() and private functions that push the transformation and push_down capabilities separately. Something like:

  public static LogicalPlanOptimizer create() {
    return create(false);
  }

  public static LogicalPlanOptimizer create(boolean isPagingRequired) {
    return new LogicalPlanOptimizer(Arrays.asList(
        /*
         * Phase 1: Transformations that rely on relational algebra equivalence
         */
        getFilterTransformations(),
        /*
         * Phase 2: Transformations that rely on data source push down capability
         */
        isPagingRequired ? getPaginatedTableScanBuilder() : getTableScanBuilder(),
        getPushDownTransformations(),
        getTableWriteBuilder()
    ));
  }

  private static List getFilterTransformations() {
    return Arrays.asList(
        new MergeFilterAndFilter(),
        new PushFilterUnderSort()
    );
  }

  private static List getTableScanBuilder() {
    return Arrays.asList(
        new CreateTableScanBuilder(),
        );
  }

  private static List getPaginatedTableScanBuilder() {
    return Arrays.asList(
        new PushPageSize(),
        new CreatePagingTableScanBuilder(),
        );
  }

[MaxKsyunz]

@Yury-Fridlyand please have a look at my comments so far. There will be more to come.

[MaxKsyunz]

Could you implement this similar to PUSH_DOWN_* rules:

• Add a pushDownPageSize method to TableScanBuilder
• Add a PUSH_DOWN_PAGE_SIZE similar to others that matches LogicalPaginate of scanBuilder and calls pushDownPageSize.

Doing so would remove the need for PushPageSize and LogicalRelation.pageSize property, be less code, and follow a pattern already used in the code base, like PUSH_DOWN_LIMIT.

[MaxKsyunz]

@penghuo does this design make sense? Would love to know what you think.

[MaxKsyunz]

Why is such a long time out necessary?

[Yury-Fridlyand]

I set it for debugging purposes, but didn't revert, because 1 min is too small.
TBD.
@penghuo, @dai-chen,
What should be the default scroll lifetime?
We're looking forward to make it configurable.

[dai-chen]

@Yury-Fridlyand you mean this configuration? https://github.com/opensearch-project/sql/blob/main/docs/user/admin/settings.rst#plugins-sql-cursor-keep-alive

[acarbonetto]

do we have a test where hasNext returns true then false based on pagedSize and numReturned?

[GabeFernandez310]

Is this TODO (and other TODOs in the PR) meant to be implemented in Phase 2?

[Yury-Fridlyand]

Good practice is to leave no TODOs behind.
I'll do my best to resolve them ASAP, but some of them are related to design of existing code and can't be fixed easily.
This very TODO related to a terminated search query - plugin doesn't handle such case for any queries.

[acarbonetto]

There's a lot of overlap here. You could create a common function create() and private functions that push the transformation and push_down capabilities separately. Something like:

  public static LogicalPlanOptimizer create() {
    return create(false);
  }

  public static LogicalPlanOptimizer create(boolean isPagingRequired) {
    return new LogicalPlanOptimizer(Arrays.asList(
        /*
         * Phase 1: Transformations that rely on relational algebra equivalence
         */
        getFilterTransformations(),
        /*
         * Phase 2: Transformations that rely on data source push down capability
         */
        isPagingRequired ? getPaginatedTableScanBuilder() : getTableScanBuilder(),
        getPushDownTransformations(),
        getTableWriteBuilder()
    ));
  }

  private static List getFilterTransformations() {
    return Arrays.asList(
        new MergeFilterAndFilter(),
        new PushFilterUnderSort()
    );
  }

  private static List getTableScanBuilder() {
    return Arrays.asList(
        new CreateTableScanBuilder(),
        );
  }

  private static List getPaginatedTableScanBuilder() {
    return Arrays.asList(
        new PushPageSize(),
        new CreatePagingTableScanBuilder(),
        );
  }

[github-advanced-security]

You have successfully added a new CodeQL configuration .github/workflows/codeql-analysis.yml:analyze/language:java. As part of the setup process, we have scanned this repository and found 5 existing alerts. Please check the repository Security tab to see all alerts.