jackjlli commented on a change in pull request #6651:
URL: https://github.com/apache/incubator-pinot/pull/6651#discussion_r589672613
##########
File path:
pinot-core/src/test/java/org/apache/pinot/queries/JsonMatchPredicateTest.java
##########
@@ -282,6 +282,36 @@ public void testJsonMatchArrayWithIndex() {
Assert.assertEquals(iterator.next()[0], "goofy");
}
+ /** Evaluate json_extract_scalar over string column that does not contain
valid json data. */
+ @Test
+ public void testJsonExtractScalarAgainstInvalidJson() {
+ // json_extract_scalar throws exception since we are trying to parse a
non-JSON string.
+ Operator operator1 = getOperatorForSqlQuery(
+ "select count(*) FROM testTable WHERE
json_extract_scalar(stringColumn, '$.name.first', 'INT') = 0");
+ try {
+ IntermediateResultsBlock block1 = (IntermediateResultsBlock)
operator1.nextBlock();
+ Assert.assertTrue(false);
Review comment:
`Assert.assertFail("Your expected error message.")`
##########
File path:
pinot-core/src/test/java/org/apache/pinot/queries/JsonMatchPredicateTest.java
##########
@@ -282,6 +282,36 @@ public void testJsonMatchArrayWithIndex() {
Assert.assertEquals(iterator.next()[0], "goofy");
}
+ /** Evaluate json_extract_scalar over string column that does not contain
valid json data. */
+ @Test
+ public void testJsonExtractScalarAgainstInvalidJson() {
+ // json_extract_scalar throws exception since we are trying to parse a
non-JSON string.
+ Operator operator1 = getOperatorForSqlQuery(
+ "select count(*) FROM testTable WHERE
json_extract_scalar(stringColumn, '$.name.first', 'INT') = 0");
+ try {
+ IntermediateResultsBlock block1 = (IntermediateResultsBlock)
operator1.nextBlock();
+ Assert.assertTrue(false);
+ } catch (RuntimeException re) {
+ Assert.assertEquals(re.toString(),
+ "java.lang.RuntimeException: Illegal Json Path: [$.name.first], when
reading [daffy duck]");
+ }
+
+ // JSON data is stored in columns of type STRING, so there is nothing
preventing the column from storing bad json
+ // string. Bad JSON string in columns will cause json_extract_scalar to
throw an exception which would terminate
+ // query processing. However, when json_extract_scalar is used within the
WHERE clause, we should return the
+ // default value instead of throwing exception. This will allow the
predicate to be evaluated to either true or
+ // false and hence allow the query to complete successfully. Returning
default value from json_extract_scalar is
+ // an undocumented feature. Ideally, json_extract_scalar should return
NULL in when it encounters bad JSON. However,
+ // NULL support is currently pending, so this is the best we can do.
+ Operator operator2 = getOperatorForSqlQuery(
+ "select count(*) FROM testTable WHERE
json_extract_scalar(stringColumn, '$.name.first', 'INT', 0) = 0");
+
+ IntermediateResultsBlock block2 = (IntermediateResultsBlock)
operator2.nextBlock();
+ Collection<Object[]> rows = block2.getSelectionResult();
+
+ Assert.assertEquals(block2.getAggregationResult().get(0), 9l);
Review comment:
Nit: `9L`
##########
File path:
pinot-core/src/test/java/org/apache/pinot/queries/JsonMatchPredicateTest.java
##########
@@ -282,6 +282,36 @@ public void testJsonMatchArrayWithIndex() {
Assert.assertEquals(iterator.next()[0], "goofy");
}
+ /** Evaluate json_extract_scalar over string column that does not contain
valid json data. */
+ @Test
+ public void testJsonExtractScalarAgainstInvalidJson() {
+ // json_extract_scalar throws exception since we are trying to parse a
non-JSON string.
+ Operator operator1 = getOperatorForSqlQuery(
+ "select count(*) FROM testTable WHERE
json_extract_scalar(stringColumn, '$.name.first', 'INT') = 0");
+ try {
+ IntermediateResultsBlock block1 = (IntermediateResultsBlock)
operator1.nextBlock();
+ Assert.assertTrue(false);
+ } catch (RuntimeException re) {
+ Assert.assertEquals(re.toString(),
Review comment:
It'd be good not to validate the actual message, as the message may
evolve.
##########
File path:
pinot-core/src/test/java/org/apache/pinot/queries/JsonMatchPredicateTest.java
##########
@@ -282,6 +282,36 @@ public void testJsonMatchArrayWithIndex() {
Assert.assertEquals(iterator.next()[0], "goofy");
}
+ /** Evaluate json_extract_scalar over string column that does not contain
valid json data. */
+ @Test
+ public void testJsonExtractScalarAgainstInvalidJson() {
+ // json_extract_scalar throws exception since we are trying to parse a
non-JSON string.
+ Operator operator1 = getOperatorForSqlQuery(
+ "select count(*) FROM testTable WHERE
json_extract_scalar(stringColumn, '$.name.first', 'INT') = 0");
+ try {
+ IntermediateResultsBlock block1 = (IntermediateResultsBlock)
operator1.nextBlock();
+ Assert.assertTrue(false);
+ } catch (RuntimeException re) {
+ Assert.assertEquals(re.toString(),
+ "java.lang.RuntimeException: Illegal Json Path: [$.name.first], when
reading [daffy duck]");
+ }
+
+ // JSON data is stored in columns of type STRING, so there is nothing
preventing the column from storing bad json
+ // string. Bad JSON string in columns will cause json_extract_scalar to
throw an exception which would terminate
+ // query processing. However, when json_extract_scalar is used within the
WHERE clause, we should return the
+ // default value instead of throwing exception. This will allow the
predicate to be evaluated to either true or
+ // false and hence allow the query to complete successfully. Returning
default value from json_extract_scalar is
+ // an undocumented feature. Ideally, json_extract_scalar should return
NULL in when it encounters bad JSON. However,
+ // NULL support is currently pending, so this is the best we can do.
+ Operator operator2 = getOperatorForSqlQuery(
+ "select count(*) FROM testTable WHERE
json_extract_scalar(stringColumn, '$.name.first', 'INT', 0) = 0");
+
+ IntermediateResultsBlock block2 = (IntermediateResultsBlock)
operator2.nextBlock();
+ Collection<Object[]> rows = block2.getSelectionResult();
+
+ Assert.assertEquals(block2.getAggregationResult().get(0), 9l);
Review comment:
And it might be good to add a comment to explain what 9L denotes, like
what count you're trying to fetch here?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
