Add pit for pagination query #2940

manasvinibs · 2024-08-15T20:30:07Z

Description

Added option to use search after with pit instead of scroll api for pagination query

Related Issues

Manual test on sample queries - #2941

Check List

New functionality includes testing.
New functionality has been documented.
New functionality has javadoc added.
New functionality has a user manual doc added.
API changes companion pull request created.
Commits are signed per the DCO using --signoff.
Public documentation issue/PR created.

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.

* Add search after for join Signed-off-by: Rupal Mahajan <maharup@amazon.com> * Enable search after by default Signed-off-by: Rupal Mahajan <maharup@amazon.com> * Add pit Signed-off-by: Rupal Mahajan <maharup@amazon.com> * nit Signed-off-by: Rupal Mahajan <maharup@amazon.com> * Fix tests Signed-off-by: Rupal Mahajan <maharup@amazon.com> * ignore joinWithGeoIntersectNL Signed-off-by: Rupal Mahajan <maharup@amazon.com> * Rerun CI with scroll Signed-off-by: Rupal Mahajan <maharup@amazon.com> * Remove unused code and retrigger CI with search_after true Signed-off-by: Rupal Mahajan <maharup@amazon.com> * Address comments Signed-off-by: Rupal Mahajan <maharup@amazon.com> * Remove unused code change Signed-off-by: Rupal Mahajan <maharup@amazon.com> * Update pit keep alive time with SQL_CURSOR_KEEP_ALIVE Signed-off-by: Rupal Mahajan <maharup@amazon.com> * Fix scroll condition Signed-off-by: Rupal Mahajan <maharup@amazon.com> * nit Signed-off-by: Rupal Mahajan <maharup@amazon.com> * Add pit before query execution Signed-off-by: Rupal Mahajan <maharup@amazon.com> * nit Signed-off-by: Rupal Mahajan <maharup@amazon.com> * Move pit from join request builder to executor Signed-off-by: Rupal Mahajan <maharup@amazon.com> * Remove unused methods Signed-off-by: Rupal Mahajan <maharup@amazon.com> * Add pit in parent class's run() Signed-off-by: Rupal Mahajan <maharup@amazon.com> * Add comment for fetching subsequent result in NestedLoopsElasticExecutor Signed-off-by: Rupal Mahajan <maharup@amazon.com> * Update comment Signed-off-by: Rupal Mahajan <maharup@amazon.com> * Add javadoc for pit handler Signed-off-by: Rupal Mahajan <maharup@amazon.com> * Add pit interface Signed-off-by: Rupal Mahajan <maharup@amazon.com> * Add pit handler unit test Signed-off-by: Rupal Mahajan <maharup@amazon.com> * Fix failed unit test CI Signed-off-by: Rupal Mahajan <maharup@amazon.com> * Fix spotless error Signed-off-by: Rupal Mahajan <maharup@amazon.com> * Rename pit class and add logs Signed-off-by: Rupal Mahajan <maharup@amazon.com> * Fix pit delete unit test Signed-off-by: Rupal Mahajan <maharup@amazon.com> --------- Signed-off-by: Rupal Mahajan <maharup@amazon.com>

