Skip to content

Commit 591fb71

Browse files
kenrickyapkenrickyap
committed
addressed PR comments
Signed-off-by: Kenrick Yap <[email protected]>
1 parent 95e996b commit 591fb71

File tree

4 files changed

+154
-33
lines changed

4 files changed

+154
-33
lines changed

‎core/src/main/java/org/opensearch/sql/utils/JsonUtils.java

+1-3
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,6 @@
66
package org.opensearch.sql.utils;
77

88
import static org.opensearch.sql.data.model.ExprValueUtils.LITERAL_FALSE;
9-
import static org.opensearch.sql.data.model.ExprValueUtils.LITERAL_MISSING;
109
import static org.opensearch.sql.data.model.ExprValueUtils.LITERAL_NULL;
1110
import static org.opensearch.sql.data.model.ExprValueUtils.LITERAL_TRUE;
1211

@@ -93,10 +92,9 @@ public static ExprValue castJson(ExprValue json) {
9392
* @return ExprValue of value at given path of json string.
9493
*/
9594
public static ExprValue extractJson(ExprValue json, ExprValue... paths) {
96-
List<ExprValue> resultList = new ArrayList<>();
95+
List<ExprValue> resultList = new ArrayList<>(paths.length);
9796

9897
for (ExprValue path : paths) {
99-
System.out.println("Processing path: " + path);
10098
if (json.isNull() || json.isMissing()) {
10199
return json;
102100
}

‎core/src/test/java/org/opensearch/sql/expression/json/JsonFunctionsTest.java

+61-24
Original file line numberDiff line numberDiff line change
@@ -13,11 +13,13 @@
1313
import static org.opensearch.sql.data.model.ExprValueUtils.LITERAL_NULL;
1414
import static org.opensearch.sql.data.model.ExprValueUtils.LITERAL_TRUE;
1515

16+
import java.util.Arrays;
1617
import java.util.LinkedHashMap;
1718
import java.util.List;
1819
import java.util.Map;
1920
import org.junit.jupiter.api.Test;
2021
import org.junit.jupiter.api.extension.ExtendWith;
22+
import org.junit.jupiter.api.extension.TestInstantiationException;
2123
import org.mockito.junit.jupiter.MockitoExtension;
2224
import org.opensearch.sql.data.model.ExprBooleanValue;
2325
import org.opensearch.sql.data.model.ExprCollectionValue;
@@ -234,7 +236,7 @@ void json_returnsSemanticCheckException() {
234236
@Test
235237
void json_extract_search() {
236238
ExprValue expected = new ExprIntegerValue(1);
237-
execute_extract_json(expected, "{\"a\":1}", "$.a");
239+
assert_equals_extract_json(expected, "{\"a\":1}", "$.a");
238240
}
239241

240242
@Test
@@ -256,17 +258,17 @@ void json_extract_search_arrays() {
256258
// extract specific index from JSON list
257259
for (int i = 0; i < expectedExprValues.size(); i++) {
258260
String path = String.format("$.a[%d]", i);
259-
execute_extract_json(expectedExprValues.get(i), jsonArray, path);
261+
assert_equals_extract_json(expectedExprValues.get(i), jsonArray, path);
260262
}
261263

262264
// extract nested object
263265
ExprValue nestedExpected =
264266
ExprTupleValue.fromExprValueMap(Map.of("d", new ExprIntegerValue(1)));
265-
execute_extract_json(nestedExpected, jsonArray, "$.a[5].c");
267+
assert_equals_extract_json(nestedExpected, jsonArray, "$.a[5].c");
266268

267269
// extract * from JSON list
268270
ExprValue starExpected = new ExprCollectionValue(expectedExprValues);
269-
execute_extract_json(starExpected, jsonArray, "$.a[*]");
271+
assert_equals_extract_json(starExpected, jsonArray, "$.a[*]");
270272
}
271273

272274
@Test
@@ -285,10 +287,11 @@ void json_extract_returns_null() {
285287
"false",
286288
"");
287289

288-
jsonStrings.forEach(str -> execute_extract_json(LITERAL_NULL, str, "$.a.path_not_found_key"));
290+
jsonStrings.forEach(
291+
str -> assert_equals_extract_json(LITERAL_NULL, str, "$.a.path_not_found_key"));
289292

290293
// null string literal
291-
assertEquals(LITERAL_NULL, DSL.jsonExtract(DSL.literal("null"), DSL.literal("$.a")).valueOf());
294+
assert_equals_extract_json(LITERAL_NULL, "null", "$.a");
292295

293296
// null json
294297
assertEquals(
@@ -300,7 +303,7 @@ void json_extract_returns_null() {
300303
DSL.jsonExtract(DSL.literal(LITERAL_MISSING), DSL.literal("$.a")).valueOf());
301304

302305
// array out of bounds
303-
execute_extract_json(LITERAL_NULL, "{\"a\":[1,2,3]}", "$.a[4]");
306+
assert_equals_extract_json(LITERAL_NULL, "{\"a\":[1,2,3]}", "$.a[4]");
304307
}
305308

306309
@Test
@@ -334,14 +337,10 @@ void json_extract_throws_SemanticCheckException() {
334337
@Test
335338
void json_extract_throws_ExpressionEvaluationException() {
336339
// null path
337-
assertThrows(
338-
ExpressionEvaluationException.class,
339-
() -> DSL.jsonExtract(DSL.literal("{\"a\":1}"), DSL.literal(LITERAL_NULL)).valueOf());
340+
assert_throws_extract_json(ExpressionEvaluationException.class, "{\"a\":1}", LITERAL_NULL);
340341

341342
// missing path
342-
assertThrows(
343-
ExpressionEvaluationException.class,
344-
() -> DSL.jsonExtract(DSL.literal("{\"a\":1}"), DSL.literal(LITERAL_MISSING)).valueOf());
343+
assert_throws_extract_json(ExpressionEvaluationException.class, "{\"a\":1}", LITERAL_MISSING);
345344
}
346345

347346
@Test
@@ -350,21 +349,59 @@ void json_extract_search_list_of_paths() {
350349
"{\"foo\": \"foo\", \"fuzz\": true, \"bar\": 1234, \"bar2\": 12.34, \"baz\": null, "
351350
+ "\"obj\": {\"internal\": \"value\"}, \"arr\": [\"string\", true, null]}";
352351

353-
ExprValue expected =
352+
// scalar results with one invalid path
353+
ExprValue expected_scalar_results =
354354
new ExprCollectionValue(
355355
List.of(new ExprStringValue("foo"), new ExprFloatValue(12.34), LITERAL_NULL));
356-
Expression pathExpr1 = DSL.literal(ExprValueUtils.stringValue("$.foo"));
357-
Expression pathExpr2 = DSL.literal(ExprValueUtils.stringValue("$.bar2"));
358-
Expression pathExpr3 = DSL.literal(ExprValueUtils.stringValue("$.potato"));
359-
Expression jsonExpr = DSL.literal(ExprValueUtils.stringValue(objectJson));
360-
ExprValue actual = DSL.jsonExtract(jsonExpr, pathExpr1, pathExpr2, pathExpr3).valueOf();
361-
assertEquals(expected, actual);
356+
357+
assert_equals_extract_json(expected_scalar_results, objectJson, "$.foo", "$.bar2", "$.potato");
358+
359+
ExprValue expected_multivalued_results =
360+
new ExprCollectionValue(
361+
List.of(
362+
new ExprCollectionValue(
363+
List.of(new ExprStringValue("string"), LITERAL_TRUE, LITERAL_NULL)),
364+
ExprTupleValue.fromExprValueMap(Map.of("internal", new ExprStringValue("value"))),
365+
new ExprFloatValue(12.34)));
366+
367+
// path returns array and struct
368+
assert_equals_extract_json(
369+
expected_multivalued_results, objectJson, "$.arr", "$.obj", "$.bar2");
370+
371+
// path returns multivalued result
372+
assert_equals_extract_json(
373+
expected_multivalued_results, objectJson, "$.arr[*]", "$.obj", "$.bar2");
362374
}
363375

364-
private static void execute_extract_json(ExprValue expected, String json, String path) {
365-
Expression pathExpr = DSL.literal(ExprValueUtils.stringValue(path));
366-
Expression jsonExpr = DSL.literal(ExprValueUtils.stringValue(json));
367-
ExprValue actual = DSL.jsonExtract(jsonExpr, pathExpr).valueOf();
376+
private static void assert_equals_extract_json(ExprValue expected, Object json, Object... paths) {
377+
ExprValue actual = execute_extract_json(json, paths);
368378
assertEquals(expected, actual);
369379
}
380+
381+
private static <T extends Throwable> void assert_throws_extract_json(
382+
Class<T> expectedError, Object json, Object... paths) {
383+
assertThrows(expectedError, () -> execute_extract_json(json, paths));
384+
}
385+
386+
private static ExprValue execute_extract_json(Object json, Object[] paths) {
387+
Expression jsonExpr = object_to_expr(json);
388+
List<Expression> pathExpressions =
389+
Arrays.stream(paths).map(JsonFunctionsTest::object_to_expr).toList();
390+
391+
return switch (paths.length) {
392+
case 1 -> DSL.jsonExtract(jsonExpr, pathExpressions.getFirst()).valueOf();
393+
case 2 -> DSL.jsonExtract(jsonExpr, pathExpressions.getFirst(), pathExpressions.get(1))
394+
.valueOf();
395+
case 3 -> DSL.jsonExtract(
396+
jsonExpr, pathExpressions.getFirst(), pathExpressions.get(1), pathExpressions.get(2))
397+
.valueOf();
398+
default -> throw new TestInstantiationException("Invalid number of paths provided.");
399+
};
400+
}
401+
402+
private static Expression object_to_expr(Object val) {
403+
return (val instanceof String)
404+
? DSL.literal(ExprValueUtils.stringValue((String) val))
405+
: DSL.literal((ExprValue) val);
406+
}
370407
}

‎docs/user/ppl/functions/json.rst

+7-6
Original file line numberDiff line numberDiff line change
@@ -69,17 +69,18 @@ ____________
6969
Description
7070
>>>>>>>>>>>
7171

72-
Usage: `json_extract(doc, path[, path])` Extracts a JSON value from a json document based on the path specified.
72+
Usage: `json_extract(doc, path[, path[, path])` Extracts a JSON value from a json document based on the path(s) specified.
7373

74-
Argument type: STRING, STRING
74+
Argument type: STRING, STRING[, STRING[, STRING]]
7575

7676
Return type: STRING/BOOLEAN/DOUBLE/INTEGER/NULL/STRUCT/ARRAY
7777

78-
- Up to 3 paths can be provided, and results of each `path` with be returned in an ARRAY.
79-
- Returns an ARRAY if `path` points to multiple results (e.g. $.a[*]) or if the `path` points to an array.
78+
- Up to 3 paths can be provided, and results of all possible `path`s will be returned in an ARRAY.
79+
- If only one `path` is provided, returns an ARRAY if `path` points to multiple results (e.g. $.a[*]) or if the `path` points to an array.
8080
- Return null if `path` is not valid, or if JSON `doc` is MISSING or NULL.
81-
- Throws SemanticCheckException if `doc` or `path` is malformed.
82-
- Throws ExpressionEvaluationException if `path` is missing.
81+
- If multiple paths are provided with paths that are not valid, will return an ARRAY where results of invalid paths are null.
82+
- Throws SemanticCheckException if `doc` or any `path` is malformed.
83+
- Throws ExpressionEvaluationException if any `path` is missing.
8384

8485
Example::
8586

‎integ-test/src/test/java/org/opensearch/sql/ppl/JsonFunctionsIT.java

+85
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@
1717
import org.json.JSONArray;
1818
import org.json.JSONObject;
1919
import org.junit.jupiter.api.Test;
20+
import org.opensearch.client.ResponseException;
2021

2122
public class JsonFunctionsIT extends PPLIntegTestCase {
2223
@Override
@@ -130,6 +131,20 @@ public void test_json() throws IOException {
130131
new JSONObject(Map.of("a", "1", "b", List.of(Map.of("c", "2"), Map.of("c", "3"))))));
131132
}
132133

134+
@Test
135+
public void test_json_throws_SemanticCheckException() throws IOException {
136+
// Invalid json provided
137+
try {
138+
executeQuery(
139+
String.format(
140+
"source=%s | where not json_valid(json_string) | eval"
141+
+ " casted=json(json_string) | fields test_name, casted",
142+
TEST_INDEX_JSON_TEST));
143+
} catch (ResponseException invalidJsonException) {
144+
assertTrue(invalidJsonException.getMessage().contains("SemanticCheckException"));
145+
}
146+
}
147+
133148
@Test
134149
public void test_cast_json_scalar_to_type() throws IOException {
135150
// cast to integer
@@ -217,4 +232,74 @@ public void test_json_extract() throws IOException {
217232
rows("json empty string", null),
218233
rows("json nested list", new JSONArray(List.of(Map.of("c", "2"), Map.of("c", "3")))));
219234
}
235+
236+
@Test
237+
public void test_json_extract_multiple_paths() throws IOException {
238+
JSONObject resultTwoPaths =
239+
executeQuery(
240+
String.format(
241+
"source=%s | where test_name = 'json nested list' | eval"
242+
+ " extracted=json_extract(json_string, '$.a', '$.b') | fields test_name,"
243+
+ " extracted",
244+
TEST_INDEX_JSON_TEST));
245+
verifySchema(
246+
resultTwoPaths,
247+
schema("test_name", null, "string"),
248+
schema("extracted", null, "undefined"));
249+
verifyDataRows(
250+
resultTwoPaths,
251+
rows(
252+
"json nested list",
253+
List.of("1", new JSONArray(List.of(Map.of("c", "2"), Map.of("c", "3"))))));
254+
255+
JSONObject resultThreePaths =
256+
executeQuery(
257+
String.format(
258+
"source=%s | where test_name = 'json nested list' | eval"
259+
+ " extracted=json_extract(json_string, '$.a', '$.b[0].c', '$.b[1].c') | fields"
260+
+ " test_name, extracted",
261+
TEST_INDEX_JSON_TEST));
262+
verifySchema(
263+
resultThreePaths,
264+
schema("test_name", null, "string"),
265+
schema("extracted", null, "undefined"));
266+
verifyDataRows(resultThreePaths, rows("json nested list", List.of("1", "2", "3")));
267+
}
268+
269+
@Test
270+
public void test_json_extract_throws_SemanticCheckException() throws IOException {
271+
// Invalid json provided
272+
try {
273+
executeQuery(
274+
String.format(
275+
"source=%s | where not json_valid(json_string) | eval"
276+
+ " extracted=json_extract(json_string, '$.a') | fields test_name, extracted",
277+
TEST_INDEX_JSON_TEST));
278+
} catch (ResponseException invalidJsonException) {
279+
assertTrue(invalidJsonException.getMessage().contains("SemanticCheckException"));
280+
}
281+
282+
// Invalid path provided
283+
try {
284+
executeQuery(
285+
String.format(
286+
"source=%s | where test_name = 'json nested list' | eval"
287+
+ " extracted=json_extract(json_string, '$a') | fields test_name, extracted",
288+
TEST_INDEX_JSON_TEST));
289+
} catch (ResponseException invalidPathException) {
290+
assertTrue(invalidPathException.getMessage().contains("SemanticCheckException"));
291+
}
292+
293+
// Invalid path with multiple paths provided
294+
try {
295+
executeQuery(
296+
String.format(
297+
"source=%s | where test_name = 'json nested list' | eval"
298+
+ " extracted=json_extract(json_string, '$a', '$.b') | fields test_name,"
299+
+ " extracted",
300+
TEST_INDEX_JSON_TEST));
301+
} catch (ResponseException invalidPathException) {
302+
assertTrue(invalidPathException.getMessage().contains("SemanticCheckException"));
303+
}
304+
}
220305
}

0 commit comments

Comments
 (0)