Skip to content

Commit 19d3ea5

Browse files
richardstartinrichardstartin
committed
upgrade JSONPath dependency and tweak logic and tests which depend on exceptions being thrown
1 parent 525a306 commit 19d3ea5

File tree

6 files changed

+97
-92
lines changed

6 files changed

+97
-92
lines changed

‎LICENSE-binary‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -241,7 +241,7 @@ com.google.http-client:google-http-client-jackson2:1.32.1
241241
com.google.http-client:google-http-client:1.32.1
242242
com.google.j2objc:j2objc-annotations:1.3
243243
com.google.oauth-client:google-oauth-client:1.30.3
244-
com.jayway.jsonpath:json-path:2.4.0
244+
com.jayway.jsonpath:json-path:2.7.0
245245
com.lmax:disruptor:3.3.4
246246
com.ning:async-http-client:1.9.21
247247
com.squareup.okio:okio:1.6.0

‎pinot-common/src/main/java/org/apache/pinot/common/function/scalar/JsonFunctions.java‎

Lines changed: 27 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -51,6 +51,7 @@ public class JsonFunctions {
5151
private JsonFunctions() {
5252
}
5353

54+
private static final Object[] EMPTY = new Object[0];
5455
private static final Predicate[] NO_PREDICATES = new Predicate[0];
5556
private static final ParseContext PARSE_CONTEXT = JsonPath.using(
5657
new Configuration.ConfigurationBuilder().jsonProvider(new ArrayAwareJacksonJsonProvider())
@@ -95,8 +96,7 @@ public static Object jsonPath(Object object, String jsonPath) {
9596
* Extract object array based on Json path
9697
*/
9798
@ScalarFunction
98-
public static Object[] jsonPathArray(Object object, String jsonPath)
99-
throws JsonProcessingException {
99+
public static Object[] jsonPathArray(Object object, String jsonPath) {
100100
if (object instanceof String) {
101101
return convertObjectToArray(PARSE_CONTEXT.parse((String) object).read(jsonPath, NO_PREDICATES));
102102
}
@@ -105,18 +105,17 @@ public static Object[] jsonPathArray(Object object, String jsonPath)
105105

106106
@ScalarFunction
107107
public static Object[] jsonPathArrayDefaultEmpty(Object object, String jsonPath) {
108-
try {
109-
return jsonPathArray(object, jsonPath);
110-
} catch (Exception e) {
111-
return new Object[0];
112-
}
108+
Object[] result = jsonPathArray(object, jsonPath);
109+
return result == null ? EMPTY : result;
113110
}
114111

115112
private static Object[] convertObjectToArray(Object arrayObject) {
116113
if (arrayObject instanceof List) {
117114
return ((List) arrayObject).toArray();
118115
} else if (arrayObject instanceof Object[]) {
119116
return (Object[]) arrayObject;
117+
} else if (arrayObject == null) {
118+
return null;
120119
}
121120
return new Object[]{arrayObject};
122121
}
@@ -131,17 +130,21 @@ public static String jsonPathString(Object object, String jsonPath)
131130
if (jsonValue instanceof String) {
132131
return (String) jsonValue;
133132
}
134-
return JsonUtils.objectToString(jsonValue);
133+
return jsonValue == null ? null : JsonUtils.objectToString(jsonValue);
135134
}
136135

137136
/**
138137
* Extract from Json with path to String
139138
*/
140139
@ScalarFunction
141140
public static String jsonPathString(Object object, String jsonPath, String defaultValue) {
141+
Object jsonValue = jsonPath(object, jsonPath);
142+
if (jsonValue instanceof String) {
143+
return (String) jsonValue;
144+
}
142145
try {
143-
return jsonPathString(object, jsonPath);
144-
} catch (Exception e) {
146+
return jsonValue == null ? defaultValue : JsonUtils.objectToString(jsonValue);
147+
} catch (JsonProcessingException e) {
145148
return defaultValue;
146149
}
147150
}
@@ -151,50 +154,45 @@ public static String jsonPathString(Object object, String jsonPath, String defau
151154
*/
152155
@ScalarFunction
153156
public static long jsonPathLong(Object object, String jsonPath) {
154-
final Object jsonValue = jsonPath(object, jsonPath);
155-
if (jsonValue == null) {
156-
return Long.MIN_VALUE;
157-
}
158-
if (jsonValue instanceof Number) {
159-
return ((Number) jsonValue).longValue();
160-
}
161-
return Long.parseLong(jsonValue.toString());
157+
return jsonPathLong(object, jsonPath, Long.MIN_VALUE);
162158
}
163159

164160
/**
165161
* Extract from Json with path to Long
166162
*/
167163
@ScalarFunction
168164
public static long jsonPathLong(Object object, String jsonPath, long defaultValue) {
169-
try {
170-
return jsonPathLong(object, jsonPath);
171-
} catch (Exception e) {
165+
Object jsonValue = jsonPath(object, jsonPath);
166+
if (jsonValue == null) {
172167
return defaultValue;
173168
}
169+
if (jsonValue instanceof Number) {
170+
return ((Number) jsonValue).longValue();
171+
}
172+
return Long.parseLong(jsonValue.toString());
174173
}
175174

176175
/**
177176
* Extract from Json with path to Double
178177
*/
179178
@ScalarFunction
180179
public static double jsonPathDouble(Object object, String jsonPath) {
181-
final Object jsonValue = jsonPath(object, jsonPath);
182-
if (jsonValue instanceof Number) {
183-
return ((Number) jsonValue).doubleValue();
184-
}
185-
return Double.parseDouble(jsonValue.toString());
180+
return jsonPathDouble(object, jsonPath, Double.NaN);
186181
}
187182

188183
/**
189184
* Extract from Json with path to Double
190185
*/
191186
@ScalarFunction
192187
public static double jsonPathDouble(Object object, String jsonPath, double defaultValue) {
193-
try {
194-
return jsonPathDouble(object, jsonPath);
195-
} catch (Exception e) {
188+
Object jsonValue = jsonPath(object, jsonPath);
189+
if (jsonValue == null) {
196190
return defaultValue;
197191
}
192+
if (jsonValue instanceof Number) {
193+
return ((Number) jsonValue).doubleValue();
194+
}
195+
return Double.parseDouble(jsonValue.toString());
198196
}
199197

200198
@VisibleForTesting

‎pinot-common/src/test/java/org/apache/pinot/common/function/JsonFunctionsTest.java‎

Lines changed: 59 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -19,18 +19,20 @@
1919
package org.apache.pinot.common.function;
2020

2121
import com.fasterxml.jackson.core.JsonProcessingException;
22+
import com.fasterxml.jackson.databind.ObjectMapper;
2223
import com.google.common.collect.ImmutableList;
2324
import com.google.common.collect.ImmutableMap;
24-
import com.jayway.jsonpath.PathNotFoundException;
2525
import java.util.ArrayList;
2626
import java.util.Arrays;
2727
import java.util.List;
2828
import java.util.Map;
2929
import org.apache.pinot.common.function.scalar.JsonFunctions;
30+
import org.testng.annotations.DataProvider;
3031
import org.testng.annotations.Test;
3132

3233
import static org.testng.Assert.assertEquals;
33-
import static org.testng.Assert.fail;
34+
import static org.testng.Assert.assertNull;
35+
import static org.testng.Assert.assertTrue;
3436

3537

3638
public class JsonFunctionsTest {
@@ -74,7 +76,9 @@ public void testJsonFunction()
7476
assertEquals(JsonFunctions.jsonPathDouble(jsonString, "$.actor.id"), 33500718.0);
7577
assertEquals(JsonFunctions.jsonPathString(jsonString, "$.actor.aaa", "null"), "null");
7678
assertEquals(JsonFunctions.jsonPathLong(jsonString, "$.actor.aaa", 100L), 100L);
79+
assertEquals(JsonFunctions.jsonPathLong(jsonString, "$.actor.aaa"), Long.MIN_VALUE);
7780
assertEquals(JsonFunctions.jsonPathDouble(jsonString, "$.actor.aaa", 53.2), 53.2);
81+
assertTrue(Double.isNaN(JsonFunctions.jsonPathDouble(jsonString, "$.actor.aaa")));
7882
}
7983

8084
@Test
@@ -111,12 +115,7 @@ public void testJsonFunctionExtractingArrayWithMissingField()
111115
throws JsonProcessingException {
112116
String jsonString = "{\"name\": \"Pete\", \"age\": 24}";
113117

114-
try {
115-
assertEquals(JsonFunctions.jsonPathArray(jsonString, "$.subjects[*].name"), new String[]{});
116-
fail();
117-
} catch (PathNotFoundException e) {
118-
assertEquals(e.getMessage(), "Missing property in path $['subjects']");
119-
}
118+
assertEquals(JsonFunctions.jsonPathArray(jsonString, "$.subjects[*].name"), new String[]{});
120119
assertEquals(JsonFunctions.jsonPathArrayDefaultEmpty(jsonString, "$.subjects[*].name"), new String[]{});
121120
assertEquals(JsonFunctions.jsonPathArrayDefaultEmpty(jsonString, "$.subjects[*].grade"), new String[]{});
122121
assertEquals(JsonFunctions.jsonPathArrayDefaultEmpty(jsonString, "$.subjects[*].homework_grades"), new Object[]{});
@@ -245,4 +244,56 @@ public void testJsonFunctionOnObjectArray()
245244
new Object[]{Arrays.asList(80, 85, 90, 95, 100), Arrays.asList(60, 65, 70, 85, 90)});
246245
assertEquals(JsonFunctions.jsonPathArray(rawData, "$.[*].score"), new Integer[]{90, 50});
247246
}
247+
248+
private static final ObjectMapper OBJECT_MAPPER = new ObjectMapper();
249+
250+
@DataProvider
251+
public static Object[][] jsonPathStringTestCases() {
252+
return new Object[][]{
253+
{ImmutableMap.of("foo", "x", "bar", ImmutableMap.of("foo", "y")), "$.foo", "x"},
254+
{ImmutableMap.of("foo", "x", "bar", ImmutableMap.of("foo", "y")), "$.qux", null},
255+
{ImmutableMap.of("foo", "x", "bar", ImmutableMap.of("foo", "y")), "$.bar", "{\"foo\":\"y\"}"},
256+
};
257+
}
258+
259+
@Test(dataProvider = "jsonPathStringTestCases")
260+
public void testJsonPathString(Map<String, Object> map, String path, String expected)
261+
throws JsonProcessingException {
262+
String value = JsonFunctions.jsonPathString(OBJECT_MAPPER.writeValueAsString(map), path);
263+
assertEquals(value, expected);
264+
}
265+
266+
@Test(dataProvider = "jsonPathStringTestCases")
267+
public void testJsonPathStringWithDefaultValue(Map<String, Object> map, String path, String expected)
268+
throws JsonProcessingException {
269+
String value = JsonFunctions.jsonPathString(OBJECT_MAPPER.writeValueAsString(map), path, expected);
270+
assertEquals(value, expected);
271+
}
272+
273+
@DataProvider
274+
public static Object[][] jsonPathArrayTestCases() {
275+
return new Object[][]{
276+
{ImmutableMap.of("foo", "x", "bar", ImmutableMap.of("foo", "y")), "$.foo", new Object[]{"x"}},
277+
{ImmutableMap.of("foo", "x", "bar", ImmutableMap.of("foo", "y")), "$.qux", null},
278+
{
279+
ImmutableMap.of("foo", "x", "bar", ImmutableMap.of("foo", "y")), "$.bar", new Object[]{
280+
ImmutableMap.of("foo", "y")
281+
}
282+
},
283+
};
284+
}
285+
286+
@Test(dataProvider = "jsonPathArrayTestCases")
287+
public void testJsonPathArray(Map<String, Object> map, String path, Object[] expected)
288+
throws JsonProcessingException {
289+
Object[] value = JsonFunctions.jsonPathArray(OBJECT_MAPPER.writeValueAsString(map), path);
290+
if (expected == null) {
291+
assertNull(value);
292+
} else {
293+
assertEquals(value.length, expected.length);
294+
for (int i = 0; i < value.length; i++) {
295+
assertEquals(value[i], expected[i]);
296+
}
297+
}
298+
}
248299
}

‎pinot-common/src/test/java/org/apache/pinot/common/function/scalar/ArrayAwareJacksonJsonProviderTest.java‎

Lines changed: 5 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@
1818
*/
1919
package org.apache.pinot.common.function.scalar;
2020

21+
import com.jayway.jsonpath.JsonPathException;
2122
import java.util.ArrayList;
2223
import java.util.Arrays;
2324
import java.util.HashMap;
@@ -48,15 +49,11 @@ public void testArrayLength() {
4849

4950
List<Object> dataInList = Arrays.asList("abc", "efg", "hij");
5051
assertEquals(jp.length(dataInList), 3);
52+
}
5153

52-
try {
53-
jp.length(null);
54-
fail();
55-
} catch (NullPointerException e) {
56-
// It's supposed to get a JsonPathException, but JsonPath library actually
57-
// has a bug leading to NullPointerException while creating the JsonPathException.
58-
assertNull(e.getMessage());
59-
}
54+
@Test(expectedExceptions = JsonPathException.class)
55+
public void testArrayLengthThrowsForNullArray() {
56+
new JsonFunctions.ArrayAwareJacksonJsonProvider().length(null);
6057
}
6158

6259
@Test

‎pinot-core/src/main/java/org/apache/pinot/core/common/evaluators/DefaultJsonPathEvaluator.java‎

Lines changed: 4 additions & 45 deletions
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,6 @@
2525
import com.jayway.jsonpath.ParseContext;
2626
import com.jayway.jsonpath.spi.json.JacksonJsonProvider;
2727
import com.jayway.jsonpath.spi.mapper.JacksonMappingProvider;
28-
import java.nio.charset.StandardCharsets;
2928
import java.util.List;
3029
import javax.annotation.Nullable;
3130
import org.apache.pinot.common.function.JsonPathCache;
@@ -390,57 +389,21 @@ public <T extends ForwardIndexReaderContext> void evaluateBlock(int[] docIds, in
390389
}
391390

392391
private <T> T extractFromBytes(Dictionary dictionary, int dictId) {
393-
try {
394-
// TODO make JsonPath accept byte[] - Jackson can
395-
return JSON_PARSER_CONTEXT.parse(new String(dictionary.getBytesValue(dictId), StandardCharsets.UTF_8))
396-
.read(_jsonPath);
397-
} catch (Exception e) {
398-
// TODO JsonPath 2.7.0 will not throw here but produce null when path not found
399-
if (_defaultValue == null) {
400-
throwPathNotFoundException(e);
401-
}
402-
return null;
403-
}
392+
return JSON_PARSER_CONTEXT.parseUtf8(dictionary.getBytesValue(dictId)).read(_jsonPath);
404393
}
405394

406395
private <T, R extends ForwardIndexReaderContext> T extractFromBytes(ForwardIndexReader<R> reader, R context,
407396
int docId) {
408-
try {
409-
// TODO make JsonPath accept byte[] - Jackson can
410-
return JSON_PARSER_CONTEXT.parse(new String(reader.getBytes(docId, context), StandardCharsets.UTF_8))
411-
.read(_jsonPath);
412-
} catch (Exception e) {
413-
// TODO JsonPath 2.7.0 will not throw here but produce null when path not found
414-
if (_defaultValue == null) {
415-
throwPathNotFoundException(e);
416-
}
417-
return null;
418-
}
397+
return JSON_PARSER_CONTEXT.parseUtf8(reader.getBytes(docId, context)).read(_jsonPath);
419398
}
420399

421400
private <T> T extractFromString(Dictionary dictionary, int dictId) {
422-
try {
423-
return JSON_PARSER_CONTEXT.parse(dictionary.getStringValue(dictId)).read(_jsonPath);
424-
} catch (Exception e) {
425-
// TODO JsonPath 2.7.0 will not throw here but produce null when path not found
426-
if (_defaultValue == null) {
427-
throwPathNotFoundException(e);
428-
}
429-
return null;
430-
}
401+
return JSON_PARSER_CONTEXT.parse(dictionary.getStringValue(dictId)).read(_jsonPath);
431402
}
432403

433404
private <T, R extends ForwardIndexReaderContext> T extractFromString(ForwardIndexReader<R> reader, R context,
434405
int docId) {
435-
try {
436-
return JSON_PARSER_CONTEXT.parse(reader.getString(docId, context)).read(_jsonPath);
437-
} catch (Exception e) {
438-
// TODO JsonPath 2.7.0 will not throw here but produce null when path not found
439-
if (_defaultValue == null) {
440-
throwPathNotFoundException(e);
441-
}
442-
return null;
443-
}
406+
return JSON_PARSER_CONTEXT.parseUtf8(reader.getBytes(docId, context)).read(_jsonPath);
444407
}
445408

446409
private void processValue(int index, Object value, int defaultValue, int[] valueBuffer) {
@@ -582,8 +545,4 @@ private void processList(int index, List<String> value, String[][] valuesBuffer)
582545
private void throwPathNotFoundException() {
583546
throw new IllegalArgumentException("Illegal Json Path: " + _jsonPath.getPath() + " does not match document");
584547
}
585-
586-
private void throwPathNotFoundException(Exception e) {
587-
throw new IllegalArgumentException("Illegal Json Path: " + _jsonPath.getPath() + " does not match document", e);
588-
}
589548
}

‎pom.xml‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -130,7 +130,7 @@
130130
<hadoop.version>2.7.0</hadoop.version>
131131
<scala.version>2.13.3</scala.version>
132132
<antlr.version>4.6</antlr.version>
133-
<jsonpath.version>2.4.0</jsonpath.version>
133+
<jsonpath.version>2.7.0</jsonpath.version>
134134
<quartz.version>2.3.2</quartz.version>
135135
<calcite.version>1.19.0</calcite.version>
136136
<lucene.version>8.2.0</lucene.version>

0 commit comments

Comments
 (0)