* Add search after for join Signed-off-by: Rupal Mahajan <maharup@amazon.com> * Enable search after by default Signed-off-by: Rupal Mahajan <maharup@amazon.com> * Add pit Signed-off-by: Rupal Mahajan <maharup@amazon.com> * nit Signed-off-by: Rupal Mahajan <maharup@amazon.com> * Fix tests Signed-off-by: Rupal Mahajan <maharup@amazon.com> * ignore joinWithGeoIntersectNL Signed-off-by: Rupal Mahajan <maharup@amazon.com> * Rerun CI with scroll Signed-off-by: Rupal Mahajan <maharup@amazon.com> * draft Signed-off-by: Rupal Mahajan <maharup@amazon.com> * Remove unused code and retrigger CI with search_after true Signed-off-by: Rupal Mahajan <maharup@amazon.com> * Address comments Signed-off-by: Rupal Mahajan <maharup@amazon.com> * Remove unused code change Signed-off-by: Rupal Mahajan <maharup@amazon.com> * Update pit keep alive time with SQL_CURSOR_KEEP_ALIVE Signed-off-by: Rupal Mahajan <maharup@amazon.com> * Fix scroll condition Signed-off-by: Rupal Mahajan <maharup@amazon.com> * nit Signed-off-by: Rupal Mahajan <maharup@amazon.com> * Add pit before query execution Signed-off-by: Rupal Mahajan <maharup@amazon.com> * Refactor get response with pit method Signed-off-by: Rupal Mahajan <maharup@amazon.com> * Update remaining scroll search calls Signed-off-by: Rupal Mahajan <maharup@amazon.com> * Fix integ test failures Signed-off-by: Rupal Mahajan <maharup@amazon.com> * nit Signed-off-by: Rupal Mahajan <maharup@amazon.com> * Move pit from join request builder to executor Signed-off-by: Rupal Mahajan <maharup@amazon.com> * Remove unused methods Signed-off-by: Rupal Mahajan <maharup@amazon.com> * Move pit from request to executor Signed-off-by: Rupal Mahajan <maharup@amazon.com> * Fix pit.delete call missed while merge Signed-off-by: Rupal Mahajan <maharup@amazon.com> * Move getResponseWithHits method to util class Signed-off-by: Rupal Mahajan <maharup@amazon.com> * add try catch for create delete pit in minus executor Signed-off-by: Rupal Mahajan <maharup@amazon.com> * move all common fields to ElasticHitsExecutor Signed-off-by: Rupal Mahajan <maharup@amazon.com> * add javadoc for ElasticHitsExecutor Signed-off-by: Rupal Mahajan <maharup@amazon.com> * Add missing javadoc Signed-off-by: Rupal Mahajan <maharup@amazon.com> * Forcing an empty commit as last commit is stuck processing updates Signed-off-by: Rupal Mahajan <maharup@amazon.com> --------- Signed-off-by: Rupal Mahajan <maharup@amazon.com>

Signed-off-by: Rupal Mahajan <maharup@amazon.com>

Signed-off-by: Manasvini B S <manasvis@amazon.com>

legacy/src/main/java/org/opensearch/sql/legacy/cursor/DefaultCursor.java

legacy/src/main/java/org/opensearch/sql/legacy/pit/PointInTimeHandlerImpl.java

vamsi-amazon · 2024-08-22T20:51:14Z

legacy/src/main/java/org/opensearch/sql/legacy/pit/PointInTimeHandlerImpl.java

