[GitHub] mattyb149 commented on a change in pull request #3302: NIFI-6000 Catch also IllegalArgumentException in ConvertAvroToORC hiv…

[GitBox <gi...@apache.org>]

mattyb149 commented on a change in pull request #3302: NIFI-6000 Catch also IllegalArgumentException in ConvertAvroToORC hiv…
URL: https://github.com/apache/nifi/pull/3302#discussion_r256146321
 
 

 ##########
 File path: nifi-nar-bundles/nifi-hive-bundle/nifi-hive-processors/src/main/java/org/apache/nifi/processors/hive/ConvertAvroToORC.java
 ##########
 @@ -283,8 +283,8 @@ public void onTrigger(final ProcessContext context, final ProcessSession session
             session.transfer(flowFile, REL_SUCCESS);
             session.getProvenanceReporter().modifyContent(flowFile, "Converted "+totalRecordCount.get()+" records", System.currentTimeMillis() - startTime);
 
-        } catch (final ProcessException pe) {
-            getLogger().error("Failed to convert {} from Avro to ORC due to {}; transferring to failure", new Object[]{flowFile, pe});
+        } catch (ProcessException | IllegalArgumentException e) {
+            getLogger().error("Failed to convert {} from Avro to ORC due to {}; transferring to failure", new Object[]{flowFile, e});
 
 Review comment:
   Since the ORC Utils can throw an IllegalArgumentException, you are right that we should catch it here for more robust handling. However, since we know the field must contain nulls or a (possibly empty) array of nulls, why not support them by creating an ORC (and Hive DDL) column of some default type, such as `boolean` (since it's the smallest)? Then instead of routing to failure, it will actually be successful and you can even create a Hive table atop it.
   
   I took the liberty of building off your PR and adding support for the Avro null type using the aforementioned approach. I updated the unit tests to go to success (and removed the Avro content validation since it has been successfully converted to ORC) and added one that still exercises your logic, namely that we don't currently support the Avro `fixed` type either. We should add support for that as well, but that can be done as a separate Jira so we can get this logic into the codebase.
   
   If you're interested in looking at and/or incorporating my commit into your PR, it is here (https://github.com/mattyb149/nifi/commit/5ca30de053ecb90845f5c2c64fa96f2a79edd393). I haven't run it all the way to see if the Hive table works, but since the column exists in the file and the table, it seems like it should work fine.
   

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services