drill-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Jason Altekruse" <altekruseja...@gmail.com>
Subject Re: Review Request 33289: DRILL-2350: Improve exception handling and error messages in JSON reader.
Date Mon, 20 Apr 2015 22:15:22 GMT


> 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.
> 
> Jason Altekruse wrote:
>     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 subsi
 tuting them with regex expressions for the appropriate type (such as %d substitutued with
[0-9]*.[0-9]*)

I'm not the best with regex, but reviewboard pulled out part of my comment, I had a star after
both numeric character classes and a backslash before the dot in the middle, this probably
does not cover everything, but it is a little more valid that what reviewboard said I posted
:p  (I should have put a ? on the dot to match it for an integer number or a decimal one)


- 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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message