+      LOG.info("Created Point In Time {} successfully.", pitId);
+      return true;
+    } catch (InterruptedException | ExecutionException e) {
+      throw new RuntimeException("Error occurred while creating PIT.", e);


What will be the end user response when we encounter these?

Query should fail. User will see exception with the failure message followed by stack trace of underlying execution exception.

Can you test it and see what is the error code and message that is coming up. You can add fake exception and test it.

you can do this after merging the code as well.

vamsi-amazon · 2024-08-22T20:52:00Z

legacy/src/main/java/org/opensearch/sql/legacy/pit/PointInTimeHandlerImpl.java

+      CreatePitResponse pitResponse = execute.get();
+      pitId = pitResponse.getId();
+      LOG.info("Created Point In Time {} successfully.", pitId);
+      return true;


What is this boolean used for?

Where is false scenario?

Currently I see this boolean is used only in unit tests for assertions. When PIT creation fails, I'm throwing exception instead of returning false to stop the query execution.

we don't change interface methods just for unit tests.
In unit tests, use some other way by checking no exceptions are made and change this method to void.

vamsi-amazon · 2024-08-23T01:50:57Z

legacy/src/main/java/org/opensearch/sql/legacy/cursor/DefaultCursor.java

+
+      Object[] sortFieldValue =
+          AccessController.doPrivileged(
+              (PrivilegedAction<Object[]>)
+                  () -> {
+                    try {
+                      return objectMapper.readValue(json.getString(SORT_FIELDS), Object[].class);
+                    } catch (JsonProcessingException e) {
+                      throw new RuntimeException(
+                          "Failed to parse sort fields from JSON string.", e);
+                    }
+                  });
+      cursor.setSortFields(sortFieldValue);
+
+      // Retrieve the SearchSourceBuilder from the JSON field
+      String searchSourceBuilderBase64 = json.getString("searchSourceBuilder");
+      byte[] bytes = Base64.getDecoder().decode(searchSourceBuilderBase64);
+      ByteArrayInputStream streamInput = new ByteArrayInputStream(bytes);
+      try {
+        XContentParser parser =
+            XContentType.JSON
+                .xContent()
+                .createParser(xContentRegistry, IGNORE_DEPRECATIONS, streamInput);
+        SearchSourceBuilder sourceBuilder = SearchSourceBuilder.fromXContent(parser);
+        cursor.setSearchSourceBuilder(sourceBuilder);
+      } catch (IOException ex) {
+        throw new RuntimeException("Failed to get searchSourceBuilder from cursor Id", ex);
+      }


can we refactor this to a new method.

sure, updated

vamsi-amazon · 2024-08-23T01:54:13Z

legacy/src/main/java/org/opensearch/sql/legacy/executor/cursor/CursorCloseExecutor.java

+      if (pit.delete()) {
+        return SUCCEEDED_TRUE;
+      } else {
+        Metrics.getInstance().getNumericalMetric(MetricName.FAILED_REQ_COUNT_SYS).increment();
+        return SUCCEEDED_FALSE;
+      }


Does this makes sense, if we are never returning false from delete method. 91-93 would never get executed.

if you want to throw exception in delete method, catch exception here and emit metric.
If you want to return boolean, don't throw exception in delete method.

If possible take insipiration from how scroll is executed below.

Yes that's right! Updated the places using delete function to catch exception and log metrics similar to scroll.

Signed-off-by: Manasvini B S <manasvis@amazon.com>

vamsi-amazon · 2024-08-23T21:05:47Z

legacy/src/main/java/org/opensearch/sql/legacy/cursor/DefaultCursor.java

    return String.format("%s:%s", type.getId(), encodeCursor(json));
  }

