Skip to content

Commit cc65d75

Browse files
dai-chendai-chen
authored
Fix SQL IT test queries, assertions, and data for engine-agnostic compatibility (#5584)
* test(integ-test): fix engine-agnostic IT queries Stabilize non-deterministic GROUP BY results with ORDER BY, normalize LENGTH() case, and use ANSI positional GROUP BY. Correctness fixes that apply regardless of execution engine. Signed-off-by: Chen Dai <daichen@amazon.com> * test(integ-test): relax schema matcher for analytics engine Signed-off-by: Chen Dai <daichen@amazon.com> --------- Signed-off-by: Chen Dai <daichen@amazon.com>
1 parent cb10516 commit cc65d75

6 files changed

Lines changed: 59 additions & 36 deletions

File tree

‎integ-test/src/test/java/org/opensearch/sql/legacy/OrdinalAliasRewriterIT.java‎

Lines changed: 13 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -25,13 +25,13 @@ public void simpleGroupByOrdinal() {
2525
String expected =
2626
executeQuery(
2727
StringUtils.format(
28-
"SELECT lastname FROM %s AS b GROUP BY lastname LIMIT 3",
28+
"SELECT lastname FROM %s AS b GROUP BY lastname ORDER BY lastname LIMIT 3",
2929
TestsConstants.TEST_INDEX_ACCOUNT),
3030
"jdbc");
3131
String actual =
3232
executeQuery(
3333
StringUtils.format(
34-
"SELECT lastname FROM %s AS b GROUP BY 1 LIMIT 3",
34+
"SELECT lastname FROM %s AS b GROUP BY 1 ORDER BY 1 LIMIT 3",
3535
TestsConstants.TEST_INDEX_ACCOUNT),
3636
"jdbc");
3737
assertThat(actual, equalTo(expected));
@@ -43,13 +43,14 @@ public void multipleGroupByOrdinal() {
4343
executeQuery(
4444
StringUtils.format(
4545
"SELECT lastname, firstname, age FROM %s AS b GROUP BY firstname, age, lastname"
46-
+ " LIMIT 3",
46+
+ " ORDER BY lastname, firstname, age LIMIT 3",
4747
TestsConstants.TEST_INDEX_ACCOUNT),
4848
"jdbc");
4949
String actual =
5050
executeQuery(
5151
StringUtils.format(
52-
"SELECT lastname, firstname, age FROM %s AS b GROUP BY 2, 3, 1 LIMIT 3",
52+
"SELECT lastname, firstname, age FROM %s AS b GROUP BY 2, 3, 1"
53+
+ " ORDER BY 1, 2, 3 LIMIT 3",
5354
TestsConstants.TEST_INDEX_ACCOUNT),
5455
"jdbc");
5556
assertThat(actual, equalTo(expected));
@@ -60,13 +61,13 @@ public void selectFieldiWithBacticksGroupByOrdinal() {
6061
String expected =
6162
executeQuery(
6263
StringUtils.format(
63-
"SELECT `lastname` FROM %s AS b GROUP BY `lastname` LIMIT 3",
64+
"SELECT `lastname` FROM %s AS b GROUP BY `lastname` ORDER BY `lastname` LIMIT 3",
6465
TestsConstants.TEST_INDEX_ACCOUNT),
6566
"jdbc");
6667
String actual =
6768
executeQuery(
6869
StringUtils.format(
69-
"SELECT `lastname` FROM %s AS b GROUP BY 1 LIMIT 3",
70+
"SELECT `lastname` FROM %s AS b GROUP BY 1 ORDER BY 1 LIMIT 3",
7071
TestsConstants.TEST_INDEX_ACCOUNT),
7172
"jdbc");
7273
assertThat(actual, equalTo(expected));
@@ -78,13 +79,14 @@ public void selectFieldiWithBacticksAndTableAliasGroupByOrdinal() {
7879
executeQuery(
7980
StringUtils.format(
8081
"SELECT `b`.`lastname`, `age`, firstname FROM %s AS b GROUP BY `age`,"
81-
+ " `b`.`lastname` , firstname LIMIT 10",
82+
+ " `b`.`lastname` , firstname ORDER BY `b`.`lastname`, `age` LIMIT 10",
8283
TestsConstants.TEST_INDEX_ACCOUNT),
8384
"jdbc");
8485
String actual =
8586
executeQuery(
8687
StringUtils.format(
87-
"SELECT `b`.`lastname`, `age`, firstname FROM %s AS b GROUP BY 2, 1, 3 LIMIT 10",
88+
"SELECT `b`.`lastname`, `age`, firstname FROM %s AS b GROUP BY 2, 1, 3 ORDER BY 1,"
89+
+ " 2 LIMIT 10",
8890
TestsConstants.TEST_INDEX_ACCOUNT),
8991
"jdbc");
9092
assertThat(actual, equalTo(expected));
@@ -166,14 +168,14 @@ public void selectFieldiWithBacticksAndTableAliasOrderByOrdinalAndNull() {
166168
executeQuery(
167169
StringUtils.format(
168170
"SELECT `b`.`lastname`, age FROM %s AS b ORDER BY `b`.`lastname` IS NOT NULL DESC,"
169-
+ " age is NULL LIMIT 3",
171+
+ " age is NULL, `b`.`lastname` LIMIT 3",
170172
TestsConstants.TEST_INDEX_ACCOUNT),
171173
"jdbc");
172174
String actual =
173175
executeQuery(
174176
StringUtils.format(
175-
"SELECT `b`.`lastname`, age FROM %s AS b ORDER BY 1 IS NOT NULL DESC, 2 IS NULL"
176-
+ " LIMIT 3",
177+
"SELECT `b`.`lastname`, age FROM %s AS b ORDER BY 1 IS NOT NULL DESC, 2 IS NULL,"
178+
+ " 1 LIMIT 3",
177179
TestsConstants.TEST_INDEX_ACCOUNT),
178180
"jdbc");
179181
assertThat(actual, equalTo(expected));

‎integ-test/src/test/java/org/opensearch/sql/legacy/PrettyFormatResponseIT.java‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -80,7 +80,11 @@ protected void init() throws Exception {
8080
loadIndex(Index.ACCOUNT);
8181
loadIndex(Index.PHRASE);
8282
loadIndex(Index.GAME_OF_THRONES);
83-
loadIndex(Index.NESTED);
83+
// Skip on the analytics-engine route, where the parquet store rejects nested_objects'
84+
// multi-value array in the scalar-mapped myNum field at bulk load.
85+
if (!TestUtils.AnalyticsIndexConfig.isEnabled()) {
86+
loadIndex(Index.NESTED);
87+
}
8488
}
8589

8690
@Override

‎integ-test/src/test/java/org/opensearch/sql/legacy/TypeInformationIT.java‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -96,11 +96,11 @@ public void testLengthWithTextFieldReturnsInt() {
9696
public void testLengthWithGroupByExpr() {
9797
JSONObject response =
9898
executeJdbcRequest(
99-
"SELECT Length(firstname) FROM "
99+
"SELECT LENGTH(firstname) FROM "
100100
+ TestsConstants.TEST_INDEX_ACCOUNT
101101
+ " GROUP BY LENGTH(firstname) LIMIT 5");
102102

103-
verifySchema(response, schema("Length(firstname)", null, "integer"));
103+
verifySchema(response, schema("LENGTH(firstname)", null, "integer"));
104104
}
105105

106106
/*

‎integ-test/src/test/java/org/opensearch/sql/sql/ConditionalIT.java‎

Lines changed: 5 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -46,12 +46,8 @@ public void init() throws Exception {
4646
public void ifnullShouldPassJDBC() throws IOException {
4747
JSONObject response =
4848
executeJdbcRequest(
49-
"SELECT IFNULL(lastname, 'unknown') AS name FROM "
50-
+ TEST_INDEX_ACCOUNT
51-
+ " GROUP BY name");
52-
assertEquals("IFNULL(lastname, 'unknown')", response.query("/schema/0/name"));
53-
assertEquals("name", response.query("/schema/0/alias"));
54-
assertEquals("keyword", response.query("/schema/0/type"));
49+
"SELECT IFNULL(lastname, 'unknown') FROM " + TEST_INDEX_ACCOUNT + " GROUP BY 1");
50+
verifySchema(response, schema("IFNULL(lastname, 'unknown')", null, "keyword"));
5551
}
5652

5753
@Test
@@ -109,9 +105,7 @@ public void ifnullWithMissingInputTest() {
109105
public void nullifShouldPassJDBC() throws IOException {
110106
JSONObject response =
111107
executeJdbcRequest("SELECT NULLIF(lastname, 'unknown') AS name FROM " + TEST_INDEX_ACCOUNT);
112-
assertEquals("NULLIF(lastname, 'unknown')", response.query("/schema/0/name"));
113-
assertEquals("name", response.query("/schema/0/alias"));
114-
assertEquals("keyword", response.query("/schema/0/type"));
108+
verifySchema(response, schema("NULLIF(lastname, 'unknown')", "name", "keyword"));
115109
}
116110

117111
@Test
@@ -152,9 +146,7 @@ public void nullifWithNullInputTest() {
152146
public void isnullShouldPassJDBC() throws IOException {
153147
JSONObject response =
154148
executeJdbcRequest("SELECT ISNULL(lastname) AS name FROM " + TEST_INDEX_ACCOUNT);
155-
assertEquals("ISNULL(lastname)", response.query("/schema/0/name"));
156-
assertEquals("name", response.query("/schema/0/alias"));
157-
assertEquals("boolean", response.query("/schema/0/type"));
149+
verifySchema(response, schema("ISNULL(lastname)", "name", "boolean"));
158150
}
159151

160152
@Ignore(
@@ -208,9 +200,7 @@ public void isnullWithMathExpr() throws IOException {
208200
public void ifShouldPassJDBC() throws IOException {
209201
JSONObject response =
210202
executeJdbcRequest("SELECT IF(2 > 0, 'hello', 'world') AS name FROM " + TEST_INDEX_ACCOUNT);
211-
assertEquals("IF(2 > 0, 'hello', 'world')", response.query("/schema/0/name"));
212-
assertEquals("name", response.query("/schema/0/alias"));
213-
assertEquals("keyword", response.query("/schema/0/type"));
203+
verifySchema(response, schema("IF(2 > 0, 'hello', 'world')", "name", "keyword"));
214204
}
215205

216206
@Test

‎integ-test/src/test/java/org/opensearch/sql/util/MatcherUtils.java‎

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@
2727
import java.util.Comparator;
2828
import java.util.List;
2929
import java.util.Map;
30+
import java.util.Set;
3031
import java.util.function.Function;
3132
import org.apache.commons.lang3.exception.ExceptionUtils;
3233
import org.apache.logging.log4j.LogManager;
@@ -39,6 +40,7 @@
3940
import org.json.JSONObject;
4041
import org.opensearch.search.SearchHit;
4142
import org.opensearch.search.SearchHits;
43+
import org.opensearch.sql.legacy.TestUtils;
4244
import org.opensearch.sql.utils.YamlFormatter;
4345

4446
public class MatcherUtils {
@@ -275,6 +277,9 @@ public static TypeSafeMatcher<JSONObject> schema(String expectedName, String exp
275277
public static TypeSafeMatcher<JSONObject> schema(
276278
String expectedName, String expectedAlias, String expectedType) {
277279
return new TypeSafeMatcher<JSONObject>() {
280+
private static final Set<String> NUMERIC_TYPES = Set.of("integer", "long", "float", "double");
281+
private static final Set<String> STRING_TYPES = Set.of("keyword", "text", "string");
282+
278283
@Override
279284
public void describeTo(Description description) {
280285
description.appendText(
@@ -287,10 +292,32 @@ protected boolean matchesSafely(JSONObject jsonObject) {
287292
String actualName = (String) jsonObject.query("/name");
288293
String actualAlias = (String) jsonObject.query("/alias");
289294
String actualType = (String) jsonObject.query("/type");
295+
296+
if (TestUtils.AnalyticsIndexConfig.isEnabled()) {
297+
// The analytics-engine route promotes the alias to the name (SQL-standard) and unifies
298+
// keyword/text/string and the numeric types, so relax name and type matching for it only.
299+
boolean nameMatches =
300+
expectedName.equals(actualName)
301+
|| (!Strings.isNullOrEmpty(expectedAlias) && expectedAlias.equals(actualName));
302+
boolean typeMatches =
303+
expectedType.equals(actualType) || isCompatibleType(expectedType, actualType);
304+
return nameMatches && typeMatches;
305+
}
306+
290307
return expectedName.equals(actualName)
291308
&& (Strings.isNullOrEmpty(expectedAlias) || expectedAlias.equals(actualAlias))
292309
&& expectedType.equals(actualType);
293310
}
311+
312+
private boolean isCompatibleType(String expected, String actual) {
313+
if (expected == null || actual == null) {
314+
return false;
315+
}
316+
String e = expected.toLowerCase();
317+
String a = actual.toLowerCase();
318+
return (NUMERIC_TYPES.contains(e) && NUMERIC_TYPES.contains(a))
319+
|| (STRING_TYPES.contains(e) && STRING_TYPES.contains(a));
320+
}
294321
};
295322
}
296323

‎integ-test/src/test/resources/game_of_thrones_complex.json‎

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,14 +1,14 @@
11
{"index":{"_id":"1"}}
2-
{"name":{"firstname":"Daenerys","lastname":"Targaryen","ofHerName":1},"nickname":"Daenerys \"Stormborn\"","house":"Targaryen","gender":"F","parents":{"father":"Aerys" , "mother":"Rhaella"},"titles":["motherOfDragons","queenOfTheAndals","breakerOfChains","Khaleesi"]}
2+
{"name": {"firstname": "Daenerys", "lastname": "Targaryen", "ofHerName": 1}, "nickname": "Daenerys \"Stormborn\"", "house": "Targaryen", "gender": "F", "parents": {"father": "Aerys", "mother": "Rhaella"}}
33
{"index":{"_id":"2"}}
4-
{"name":{"firstname":"Eddard","lastname":"Stark","ofHisName":1},"house":"Stark", "parents":{"father":"Rickard" , "mother":"Lyarra"} ,"gender":"M","titles":["lordOfWinterfell","wardenOfTheNorth","handOfTheKing"]}
4+
{"name": {"firstname": "Eddard", "lastname": "Stark", "ofHisName": 1}, "house": "Stark", "parents": {"father": "Rickard", "mother": "Lyarra"}, "gender": "M"}
55
{"index":{"_id":"3"}}
6-
{"name":{"firstname":"Brandon","lastname":"Stark","ofHisName":4},"house":"Stark","parents":{"father":"Eddard","mother":"Catelyn"},"gender":"M","titles":["princeOfWinterfell"],"@wolf":"Summer"}
6+
{"name": {"firstname": "Brandon", "lastname": "Stark", "ofHisName": 4}, "house": "Stark", "parents": {"father": "Eddard", "mother": "Catelyn"}, "gender": "M", "@wolf": "Summer"}
77
{"index":{"_id":"4"}}
8-
{"name":{"firstname":"Jaime","lastname":"Lannister","ofHisName":1},"gender":"M","house":"Lannister","parents":{"father":"Tywin","mother":"Joanna"},"titles":["kingSlayer","lordCommanderOfTheKingsguard","Ser"]}
8+
{"name": {"firstname": "Jaime", "lastname": "Lannister", "ofHisName": 1}, "gender": "M", "house": "Lannister", "parents": {"father": "Tywin", "mother": "Joanna"}}
99
{"index":{"_id":"5"}}
10-
{"words":"fireAndBlood","hname":"Targaryen","sigil":"Dragon","seat":"Dragonstone"}
10+
{"words": "fireAndBlood", "hname": "Targaryen", "sigil": "Dragon", "seat": "Dragonstone"}
1111
{"index":{"_id":"6"}}
12-
{"words":"winterIsComing" , "hname":"Stark","sigil":"direwolf","seat":"Winterfell"}
12+
{"words": "winterIsComing", "hname": "Stark", "sigil": "direwolf", "seat": "Winterfell"}
1313
{"index":{"_id":"7"}}
14-
{"words":"hearMeRoar" , "hname":"Lannister","sigil":"lion","seat":"CasterlyRock"}
14+
{"words": "hearMeRoar", "hname": "Lannister", "sigil": "lion", "seat": "CasterlyRock"}

0 commit comments

Comments
 (0)