[GitHub] [drill] cgivre commented on a change in pull request #2076: DRILL-7729: Use java.time in column accessors

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

cgivre commented on a change in pull request #2076:
URL: https://github.com/apache/drill/pull/2076#discussion_r421900398



##########
File path: exec/vector/src/main/java/org/apache/drill/exec/vector/DateUtilities.java
##########
@@ -195,7 +197,27 @@ public static int timeToMillis(int hours, int minutes, int seconds, int millis)
    * @param localTime Java local time
    * @return Drill form of the time

Review comment:
       @paul-rogers 
   Would you mind please adding a brief javadoc to this class explaining what a Drill form of the time is?

##########
File path: exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/scan/convert/StandardConversions.java
##########
@@ -166,6 +171,12 @@ public static DirectConverter newInstance(
     try {
       final Constructor<? extends DirectConverter> ctor = conversionClass.getDeclaredConstructor(ScalarWriter.class, Map.class);
       return ctor.newInstance(baseWriter, properties);
+    } catch (final InvocationTargetException e) {
+      throw UserException.validationError(e.getTargetException())
+        .addContext("Converter setup failed")
+        .addContext("Conversion class" + conversionClass.getName())
+        // .addContext(errorContext) // Add after merge

Review comment:
       Should we add this line back in?




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to 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