SQL: Implement binary format support for SQL clear cursor #84230

luigidellaquila · 2022-02-22T14:44:34Z

support binary_format parameter on _sql/close
make cursor close response consistent with the query protocol
(ie. in ODBC/JDBC/CLI return CBOR response for cursor close - as for query)

- support binary_format parameter on _sql/close - make cursor close response consistent with the query protocol (ie. in ODBC/JDBC/CLI return CBOR response for cursor close - as for query)

elasticmachine · 2022-02-22T14:44:36Z

Pinging @elastic/es-ql (Team:QL)

luigidellaquila · 2022-02-22T14:45:19Z

@elasticmachine update branch

elasticsearchmachine · 2022-02-22T14:45:20Z

Hi @luigidellaquila, I've created a changelog YAML for you.

elasticmachine · 2022-02-22T14:45:20Z

expected head sha didn’t match current head ref.

luigidellaquila · 2022-02-22T16:57:10Z

@elasticmachine update branch

wrong randomization

Luegg

I've left some comments but I don't understand the motivation behind the change. The JDBC driver and CLI seem to work as is (but 👍 for adding JdbcCloseCursorIT) and the binary_format parameter is not documented anywhere.

Also, out of curiosity, I couldn't find much information about binary_format. Why is this a flag instead of using Accept for the content negotiation?

Maybe @bpintea do you have more background?

...lugin/sql/qa/jdbc/src/main/java/org/elasticsearch/xpack/sql/qa/jdbc/CloseCursorTestCase.java

...gin/sql/sql-proto/src/main/java/org/elasticsearch/xpack/sql/proto/SqlClearCursorRequest.java

...gin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/SqlClearCursorResponseListener.java

luigidellaquila · 2022-02-23T10:42:40Z

Thank you very much for the comments @Luegg
I'm also in doubt of misinterpreting #53359, probably @bpintea can shed some light.
JDBC and CLI work fine already indeed, this fix just adds a little bit of consistency (ie. ask for binary, get CBOR), but I agree with you that it's not strictly necessary.

bpintea · 2022-02-24T15:36:58Z

I don't understand the motivation behind the change.

