From d851cdc575d044cbd503b98fcb9f0208da583d4a Mon Sep 17 00:00:00 2001 From: Lantao Jin Date: Mon, 17 Jun 2024 17:18:10 +0800 Subject: [PATCH 1/5] SQL pagination should work with the pretty parameter Signed-off-by: Lantao Jin --- .../sql/sql/domain/SQLQueryRequest.java | 32 ++++++++++------- .../sql/sql/domain/SQLQueryRequestTest.java | 36 +++++++++++++++++++ 2 files changed, 55 insertions(+), 13 deletions(-) diff --git a/sql/src/main/java/org/opensearch/sql/sql/domain/SQLQueryRequest.java b/sql/src/main/java/org/opensearch/sql/sql/domain/SQLQueryRequest.java index 4e902cb67d..1cd09a103b 100644 --- a/sql/src/main/java/org/opensearch/sql/sql/domain/SQLQueryRequest.java +++ b/sql/src/main/java/org/opensearch/sql/sql/domain/SQLQueryRequest.java @@ -6,10 +6,12 @@ package org.opensearch.sql.sql.domain; import java.util.Collections; +import java.util.List; import java.util.Locale; import java.util.Map; import java.util.Optional; import java.util.Set; +import java.util.function.Predicate; import java.util.stream.Stream; import lombok.EqualsAndHashCode; import lombok.Getter; @@ -29,6 +31,7 @@ public class SQLQueryRequest { Set.of("query", "fetch_size", "parameters", QUERY_FIELD_CURSOR); private static final String QUERY_PARAMS_FORMAT = "format"; private static final String QUERY_PARAMS_SANITIZE = "sanitize"; + private static final String QUERY_PARAMS_PRETTY = "pretty"; /** JSON payload in REST request. */ private final JSONObject jsonContent; @@ -79,21 +82,24 @@ public SQLQueryRequest( * @return true if supported. */ public boolean isSupported() { - var noCursor = !isCursor(); - var noQuery = query == null; - var noUnsupportedParams = - params.isEmpty() || (params.size() == 1 && params.containsKey(QUERY_PARAMS_FORMAT)); - var noContent = jsonContent == null || jsonContent.isEmpty(); - - return ((!noCursor - && noQuery - && noUnsupportedParams - && noContent) // if cursor is given, but other things - || (noCursor && !noQuery)) // or if cursor is not given, but query - && isOnlySupportedFieldInPayload() // and request has supported fields only - && isSupportedFormat(); // and request is in supported format + boolean hasCursor = isCursor(); + boolean hasQuery = query != null; + boolean hasContent = jsonContent != null && !jsonContent.isEmpty(); + boolean hasUnsupportedParams = + (!params.isEmpty()) + && params.keySet().stream().dropWhile(builtinSupported).findAny().isPresent(); + + boolean validCursor = hasCursor && !hasQuery && !hasUnsupportedParams && !hasContent; + boolean validQuery = !hasCursor && hasQuery; + + return (validCursor || validQuery) // It's a valid cursor or a valid query + && isOnlySupportedFieldInPayload() // and request must contain supported fields only + && isSupportedFormat(); // and request must be a supported format } + private final Predicate builtinSupported = + List.of(QUERY_PARAMS_FORMAT, QUERY_PARAMS_PRETTY)::contains; + private boolean isCursor() { return cursor != null && !cursor.isEmpty(); } diff --git a/sql/src/test/java/org/opensearch/sql/sql/domain/SQLQueryRequestTest.java b/sql/src/test/java/org/opensearch/sql/sql/domain/SQLQueryRequestTest.java index 2b64b13b35..c92af990c2 100644 --- a/sql/src/test/java/org/opensearch/sql/sql/domain/SQLQueryRequestTest.java +++ b/sql/src/test/java/org/opensearch/sql/sql/domain/SQLQueryRequestTest.java @@ -106,6 +106,42 @@ public void should_support_cursor_request() { () -> assertTrue(cursorRequest.isSupported())); } + @Test + public void should_support_cursor_request_with_builtin_parameters() { + SQLQueryRequest fetchSizeRequest = + SQLQueryRequestBuilder.request("SELECT 1") + .jsonContent("{\"query\": \"SELECT 1\", \"fetch_size\": 5}") + .build(); + + SQLQueryRequest cursorRequest = + SQLQueryRequestBuilder.request(null) + .cursor("abcdefgh...") + .params(Map.of("format", "csv", "pretty", "true")) + .build(); + + assertAll( + () -> assertTrue(fetchSizeRequest.isSupported()), + () -> assertTrue(cursorRequest.isSupported())); + } + + @Test + public void should_not_support_cursor_request_with_unsupported_parameters() { + SQLQueryRequest fetchSizeRequest = + SQLQueryRequestBuilder.request("SELECT 1") + .jsonContent("{\"query\": \"SELECT 1\", \"fetch_size\": 5}") + .build(); + + SQLQueryRequest cursorRequest = + SQLQueryRequestBuilder.request(null) + .cursor("abcdefgh...") + .params(Map.of("one", "two")) + .build(); + + assertAll( + () -> assertTrue(fetchSizeRequest.isSupported()), + () -> assertFalse(cursorRequest.isSupported())); + } + @Test public void should_support_cursor_close_request() { SQLQueryRequest closeRequest = From 330a149cb6ecd6ee240e11f21ae62f78e901ee26 Mon Sep 17 00:00:00 2001 From: Lantao Jin Date: Mon, 17 Jun 2024 17:49:00 +0800 Subject: [PATCH 2/5] rename Signed-off-by: Lantao Jin --- .../org/opensearch/sql/sql/domain/SQLQueryRequest.java | 8 ++++---- .../opensearch/sql/sql/domain/SQLQueryRequestTest.java | 2 +- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/sql/src/main/java/org/opensearch/sql/sql/domain/SQLQueryRequest.java b/sql/src/main/java/org/opensearch/sql/sql/domain/SQLQueryRequest.java index 1cd09a103b..e1eacef5f0 100644 --- a/sql/src/main/java/org/opensearch/sql/sql/domain/SQLQueryRequest.java +++ b/sql/src/main/java/org/opensearch/sql/sql/domain/SQLQueryRequest.java @@ -85,9 +85,12 @@ public boolean isSupported() { boolean hasCursor = isCursor(); boolean hasQuery = query != null; boolean hasContent = jsonContent != null && !jsonContent.isEmpty(); + + Predicate supportedParams = + List.of(QUERY_PARAMS_FORMAT, QUERY_PARAMS_PRETTY)::contains; boolean hasUnsupportedParams = (!params.isEmpty()) - && params.keySet().stream().dropWhile(builtinSupported).findAny().isPresent(); + && params.keySet().stream().dropWhile(supportedParams).findAny().isPresent(); boolean validCursor = hasCursor && !hasQuery && !hasUnsupportedParams && !hasContent; boolean validQuery = !hasCursor && hasQuery; @@ -97,9 +100,6 @@ && isOnlySupportedFieldInPayload() // and request must contain supported fields && isSupportedFormat(); // and request must be a supported format } - private final Predicate builtinSupported = - List.of(QUERY_PARAMS_FORMAT, QUERY_PARAMS_PRETTY)::contains; - private boolean isCursor() { return cursor != null && !cursor.isEmpty(); } diff --git a/sql/src/test/java/org/opensearch/sql/sql/domain/SQLQueryRequestTest.java b/sql/src/test/java/org/opensearch/sql/sql/domain/SQLQueryRequestTest.java index c92af990c2..b569a89a2e 100644 --- a/sql/src/test/java/org/opensearch/sql/sql/domain/SQLQueryRequestTest.java +++ b/sql/src/test/java/org/opensearch/sql/sql/domain/SQLQueryRequestTest.java @@ -107,7 +107,7 @@ public void should_support_cursor_request() { } @Test - public void should_support_cursor_request_with_builtin_parameters() { + public void should_support_cursor_request_with_supported_parameters() { SQLQueryRequest fetchSizeRequest = SQLQueryRequestBuilder.request("SELECT 1") .jsonContent("{\"query\": \"SELECT 1\", \"fetch_size\": 5}") From bb294109c34c93acd55ca7d5b28819a1992f397e Mon Sep 17 00:00:00 2001 From: Lantao Jin Date: Tue, 18 Jun 2024 14:00:52 +0800 Subject: [PATCH 3/5] fix spotless Signed-off-by: Lantao Jin --- .../java/org/opensearch/sql/sql/domain/SQLQueryRequest.java | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/sql/src/main/java/org/opensearch/sql/sql/domain/SQLQueryRequest.java b/sql/src/main/java/org/opensearch/sql/sql/domain/SQLQueryRequest.java index e1eacef5f0..9cb3c31b4f 100644 --- a/sql/src/main/java/org/opensearch/sql/sql/domain/SQLQueryRequest.java +++ b/sql/src/main/java/org/opensearch/sql/sql/domain/SQLQueryRequest.java @@ -86,8 +86,7 @@ public boolean isSupported() { boolean hasQuery = query != null; boolean hasContent = jsonContent != null && !jsonContent.isEmpty(); - Predicate supportedParams = - List.of(QUERY_PARAMS_FORMAT, QUERY_PARAMS_PRETTY)::contains; + Predicate supportedParams = List.of(QUERY_PARAMS_FORMAT, QUERY_PARAMS_PRETTY)::contains; boolean hasUnsupportedParams = (!params.isEmpty()) && params.keySet().stream().dropWhile(supportedParams).findAny().isPresent(); From c25aca661616ec80ce4b8e1aee26386963ab5572 Mon Sep 17 00:00:00 2001 From: Lantao Jin Date: Tue, 25 Jun 2024 14:56:20 +0800 Subject: [PATCH 4/5] address comments Signed-off-by: Lantao Jin --- .../org/opensearch/sql/jdbc/CursorIT.java | 19 +++++++++++++++++++ .../sql/sql/domain/SQLQueryRequest.java | 3 +-- 2 files changed, 20 insertions(+), 2 deletions(-) diff --git a/integ-test/src/test/java/org/opensearch/sql/jdbc/CursorIT.java b/integ-test/src/test/java/org/opensearch/sql/jdbc/CursorIT.java index 325c81107f..10fd032aa0 100644 --- a/integ-test/src/test/java/org/opensearch/sql/jdbc/CursorIT.java +++ b/integ-test/src/test/java/org/opensearch/sql/jdbc/CursorIT.java @@ -22,6 +22,7 @@ import java.sql.ResultSet; import java.sql.Statement; import java.util.List; +import java.util.Map; import javax.annotation.Nullable; import lombok.SneakyThrows; import org.json.JSONObject; @@ -115,6 +116,8 @@ public void select_all_no_cursor() { var restResponse = executeRestQuery(query, null); assertEquals(rows, restResponse.getInt("total")); + var restPrettyResponse = executeRestQuery(query, null, Map.of("pretty", "true")); + assertEquals(rows, restPrettyResponse.getInt("total")); } } @@ -133,6 +136,8 @@ public void select_count_all_no_cursor() { var restResponse = executeRestQuery(query, null); assertEquals(rows, restResponse.getInt("total")); + var restPrettyResponse = executeRestQuery(query, null, Map.of("pretty", "true")); + assertEquals(rows, restPrettyResponse.getInt("total")); } } @@ -151,6 +156,8 @@ public void select_all_small_table_big_cursor() { var restResponse = executeRestQuery(query, null); assertEquals(rows, restResponse.getInt("total")); + var restPrettyResponse = executeRestQuery(query, null, Map.of("pretty", "true")); + assertEquals(rows, restPrettyResponse.getInt("total")); } } @@ -169,6 +176,8 @@ public void select_all_small_table_small_cursor() { var restResponse = executeRestQuery(query, null); assertEquals(rows, restResponse.getInt("total")); + var restPrettyResponse = executeRestQuery(query, null, Map.of("pretty", "true")); + assertEquals(rows, restPrettyResponse.getInt("total")); } } @@ -187,6 +196,8 @@ public void select_all_big_table_small_cursor() { var restResponse = executeRestQuery(query, null); assertEquals(rows, restResponse.getInt("total")); + var restPrettyResponse = executeRestQuery(query, null, Map.of("pretty", "true")); + assertEquals(rows, restPrettyResponse.getInt("total")); } } @@ -205,6 +216,8 @@ public void select_all_big_table_big_cursor() { var restResponse = executeRestQuery(query, null); assertEquals(rows, restResponse.getInt("total")); + var restPrettyResponse = executeRestQuery(query, null, Map.of("pretty", "true")); + assertEquals(rows, restPrettyResponse.getInt("total")); } } @@ -217,6 +230,11 @@ private static String getConnectionString() { @SneakyThrows protected JSONObject executeRestQuery(String query, @Nullable Integer fetch_size) { + return executeRestQuery(query, fetch_size, Map.of()); + } + + @SneakyThrows + protected JSONObject executeRestQuery(String query, @Nullable Integer fetch_size, Map params) { Request request = new Request("POST", QUERY_API_ENDPOINT); if (fetch_size != null) { request.setJsonEntity( @@ -224,6 +242,7 @@ protected JSONObject executeRestQuery(String query, @Nullable Integer fetch_size } else { request.setJsonEntity(String.format("{ \"query\": \"%s\" }", query)); } + request.addParameters(params); RequestOptions.Builder restOptionsBuilder = RequestOptions.DEFAULT.toBuilder(); restOptionsBuilder.addHeader("Content-Type", "application/json"); diff --git a/sql/src/main/java/org/opensearch/sql/sql/domain/SQLQueryRequest.java b/sql/src/main/java/org/opensearch/sql/sql/domain/SQLQueryRequest.java index 9cb3c31b4f..1d17610fb4 100644 --- a/sql/src/main/java/org/opensearch/sql/sql/domain/SQLQueryRequest.java +++ b/sql/src/main/java/org/opensearch/sql/sql/domain/SQLQueryRequest.java @@ -6,7 +6,6 @@ package org.opensearch.sql.sql.domain; import java.util.Collections; -import java.util.List; import java.util.Locale; import java.util.Map; import java.util.Optional; @@ -86,7 +85,7 @@ public boolean isSupported() { boolean hasQuery = query != null; boolean hasContent = jsonContent != null && !jsonContent.isEmpty(); - Predicate supportedParams = List.of(QUERY_PARAMS_FORMAT, QUERY_PARAMS_PRETTY)::contains; + Predicate supportedParams = Set.of(QUERY_PARAMS_FORMAT, QUERY_PARAMS_PRETTY)::contains; boolean hasUnsupportedParams = (!params.isEmpty()) && params.keySet().stream().dropWhile(supportedParams).findAny().isPresent(); From 6607f359c68d6bcbbd18ecaf3ccc09f7dcc7c63f Mon Sep 17 00:00:00 2001 From: Lantao Jin Date: Tue, 25 Jun 2024 15:09:47 +0800 Subject: [PATCH 5/5] apply spotless Signed-off-by: Lantao Jin --- integ-test/src/test/java/org/opensearch/sql/jdbc/CursorIT.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/integ-test/src/test/java/org/opensearch/sql/jdbc/CursorIT.java b/integ-test/src/test/java/org/opensearch/sql/jdbc/CursorIT.java index 10fd032aa0..e2b6287191 100644 --- a/integ-test/src/test/java/org/opensearch/sql/jdbc/CursorIT.java +++ b/integ-test/src/test/java/org/opensearch/sql/jdbc/CursorIT.java @@ -234,7 +234,8 @@ protected JSONObject executeRestQuery(String query, @Nullable Integer fetch_size } @SneakyThrows - protected JSONObject executeRestQuery(String query, @Nullable Integer fetch_size, Map params) { + protected JSONObject executeRestQuery( + String query, @Nullable Integer fetch_size, Map params) { Request request = new Request("POST", QUERY_API_ENDPOINT); if (fetch_size != null) { request.setJsonEntity(