Skip to content

Commit d57149f

Browse files
author
Lukas Wegmann
authored
[7.x] backwards compatible deserialization of SqlQueryResponse and fix SQL bwc tests (#78465)
* make deserialization of SqlQueryResponse backwards compatible and fix SQL bwc tests * address comments * reenable tests
1 parent 0d23a0b commit d57149f

File tree

4 files changed

+66
-30
lines changed

4 files changed

+66
-30
lines changed

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

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -21,8 +21,7 @@ testClusters.configureEach {
2121
tasks.named("integTest").configure{ enabled = false}
2222

2323
// A bug (https://github.com/elastic/elasticsearch/issues/68439) limits us to perform tests with versions from 7.10.3 onwards
24-
25-
BuildParams.bwcVersions.withWireCompatiple(v -> v.onOrAfter("7.10.0") &&
24+
BuildParams.bwcVersions.withWireCompatiple(v -> v.onOrAfter("7.10.3") &&
2625
v != VersionProperties.getElasticsearchVersion()) { bwcVersion, baseName ->
2726

2827
def baseCluster = testClusters.register(baseName) {

‎x-pack/plugin/sql/qa/mixed-node/src/test/java/org/elasticsearch/xpack/sql/qa/mixed_node/SqlCompatIT.java‎

Lines changed: 22 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -8,11 +8,13 @@
88
package org.elasticsearch.xpack.sql.qa.mixed_node;
99

1010
import org.apache.http.HttpHost;
11-
import org.apache.lucene.util.LuceneTestCase;
1211
import org.elasticsearch.Version;
1312
import org.elasticsearch.client.Request;
1413
import org.elasticsearch.client.Response;
1514
import org.elasticsearch.client.RestClient;
15+
import org.elasticsearch.common.Strings;
16+
import org.elasticsearch.common.xcontent.XContentBuilder;
17+
import org.elasticsearch.common.xcontent.XContentFactory;
1618
import org.elasticsearch.common.xcontent.XContentHelper;
1719
import org.elasticsearch.common.xcontent.json.JsonXContent;
1820
import org.elasticsearch.core.internal.io.IOUtils;
@@ -65,7 +67,6 @@ public static void cleanUpClients() throws IOException {
6567
});
6668
}
6769

68-
@LuceneTestCase.AwaitsFix(bugUrl = "https://github.com/elastic/elasticsearch/issues/78424")
6970
public void testNullsOrderBeforeMissingOrderSupportQueryingNewNode() throws IOException {
7071
testNullsOrderBeforeMissingOrderSupport(newNodesClient);
7172
}
@@ -125,8 +126,8 @@ private List<Integer> runOrderByNullsLastQuery(RestClient queryClient) throws IO
125126
indexDocs.setJsonEntity(bulk.toString());
126127
client().performRequest(indexDocs);
127128

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

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

141+
public static String sqlQueryEntityWithOptionalMode(String query, Version bwcVersion) throws IOException {
142+
XContentBuilder json = XContentFactory.jsonBuilder().startObject();
143+
json.field("query", query);
144+
if (bwcVersion.before(Version.V_7_12_0)) {
145+
// a bug previous to 7.12 caused a NullPointerException when accessing displaySize in ColumnInfo. The bug has been addressed in
146+
// https://github.com/elastic/elasticsearch/pull/68802/files
147+
// #diff-2faa4e2df98a4636300a19d9d890a1bd7174e9b20dd3a8589d2c78a3d9e5cbc0L110
148+
// as a workaround, use JDBC (driver) mode in versions prior to 7.12
149+
json.field("mode", "jdbc");
150+
json.field("binary_format", false);
151+
json.field("version", bwcVersion.toString());
152+
}
153+
json.endObject();
154+
155+
return Strings.toString(json);
156+
}
157+
140158
}

‎x-pack/plugin/sql/qa/mixed-node/src/test/java/org/elasticsearch/xpack/sql/qa/mixed_node/SqlSearchIT.java‎

Lines changed: 25 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,6 @@
3838
import static java.util.Collections.unmodifiableMap;
3939
import static org.elasticsearch.xpack.ql.TestUtils.buildNodeAndVersions;
4040
import static org.elasticsearch.xpack.ql.TestUtils.readResource;
41-
import static org.elasticsearch.xpack.ql.execution.search.QlSourceBuilder.SWITCH_TO_FIELDS_API_VERSION;
4241

4342
public class SqlSearchIT extends ESRestTestCase {
4443

@@ -56,9 +55,7 @@ public class SqlSearchIT extends ESRestTestCase {
5655
private static List<TestNode> newNodes;
5756
private static List<TestNode> bwcNodes;
5857
private static Version bwcVersion;
59-
private static Version newVersion;
6058
private static boolean isBwcNodeBeforeFieldsApiInQL;
61-
private static boolean isBwcNodeBeforeFieldsApiInES;
6259
private static boolean halfFloatMightReturnFullFloatPrecision;
6360

6461
@Before
@@ -69,9 +66,7 @@ public void createIndex() throws IOException {
6966
newNodes = new ArrayList<>(nodes.getNewNodes());
7067
bwcNodes = new ArrayList<>(nodes.getBWCNodes());
7168
bwcVersion = nodes.getBWCNodes().get(0).getVersion();
72-
newVersion = nodes.getNewNodes().get(0).getVersion();
7369
isBwcNodeBeforeFieldsApiInQL = bwcVersion.before(FIELDS_API_QL_INTRODUCTION);
74-
isBwcNodeBeforeFieldsApiInES = bwcVersion.before(SWITCH_TO_FIELDS_API_VERSION);
7570
halfFloatMightReturnFullFloatPrecision = bwcVersion.before(Version.V_7_13_0);
7671

7772
String mappings = readResource(SqlSearchIT.class.getResourceAsStream("/all_field_types.json"));
@@ -150,7 +145,6 @@ public void testAllTypesWithRequestToOldNodes() throws Exception {
150145
assertAllTypesWithNodes(expectedResponse, bwcNodes);
151146
}
152147

153-
@AwaitsFix(bugUrl = "https://github.com/elastic/elasticsearch/issues/78424")
154148
public void testAllTypesWithRequestToUpgradedNodes() throws Exception {
155149
Map<String, Object> expectedResponse = prepareTestData(
156150
columns -> {
@@ -162,14 +156,14 @@ public void testAllTypesWithRequestToUpgradedNodes() throws Exception {
162156
* that version we don't fetch the half float because we
163157
* can't make any assertions about it.
164158
*/
165-
if (isBwcNodeBeforeFieldsApiInQL && isBwcNodeBeforeFieldsApiInES || false == halfFloatMightReturnFullFloatPrecision) {
159+
if (isBwcNodeBeforeFieldsApiInQL || false == halfFloatMightReturnFullFloatPrecision) {
166160
columns.add(columnInfo("half_float_field", "half_float"));
167161
}
168162
},
169163
(builder, fieldValues) -> {
170164
Float randomFloat = randomFloat();
171165
builder.append(",");
172-
if (isBwcNodeBeforeFieldsApiInQL && isBwcNodeBeforeFieldsApiInES) {
166+
if (isBwcNodeBeforeFieldsApiInQL) {
173167
builder.append("\"geo_point_field\":{\"lat\":\"37.386483\", \"lon\":\"-122.083843\"},");
174168
fieldValues.put("geo_point_field", "POINT (-122.08384302444756 37.38648299127817)");
175169
builder.append("\"float_field\":" + randomFloat + ",");
@@ -291,20 +285,38 @@ private void assertAllTypesWithNodes(Map<String, Object> expectedResponse, List<
291285
) {
292286
@SuppressWarnings("unchecked")
293287
List<Map<String, Object>> columns = (List<Map<String, Object>>) expectedResponse.get("columns");
288+
294289
String intervalYearMonth = "INTERVAL '150' YEAR AS interval_year, ";
295290
String intervalDayTime = "INTERVAL '163' MINUTE AS interval_minute, ";
296-
297291
// get all fields names from the expected response built earlier, skipping the intervals as they execute locally
298292
// and not taken from the index itself
299-
String fieldsList = columns.stream().map(m -> (String) m.get("name")).filter(str -> str.startsWith("interval") == false)
300-
.collect(Collectors.toList()).stream().collect(Collectors.joining(", "));
293+
String fieldsList = columns.stream()
294+
.map(m -> (String) m.get("name"))
295+
.filter(str -> str.startsWith("interval") == false)
296+
.collect(Collectors.toList())
297+
.stream()
298+
.collect(Collectors.joining(", "));
301299
String query = "SELECT " + intervalYearMonth + intervalDayTime + fieldsList + " FROM " + index + " ORDER BY id";
300+
302301
Request request = new Request("POST", "_sql");
303-
request.setJsonEntity("{\"query\":\"" + query + "\"}");
304-
assertBusy(() -> { assertResponse(expectedResponse, runSql(client, request)); });
302+
request.setJsonEntity(SqlCompatIT.sqlQueryEntityWithOptionalMode(query, bwcVersion));
303+
assertBusy(() -> {
304+
assertResponse(expectedResponse, dropDisplaySizes(runSql(client, request)));
305+
});
305306
}
306307
}
307308

309+
private Map<String, Object> dropDisplaySizes(Map<String, Object> response) {
310+
// in JDBC mode, display_size will be part of the response, so remove it because it's not part of the expected response
311+
@SuppressWarnings("unchecked")
312+
List<Map<String, Object>> columns = (List<Map<String, Object>>) response.get("columns");
313+
List<Map<String, Object>> columnsWithoutDisplaySizes = columns.stream()
314+
.peek(column -> column.remove("display_size"))
315+
.collect(Collectors.toList());
316+
response.put("columns", columnsWithoutDisplaySizes);
317+
return response;
318+
}
319+
308320
private void assertResponse(Map<String, Object> expected, Map<String, Object> actual) {
309321
if (false == expected.equals(actual)) {
310322
NotEqualMessageBuilder message = new NotEqualMessageBuilder();

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

Lines changed: 18 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -6,26 +6,27 @@
66
*/
77
package org.elasticsearch.xpack.sql.action;
88

9-
import java.io.IOException;
10-
import java.time.ZonedDateTime;
11-
import java.util.ArrayList;
12-
import java.util.List;
13-
import java.util.Objects;
14-
9+
import org.elasticsearch.Version;
1510
import org.elasticsearch.action.ActionResponse;
16-
import org.elasticsearch.core.Nullable;
1711
import org.elasticsearch.common.Strings;
1812
import org.elasticsearch.common.io.stream.StreamInput;
1913
import org.elasticsearch.common.io.stream.StreamOutput;
2014
import org.elasticsearch.common.xcontent.ToXContentObject;
2115
import org.elasticsearch.common.xcontent.XContentBuilder;
16+
import org.elasticsearch.core.Nullable;
2217
import org.elasticsearch.xpack.ql.async.QlStatusResponse;
2318
import org.elasticsearch.xpack.sql.proto.ColumnInfo;
2419
import org.elasticsearch.xpack.sql.proto.Mode;
2520
import org.elasticsearch.xpack.sql.proto.Protocol;
2621
import org.elasticsearch.xpack.sql.proto.SqlVersion;
2722
import org.elasticsearch.xpack.sql.proto.StringUtils;
2823

24+
import java.io.IOException;
25+
import java.time.ZonedDateTime;
26+
import java.util.ArrayList;
27+
import java.util.List;
28+
import java.util.Objects;
29+
2930
import static java.util.Collections.unmodifiableList;
3031
import static org.elasticsearch.Version.CURRENT;
3132
import static org.elasticsearch.xpack.sql.action.AbstractSqlQueryRequest.CURSOR;
@@ -81,10 +82,16 @@ public SqlQueryResponse(StreamInput in) throws IOException {
8182
}
8283
}
8384
this.rows = unmodifiableList(rows);
84-
columnar = in.readBoolean();
85-
asyncExecutionId = in.readOptionalString();
86-
isPartial = in.readBoolean();
87-
isRunning = in.readBoolean();
85+
if (in.getVersion().onOrAfter(Version.V_7_14_0)) {
86+
columnar = in.readBoolean();
87+
asyncExecutionId = in.readOptionalString();
88+
isPartial = in.readBoolean();
89+
isRunning = in.readBoolean();
90+
} else {
91+
asyncExecutionId = null;
92+
isPartial = false;
93+
isRunning = false;
94+
}
8895
}
8996

9097
public SqlQueryResponse(

0 commit comments

Comments
 (0)