SPARKC-516: Add column name to the error message in case of conversion error #1155
Conversation
| buffer(i) = converters(i).convert(colValue) | ||
| buffer(i) = Try(converters(i).convert(colValue)) match { | ||
| case Success(value) => value | ||
| case Failure(ex: IllegalArgumentException) => |
There was a problem hiding this comment.
The most exceptions I've seen that were hard to track down were either IllegalArgumentException or its subclass.
| case Failure(ex: IllegalArgumentException) => | ||
| val columnType = columnTypes(i).scalaTypeName | ||
| throw new TypeConversionException( | ||
| s"Cannot convert value $colValue of column ${columnNames(i)} to type $columnType", ex) |
There was a problem hiding this comment.
The message will look like Cannot convert value somevalue of column somecolumn to type Int without any wrapping of somevalue (like single or double quotes). Any suggestions on how to improve readability?
|
We probably will want to do some perf testing around this change since this is adding a try catch into a pretty tight loop. |
|
Would it be ok to change it from using |
|
Mostly I was worried about the Try/Catch block in the very inner loop of the conversion here. Sorry I've been so long in responding, I do think that for fixing this issue better maybe we should force Spark to do the conversion IE not accepting a String for a numeric type ... or something like that so Spark does the conversion instead of us in the type converter code ... Gotta think more on this ... |
The change adds new exception on top of the actual conversion exception with debug information about column name and expected type.