You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "vrajat (via GitHub)" <gi...@apache.org> on 2023/12/14 13:37:02 UTC
[PR] Add column name to JSONParsing exception message during index build [pinot]
vrajat opened a new pull request, #12151:
URL: https://github.com/apache/pinot/pull/12151
JSON Index build may fail due to errors in JSON content. When there are multiple JSON indices or when the schema is not immediately available, the column for which index build broke is not clear.
Add column name to the JSONParsing Exception message to help debug JSON index builds.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Add column name to JSONParsing exception message during index build [pinot]
Posted by "swaminathanmanish (via GitHub)" <gi...@apache.org>.
swaminathanmanish commented on code in PR #12151:
URL: https://github.com/apache/pinot/pull/12151#discussion_r1427436376
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/impl/SegmentColumnarIndexCreator.java:
##########
@@ -315,11 +316,14 @@ public void indexRow(GenericRow row)
FieldSpec fieldSpec = _schema.getFieldSpecFor(columnName);
SegmentDictionaryCreator dictionaryCreator = _dictionaryCreatorMap.get(columnName);
-
- if (fieldSpec.isSingleValueField()) {
- indexSingleValueRow(dictionaryCreator, columnValueToIndex, creatorsByIndex);
- } else {
- indexMultiValueRow(dictionaryCreator, (Object[]) columnValueToIndex, creatorsByIndex);
+ try {
+ if (fieldSpec.isSingleValueField()) {
+ indexSingleValueRow(dictionaryCreator, columnValueToIndex, creatorsByIndex);
+ } else {
+ indexMultiValueRow(dictionaryCreator, (Object[]) columnValueToIndex, creatorsByIndex);
+ }
+ } catch (JsonParseException jpe) {
Review Comment:
Thanks ! This will be pretty useful for debugging.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Add column name to JSONParsing exception message during index build [pinot]
Posted by "vrajat (via GitHub)" <gi...@apache.org>.
vrajat commented on code in PR #12151:
URL: https://github.com/apache/pinot/pull/12151#discussion_r1429450578
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/impl/ColumnJsonParserException.java:
##########
@@ -0,0 +1,55 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.segment.local.segment.creator.impl;
+
+import com.fasterxml.jackson.core.JsonLocation;
+import com.fasterxml.jackson.core.JsonParseException;
+
+public class ColumnJsonParserException extends JsonParseException {
+ /**
+ * Exception type for parsing problems when
+ * processing JSON content in a column
+ * Sub-class of {@link com.fasterxml.jackson.core.JsonParseException}.
+ */
+
+ final String _columnName;
+
+ protected ColumnJsonParserException(String columnName, JsonParseException jpe) {
+ super(jpe.getProcessor(), jpe.getOriginalMessage(), jpe.getCause());
+ _columnName = columnName;
+ }
+
+ /**
+ * Default method overridden so that we can add column and location information
+ */
+ @Override
+ public String getMessage() {
+ StringBuilder sb = new StringBuilder();
+ sb.append("Column: ");
+ sb.append(_columnName);
+ sb.append("\n");
+ sb.append(super.getMessage());
Review Comment:
Old habits. Intellij suggested with `+` operator as well. I'll use that coding format.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Add column name to JSONParsing exception message during index build [pinot]
Posted by "vrajat (via GitHub)" <gi...@apache.org>.
vrajat commented on PR #12151:
URL: https://github.com/apache/pinot/pull/12151#issuecomment-1862440379
Ran a manual test and was able to repro the error as well get the modified error message.
I used this diff and ran `JsonIndexQuickStart`:
```
diff --git a/pinot-tools/src/main/resources/examples/batch/githubEvents/githubEvents_schema.json b/pinot-tools/src/main/resources/examples/batch/githubEvents/githubEvents_schema.json
index aafd259146..30ee206eea 100644
--- a/pinot-tools/src/main/resources/examples/batch/githubEvents/githubEvents_schema.json
+++ b/pinot-tools/src/main/resources/examples/batch/githubEvents/githubEvents_schema.json
@@ -10,7 +10,7 @@
},
{
"name": "actor",
- "dataType": "JSON"
+ "dataType": "STRING"
},
{
"name": "repo",
diff --git a/pinot-tools/src/main/resources/examples/batch/githubEvents/rawdata/githubEvents_data.json b/pinot-tools/src/main/resources/examples/batch/githubEvents/rawdata/githubEvents_data.json
index ece16582b3..2baf2a760f 100644
--- a/pinot-tools/src/main/resources/examples/batch/githubEvents/rawdata/githubEvents_data.json
+++ b/pinot-tools/src/main/resources/examples/batch/githubEvents/rawdata/githubEvents_data.json
@@ -1,4 +1,4 @@
< Introduced an error in the actor field>
```
Error message is:
```
Caused by: org.apache.pinot.segment.local.segment.creator.impl.ColumnJsonParserException: Column: actor
Unexpected character ('1' (code 49)): was expecting a colon to separate field name and value
at [Source: (String)"{"id"18542751,"login":"LimeVista","display_login":"LimeVista","gravatar_id":"","url":"https://api.github.com/users/LimeVista","avatar_url":"https://avatars.githubusercontent.com/u/18542751?"}"; line: 1, column: 192]
```
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Add column name to JSONParsing exception message during index build [pinot]
Posted by "ege-st (via GitHub)" <gi...@apache.org>.
ege-st commented on PR #12151:
URL: https://github.com/apache/pinot/pull/12151#issuecomment-1860961248
> > By the way, besides unit tests, were you able to do a manual test and see what happens when Pinot is running and this error happens?
>
> No I did not. Do I have to implement an integration test as well?
For the manual test: probably best to do a simple one, Pinot's a complex system and I like to do quick manual tests after my unit tests just to make sure there are no unexpected interactions.
For the integration test: that's a good question, yeah, I think it'd be great to have integration tests to check that Pinot correctly handles invalid inputs.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Add column name to JSONParsing exception message during index build [pinot]
Posted by "swaminathanmanish (via GitHub)" <gi...@apache.org>.
swaminathanmanish commented on code in PR #12151:
URL: https://github.com/apache/pinot/pull/12151#discussion_r1427440402
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/impl/SegmentColumnarIndexCreator.java:
##########
@@ -315,11 +316,14 @@ public void indexRow(GenericRow row)
FieldSpec fieldSpec = _schema.getFieldSpecFor(columnName);
SegmentDictionaryCreator dictionaryCreator = _dictionaryCreatorMap.get(columnName);
-
- if (fieldSpec.isSingleValueField()) {
- indexSingleValueRow(dictionaryCreator, columnValueToIndex, creatorsByIndex);
- } else {
- indexMultiValueRow(dictionaryCreator, (Object[]) columnValueToIndex, creatorsByIndex);
+ try {
+ if (fieldSpec.isSingleValueField()) {
+ indexSingleValueRow(dictionaryCreator, columnValueToIndex, creatorsByIndex);
+ } else {
+ indexMultiValueRow(dictionaryCreator, (Object[]) columnValueToIndex, creatorsByIndex);
+ }
+ } catch (JsonParseException jpe) {
Review Comment:
Does it make sense to catch this exception to where json index is created (JsonIndexCreator?) or where parsing actually happens.
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/impl/ColumnJsonParserException.java:
##########
@@ -0,0 +1,59 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.segment.local.segment.creator.impl;
+
+import com.fasterxml.jackson.core.JsonLocation;
+import com.fasterxml.jackson.core.JsonParseException;
+
+public class ColumnJsonParserException extends JsonParseException {
+ /**
+ * Exception type for parsing problems when
+ * processing JSON content in a column
+ * Sub-class of {@link com.fasterxml.jackson.core.JsonParseException}.
+ */
+ final static long serialVersionUID = 123; // Stupid eclipse...
Review Comment:
Whats the issue here with eclipse?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Add column name to JSONParsing exception message during index build [pinot]
Posted by "ege-st (via GitHub)" <gi...@apache.org>.
ege-st commented on PR #12151:
URL: https://github.com/apache/pinot/pull/12151#issuecomment-1860774295
By the way, besides unit tests, were you able to do a manual test and see what happens when Pinot is running and this error happens?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Add column name to JSONParsing exception message during index build [pinot]
Posted by "walterddr (via GitHub)" <gi...@apache.org>.
walterddr merged PR #12151:
URL: https://github.com/apache/pinot/pull/12151
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Add column name to JSONParsing exception message during index build [pinot]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #12151:
URL: https://github.com/apache/pinot/pull/12151#issuecomment-1857756476
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/12151?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
Attention: `15 lines` in your changes are missing coverage. Please review.
> Comparison is base [(`d38e15d`)](https://app.codecov.io/gh/apache/pinot/commit/d38e15d1fabe4aa213513f919b719f3249b259d3?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) 61.61% compared to head [(`e5cdc8f`)](https://app.codecov.io/gh/apache/pinot/pull/12151?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) 46.47%.
> Report is 8 commits behind head on master.
| [Files](https://app.codecov.io/gh/apache/pinot/pull/12151?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...egment/creator/impl/ColumnJsonParserException.java](https://app.codecov.io/gh/apache/pinot/pull/12151?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L2NyZWF0b3IvaW1wbC9Db2x1bW5Kc29uUGFyc2VyRXhjZXB0aW9uLmphdmE=) | 0.00% | [10 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/12151?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...ment/creator/impl/SegmentColumnarIndexCreator.java](https://app.codecov.io/gh/apache/pinot/pull/12151?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L2NyZWF0b3IvaW1wbC9TZWdtZW50Q29sdW1uYXJJbmRleENyZWF0b3IuamF2YQ==) | 0.00% | [5 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/12151?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #12151 +/- ##
=============================================
- Coverage 61.61% 46.47% -15.15%
+ Complexity 1153 943 -210
=============================================
Files 2407 1810 -597
Lines 130922 95214 -35708
Branches 20223 15350 -4873
=============================================
- Hits 80669 44250 -36419
- Misses 44366 47815 +3449
+ Partials 5887 3149 -2738
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/12151/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [custom-integration1](https://app.codecov.io/gh/apache/pinot/pull/12151/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration](https://app.codecov.io/gh/apache/pinot/pull/12151/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration1](https://app.codecov.io/gh/apache/pinot/pull/12151/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration2](https://app.codecov.io/gh/apache/pinot/pull/12151/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [java-11](https://app.codecov.io/gh/apache/pinot/pull/12151/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [java-21](https://app.codecov.io/gh/apache/pinot/pull/12151/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `46.47% <0.00%> (-15.03%)` | :arrow_down: |
| [skip-bytebuffers-false](https://app.codecov.io/gh/apache/pinot/pull/12151/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `46.47% <0.00%> (-15.14%)` | :arrow_down: |
| [skip-bytebuffers-true](https://app.codecov.io/gh/apache/pinot/pull/12151/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [temurin](https://app.codecov.io/gh/apache/pinot/pull/12151/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `46.47% <0.00%> (-15.15%)` | :arrow_down: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/12151/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `46.47% <0.00%> (-15.14%)` | :arrow_down: |
| [unittests1](https://app.codecov.io/gh/apache/pinot/pull/12151/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `46.47% <0.00%> (-0.20%)` | :arrow_down: |
| [unittests2](https://app.codecov.io/gh/apache/pinot/pull/12151/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/pinot/pull/12151?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Add column name to JSONParsing exception message during index build [pinot]
Posted by "vrajat (via GitHub)" <gi...@apache.org>.
vrajat commented on PR #12151:
URL: https://github.com/apache/pinot/pull/12151#issuecomment-1861065440
I'll do the manual test.
re: integration test, the current test builds a segment for real. I copied the idea from another test that tests segment build. So hopefully this test is enough w.r.t automation?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Add column name to JSONParsing exception message during index build [pinot]
Posted by "ege-st (via GitHub)" <gi...@apache.org>.
ege-st commented on code in PR #12151:
URL: https://github.com/apache/pinot/pull/12151#discussion_r1428227636
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/impl/SegmentColumnarIndexCreator.java:
##########
@@ -315,11 +316,14 @@ public void indexRow(GenericRow row)
FieldSpec fieldSpec = _schema.getFieldSpecFor(columnName);
SegmentDictionaryCreator dictionaryCreator = _dictionaryCreatorMap.get(columnName);
-
- if (fieldSpec.isSingleValueField()) {
- indexSingleValueRow(dictionaryCreator, columnValueToIndex, creatorsByIndex);
- } else {
- indexMultiValueRow(dictionaryCreator, (Object[]) columnValueToIndex, creatorsByIndex);
+ try {
+ if (fieldSpec.isSingleValueField()) {
+ indexSingleValueRow(dictionaryCreator, columnValueToIndex, creatorsByIndex);
+ } else {
+ indexMultiValueRow(dictionaryCreator, (Object[]) columnValueToIndex, creatorsByIndex);
+ }
+ } catch (JsonParseException jpe) {
Review Comment:
That makes sense, this seems like the best place to put the exception without doing some major refactoring.
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/impl/ColumnJsonParserException.java:
##########
@@ -0,0 +1,55 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.segment.local.segment.creator.impl;
+
+import com.fasterxml.jackson.core.JsonLocation;
+import com.fasterxml.jackson.core.JsonParseException;
+
+public class ColumnJsonParserException extends JsonParseException {
+ /**
+ * Exception type for parsing problems when
+ * processing JSON content in a column
+ * Sub-class of {@link com.fasterxml.jackson.core.JsonParseException}.
+ */
+
+ final String _columnName;
+
+ protected ColumnJsonParserException(String columnName, JsonParseException jpe) {
+ super(jpe.getProcessor(), jpe.getOriginalMessage(), jpe.getCause());
+ _columnName = columnName;
+ }
+
+ /**
+ * Default method overridden so that we can add column and location information
+ */
+ @Override
+ public String getMessage() {
+ StringBuilder sb = new StringBuilder();
+ sb.append("Column: ");
+ sb.append(_columnName);
+ sb.append("\n");
+ sb.append(super.getMessage());
Review Comment:
I believe the Java compiler will automatically using StringBuilder when concatenating strings, so you can have this as `"Column: " + _columnName + "\n" + super.getMessage()` and the compiler will create the StringBuilder code for you. (Actually, from some reading the Compiler will pick between StringBuilder and StringBuffer based upon which is more optimized for the situation).
Though, I'm curious: why not use `String.format("Column: %s\n%s", _columnName, super.getMessage())`?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Add column name to JSONParsing exception message during index build [pinot]
Posted by "vrajat (via GitHub)" <gi...@apache.org>.
vrajat commented on code in PR #12151:
URL: https://github.com/apache/pinot/pull/12151#discussion_r1427520654
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/impl/ColumnJsonParserException.java:
##########
@@ -0,0 +1,59 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.segment.local.segment.creator.impl;
+
+import com.fasterxml.jackson.core.JsonLocation;
+import com.fasterxml.jackson.core.JsonParseException;
+
+public class ColumnJsonParserException extends JsonParseException {
+ /**
+ * Exception type for parsing problems when
+ * processing JSON content in a column
+ * Sub-class of {@link com.fasterxml.jackson.core.JsonParseException}.
+ */
+ final static long serialVersionUID = 123; // Stupid eclipse...
Review Comment:
Copy-paste. Will remove. TIL: https://stackoverflow.com/questions/285793/what-is-a-serialversionuid-and-why-should-i-use-it
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Add column name to JSONParsing exception message during index build [pinot]
Posted by "vrajat (via GitHub)" <gi...@apache.org>.
vrajat commented on code in PR #12151:
URL: https://github.com/apache/pinot/pull/12151#discussion_r1427519144
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/impl/SegmentColumnarIndexCreator.java:
##########
@@ -315,11 +316,14 @@ public void indexRow(GenericRow row)
FieldSpec fieldSpec = _schema.getFieldSpecFor(columnName);
SegmentDictionaryCreator dictionaryCreator = _dictionaryCreatorMap.get(columnName);
-
- if (fieldSpec.isSingleValueField()) {
- indexSingleValueRow(dictionaryCreator, columnValueToIndex, creatorsByIndex);
- } else {
- indexMultiValueRow(dictionaryCreator, (Object[]) columnValueToIndex, creatorsByIndex);
+ try {
+ if (fieldSpec.isSingleValueField()) {
+ indexSingleValueRow(dictionaryCreator, columnValueToIndex, creatorsByIndex);
+ } else {
+ indexMultiValueRow(dictionaryCreator, (Object[]) columnValueToIndex, creatorsByIndex);
+ }
+ } catch (JsonParseException jpe) {
Review Comment:
In any level higher than `SegmentColumnarIndexCreator`, it is not easy to figure out the column name. IIUC, callers of this function, are indexing a row and there maybe different indices built for different columns. This is the first function that is iterating through all the columns and building an index for each one. So columnName is obvious.
This function passes the column index value instead of names to `IndexSingleValueRow`. So names are not available again.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Add column name to JSONParsing exception message during index build [pinot]
Posted by "swaminathanmanish (via GitHub)" <gi...@apache.org>.
swaminathanmanish commented on code in PR #12151:
URL: https://github.com/apache/pinot/pull/12151#discussion_r1427440402
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/impl/SegmentColumnarIndexCreator.java:
##########
@@ -315,11 +316,14 @@ public void indexRow(GenericRow row)
FieldSpec fieldSpec = _schema.getFieldSpecFor(columnName);
SegmentDictionaryCreator dictionaryCreator = _dictionaryCreatorMap.get(columnName);
-
- if (fieldSpec.isSingleValueField()) {
- indexSingleValueRow(dictionaryCreator, columnValueToIndex, creatorsByIndex);
- } else {
- indexMultiValueRow(dictionaryCreator, (Object[]) columnValueToIndex, creatorsByIndex);
+ try {
+ if (fieldSpec.isSingleValueField()) {
+ indexSingleValueRow(dictionaryCreator, columnValueToIndex, creatorsByIndex);
+ } else {
+ indexMultiValueRow(dictionaryCreator, (Object[]) columnValueToIndex, creatorsByIndex);
+ }
+ } catch (JsonParseException jpe) {
Review Comment:
Does it make sense to catch this exception where json index is created (JsonIndexCreator?) or where parsing actually happens.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Add column name to JSONParsing exception message during index build [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #12151:
URL: https://github.com/apache/pinot/pull/12151#discussion_r1432040654
##########
pinot-core/src/test/java/org/apache/pinot/queries/JsonMalformedIndexTest.java:
##########
@@ -0,0 +1,139 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.queries;
+
+import org.apache.commons.io.FileUtils;
+import org.apache.pinot.common.response.broker.BrokerResponseNative;
+import org.apache.pinot.segment.local.indexsegment.immutable.ImmutableSegmentLoader;
+import org.apache.pinot.segment.local.segment.creator.impl.ColumnJsonParserException;
+import org.apache.pinot.segment.local.segment.creator.impl.SegmentIndexCreationDriverImpl;
+import org.apache.pinot.segment.local.segment.index.loader.IndexLoadingConfig;
+import org.apache.pinot.segment.local.segment.readers.GenericRowRecordReader;
+import org.apache.pinot.segment.spi.ImmutableSegment;
+import org.apache.pinot.segment.spi.IndexSegment;
+import org.apache.pinot.segment.spi.creator.SegmentGeneratorConfig;
+import org.apache.pinot.spi.config.table.TableConfig;
+import org.apache.pinot.spi.config.table.TableType;
+import org.apache.pinot.spi.data.FieldSpec;
+import org.apache.pinot.spi.data.Schema;
+import org.apache.pinot.spi.data.readers.GenericRow;
+import org.apache.pinot.spi.utils.ReadMode;
+import org.apache.pinot.spi.utils.builder.TableConfigBuilder;
+import org.testng.annotations.BeforeClass;
+import org.testng.annotations.Test;
+
+import java.io.File;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.HashSet;
+import java.util.List;
+
+public class JsonMalformedIndexTest extends BaseQueriesTest {
Review Comment:
(Code format) Please reformat all the changes with [Pinot Style](https://docs.pinot.apache.org/developers/developers-and-contributors/code-setup#set-up-ide) and remove the extra empty lines
##########
pinot-core/src/test/java/org/apache/pinot/queries/JsonMalformedIndexTest.java:
##########
@@ -0,0 +1,139 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.queries;
+
+import org.apache.commons.io.FileUtils;
+import org.apache.pinot.common.response.broker.BrokerResponseNative;
+import org.apache.pinot.segment.local.indexsegment.immutable.ImmutableSegmentLoader;
+import org.apache.pinot.segment.local.segment.creator.impl.ColumnJsonParserException;
+import org.apache.pinot.segment.local.segment.creator.impl.SegmentIndexCreationDriverImpl;
+import org.apache.pinot.segment.local.segment.index.loader.IndexLoadingConfig;
+import org.apache.pinot.segment.local.segment.readers.GenericRowRecordReader;
+import org.apache.pinot.segment.spi.ImmutableSegment;
+import org.apache.pinot.segment.spi.IndexSegment;
+import org.apache.pinot.segment.spi.creator.SegmentGeneratorConfig;
+import org.apache.pinot.spi.config.table.TableConfig;
+import org.apache.pinot.spi.config.table.TableType;
+import org.apache.pinot.spi.data.FieldSpec;
+import org.apache.pinot.spi.data.Schema;
+import org.apache.pinot.spi.data.readers.GenericRow;
+import org.apache.pinot.spi.utils.ReadMode;
+import org.apache.pinot.spi.utils.builder.TableConfigBuilder;
+import org.testng.annotations.BeforeClass;
+import org.testng.annotations.Test;
+
+import java.io.File;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.HashSet;
+import java.util.List;
+
+public class JsonMalformedIndexTest extends BaseQueriesTest {
+
+
+ static final String RAW_TABLE_NAME = "testTable";
Review Comment:
All the member variables and methods except for the ones annotated can be `private`
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/impl/ColumnJsonParserException.java:
##########
@@ -0,0 +1,53 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.segment.local.segment.creator.impl;
+
+import com.fasterxml.jackson.core.JsonLocation;
+import com.fasterxml.jackson.core.JsonParseException;
+
+public class ColumnJsonParserException extends JsonParseException {
+ /**
+ * Exception type for parsing problems when
+ * processing JSON content in a column
+ * Sub-class of {@link com.fasterxml.jackson.core.JsonParseException}.
+ */
+
+ final String _columnName;
Review Comment:
```suggestion
private final String _columnName;
```
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/impl/ColumnJsonParserException.java:
##########
@@ -0,0 +1,53 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.segment.local.segment.creator.impl;
+
+import com.fasterxml.jackson.core.JsonLocation;
+import com.fasterxml.jackson.core.JsonParseException;
+
+public class ColumnJsonParserException extends JsonParseException {
+ /**
+ * Exception type for parsing problems when
+ * processing JSON content in a column
+ * Sub-class of {@link com.fasterxml.jackson.core.JsonParseException}.
+ */
+
Review Comment:
Remove empty line
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/impl/ColumnJsonParserException.java:
##########
@@ -0,0 +1,53 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.segment.local.segment.creator.impl;
+
+import com.fasterxml.jackson.core.JsonLocation;
+import com.fasterxml.jackson.core.JsonParseException;
+
+public class ColumnJsonParserException extends JsonParseException {
+ /**
+ * Exception type for parsing problems when
+ * processing JSON content in a column
+ * Sub-class of {@link com.fasterxml.jackson.core.JsonParseException}.
+ */
+
+ final String _columnName;
+
+ protected ColumnJsonParserException(String columnName, JsonParseException jpe) {
Review Comment:
```suggestion
public ColumnJsonParserException(String columnName, JsonParseException jpe) {
```
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Add column name to JSONParsing exception message during index build [pinot]
Posted by "vrajat (via GitHub)" <gi...@apache.org>.
vrajat commented on code in PR #12151:
URL: https://github.com/apache/pinot/pull/12151#discussion_r1432299319
##########
pinot-core/src/test/java/org/apache/pinot/queries/JsonMalformedIndexTest.java:
##########
@@ -0,0 +1,139 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.queries;
+
+import org.apache.commons.io.FileUtils;
+import org.apache.pinot.common.response.broker.BrokerResponseNative;
+import org.apache.pinot.segment.local.indexsegment.immutable.ImmutableSegmentLoader;
+import org.apache.pinot.segment.local.segment.creator.impl.ColumnJsonParserException;
+import org.apache.pinot.segment.local.segment.creator.impl.SegmentIndexCreationDriverImpl;
+import org.apache.pinot.segment.local.segment.index.loader.IndexLoadingConfig;
+import org.apache.pinot.segment.local.segment.readers.GenericRowRecordReader;
+import org.apache.pinot.segment.spi.ImmutableSegment;
+import org.apache.pinot.segment.spi.IndexSegment;
+import org.apache.pinot.segment.spi.creator.SegmentGeneratorConfig;
+import org.apache.pinot.spi.config.table.TableConfig;
+import org.apache.pinot.spi.config.table.TableType;
+import org.apache.pinot.spi.data.FieldSpec;
+import org.apache.pinot.spi.data.Schema;
+import org.apache.pinot.spi.data.readers.GenericRow;
+import org.apache.pinot.spi.utils.ReadMode;
+import org.apache.pinot.spi.utils.builder.TableConfigBuilder;
+import org.testng.annotations.BeforeClass;
+import org.testng.annotations.Test;
+
+import java.io.File;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.HashSet;
+import java.util.List;
+
+public class JsonMalformedIndexTest extends BaseQueriesTest {
Review Comment:
I did setup pinot code style in intellij but doesnt detect some of these changes. I'll check my setup again.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Add column name to JSONParsing exception message during index build [pinot]
Posted by "vrajat (via GitHub)" <gi...@apache.org>.
vrajat commented on PR #12151:
URL: https://github.com/apache/pinot/pull/12151#issuecomment-1860951021
> By the way, besides unit tests, were you able to do a manual test and see what happens when Pinot is running and this error happens?
No I did not. Do I have to implement an integration test as well?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org