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:08:23 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.

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


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