+  private void setSearchRequestString(JSONObject cursorJson, SearchSourceBuilder sourceBuilder) {
+    try {
+      ByteArrayOutputStream outputStream = new ByteArrayOutputStream();


I don't know if we really need output stream here:

try (XContentBuilder builder = XContentFactory.jsonBuilder()) {
sourceBuilder.toXContent(builder, ToXContent.EMPTY_PARAMS);
String jsonString = builder.toString();
searchRequestBase64 = Base64.getEncoder().encodeToString(jsonString.getBytes(StandardCharsets.UTF_8));
}
can you take this after the PR?

vamsi-amazon · 2024-08-23T21:06:39Z

I am approving for now. please follow up with the issues created out of the review.

ykmr1224 · 2024-08-23T21:15:44Z

legacy/src/main/java/org/opensearch/sql/legacy/cursor/DefaultCursor.java

+    return String.format("%s:%s", type.getId(), encodeCursor(json));
+  }
+
+  private void setSearchRequestString(JSONObject cursorJson, SearchSourceBuilder sourceBuilder) {


nit: we should rather return the searchRequestBase64 from this method and set it to json at caller side. (it is not a good practice to modify parameter object in a method for readability/maintainability) Please address it in later PR.

opensearch-trigger-bot · 2024-08-23T21:31:07Z

The backport to 2.x failed:

The process '/usr/bin/git' failed with exit code 128

To backport manually, run these commands in your terminal:

# Navigate to the root of your repository
cd $(git rev-parse --show-toplevel)
# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/sql/backport-2.x 2.x
# Navigate to the new working tree
pushd ../.worktrees/sql/backport-2.x
# Create a new branch
git switch --create backport/backport-2940-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 69853fe99c4ee940fc739373e0ba04418bda71c6
# Push it to GitHub
git push --set-upstream origin backport/backport-2940-to-2.x
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/sql/backport-2.x

Then, create a pull request where the base branch is 2.x and the compare/head branch is backport/backport-2940-to-2.x.

* Add pit for join queries (opensearch-project#2703) * Add search after for join Signed-off-by: Rupal Mahajan <maharup@amazon.com> * Enable search after by default Signed-off-by: Rupal Mahajan <maharup@amazon.com> * Add pit Signed-off-by: Rupal Mahajan <maharup@amazon.com> * nit Signed-off-by: Rupal Mahajan <maharup@amazon.com> * Fix tests Signed-off-by: Rupal Mahajan <maharup@amazon.com> * ignore joinWithGeoIntersectNL Signed-off-by: Rupal Mahajan <maharup@amazon.com> * Rerun CI with scroll Signed-off-by: Rupal Mahajan <maharup@amazon.com> * Remove unused code and retrigger CI with search_after true Signed-off-by: Rupal Mahajan <maharup@amazon.com> * Address comments Signed-off-by: Rupal Mahajan <maharup@amazon.com> * Remove unused code change Signed-off-by: Rupal Mahajan <maharup@amazon.com> * Update pit keep alive time with SQL_CURSOR_KEEP_ALIVE Signed-off-by: Rupal Mahajan <maharup@amazon.com> * Fix scroll condition Signed-off-by: Rupal Mahajan <maharup@amazon.com> * nit Signed-off-by: Rupal Mahajan <maharup@amazon.com> * Add pit before query execution Signed-off-by: Rupal Mahajan <maharup@amazon.com> * nit Signed-off-by: Rupal Mahajan <maharup@amazon.com> * Move pit from join request builder to executor Signed-off-by: Rupal Mahajan <maharup@amazon.com> * Remove unused methods Signed-off-by: Rupal Mahajan <maharup@amazon.com> * Add pit in parent class's run() Signed-off-by: Rupal Mahajan <maharup@amazon.com> * Add comment for fetching subsequent result in NestedLoopsElasticExecutor Signed-off-by: Rupal Mahajan <maharup@amazon.com> * Update comment Signed-off-by: Rupal Mahajan <maharup@amazon.com> * Add javadoc for pit handler Signed-off-by: Rupal Mahajan <maharup@amazon.com> * Add pit interface Signed-off-by: Rupal Mahajan <maharup@amazon.com> * Add pit handler unit test Signed-off-by: Rupal Mahajan <maharup@amazon.com> * Fix failed unit test CI Signed-off-by: Rupal Mahajan <maharup@amazon.com> * Fix spotless error Signed-off-by: Rupal Mahajan <maharup@amazon.com> * Rename pit class and add logs Signed-off-by: Rupal Mahajan <maharup@amazon.com> * Fix pit delete unit test Signed-off-by: Rupal Mahajan <maharup@amazon.com> --------- Signed-off-by: Rupal Mahajan <maharup@amazon.com> * Add pit for multi query (opensearch-project#2753) * Add search after for join Signed-off-by: Rupal Mahajan <maharup@amazon.com> * Enable search after by default Signed-off-by: Rupal Mahajan <maharup@amazon.com> * Add pit Signed-off-by: Rupal Mahajan <maharup@amazon.com> * nit Signed-off-by: Rupal Mahajan <maharup@amazon.com> * Fix tests Signed-off-by: Rupal Mahajan <maharup@amazon.com> * ignore joinWithGeoIntersectNL Signed-off-by: Rupal Mahajan <maharup@amazon.com> * Rerun CI with scroll Signed-off-by: Rupal Mahajan <maharup@amazon.com> * draft Signed-off-by: Rupal Mahajan <maharup@amazon.com> * Remove unused code and retrigger CI with search_after true Signed-off-by: Rupal Mahajan <maharup@amazon.com> * Address comments Signed-off-by: Rupal Mahajan <maharup@amazon.com> * Remove unused code change Signed-off-by: Rupal Mahajan <maharup@amazon.com> * Update pit keep alive time with SQL_CURSOR_KEEP_ALIVE Signed-off-by: Rupal Mahajan <maharup@amazon.com> * Fix scroll condition Signed-off-by: Rupal Mahajan <maharup@amazon.com> * nit Signed-off-by: Rupal Mahajan <maharup@amazon.com> * Add pit before query execution Signed-off-by: Rupal Mahajan <maharup@amazon.com> * Refactor get response with pit method Signed-off-by: Rupal Mahajan <maharup@amazon.com> * Update remaining scroll search calls Signed-off-by: Rupal Mahajan <maharup@amazon.com> * Fix integ test failures Signed-off-by: Rupal Mahajan <maharup@amazon.com> * nit Signed-off-by: Rupal Mahajan <maharup@amazon.com> * Move pit from join request builder to executor Signed-off-by: Rupal Mahajan <maharup@amazon.com> * Remove unused methods Signed-off-by: Rupal Mahajan <maharup@amazon.com> * Move pit from request to executor Signed-off-by: Rupal Mahajan <maharup@amazon.com> * Fix pit.delete call missed while merge Signed-off-by: Rupal Mahajan <maharup@amazon.com> * Move getResponseWithHits method to util class Signed-off-by: Rupal Mahajan <maharup@amazon.com> * add try catch for create delete pit in minus executor Signed-off-by: Rupal Mahajan <maharup@amazon.com> * move all common fields to ElasticHitsExecutor Signed-off-by: Rupal Mahajan <maharup@amazon.com> * add javadoc for ElasticHitsExecutor Signed-off-by: Rupal Mahajan <maharup@amazon.com> * Add missing javadoc Signed-off-by: Rupal Mahajan <maharup@amazon.com> * Forcing an empty commit as last commit is stuck processing updates Signed-off-by: Rupal Mahajan <maharup@amazon.com> --------- Signed-off-by: Rupal Mahajan <maharup@amazon.com> * Add pit to default cursor Signed-off-by: Rupal Mahajan <maharup@amazon.com> * Run CI without pit unit test Signed-off-by: Rupal Mahajan <maharup@amazon.com> * Rerun CI without pit unit test Signed-off-by: Rupal Mahajan <maharup@amazon.com> * FIx unit tests for PIT changes Signed-off-by: Manasvini B S <manasvis@amazon.com> * Addressed comments Signed-off-by: Manasvini B S <manasvis@amazon.com> --------- Signed-off-by: Rupal Mahajan <maharup@amazon.com> Signed-off-by: Manasvini B S <manasvis@amazon.com> Co-authored-by: Rupal Mahajan <maharup@amazon.com>

rupal-bq and others added 6 commits August 15, 2024 13:24

Add pit to default cursor

b30de65

Signed-off-by: Rupal Mahajan <maharup@amazon.com>

Run CI without pit unit test

162ef0c

Signed-off-by: Rupal Mahajan <maharup@amazon.com>

Rerun CI without pit unit test

8c8aba8

Signed-off-by: Rupal Mahajan <maharup@amazon.com>

FIx unit tests for PIT changes

167dce0

Signed-off-by: Manasvini B S <manasvis@amazon.com>

manasvinibs changed the title ~~Pit pagination~~ Add pit for pagination query Aug 15, 2024

manasvinibs marked this pull request as ready for review August 15, 2024 21:22

manasvinibs requested review from ps48, kavithacm, derek-ho, joshuali925, dai-chen, YANG-DB, rupal-bq, mengweieric, vamsi-amazon, Swiddis, penghuo, seankao-az, MaxKsyunz, Yury-Fridlyand, anirudha, forestmvey, acarbonetto, GumpacG, ykmr1224 and LantaoJin as code owners August 15, 2024 21:22

manasvinibs mentioned this pull request Aug 19, 2024

Manual tests for Sample queries for Scroll to PIT pagination queries #2941

Open

vamsi-amazon reviewed Aug 19, 2024

View reviewed changes

legacy/src/main/java/org/opensearch/sql/legacy/cursor/DefaultCursor.java Outdated Show resolved Hide resolved

vamsi-amazon reviewed Aug 22, 2024

View reviewed changes

legacy/src/main/java/org/opensearch/sql/legacy/cursor/DefaultCursor.java Outdated Show resolved Hide resolved

vamsi-amazon reviewed Aug 22, 2024

View reviewed changes

legacy/src/main/java/org/opensearch/sql/legacy/cursor/DefaultCursor.java Outdated Show resolved Hide resolved

vamsi-amazon reviewed Aug 22, 2024

View reviewed changes

legacy/src/main/java/org/opensearch/sql/legacy/pit/PointInTimeHandlerImpl.java Outdated Show resolved Hide resolved

manasvinibs force-pushed the PIT-pagination branch 2 times, most recently from 9f8e7c1 to fe91e52 Compare August 22, 2024 06:32

manasvinibs requested review from ykmr1224 and vamsi-amazon August 22, 2024 17:24

ykmr1224 previously approved these changes Aug 22, 2024

View reviewed changes

vamsi-amazon reviewed Aug 22, 2024

View reviewed changes

manasvinibs requested a review from vamsi-amazon August 22, 2024 21:30

vamsi-amazon reviewed Aug 23, 2024

View reviewed changes

Addressed comments

26b44a3

Signed-off-by: Manasvini B S <manasvis@amazon.com>

manasvinibs dismissed ykmr1224’s stale review via 26b44a3 August 23, 2024 17:46

manasvinibs force-pushed the PIT-pagination branch from fe91e52 to 26b44a3 Compare August 23, 2024 17:46

manasvinibs requested review from vamsi-amazon and ykmr1224 August 23, 2024 18:02

vamsi-amazon reviewed Aug 23, 2024

View reviewed changes

vamsi-amazon approved these changes Aug 23, 2024

View reviewed changes

ykmr1224 reviewed Aug 23, 2024

View reviewed changes

ykmr1224 approved these changes Aug 23, 2024

View reviewed changes

ykmr1224 added the feature label Aug 23, 2024

ykmr1224 merged commit 69853fe into opensearch-project:main Aug 23, 2024
14 of 15 checks passed

manasvinibs added backport 2.x v2.17.0 labels Aug 23, 2024

opensearch-trigger-bot bot added the backport-failed label Aug 23, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add pit for pagination query #2940

Add pit for pagination query #2940

manasvinibs commented Aug 15, 2024 •

edited

Loading

vamsi-amazon Aug 22, 2024

manasvinibs Aug 22, 2024 •

edited

Loading

vamsi-amazon Aug 23, 2024

vamsi-amazon Aug 23, 2024

vamsi-amazon Aug 22, 2024

manasvinibs Aug 22, 2024

vamsi-amazon Aug 23, 2024

vamsi-amazon Aug 23, 2024

manasvinibs Aug 23, 2024

vamsi-amazon Aug 23, 2024

vamsi-amazon Aug 23, 2024 •

edited

Loading

manasvinibs Aug 23, 2024

vamsi-amazon Aug 23, 2024

vamsi-amazon commented Aug 23, 2024

ykmr1224 Aug 23, 2024

opensearch-trigger-bot bot commented Aug 23, 2024

Add pit for pagination query #2940

Add pit for pagination query #2940

Conversation

manasvinibs commented Aug 15, 2024 • edited Loading

Description

Related Issues

Check List

Choose a reason for hiding this comment

manasvinibs Aug 22, 2024 • 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

vamsi-amazon Aug 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vamsi-amazon commented Aug 23, 2024

Choose a reason for hiding this comment

opensearch-trigger-bot bot commented Aug 23, 2024

manasvinibs commented Aug 15, 2024 •

edited

Loading

manasvinibs Aug 22, 2024 •

edited

Loading

vamsi-amazon Aug 23, 2024 •

edited

Loading