backwards compatible deserialization of SqlQueryResponse and fix SQL bwc tests #78467

Luegg · 2021-09-29T15:28:14Z

Resolves #78424

With the merge of #78363, it's the first time in a while that the bwc tests run cases where upgraded nodes redirect SQL queries to older nodes. Unfortunately, the upgraded nodes were not able to redirect the response from the old nodes to the client due to a bug in deserialization of SqlQueryResponse.

This PR fixes the deserialization issue. I also had to work around some follow up issues with SqlSearchIT because the new query redirect triggers some new code paths that were not yet correct in all wire compatible versions.

… SQL bwc tests

Luegg · 2021-09-29T15:39:57Z

I've also already created the backport PR #78465 to ensure the 7.x bwc tests turn green as well.

bpintea

Lgtm, thanks.

bpintea · 2021-09-29T15:55:02Z

...plugin/sql/sql-action/src/main/java/org/elasticsearch/xpack/sql/action/SqlQueryResponse.java

 import org.elasticsearch.xpack.ql.async.QlStatusResponse;
 import org.elasticsearch.xpack.sql.proto.ColumnInfo;
 import org.elasticsearch.xpack.sql.proto.Mode;
 import org.elasticsearch.xpack.sql.proto.Protocol;
 import org.elasticsearch.xpack.sql.proto.SqlVersion;
 import org.elasticsearch.xpack.sql.proto.StringUtils;

+import java.io.IOException;


have these been reordered automatically by the editor?

yes. I think that follows the default imports layout.

bpintea · 2021-09-29T16:02:02Z

...n/sql/qa/mixed-node/src/test/java/org/elasticsearch/xpack/sql/qa/mixed_node/SqlSearchIT.java

        }
    }

