Skip to content

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

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

Merged
merged 5 commits into from
Oct 6, 2021
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
0010262
make deserialization of SqlQueryResponse backwards compatible and fix…
Sep 29, 2021
cadf894
address comments
Oct 4, 2021
6f18868
Merge origin/main into fix/sqlBwcMaster
Oct 4, 2021
4f3977d
reenable tests
Oct 4, 2021
bcd3c2f
Merge origin/main into fix/sqlBwcMaster
Oct 6, 2021
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
3 changes: 1 addition & 2 deletions x-pack/plugin/sql/qa/mixed-node/build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,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

BuildParams.bwcVersions.withWireCompatiple(v -> v.onOrAfter("7.10.0") &&
BuildParams.bwcVersions.withWireCompatiple(v -> v.onOrAfter("7.10.3") &&
v != VersionProperties.getElasticsearchVersion()) { bwcVersion, baseName ->

def baseCluster = testClusters.register(baseName) {
Expand Down
24 changes: 22 additions & 2 deletions ...ql/qa/mixed-node/src/test/java/org/elasticsearch/xpack/sql/qa/mixed_node/SqlCompatIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,9 @@
import org.elasticsearch.client.Request;
import org.elasticsearch.client.Response;
import org.elasticsearch.client.RestClient;
import org.elasticsearch.common.Strings;
import org.elasticsearch.common.xcontent.XContentBuilder;
import org.elasticsearch.common.xcontent.XContentFactory;
import org.elasticsearch.common.xcontent.XContentHelper;
import org.elasticsearch.common.xcontent.json.JsonXContent;
import org.elasticsearch.core.internal.io.IOUtils;
Expand Down Expand Up @@ -123,8 +126,8 @@ private List<Integer> runOrderByNullsLastQuery(RestClient queryClient) throws IO
indexDocs.setJsonEntity(bulk.toString());
client().performRequest(indexDocs);

Request query = new Request("GET", "_sql");
query.setJsonEntity("{\"query\":\"SELECT int FROM test GROUP BY 1 ORDER BY 1 NULLS LAST\"}");
Request query = new Request("POST", "_sql");
query.setJsonEntity(sqlQueryEntityWithOptionalMode("SELECT int FROM test GROUP BY 1 ORDER BY 1 NULLS LAST", bwcVersion));
Response queryResponse = queryClient.performRequest(query);

assertEquals(200, queryResponse.getStatusLine().getStatusCode());
Expand All @@ -135,4 +138,21 @@ private List<Integer> runOrderByNullsLastQuery(RestClient queryClient) throws IO
return rows.stream().map(row -> (Integer) row.get(0)).collect(Collectors.toList());
}

public static String sqlQueryEntityWithOptionalMode(String query, Version bwcVersion) throws IOException {
XContentBuilder json = XContentFactory.jsonBuilder().startObject();
json.field("query", query);
if (bwcVersion.before(Version.V_7_12_0)) {
// a bug previous to 7.12 caused a NullPointerException when accessing displaySize in ColumnInfo. The bug has been addressed in
// https://github.com/elastic/elasticsearch/pull/68802/files
// #diff-2faa4e2df98a4636300a19d9d890a1bd7174e9b20dd3a8589d2c78a3d9e5cbc0L110
// as a workaround, use JDBC (driver) mode in versions prior to 7.12
json.field("mode", "jdbc");
json.field("binary_format", false);
json.field("version", bwcVersion.toString());
}
json.endObject();

return Strings.toString(json);
}

}
35 changes: 24 additions & 11 deletions ...ql/qa/mixed-node/src/test/java/org/elasticsearch/xpack/sql/qa/mixed_node/SqlSearchIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,6 @@
import static java.util.Collections.unmodifiableMap;
import static org.elasticsearch.xpack.ql.TestUtils.buildNodeAndVersions;
import static org.elasticsearch.xpack.ql.TestUtils.readResource;
import static org.elasticsearch.xpack.ql.execution.search.QlSourceBuilder.SWITCH_TO_FIELDS_API_VERSION;

public class SqlSearchIT extends ESRestTestCase {

Expand All @@ -56,9 +55,7 @@ public class SqlSearchIT extends ESRestTestCase {
private static List<TestNode> newNodes;
private static List<TestNode> bwcNodes;
private static Version bwcVersion;
private static Version newVersion;
private static boolean isBwcNodeBeforeFieldsApiInQL;
private static boolean isBwcNodeBeforeFieldsApiInES;

@Before
public void createIndex() throws IOException {
Expand All @@ -68,9 +65,7 @@ public void createIndex() throws IOException {
newNodes = new ArrayList<>(nodes.getNewNodes());
bwcNodes = new ArrayList<>(nodes.getBWCNodes());
bwcVersion = nodes.getBWCNodes().get(0).getVersion();
newVersion = nodes.getNewNodes().get(0).getVersion();
isBwcNodeBeforeFieldsApiInQL = bwcVersion.before(FIELDS_API_QL_INTRODUCTION);
isBwcNodeBeforeFieldsApiInES = bwcVersion.before(SWITCH_TO_FIELDS_API_VERSION);

String mappings = readResource(SqlSearchIT.class.getResourceAsStream("/all_field_types.json"));
createIndex(
Expand Down Expand Up @@ -142,7 +137,7 @@ public void testAllTypesWithRequestToUpgradedNodes() throws Exception {
(builder, fieldValues) -> {
Float randomFloat = randomFloat();
builder.append(",");
if (isBwcNodeBeforeFieldsApiInQL && isBwcNodeBeforeFieldsApiInES) {
if (isBwcNodeBeforeFieldsApiInQL) {
builder.append("\"geo_point_field\":{\"lat\":\"37.386483\", \"lon\":\"-122.083843\"},");
fieldValues.put("geo_point_field", "POINT (-122.08384302444756 37.38648299127817)");
builder.append("\"float_field\":" + randomFloat + ",");
Expand Down Expand Up @@ -256,20 +251,38 @@ private void assertAllTypesWithNodes(Map<String, Object> expectedResponse, List<
) {
@SuppressWarnings("unchecked")
List<Map<String, Object>> columns = (List<Map<String, Object>>) expectedResponse.get("columns");

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

// get all fields names from the expected response built earlier, skipping the intervals as they execute locally
// and not taken from the index itself
String fieldsList = columns.stream().map(m -> (String) m.get("name")).filter(str -> str.startsWith("interval") == false)
.collect(Collectors.toList()).stream().collect(Collectors.joining(", "));
String fieldsList = columns.stream()
.map(m -> (String) m.get("name"))
.filter(str -> str.startsWith("interval") == false)
.collect(Collectors.toList())
.stream()
.collect(Collectors.joining(", "));
String query = "SELECT " + intervalYearMonth + intervalDayTime + fieldsList + " FROM " + index + " ORDER BY id";

Request request = new Request("POST", "_sql");
request.setJsonEntity("{\"query\":\"" + query + "\"}");
assertBusy(() -> { assertResponse(expectedResponse, runSql(client, request)); });
request.setJsonEntity(SqlCompatIT.sqlQueryEntityWithOptionalMode(query, bwcVersion));
assertBusy(() -> {
assertResponse(expectedResponse, dropDisplaySizes(runSql(client, request)));
});
}
}

private Map<String, Object> dropDisplaySizes(Map<String, Object> response) {
// in JDBC mode, display_size will be part of the response, so remove it because it's not part of the expected response
@SuppressWarnings("unchecked")
List<Map<String, Object>> columns = (List<Map<String, Object>>) response.get("columns");
List<Map<String, Object>> columnsWithoutDisplaySizes = columns.stream()
.peek(column -> column.remove("display_size"))
.collect(Collectors.toList());
response.put("columns", columnsWithoutDisplaySizes);
return response;
}

private void assertResponse(Map<String, Object> expected, Map<String, Object> actual) {
if (false == expected.equals(actual)) {
NotEqualMessageBuilder message = new NotEqualMessageBuilder();
Expand Down
29 changes: 18 additions & 11 deletions ...gin/sql/sql-action/src/main/java/org/elasticsearch/xpack/sql/action/SqlQueryResponse.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,26 +6,27 @@
*/
package org.elasticsearch.xpack.sql.action;

import java.io.IOException;
import java.time.ZonedDateTime;
import java.util.ArrayList;
import java.util.List;
import java.util.Objects;

import org.elasticsearch.Version;
import org.elasticsearch.action.ActionResponse;
import org.elasticsearch.core.Nullable;
import org.elasticsearch.common.Strings;
import org.elasticsearch.common.io.stream.StreamInput;
import org.elasticsearch.common.io.stream.StreamOutput;
import org.elasticsearch.common.xcontent.ToXContentObject;
import org.elasticsearch.common.xcontent.XContentBuilder;
import org.elasticsearch.core.Nullable;
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;
Copy link
Contributor

@bpintea bpintea Sep 29, 2021

Choose a reason for hiding this comment

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

have these been reordered automatically by the editor?

Copy link
Contributor Author

@Luegg Luegg Oct 4, 2021

Choose a reason for hiding this comment

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

yes. I think that follows the default imports layout.

import java.time.ZonedDateTime;
import java.util.ArrayList;
import java.util.List;
import java.util.Objects;

import static java.util.Collections.unmodifiableList;
import static org.elasticsearch.Version.CURRENT;
import static org.elasticsearch.xpack.sql.action.AbstractSqlQueryRequest.CURSOR;
Expand Down Expand Up @@ -81,10 +82,16 @@ public SqlQueryResponse(StreamInput in) throws IOException {
}
}
this.rows = unmodifiableList(rows);
columnar = in.readBoolean();
asyncExecutionId = in.readOptionalString();
isPartial = in.readBoolean();
isRunning = in.readBoolean();
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;
}
Comment on lines +85 to +94
Copy link
Contributor

@astefan astefan Sep 30, 2021

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@bpintea bpintea Sep 30, 2021

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@astefan astefan Sep 30, 2021

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@astefan astefan Oct 4, 2021

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@bpintea bpintea Oct 4, 2021

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@Luegg Luegg Oct 4, 2021

Choose a reason for hiding this comment

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

Thanks @bpintea, I was getting confused myself.

}

public SqlQueryResponse(
Expand Down