Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SQL pagination should work with the pretty parameter #2759

Merged
merged 6 commits into from
Jul 15, 2024
Merged

SQL pagination should work with the pretty parameter #2759

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
d851cdc
SQL pagination should work with the pretty parameter
LantaoJin Jun 17, 2024
330a149
rename
LantaoJin Jun 17, 2024
bb29410
fix spotless
LantaoJin Jun 18, 2024
c25aca6
address comments
LantaoJin Jun 25, 2024
6607f35
apply spotless
LantaoJin Jun 25, 2024
4a6a653
Merge remote-tracking branch 'upstream/main' into bugfix_issue_2460
LantaoJin Jul 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 18 additions & 13 deletions sql/src/main/java/org/opensearch/sql/sql/domain/SQLQueryRequest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -79,19 +82,21 @@ 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();

Predicate<String> supportedParams = List.of(QUERY_PARAMS_FORMAT, QUERY_PARAMS_PRETTY)::contains;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[NIT]won't Set.of would be more appropriate here?

boolean hasUnsupportedParams =
(!params.isEmpty())
&& params.keySet().stream().dropWhile(supportedParams).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 boolean isCursor() {
Expand Down
36 changes: 36 additions & 0 deletions sql/src/test/java/org/opensearch/sql/sql/domain/SQLQueryRequestTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,42 @@ public void should_support_cursor_request() {
() -> assertTrue(cursorRequest.isSupported()));
}

@Test
public void should_support_cursor_request_with_supported_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 =
Expand Down
Loading