+    private Map<String, Object> dropDisplaySizes(Map<String, Object> response) {
+        // if JDBC mode is used, display_size will also be part of the response, remove it because it's not part of the expected response


Suggested change

// if JDBC mode is used, display_size will also be part of the response, remove it because it's not part of the expected response

// in JDBC mode, display_size will be part of the response, so remove it because it's not part of the expected response

bpintea · 2021-09-29T16:04:25Z

...n/sql/qa/mixed-node/src/test/java/org/elasticsearch/xpack/sql/qa/mixed_node/SqlSearchIT.java

+                Map<String, Object> actualResponse = dropDisplaySizes(runSql(client, request));
+                assertResponse(expectedResponse, actualResponse);


optional nit: actualResponse could be inlined.

astefan

Left some comments.

...n/sql/qa/mixed-node/src/test/java/org/elasticsearch/xpack/sql/qa/mixed_node/SqlCompatIT.java

astefan · 2021-09-30T06:17:48Z

...n/sql/qa/mixed-node/src/test/java/org/elasticsearch/xpack/sql/qa/mixed_node/SqlSearchIT.java

-            String intervalYearMonth = "INTERVAL '150' YEAR AS interval_year, ";
-            String intervalDayTime = "INTERVAL '163' MINUTE AS interval_minute, ";


I find this style to be more "visible" and easier to read. Can you, please, keep the original version?

sure, I inlined it because I thought it was a leftover from using a conditional here in earlier revisions.

astefan · 2021-09-30T06:21:46Z

...plugin/sql/sql-action/src/main/java/org/elasticsearch/xpack/sql/action/SqlQueryResponse.java

+        if (in.getVersion().onOrAfter(Version.V_7_14_0)) {
+            columnar = in.readBoolean();
+            asyncExecutionId = in.readOptionalString();
+            isPartial = in.readBoolean();
+            isRunning = in.readBoolean();
+        } else {
+            asyncExecutionId = null;
+            isPartial = false;
+            isRunning = false;
+        }


Why this change and why is columnar included there (it seems you grouped together the async specific settings)?

It hadn't been serialised before the given version: https://github.com/elastic/elasticsearch/pull/73991/files#diff-3c77d10764f4a72e650dfabe508e5d372bf024a776ca17b81c2f4666f859fe01R187 , which was probably an oversight.

The fact that we discovered this in a BWC test and not when the async search was introduced is troubling. At some point in future we will need a set of tests that should cover the serialization differences between bwc versions for both the request and response objects. I did something similar for EQL here: #68339. @Luegg please open an issue for improving the tests coverage in this area.

My initial question still stands.
To clarify a bit: columnar has been added in 7.2.0 (#39287). If we should handle it differently depending on the version it shouldn't be 7.14, since that's not the version that added it. Or I am missing something.

it shouldn't be 7.14, since that's not the version that added it.

True, but that's the version that added it to the serialisation.
If not mistaking, between (and before) 7.2 and 7.14, a serialised SqlQueryResponse didn't contain the columnar field. So trying to deserialise a SqlQueryResponse and read the columnar field would eventually fail.
A 7.14+ serialised SqlQueryResponse will otoh contain it and needs to be read.

Thanks @bpintea, I was getting confused myself.

astefan

Looks good in general but the original question I had about columnar still stands.

astefan · 2021-10-04T10:06:34Z

...plugin/sql/sql-action/src/main/java/org/elasticsearch/xpack/sql/action/SqlQueryResponse.java

+        if (in.getVersion().onOrAfter(Version.V_7_14_0)) {
+            columnar = in.readBoolean();
+            asyncExecutionId = in.readOptionalString();
+            isPartial = in.readBoolean();
+            isRunning = in.readBoolean();
+        } else {
+            asyncExecutionId = null;
+            isPartial = false;
+            isRunning = false;
+        }


My initial question still stands.
To clarify a bit: columnar has been added in 7.2.0 (#39287). If we should handle it differently depending on the version it shouldn't be 7.14, since that's not the version that added it. Or I am missing something.

costin

LGTM

costin · 2021-10-06T12:47:53Z

x-pack/plugin/sql/qa/mixed-node/build.gradle

@@ -22,7 +22,7 @@ testClusters.configureEach {
 tasks.named("integTest").configure{ enabled = false}

 // A bug (https://github.com/elastic/elasticsearch/issues/68439) limits us to perform tests with versions from 7.10.3 onwards
-for (Version bwcVersion : BuildParams.bwcVersions.wireCompatible.findAll { it.onOrAfter('7.10.0') }) {
+for (Version bwcVersion : BuildParams.bwcVersions.wireCompatible.findAll { it.onOrAfter('7.10.3') }) {


small nit but a comment would help on what changed in 7.10.3 vs 7.10.0 to make this version being considered.

Using 7.10.0 was a bug. As the comment on line 24 says the version should always have been 7.10.3. But accidentally, the tests also worked with 7.10.0 until I introduced missing_order.

...n/sql/qa/mixed-node/src/test/java/org/elasticsearch/xpack/sql/qa/mixed_node/SqlCompatIT.java

make deserialization of SqlQueryResponse backwards compatible and fix…

0010262

… SQL bwc tests

Luegg added the test-full-bwc Trigger full BWC version matrix tests label Sep 29, 2021

Luegg mentioned this pull request Sep 29, 2021

[7.x] backwards compatible deserialization of SqlQueryResponse and fix SQL bwc tests #78465

Merged

elasticsearchmachine added the v8.0.0 label Sep 29, 2021

Luegg added :Analytics/SQL SQL querying v8.0.0 v7.16.0 and removed v8.0.0 labels Sep 29, 2021

Luegg requested review from astefan, bpintea and costin September 29, 2021 15:38

Luegg marked this pull request as ready for review September 29, 2021 15:39

bpintea approved these changes Sep 29, 2021

View reviewed changes

astefan reviewed Sep 30, 2021

View reviewed changes

Luegg added 3 commits October 4, 2021 10:21

address comments

cadf894

Merge origin/main into fix/sqlBwcMaster

6f18868

reenable tests

4f3977d

Luegg requested a review from astefan October 4, 2021 08:30

astefan approved these changes Oct 4, 2021

View reviewed changes

costin approved these changes Oct 6, 2021

View reviewed changes

Merge origin/main into fix/sqlBwcMaster

bcd3c2f

Luegg added the auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) label Oct 6, 2021

elasticsearchmachine merged commit 8af3bc1 into elastic:master Oct 6, 2021

Luegg deleted the fix/sqlBwcMaster branch October 6, 2021 14:25

jakelandis added v8.0.0-beta1 and removed v8.0.0 labels Oct 27, 2021

danhermann added the >test Issues or PRs that are addressing/adding tests label Dec 3, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

backwards compatible deserialization of SqlQueryResponse and fix SQL bwc tests #78467

backwards compatible deserialization of SqlQueryResponse and fix SQL bwc tests #78467

Luegg commented Sep 29, 2021 •

edited

Loading

Luegg commented Sep 29, 2021

bpintea left a comment

bpintea Sep 29, 2021

Luegg Oct 4, 2021

bpintea Sep 29, 2021

Luegg Oct 4, 2021

bpintea Sep 29, 2021

astefan left a comment

astefan Sep 30, 2021

Luegg Oct 4, 2021

astefan Sep 30, 2021

bpintea Sep 30, 2021

astefan Sep 30, 2021

astefan Oct 4, 2021

bpintea Oct 4, 2021

Luegg Oct 4, 2021

astefan left a comment

astefan Oct 4, 2021

costin left a comment

costin Oct 6, 2021

Luegg Oct 6, 2021

	// if JDBC mode is used, display_size will also be part of the response, remove it because it's not part of the expected response
	// in JDBC mode, display_size will be part of the response, so remove it because it's not part of the expected response

		Map<String, Object> actualResponse = dropDisplaySizes(runSql(client, request));
		assertResponse(expectedResponse, actualResponse);

		String intervalYearMonth = "INTERVAL '150' YEAR AS interval_year, ";
		String intervalDayTime = "INTERVAL '163' MINUTE AS interval_minute, ";

backwards compatible deserialization of SqlQueryResponse and fix SQL bwc tests #78467

backwards compatible deserialization of SqlQueryResponse and fix SQL bwc tests #78467

Conversation

Luegg commented Sep 29, 2021 • edited Loading

Luegg commented Sep 29, 2021

bpintea left a 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

astefan left a 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

Choose a reason for hiding this comment

astefan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

costin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Luegg commented Sep 29, 2021 •

edited

Loading