As noted in PR's description, the motivation is to "make cursor close response consistent": currently, CBOR is used by default, both for requests and responses. The drivers indicate the need for a CBOR answer with the binary_format body attribute. This indication works currently throughout the SQL API, except for the _sql/close endpoint. In this case the driver's can not ask for CBOR answer (since the request would fail, #53359).

The JDBC driver and CLI seem to work as is

They either "work" inconsistently (in respect to the body format), or leave cursors unclosed.

the binary_format parameter is not documented anywhere.

This has been by choice, I believe.

Why is this a flag instead of using Accept for the content negotiation?

No longer sure either, might have been to keep the format unchangeable to (HTTP) proxies. #48261 has a few comments around, but not on this question.

Luegg · 2022-02-24T15:59:45Z

Thanks for the elaborations 👍 Consistency itself also makes sense of course, but I was unsure wether there was something else.

bpintea

Looks good, left a few more notes.

x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/plugin/SqlMediaTypeParserTests.java

...gin/sql/sql-proto/src/main/java/org/elasticsearch/xpack/sql/proto/SqlClearCursorRequest.java

...l/sql-action/src/test/java/org/elasticsearch/xpack/sql/action/TestSqlClearCursorRequest.java

.../sql/sql-action/src/test/java/org/elasticsearch/xpack/sql/action/SqlRequestParsersTests.java

bpintea · 2022-03-04T13:27:40Z

...gin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/SqlClearCursorResponseListener.java

+
+    @Override
+    public RestResponse buildResponse(SqlClearCursorResponse response) throws Exception {
+        assert mediaType instanceof XContentType;


Indeed, this could actually be checked (and tested), instead of an assert: one could still send an Accept with a CSV value or something that wouldn't make sense?

luigidellaquila · 2022-03-07T14:59:06Z

@elasticmachine update branch

luigidellaquila · 2022-03-08T14:19:19Z

Added a new batch of changes that addresses all the comments.

...lugin/sql/qa/jdbc/src/main/java/org/elasticsearch/xpack/sql/qa/jdbc/CloseCursorTestCase.java

...gin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/SqlClearCursorResponseListener.java

costin

No need to extend the logic beyond CBOR (binary) and XContent.
Please include also a test/set of backwards compatibility tests to check the proper interaction between old clients (that do NOT set the backwards compatibility flag) with the current code.

...lugin/sql/qa/jdbc/src/main/java/org/elasticsearch/xpack/sql/qa/jdbc/CloseCursorTestCase.java

costin · 2022-03-09T16:56:27Z

...lugin/sql/qa/jdbc/src/main/java/org/elasticsearch/xpack/sql/qa/jdbc/CloseCursorTestCase.java

+        index("library", "2", builder -> { builder.field("name", "bar"); });
+        index("library", "3", builder -> { builder.field("name", "baz"); });
+
+        try (Connection connection = createConnection(connectionProperties())) {


Same comment as above.

costin · 2022-03-09T17:01:36Z

...n/sql/sql-action/src/main/java/org/elasticsearch/xpack/sql/action/SqlClearCursorRequest.java

@@ -43,9 +48,11 @@
        PARSER.declareString(optionalConstructorArg(), MODE);
        PARSER.declareString(optionalConstructorArg(), CLIENT_ID);
        PARSER.declareString(optionalConstructorArg(), VERSION);
+        PARSER.declareBoolean(SqlClearCursorRequest::binaryCommunication, BINARY_FORMAT);


Declaring this field mandatory (instead of optional), breaks backwards compatibility for clients that do not set this parameter.

Not sure I got it, maybe I'm misinterpreting some logic here.
Isn't it optional already? (see also test https://github.com/elastic/elasticsearch/pull/84230/files#diff-7d14bc9a0a5fb5167a3d7eea6b457ea14324136f334a33df4417ec5f78fefb84R102 )

I read the above as having constructorArg() in a similar style to the previous declaration.

...gin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/SqlClearCursorResponseListener.java

costin · 2022-03-09T17:07:20Z

x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/SqlMediaTypeParser.java

+    /*
+     * see getResponseMediaType(RestRequest, SqlQueryRequest)
+     */
+    public static MediaType getResponseMediaType(RestRequest request, SqlClearCursorRequest sqlRequest) {


This shouldn't be necessary, see my previous comment.

luigidellaquila · 2022-03-10T16:26:34Z

@elasticmachine update branch

…_close' into binary_format_on_cursor_close

...lugin/sql/qa/jdbc/src/main/java/org/elasticsearch/xpack/sql/qa/jdbc/CloseCursorTestCase.java

...gin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/SqlClearCursorResponseListener.java

x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/plugin/SqlMediaTypeParserTests.java

and refactor test cases

costin

Left comments.

...ck/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/RestSqlClearCursorAction.java

...gin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/SqlClearCursorResponseListener.java

bpintea

LGTM

astefan

Good work so far. I've left some questions mostly looking for explanations on why a specific approach has been considered.
Also, please, have a look at JdbcHttpClientRequestTests where the binary comm is checked for a query request. I think the same should happen for a query close request.

...lugin/sql/qa/jdbc/src/main/java/org/elasticsearch/xpack/sql/qa/jdbc/CloseCursorTestCase.java

...n/sql/sql-action/src/main/java/org/elasticsearch/xpack/sql/action/SqlClearCursorRequest.java

...l/sql-action/src/test/java/org/elasticsearch/xpack/sql/action/TestSqlClearCursorRequest.java

...gin/sql/sql-proto/src/main/java/org/elasticsearch/xpack/sql/proto/SqlClearCursorRequest.java

...n/sql/sql-action/src/main/java/org/elasticsearch/xpack/sql/action/SqlClearCursorRequest.java

and refactor test cases

luigidellaquila · 2022-03-17T15:53:47Z

@elasticmachine update branch

luigidellaquila · 2022-03-18T07:36:16Z

@elasticmachine update branch

costin

LGTM

astefan

You need to adjust SqlClearCursorRequestTests to account for the new component.
mutateInstance should probably look something like this

    protected TestSqlClearCursorRequest mutateInstance(TestSqlClearCursorRequest instance) throws IOException {
        @SuppressWarnings("unchecked")
        Consumer<TestSqlClearCursorRequest> mutator = randomFrom(
            request -> request.requestInfo(randomValueOtherThan(request.requestInfo(), this::randomRequestInfo)),
            request -> request.setCursor(randomValueOtherThan(request.getCursor(), SqlQueryResponseTests::randomStringCursor)),
            request -> request.binaryCommunication(randomValueOtherThan(request.binaryCommunication(), () -> randomBoolean()))
        );
        TestSqlClearCursorRequest newRequest = new TestSqlClearCursorRequest(instance.requestInfo(), instance.getCursor());
        newRequest.binaryCommunication(instance.binaryCommunication());
        mutator.accept(newRequest);
        return newRequest;
    }

createTestInstance() should also consider a random value for binaryCommunication.

…_close' into binary_format_on_cursor_close

luigidellaquila · 2022-03-23T08:08:26Z

@elasticmachine update branch

astefan

LGTM

Implement binary format support for SQL clear cursor

9ab021a

- support binary_format parameter on _sql/close - make cursor close response consistent with the query protocol (ie. in ODBC/JDBC/CLI return CBOR response for cursor close - as for query)

luigidellaquila added >bug :Analytics/SQL SQL querying Team:QL (Deprecated) Meta label for query languages team labels Feb 22, 2022

luigidellaquila requested review from costin, astefan, Luegg and bpintea February 22, 2022 14:44

elasticsearchmachine added the v8.2.0 label Feb 22, 2022

Update docs/changelog/84230.yaml

eedaa4b

elasticmachine and others added 2 commits February 22, 2022 10:57

Merge branch 'master' into binary_format_on_cursor_close

e0e9e22

Fix test case

a968837

wrong randomization

Luegg reviewed Feb 23, 2022

View reviewed changes

bpintea reviewed Mar 4, 2022

View reviewed changes

Enhance test cases on binary option (Clear Cursor response)

1f8d2b5

Merge branch 'master' into binary_format_on_cursor_close

b26c949

luigidellaquila requested review from Luegg and bpintea March 8, 2022 14:19

bpintea reviewed Mar 9, 2022

View reviewed changes

costin requested changes Mar 9, 2022

View reviewed changes

elasticmachine and others added 3 commits March 10, 2022 10:26

Merge branch 'master' into binary_format_on_cursor_close

c53c7b8

SQL: Support JSON and CBOR in clear cursor response

ca1eefd

Merge remote-tracking branch 'luigidellaquila/binary_format_on_cursor…

3fdcde0

…_close' into binary_format_on_cursor_close

bpintea reviewed Mar 14, 2022

View reviewed changes

Better handle the choice of CBOR vs JSON based on driver mode

c9481a5

and refactor test cases

costin requested changes Mar 15, 2022

View reviewed changes

...ck/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/RestSqlClearCursorAction.java Outdated Show resolved Hide resolved

...gin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/SqlClearCursorResponseListener.java Outdated Show resolved Hide resolved

Refactor clear cursor action to be an anonymous inner class

c8e9ce3

bpintea approved these changes Mar 16, 2022

View reviewed changes

luigidellaquila requested a review from costin March 16, 2022 15:32

astefan requested changes Mar 17, 2022

View reviewed changes

Make SqlClearCursorRequest action and proto more consistent

5ad8c0b

and refactor test cases

luigidellaquila requested a review from astefan March 17, 2022 15:38

Merge branch 'master' into binary_format_on_cursor_close

c03406c

Merge branch 'master' into binary_format_on_cursor_close

9eebe9c

costin approved these changes Mar 19, 2022

View reviewed changes

astefan requested changes Mar 22, 2022

View reviewed changes

luigidellaquila added 2 commits March 23, 2022 08:36

Improve test randomization

ceab361

Merge remote-tracking branch 'luigidellaquila/binary_format_on_cursor…

6fd1163

…_close' into binary_format_on_cursor_close

luigidellaquila requested a review from astefan March 23, 2022 07:38

Merge branch 'master' into binary_format_on_cursor_close

ce6922f

astefan approved these changes Mar 28, 2022

View reviewed changes

salvatore-campagna added v8.3.0 and removed v8.2.0 labels Mar 30, 2022

Luegg approved these changes Mar 31, 2022

View reviewed changes

luigidellaquila merged commit d6486ab into elastic:master Apr 1, 2022

Luegg mentioned this pull request Apr 14, 2022

SQL: Fix pagination during rolling upgrade #85399

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SQL: Implement binary format support for SQL clear cursor #84230

SQL: Implement binary format support for SQL clear cursor #84230

luigidellaquila commented Feb 22, 2022

elasticmachine commented Feb 22, 2022

luigidellaquila commented Feb 22, 2022

elasticsearchmachine commented Feb 22, 2022

elasticmachine commented Feb 22, 2022

luigidellaquila commented Feb 22, 2022

Luegg left a comment

luigidellaquila commented Feb 23, 2022

bpintea commented Feb 24, 2022

Luegg commented Feb 24, 2022

bpintea left a comment

bpintea Mar 4, 2022

luigidellaquila commented Mar 7, 2022

luigidellaquila commented Mar 8, 2022

costin left a comment

costin Mar 9, 2022

costin Mar 9, 2022

luigidellaquila Mar 10, 2022 •

edited

Loading

costin Mar 15, 2022

costin Mar 9, 2022

luigidellaquila commented Mar 10, 2022

costin left a comment

bpintea left a comment

astefan left a comment

luigidellaquila commented Mar 17, 2022

luigidellaquila commented Mar 18, 2022

costin left a comment

astefan left a comment

luigidellaquila commented Mar 23, 2022

astefan left a comment

SQL: Implement binary format support for SQL clear cursor #84230

SQL: Implement binary format support for SQL clear cursor #84230

Conversation

luigidellaquila commented Feb 22, 2022

elasticmachine commented Feb 22, 2022

luigidellaquila commented Feb 22, 2022

elasticsearchmachine commented Feb 22, 2022

elasticmachine commented Feb 22, 2022

luigidellaquila commented Feb 22, 2022

Luegg left a comment

Choose a reason for hiding this comment

luigidellaquila commented Feb 23, 2022

bpintea commented Feb 24, 2022

Luegg commented Feb 24, 2022

bpintea left a comment

Choose a reason for hiding this comment

bpintea Mar 4, 2022

Choose a reason for hiding this comment

luigidellaquila commented Mar 7, 2022

luigidellaquila commented Mar 8, 2022

costin left a comment

Choose a reason for hiding this comment

costin Mar 9, 2022

Choose a reason for hiding this comment

costin Mar 9, 2022

Choose a reason for hiding this comment

luigidellaquila Mar 10, 2022 • edited Loading

Choose a reason for hiding this comment

costin Mar 15, 2022

Choose a reason for hiding this comment

costin Mar 9, 2022

Choose a reason for hiding this comment

luigidellaquila commented Mar 10, 2022

costin left a comment

Choose a reason for hiding this comment

bpintea left a comment

Choose a reason for hiding this comment

astefan left a comment

Choose a reason for hiding this comment

luigidellaquila commented Mar 17, 2022

luigidellaquila commented Mar 18, 2022

costin left a comment

Choose a reason for hiding this comment

astefan left a comment

Choose a reason for hiding this comment

luigidellaquila commented Mar 23, 2022

astefan left a comment

Choose a reason for hiding this comment

luigidellaquila Mar 10, 2022 •

edited

Loading