[GitHub] [drill] paul-rogers commented on a change in pull request #2268: DRILL-7926: Age function fix

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

paul-rogers commented on a change in pull request #2268:
URL: https://github.com/apache/drill/pull/2268#discussion_r664950317



##########
File path: exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/impl/DateTypeFunctions.java
##########
@@ -424,26 +424,35 @@ public void eval() {
     public static class AgeTimeStampFunction implements DrillSimpleFunc {
         @Param TimeStampHolder left;
         @Param TimeStampHolder right;
+        @Workspace org.joda.time.PeriodType periodType;
         @Output IntervalHolder out;
 
         @Override
         public void setup() {
+            periodType = org.joda.time.PeriodType.forFields(
+                new org.joda.time.DurationFieldType[] {
+                    org.joda.time.DurationFieldType.months(),
+                    org.joda.time.DurationFieldType.days(),
+                    org.joda.time.DurationFieldType.millis()
+                }
+            );
         }
 
         @Override
         public void eval() {
-            long diff = left.value - right.value;
-            long days = diff / org.apache.drill.exec.vector.DateUtilities.daysToStandardMillis;
-            out.months = (int) (days / org.apache.drill.exec.vector.DateUtilities.monthToStandardDays);
-            out.days = (int) (days % org.apache.drill.exec.vector.DateUtilities.monthToStandardDays);
-            out.milliseconds = (int) (diff % org.apache.drill.exec.vector.DateUtilities.daysToStandardMillis);
+            org.joda.time.Period interval = new org.joda.time.Period(right.value, left.value, periodType);

Review comment:
       Using Joda is certainly a quick fix! What was wrong with the `DateUtilities` code? Since that code is used in multiple places, could we fix that code? Or, if the fix is complicated (everything with dates is complicated), should we change all other uses of the same constants and delete the constants in `DateUtilities`?
   
   Also, Joda is long obsolete. Is there a "modern" way to do these calculations?

##########
File path: exec/java-exec/src/test/java/org/apache/drill/exec/fn/impl/TestDateFunctions.java
##########
@@ -104,10 +104,10 @@ public void testDateDifferenceArithmetic() throws Exception {
 
   @Test
   public void testAge() throws Exception {
-    String[] expectedResults = {"P109M16DT82800S",
-                                "P172M27D",
-                                "P-172M-27D",
-                                "P-39M-18DT-63573S"};
+    String[] expectedResults = {"P107M30DT82800S",

Review comment:
       Thanks for the tests! The code changed four functions, yet we run only one test. Should we have tests for each of the four functions? (The underlying cause of this bug is that we did not have proper tests.)
   
   Also, it is not clear what was wrong with the code: corner cases? Should we test such corner cases here? Diff of two dates across Feb. 29 on a leap year? What other things are wrong with the old approach?
   
   Note that, if we have good coverage, then there is no reason to add a test specific for the one case in DRILL-7926. That case should be just one of many that we include in the overall test of the feature.

##########
File path: exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/impl/DateTypeFunctions.java
##########
@@ -424,26 +424,35 @@ public void eval() {
     public static class AgeTimeStampFunction implements DrillSimpleFunc {
         @Param TimeStampHolder left;
         @Param TimeStampHolder right;
+        @Workspace org.joda.time.PeriodType periodType;

Review comment:
       Looks like the period type is constant. Can it be made a `static final` in, say, the `DateUtilities` class? Is the object thread-safe so that it can be shared instead of generating the code into each query (and duplicating the definition)? Looks like we need one for months/days/millis and another for months/days?




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

To unsubscribe, e-mail: dev-unsubscribe@drill.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org