skip invalid json string rather than throwing error during json indexing #12238

wirybeaver · 2024-01-08T17:21:56Z

Add a new configuration to skip broken json string and continue indexing following json strings instead of throw exception.

codecov-commenter · 2024-01-08T18:09:05Z

Codecov Report

Attention: 5 lines in your changes are missing coverage. Please review.

Comparison is base (640ebe5) 61.58% compared to head (fc5a157) 61.73%.
Report is 48 commits behind head on master.

Files	Patch %	Lines
...apache/pinot/spi/config/table/JsonIndexConfig.java	50.00%	3 Missing ⚠️
...ain/java/org/apache/pinot/spi/utils/JsonUtils.java	77.77%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             master   #12238      +/-   ##
============================================
+ Coverage     61.58%   61.73%   +0.14%     
+ Complexity     1153      207     -946     
============================================
  Files          2415     2425      +10     
  Lines        131502   132728    +1226     
  Branches      20310    20533     +223     
============================================
+ Hits          80988    81940     +952     
- Misses        44587    44770     +183     
- Partials       5927     6018      +91

Flag	Coverage Δ
custom-integration1	`<0.01% <0.00%> (-0.01%)`	⬇️
integration	`<0.01% <0.00%> (-0.01%)`	⬇️
integration1	`<0.01% <0.00%> (-0.01%)`	⬇️
integration2	`0.00% <0.00%> (ø)`
java-11	`61.69% <70.58%> (+0.18%)`	⬆️
java-21	`61.60% <70.58%> (+0.13%)`	⬆️
skip-bytebuffers-false	`61.71% <70.58%> (+0.16%)`	⬆️
skip-bytebuffers-true	`61.58% <70.58%> (+0.14%)`	⬆️
temurin	`61.73% <70.58%> (+0.14%)`	⬆️
unittests	`61.73% <70.58%> (+0.14%)`	⬆️
unittests1	`46.88% <58.82%> (+0.26%)`	⬆️
unittests2	`27.75% <11.76%> (-0.05%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

ankitsultana · 2024-01-10T21:14:55Z

pinot-spi/src/main/java/org/apache/pinot/spi/utils/JsonUtils.java

+    JsonNode jsonNode;
+    try {
+      jsonNode = JsonUtils.stringToJsonNode(jsonString);
+    } catch (IOException e) {


Why limit to IOException? Can we catch a Throwable here?

i think the actual exception here we supposed to cathc is malform json exception only. all other issues should still throw --> e..g we should not return incorrect result or simply replace that with the default when there are other issues.

Thanks for the explanation @walterddr ,

@ankitsultana @walterddr
I change the source catch JsonProcessingException in the most recent commit because the stringToJsonNode internal invoke the DEFAULT_READER.readTree() which throws JsonProcessingException only.

JsonUtils.stringToJsonNode(...) throws IOException { DEFAULT_READER.readTree(...); } class ObjectReader { public JsonNode readTree(String json) throws JsonProcessingException, JsonMappingException; }

JsonMappingException is a child of JsonProcessingException.

pinot-spi/src/main/java/org/apache/pinot/spi/utils/JsonUtils.java

walterddr · 2024-01-11T01:21:45Z

pinot-spi/src/main/java/org/apache/pinot/spi/utils/JsonUtils.java

+    JsonNode jsonNode;
+    try {
+      jsonNode = JsonUtils.stringToJsonNode(jsonString);
+    } catch (IOException e) {


i think the actual exception here we supposed to cathc is malform json exception only. all other issues should still throw --> e..g we should not return incorrect result or simply replace that with the default when there are other issues.

walterddr

lgtm mostly. one more comment on the testing.

walterddr · 2024-01-16T19:05:52Z

...-segment-local/src/test/java/org/apache/pinot/segment/local/segment/index/JsonIndexTest.java

+  @Test
+  public void testSkipInvalidJson() throws Exception {
+    JsonIndexConfig jsonIndexConfig = new JsonIndexConfig();
+    jsonIndexConfig.setSkipInvalidJson(true);


can you

add a test when it is false it still throws?

add a test for other exception is still throws when this is set to true?

can you

add a test when it is false it still throws?

add a test for other exception is still throws when this is set to true?

for the 2nd requirement, I haven't figure a way to test since the JsonUtils make the objectReader as a static final member and I cannot replace a static final member without changing the constructor signature or not using more powerful mock libraray.

public static final ObjectReader DEFAULT_READER = DEFAULT_MAPPER.reader();

sorry for the late reply. that fine if we can't find it.

wirybeaver · 2024-01-24T20:55:56Z

Deployed this change to the company's prod cluster with following config. The ERROR segments containing invalid json rows become ONLINE.

    "fieldConfigList": [
      {
        "name": "foo",
        "encodingType": "RAW",
        "indexTypes": [],
        "compressionCodec": "ZSTANDARD",
        "indexes": {
          "json": {
            "maxLevels": 2,
            "disableCrossArrayUnnest": true,
            "skipInvalidJson": true
          },
          "text": {}
        },
        "properties": {
          "deriveNumDocsPerChunkForRawIndex": "true",
          "rawIndexWriterVersion": "3",
        },
        "tierOverwrites": null
      }
    ]

...-segment-local/src/test/java/org/apache/pinot/segment/local/segment/index/JsonIndexTest.java

pinot-spi/src/main/java/org/apache/pinot/spi/utils/JsonUtils.java

ankitsultana

Apologies. Had stamped but had a question. @wirybeaver can you take a look?

wirybeaver · 2024-02-07T23:36:31Z

Apologies. Had stamped but had a question. @wirybeaver can you take a look?

no worries

wirybeaver · 2024-02-08T02:20:53Z

@walterddr Could you review again to see if there's any objection?

ankitsultana · 2024-02-08T23:20:42Z

Merged since very minor changes since Rong's last review. (Rong is OOO for a week)

…ing (apache#12238)

skip invalid json string rather than throwing error during indexing

a60cf46

ankitsultana reviewed Jan 10, 2024

View reviewed changes

walterddr reviewed Jan 11, 2024

View reviewed changes

walterddr added the feature label Jan 11, 2024

wirybeaver added 2 commits January 14, 2024 16:20

fix format and add singleton variable

0786ea3

Merge remote-tracking branch 'fork/master' into skipInvalidJson

f9908c8

walterddr reviewed Jan 16, 2024

View reviewed changes

wirybeaver added 2 commits January 19, 2024 17:46

check whether it is a json processing exception

f746c58

Merge remote-tracking branch 'fork/master' into skipInvalidJson

fcbf41f

wirybeaver requested review from ankitsultana and walterddr January 24, 2024 21:03

walterddr approved these changes Jan 24, 2024

View reviewed changes

chenboat reviewed Jan 26, 2024

View reviewed changes

...-segment-local/src/test/java/org/apache/pinot/segment/local/segment/index/JsonIndexTest.java Outdated Show resolved Hide resolved

chenboat reviewed Jan 26, 2024

View reviewed changes

...-segment-local/src/test/java/org/apache/pinot/segment/local/segment/index/JsonIndexTest.java Outdated Show resolved Hide resolved

test default option

1249d9d

wirybeaver requested a review from chenboat February 3, 2024 01:00

ankitsultana approved these changes Feb 7, 2024

View reviewed changes

ankitsultana reviewed Feb 7, 2024

View reviewed changes

pinot-spi/src/main/java/org/apache/pinot/spi/utils/JsonUtils.java Show resolved Hide resolved

ankitsultana requested changes Feb 7, 2024

View reviewed changes

make the old flatten method protected

fc5a157

ankitsultana approved these changes Feb 8, 2024

View reviewed changes

ankitsultana merged commit 8684e04 into apache:master Feb 8, 2024
19 checks passed

suyashpatel98 pushed a commit to suyashpatel98/pinot that referenced this pull request Feb 28, 2024

Skip invalid json string rather than throwing error during json index…

74ff19c

…ing (apache#12238)

tibrewalpratik17 mentioned this pull request Mar 8, 2024

Add isJson UDF #12603

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

skip invalid json string rather than throwing error during json indexing #12238

skip invalid json string rather than throwing error during json indexing #12238

wirybeaver commented Jan 8, 2024

codecov-commenter commented Jan 8, 2024 •

edited

Loading

ankitsultana Jan 10, 2024

walterddr Jan 11, 2024

wirybeaver Jan 15, 2024

wirybeaver Jan 20, 2024 •

edited

Loading

walterddr Jan 11, 2024

walterddr left a comment

walterddr Jan 16, 2024

wirybeaver Jan 16, 2024

wirybeaver Jan 20, 2024 •

edited

Loading

walterddr Jan 24, 2024

wirybeaver commented Jan 24, 2024 •

edited

Loading

ankitsultana left a comment

wirybeaver commented Feb 7, 2024

wirybeaver commented Feb 8, 2024

ankitsultana commented Feb 8, 2024 •

edited

Loading

skip invalid json string rather than throwing error during json indexing #12238

skip invalid json string rather than throwing error during json indexing #12238

Conversation

wirybeaver commented Jan 8, 2024

codecov-commenter commented Jan 8, 2024 • edited Loading

Codecov Report

ankitsultana Jan 10, 2024

Choose a reason for hiding this comment

walterddr Jan 11, 2024

Choose a reason for hiding this comment

wirybeaver Jan 15, 2024

Choose a reason for hiding this comment

wirybeaver Jan 20, 2024 • edited Loading

Choose a reason for hiding this comment

walterddr Jan 11, 2024

Choose a reason for hiding this comment

walterddr left a comment

Choose a reason for hiding this comment

walterddr Jan 16, 2024

Choose a reason for hiding this comment

wirybeaver Jan 16, 2024

Choose a reason for hiding this comment

wirybeaver Jan 20, 2024 • edited Loading

Choose a reason for hiding this comment

walterddr Jan 24, 2024

Choose a reason for hiding this comment

wirybeaver commented Jan 24, 2024 • edited Loading

ankitsultana left a comment

Choose a reason for hiding this comment

wirybeaver commented Feb 7, 2024

wirybeaver commented Feb 8, 2024

ankitsultana commented Feb 8, 2024 • edited Loading

codecov-commenter commented Jan 8, 2024 •

edited

Loading

wirybeaver Jan 20, 2024 •

edited

Loading

wirybeaver Jan 20, 2024 •

edited

Loading

wirybeaver commented Jan 24, 2024 •

edited

Loading

ankitsultana commented Feb 8, 2024 •

edited

Loading