> On April 20, 2015, 9:46 p.m., Jason Altekruse wrote: > > exec/java-exec/src/test/java/org/apache/drill/exec/store/json/TestJsonRecordReader.java, > > line 81 > > <https://reviews.apache.org/r/33289/diff/2-3/?file=936938#file936938line81> > > > > I don't aggree with this suggestion, this test could 'pass' if any > > error thrown in the system comes back with as an unsupported operation > > exception type. I think we should be checking messages. If we want to avoid > > some mangement overhead, the tests and project live together in the same > > source. You can refer to a static final string from wherever in the source > > the error message is produced when you go to check it in the test. > > abdelhakim deneche wrote: > True, but testing if the error message contains "UNSUPPORTED OPERATION > ERROR" won't be enough either as all unsupported operation errors will > contain this string.
I should have looked more carefully, I didn't realize the only string he was checking for before was the portion that will be common to unsupported operation exceptions. What I should ahve said is that on top of a check for the type of exception we should at least make an effort to make sure that the correct path we intend to check is being executed. This could be as simple as checking for the string JSON in the error message for at least basic coverage in this case. Obviously it would be better to check for the whole message, but there is the burden then of dealing with cases where error messages are actually format strings with %s, %d, etc. and we would need to have the same String.format statement in the test to produce the full error message. We could possibly look at also implementing a little code to help on the test writing side, possibly looking for the format sequences in the test verification of the messages and giving the option to have them somewhat ignored by subsituting them with regex expressions for the appropriate type (such as %d substitutued with [0-9]*.[0-9]*) - Jason ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33289/#review80840 ----------------------------------------------------------- On April 20, 2015, 9:31 p.m., Parth Chandra wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/33289/ > ----------------------------------------------------------- > > (Updated April 20, 2015, 9:31 p.m.) > > > Review request for drill, abdelhakim deneche and Jason Altekruse. > > > Repository: drill-git > > > Description > ------- > > DRILL-2350: Improve exception handling and error messages in JSON reader. > > Errors reported from the JSON reader will now look like the following: > > Query failed: UNSUPPORTED_OPERATION ERROR: In a list of type FLOAT8, > encountered a value of type BIGINT. Drill does not support lists of different > types. > > File: /Users/pchandra/work/data/test/DRILL-2350.json > Record: 1 > Line: 2 > Column: 23 > Field: loc > > [c4b33cca-0a52-4126-a300-2ab91d0ed9d1 on localhost:31010] > > > Diffs > ----- > > exec/java-exec/src/main/codegen/includes/vv_imports.ftl 371d8d0 > exec/java-exec/src/main/codegen/templates/ListWriters.java 29708d7 > > exec/java-exec/src/main/java/org/apache/drill/exec/store/easy/json/JSONRecordReader.java > b41de31 > > exec/java-exec/src/main/java/org/apache/drill/exec/store/easy/json/JsonProcessor.java > b310818 > > exec/java-exec/src/main/java/org/apache/drill/exec/store/easy/json/reader/BaseJsonProcessor.java > 718bb09 > > exec/java-exec/src/main/java/org/apache/drill/exec/vector/complex/fn/JsonReader.java > c196fd2 > > exec/java-exec/src/test/java/org/apache/drill/exec/store/json/TestJsonRecordReader.java > 8b09e80 > exec/java-exec/src/test/resources/jsoninput/DRILL-2350.json PRE-CREATION > > Diff: https://reviews.apache.org/r/33289/diff/ > > > Testing > ------- > > All unit tests. > > > Thanks, > > Parth Chandra > >
