Repository: drill Updated Branches: refs/heads/master eb199d614 -> 7289cabb5
DRILL-2350: Improve exception handling and error messages in JSON reader. Project: http://git-wip-us.apache.org/repos/asf/drill/repo Commit: http://git-wip-us.apache.org/repos/asf/drill/commit/319b94c0 Tree: http://git-wip-us.apache.org/repos/asf/drill/tree/319b94c0 Diff: http://git-wip-us.apache.org/repos/asf/drill/diff/319b94c0 Branch: refs/heads/master Commit: 319b94c01c798af0ff0ff95b8b3af2e80d2a5ef7 Parents: eb199d6 Author: Parth Chandra <pchan...@maprtech.com> Authored: Thu Apr 9 17:11:14 2015 -0700 Committer: Parth Chandra <pchan...@maprtech.com> Committed: Fri Apr 24 16:21:53 2015 -0700 ---------------------------------------------------------------------- .../src/main/codegen/includes/vv_imports.ftl | 3 + .../src/main/codegen/templates/ListWriters.java | 23 +++-- .../exec/store/easy/json/JSONRecordReader.java | 23 ++--- .../exec/store/easy/json/JsonProcessor.java | 13 +++ .../easy/json/reader/BaseJsonProcessor.java | 26 ++++++ .../exec/vector/complex/fn/JsonReader.java | 97 ++++++++++++++------ .../exec/store/json/TestJsonRecordReader.java | 18 ++++ .../test/resources/jsoninput/DRILL-2350.json | 1 + 8 files changed, 160 insertions(+), 44 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/drill/blob/319b94c0/exec/java-exec/src/main/codegen/includes/vv_imports.ftl ---------------------------------------------------------------------- diff --git a/exec/java-exec/src/main/codegen/includes/vv_imports.ftl b/exec/java-exec/src/main/codegen/includes/vv_imports.ftl index 371d8d0..d0f6291 100644 --- a/exec/java-exec/src/main/codegen/includes/vv_imports.ftl +++ b/exec/java-exec/src/main/codegen/includes/vv_imports.ftl @@ -21,9 +21,12 @@ import io.netty.buffer.*; import org.apache.commons.lang3.ArrayUtils; +import org.apache.drill.common.exceptions.UserException; import org.apache.drill.exec.expr.fn.impl.StringFunctionUtil; import org.apache.drill.exec.memory.*; import org.apache.drill.exec.proto.SchemaDefProtos; +import org.apache.drill.exec.proto.UserBitShared; +import org.apache.drill.exec.proto.UserBitShared.DrillPBError; import org.apache.drill.exec.proto.UserBitShared.SerializedField; import org.apache.drill.exec.record.*; import org.apache.drill.exec.vector.*; http://git-wip-us.apache.org/repos/asf/drill/blob/319b94c0/exec/java-exec/src/main/codegen/templates/ListWriters.java ---------------------------------------------------------------------- diff --git a/exec/java-exec/src/main/codegen/templates/ListWriters.java b/exec/java-exec/src/main/codegen/templates/ListWriters.java index 29708d7..6df4248 100644 --- a/exec/java-exec/src/main/codegen/templates/ListWriters.java +++ b/exec/java-exec/src/main/codegen/templates/ListWriters.java @@ -97,8 +97,8 @@ public class ${mode}ListWriter extends AbstractFieldWriter{ case IN_MAP: return writer; } - - throw new IllegalStateException(String.format("Needed to be in state INIT or IN_MAP but in mode %s", mode.name())); + + throw UserException.unsupportedError().message(getUnsupportedErrorMsg("MAP", mode.name())).build(); } @@ -116,8 +116,8 @@ public class ${mode}ListWriter extends AbstractFieldWriter{ case IN_LIST: return writer; } - - throw new IllegalStateException(String.format("Needed to be in state INIT or IN_LIST but in mode %s", mode.name())); + + throw UserException.unsupportedError().message(getUnsupportedErrorMsg("LIST", mode.name())).build(); } @@ -143,8 +143,9 @@ public class ${mode}ListWriter extends AbstractFieldWriter{ case IN_${upperName}: return writer; } - - throw new IllegalStateException(String.format("Needed to be in state INIT or IN_${upperName} but in mode %s", mode.name())); + + throw UserException.unsupportedError().message(getUnsupportedErrorMsg("${upperName}", mode.name())).build(); + } </#list></#list> @@ -198,7 +199,15 @@ public class ${mode}ListWriter extends AbstractFieldWriter{ } </#if> -} + private String getUnsupportedErrorMsg(String expected, String found ){ + String f = found.substring(3); + return String.format("In a list of type %s, encountered a value of type %s. "+ + "Drill does not support lists of different types.", + f, expected + ); + } + + } </#list> http://git-wip-us.apache.org/repos/asf/drill/blob/319b94c0/exec/java-exec/src/main/java/org/apache/drill/exec/store/easy/json/JSONRecordReader.java ---------------------------------------------------------------------- diff --git a/exec/java-exec/src/main/java/org/apache/drill/exec/store/easy/json/JSONRecordReader.java b/exec/java-exec/src/main/java/org/apache/drill/exec/store/easy/json/JSONRecordReader.java index b41de31..2cccc64 100644 --- a/exec/java-exec/src/main/java/org/apache/drill/exec/store/easy/json/JSONRecordReader.java +++ b/exec/java-exec/src/main/java/org/apache/drill/exec/store/easy/json/JSONRecordReader.java @@ -124,7 +124,7 @@ public class JSONRecordReader extends AbstractRecordReader { } setupParser(); }catch(final Exception e){ - handleAndRaise("Failure reading JSON file.", e); + handleAndRaise("Failure reading JSON file", e); } } @@ -159,12 +159,15 @@ public class JSONRecordReader extends AbstractRecordReader { columnNr = ex.getLocation().getColumnNr(); } - throw UserException.dataReadError(e) - .message("%s - %s", suffix, message) - .addContext("Filename", hadoopPath.toUri().getPath()) - .addContext("Record", recordCount + 1) - .addContext("Column", columnNr) - .build(); + UserException.Builder exceptionBuilder = UserException.dataReadError(e) + .message("%s - %s", suffix, message); + if (columnNr > 0) { + exceptionBuilder.pushContext("Column ", columnNr); + } + exceptionBuilder.pushContext("Record ", recordCount + 1) + .pushContext("File ", hadoopPath.toUri().getPath()); + + throw exceptionBuilder.build(); } @@ -208,10 +211,8 @@ public class JSONRecordReader extends AbstractRecordReader { return recordCount; - } catch (final JsonParseException e) { - handleAndRaise("Error parsing JSON.", e); - } catch (final IOException e) { - handleAndRaise("Error reading JSON.", e); + } catch (final Exception e) { + handleAndRaise("Error parsing JSON", e); } // this is never reached return 0; http://git-wip-us.apache.org/repos/asf/drill/blob/319b94c0/exec/java-exec/src/main/java/org/apache/drill/exec/store/easy/json/JsonProcessor.java ---------------------------------------------------------------------- diff --git a/exec/java-exec/src/main/java/org/apache/drill/exec/store/easy/json/JsonProcessor.java b/exec/java-exec/src/main/java/org/apache/drill/exec/store/easy/json/JsonProcessor.java index b310818..4d8d4ba 100644 --- a/exec/java-exec/src/main/java/org/apache/drill/exec/store/easy/json/JsonProcessor.java +++ b/exec/java-exec/src/main/java/org/apache/drill/exec/store/easy/json/JsonProcessor.java @@ -20,6 +20,8 @@ package org.apache.drill.exec.store.easy.json; import java.io.IOException; import java.io.InputStream; +import org.apache.drill.common.exceptions.UserException; +import org.apache.drill.exec.proto.UserBitShared; import org.apache.drill.exec.vector.complex.writer.BaseWriter; import com.fasterxml.jackson.databind.JsonNode; @@ -37,4 +39,15 @@ public interface JsonProcessor { void setSource(JsonNode node); void ensureAtLeastOneField(BaseWriter.ComplexWriter writer); + + public UserException.Builder getExceptionWithContext(UserException.Builder exceptionBuilder, + String field, + String msg, + Object... args); + + public UserException.Builder getExceptionWithContext(Throwable exception, + String field, + String msg, + Object... args); + } http://git-wip-us.apache.org/repos/asf/drill/blob/319b94c0/exec/java-exec/src/main/java/org/apache/drill/exec/store/easy/json/reader/BaseJsonProcessor.java ---------------------------------------------------------------------- diff --git a/exec/java-exec/src/main/java/org/apache/drill/exec/store/easy/json/reader/BaseJsonProcessor.java b/exec/java-exec/src/main/java/org/apache/drill/exec/store/easy/json/reader/BaseJsonProcessor.java index 718bb09..7833631 100644 --- a/exec/java-exec/src/main/java/org/apache/drill/exec/store/easy/json/reader/BaseJsonProcessor.java +++ b/exec/java-exec/src/main/java/org/apache/drill/exec/store/easy/json/reader/BaseJsonProcessor.java @@ -29,6 +29,7 @@ import com.fasterxml.jackson.databind.JsonNode; import com.fasterxml.jackson.databind.ObjectMapper; import com.fasterxml.jackson.databind.node.TreeTraversingParser; import com.google.common.base.Preconditions; +import org.apache.drill.common.exceptions.UserException; public abstract class BaseJsonProcessor implements JsonProcessor { @@ -53,4 +54,29 @@ public abstract class BaseJsonProcessor implements JsonProcessor { this.parser = new TreeTraversingParser(node); } + @Override + public UserException.Builder getExceptionWithContext(UserException.Builder exceptionBuilder, + String field, + String msg, + Object... args) { + if (msg != null){ + exceptionBuilder.message(msg, args); + } + if(field!=null) { + exceptionBuilder.pushContext("Field ", field); + } + exceptionBuilder.pushContext("Column ", parser.getCurrentLocation().getColumnNr()+1) + .pushContext("Line ", parser.getCurrentLocation().getLineNr()); + return exceptionBuilder; + } + + @Override + public UserException.Builder getExceptionWithContext(Throwable e, + String field, + String msg, + Object... args) { + UserException.Builder exceptionBuilder = UserException.dataReadError(e); + return getExceptionWithContext(exceptionBuilder, field, msg, args); + } + } http://git-wip-us.apache.org/repos/asf/drill/blob/319b94c0/exec/java-exec/src/main/java/org/apache/drill/exec/vector/complex/fn/JsonReader.java ---------------------------------------------------------------------- diff --git a/exec/java-exec/src/main/java/org/apache/drill/exec/vector/complex/fn/JsonReader.java b/exec/java-exec/src/main/java/org/apache/drill/exec/vector/complex/fn/JsonReader.java index c196fd2..696fe7a 100644 --- a/exec/java-exec/src/main/java/org/apache/drill/exec/vector/complex/fn/JsonReader.java +++ b/exec/java-exec/src/main/java/org/apache/drill/exec/vector/complex/fn/JsonReader.java @@ -23,7 +23,7 @@ import java.io.IOException; import java.io.InputStream; import java.util.List; -import org.apache.drill.common.exceptions.DrillRuntimeException; +import org.apache.drill.common.exceptions.UserException; import org.apache.drill.common.expression.PathSegment; import org.apache.drill.common.expression.SchemaPath; import org.apache.drill.exec.physical.base.GroupScan; @@ -65,6 +65,10 @@ public class JsonReader extends BaseJsonProcessor { * Whether the reader is currently in a situation where we are unwrapping an outer list. */ private boolean inOuterList; + /** + * The name of the current field being parsed. For Error messages. + */ + private String currentFieldName; private FieldSelection selection; @@ -82,6 +86,7 @@ public class JsonReader extends BaseJsonProcessor { this.columns = columns; this.mapOutput = new MapVectorOutput(workingBuffer); this.listOutput = new ListVectorOutput(workingBuffer); + this.currentFieldName="<none>"; } @Override @@ -146,7 +151,11 @@ public class JsonReader extends BaseJsonProcessor { case WRITE_SUCCEED: break; default: - throw new IllegalStateException(); + throw + getExceptionWithContext( + UserException.dataReadError(), currentFieldName, null) + .message("Failure while reading JSON. (Got an invalid read state %s )", readState.toString()) + .build(); } return readState; @@ -155,9 +164,13 @@ public class JsonReader extends BaseJsonProcessor { private void confirmLast() throws IOException{ parser.nextToken(); if(!parser.isClosed()){ - throw new JsonParseException("Drill attempted to unwrap a toplevel list " - + "in your document. However, it appears that there is trailing content after this top level list. Drill only " - + "supports querying a set of distinct maps or a single json array with multiple inner maps.", parser.getCurrentLocation()); + throw + getExceptionWithContext( + UserException.dataReadError(), currentFieldName, null) + .message("Drill attempted to unwrap a toplevel list " + + "in your document. However, it appears that there is trailing content after this top level list. Drill only " + + "supports querying a set of distinct maps or a single json array with multiple inner maps.") + .build(); } } @@ -168,8 +181,12 @@ public class JsonReader extends BaseJsonProcessor { break; case START_ARRAY: if(inOuterList){ - throw new JsonParseException("The top level of your document must either be a single array of maps or a set " - + "of white space delimited maps.", parser.getCurrentLocation()); + throw + getExceptionWithContext( + UserException.dataReadError(), currentFieldName, null) + .message("The top level of your document must either be a single array of maps or a set " + + "of white space delimited maps.") + .build(); } if(skipOuterList){ @@ -178,8 +195,12 @@ public class JsonReader extends BaseJsonProcessor { inOuterList = true; writeDataSwitch(writer.rootAsMap()); }else{ - throw new JsonParseException("The top level of your document must either be a single array of maps or a set " - + "of white space delimited maps.", parser.getCurrentLocation()); + throw + getExceptionWithContext( + UserException.dataReadError(), currentFieldName, null) + .message("The top level of your document must either be a single array of maps or a set " + + "of white space delimited maps.") + .build(); } }else{ @@ -192,16 +213,22 @@ public class JsonReader extends BaseJsonProcessor { confirmLast(); return ReadState.END_OF_STREAM; }else{ - throw new JsonParseException(String.format("Failure while parsing JSON. Ran across unexpected %s.", JsonToken.END_ARRAY), parser.getCurrentLocation()); + throw + getExceptionWithContext( + UserException.dataReadError(), currentFieldName, null) + .message("Failure while parsing JSON. Ran across unexpected %s.", JsonToken.END_ARRAY) + .build(); } case NOT_AVAILABLE: return ReadState.END_OF_STREAM; default: - throw new JsonParseException(String.format( - "Failure while parsing JSON. Found token of [%s] Drill currently only supports parsing " - + "json strings that contain either lists or maps. The root object cannot be a scalar.", t), - parser.getCurrentLocation()); + throw + getExceptionWithContext( + UserException.dataReadError(), currentFieldName, null) + .message("Failure while parsing JSON. Found token of [%s]. Drill currently only supports parsing " + + "json strings that contain either lists or maps. The root object cannot be a scalar.", t) + .build(); } return ReadState.WRITE_SUCCEED; @@ -266,7 +293,7 @@ public class JsonReader extends BaseJsonProcessor { assert t == JsonToken.FIELD_NAME : String.format("Expected FIELD_NAME but got %s.", t.name()); final String fieldName = parser.getText(); - + this.currentFieldName = fieldName; FieldSelection childSelection = selection.getChild(fieldName); if (childSelection.isNeverValid()) { consumeEntireNextValue(); @@ -312,8 +339,11 @@ public class JsonReader extends BaseJsonProcessor { break; default: - throw new IllegalStateException("Unexpected token " + parser.getCurrentToken()); - + throw + getExceptionWithContext( + UserException.dataReadError(), currentFieldName, null) + .message("Unexpected token %s", parser.getCurrentToken()) + .build(); } } @@ -343,6 +373,7 @@ public class JsonReader extends BaseJsonProcessor { assert t == JsonToken.FIELD_NAME : String.format("Expected FIELD_NAME but got %s.", t.name()); final String fieldName = parser.getText(); + this.currentFieldName = fieldName; FieldSelection childSelection = selection.getChild(fieldName); if (childSelection.isNeverValid()) { consumeEntireNextValue(); @@ -375,8 +406,11 @@ public class JsonReader extends BaseJsonProcessor { break; default: - throw new IllegalStateException("Unexpected token " + parser.getCurrentToken()); - + throw + getExceptionWithContext( + UserException.dataReadError(), currentFieldName, null) + .message("Unexpected token %s", parser.getCurrentToken()) + .build(); } } map.end(); @@ -426,7 +460,7 @@ public class JsonReader extends BaseJsonProcessor { private void writeData(ListWriter list) throws IOException { list.start(); outside: while (true) { - + try { switch (parser.nextToken()) { case START_ARRAY: writeData(list.list()); @@ -452,9 +486,11 @@ public class JsonReader extends BaseJsonProcessor { break; } case VALUE_NULL: - throw new DrillRuntimeException("Drill does not currently null values in lists. " - + "Please set `store.json.all_text_mode` to true to read lists containing nulls. " - + "Be advised that this will treat JSON null values as string containing the word 'null'."); + throw UserException.unsupportedError() + .message("Null values are not supported in lists by default. " + + "Please set `store.json.all_text_mode` to true to read lists containing nulls. " + + "Be advised that this will treat JSON null values as a string containing the word 'null'.") + .build(); case VALUE_NUMBER_FLOAT: list.float8().writeFloat8(parser.getDoubleValue()); atLeastOneWrite = true; @@ -468,8 +504,13 @@ public class JsonReader extends BaseJsonProcessor { atLeastOneWrite = true; break; default: - throw new IllegalStateException("Unexpected token " + parser.getCurrentToken()); - } + throw UserException.dataReadError() + .message("Unexpected token %s", parser.getCurrentToken()) + .build(); + } + } catch (Exception e) { + throw getExceptionWithContext(e, this.currentFieldName, null).build(); + } } list.end(); @@ -503,7 +544,11 @@ public class JsonReader extends BaseJsonProcessor { atLeastOneWrite = true; break; default: - throw new IllegalStateException("Unexpected token " + parser.getCurrentToken()); + throw + getExceptionWithContext( + UserException.dataReadError(), currentFieldName, null) + .message("Unexpected token %s", parser.getCurrentToken()) + .build(); } } list.end(); http://git-wip-us.apache.org/repos/asf/drill/blob/319b94c0/exec/java-exec/src/test/java/org/apache/drill/exec/store/json/TestJsonRecordReader.java ---------------------------------------------------------------------- diff --git a/exec/java-exec/src/test/java/org/apache/drill/exec/store/json/TestJsonRecordReader.java b/exec/java-exec/src/test/java/org/apache/drill/exec/store/json/TestJsonRecordReader.java index 8b09e80..44e2a2d 100644 --- a/exec/java-exec/src/test/java/org/apache/drill/exec/store/json/TestJsonRecordReader.java +++ b/exec/java-exec/src/test/java/org/apache/drill/exec/store/json/TestJsonRecordReader.java @@ -18,7 +18,12 @@ package org.apache.drill.exec.store.json; import org.apache.drill.BaseTestQuery; +import org.apache.drill.common.exceptions.UserException; +import org.apache.drill.exec.proto.UserBitShared; import org.junit.Test; +import org.junit.Assert; + +import static org.junit.Assert.assertEquals; public class TestJsonRecordReader extends BaseTestQuery{ @@ -68,4 +73,17 @@ public class TestJsonRecordReader extends BaseTestQuery{ testNoResult("alter session set `store.json.all_text_mode`= true"); test("select * from cp.`jsoninput/big_numeric.json`"); } + + @Test + public void testExceptionHandling() throws Exception { + try { + test("select * from cp.`jsoninput/DRILL-2350.json`"); + } catch(UserException e) { + Assert.assertEquals(UserBitShared.DrillPBError.ErrorType.UNSUPPORTED_OPERATION, e.getOrCreatePBError(false).getErrorType()); + String s = e.getMessage(); + assertEquals("Expected Unsupported Operation Exception.", true, s.contains("Drill does not support lists of different types.")); + } + + } + } http://git-wip-us.apache.org/repos/asf/drill/blob/319b94c0/exec/java-exec/src/test/resources/jsoninput/DRILL-2350.json ---------------------------------------------------------------------- diff --git a/exec/java-exec/src/test/resources/jsoninput/DRILL-2350.json b/exec/java-exec/src/test/resources/jsoninput/DRILL-2350.json new file mode 100644 index 0000000..3433e89 --- /dev/null +++ b/exec/java-exec/src/test/resources/jsoninput/DRILL-2350.json @@ -0,0 +1 @@ +{ "loc" : [ -75.2, 40 ] }
