SQL: Fix issue with mins & hours for DATEDIFF (elastic#49252)

Previously, DATEDIFF for minutes and hours was doing a rounding calculation using all the time fields (secs, msecs/micros/nanos). Instead it should first truncate the 2 dates to the respective field (mins or hours) zeroing out all the more detailed time fields and then make the subtraction. (cherry picked from commit 124cd18)
original-brownbear · Nov 19, 2019 · fd1bb4a · fd1bb4a
1 parent 19602fd
commit fd1bb4a
Show file tree

Hide file tree

Showing 5 changed files with 93 additions and 7 deletions.
diff --git a/docs/reference/sql/functions/date-time.asciidoc b/docs/reference/sql/functions/date-time.asciidoc
@@ -395,6 +395,20 @@ include-tagged::{sql-specs}/docs/docs.csv-spec[dateDiffDateTimeSeconds]
 include-tagged::{sql-specs}/docs/docs.csv-spec[dateDiffDateQuarters]
 --------------------------------------------------
 
+[NOTE]
+For `hour` and `minute`, `DATEDIFF` doesn't do any rounding, but instead first truncates
+the more detailed time fields on the 2 dates to zero and then calculates the subtraction.
+
+[source, sql]
+--------------------------------------------------
+include-tagged::{sql-specs}/docs/docs.csv-spec[dateDiffDateTimeHours]
+--------------------------------------------------
+
+[source, sql]
+--------------------------------------------------
+include-tagged::{sql-specs}/docs/docs.csv-spec[dateDiffDateTimeMinutes]
+--------------------------------------------------
+
 [source, sql]
 --------------------------------------------------
 include-tagged::{sql-specs}/docs/docs.csv-spec[dateDiffDateMinutes]

diff --git a/x-pack/plugin/sql/qa/src/main/resources/datetime.csv-spec b/x-pack/plugin/sql/qa/src/main/resources/datetime.csv-spec
@@ -307,7 +307,7 @@ DATE_DIFF('mcs', '2019-09-04T11:25:21.123456Z'::datetime, '2019-09-04T11:22:33.9
 
  diff_year | diff_quarter | diff_month | diff_week | diff_day | diff_hours | diff_min | diff_sec  | diff_millis | diff_mcsec | diff_nsec
 -----------+--------------+------------+-----------+----------+------------+----------+-----------+-------------+------------+----------
-57         | -114         | 406        | -947      | 2825     | -123228    | 3762357  | -10265677 | 205849864   | -167135802 | 135802468
+57         | -114         | 406        | -947      | 2825     | -123228    | 3762356  | -10265677 | 205849864   | -167135802 | 135802468
 ;
 
 selectDiffWithDate

diff --git a/x-pack/plugin/sql/qa/src/main/resources/docs/docs.csv-spec b/x-pack/plugin/sql/qa/src/main/resources/docs/docs.csv-spec
@@ -2495,6 +2495,27 @@ SELECT DATE_DIFF('week', '2019-09-04T11:22:33.000Z'::datetime, '2016-12-08T22:33
 // end::dateDiffDateTimeWeeks
 ;
 
+dateDiffDateTimeHours
+// tag::dateDiffDateTimeHours
+SELECT DATEDIFF('hours', '2019-11-10T12:10:00.000Z'::datetime, '2019-11-10T23:59:59.999Z'::datetime) AS "diffInHours";
+
+      diffInHours
+------------------------
+11
+// end::dateDiffDateTimeHours
+;
+
+
+dateDiffDateTimeMinutes
+// tag::dateDiffDateTimeMinutes
+SELECT DATEDIFF('minute', '2019-11-10T12:10:00.000Z'::datetime, '2019-11-10T12:15:59.999Z'::datetime) AS "diffInMinutes";
+
+      diffInMinutes
+------------------------
+5
+// end::dateDiffDateTimeMinutes
+;
+
 dateDiffDateTimeSeconds
 // tag::dateDiffDateTimeSeconds
 SELECT DATE_DIFF('seconds', '2019-09-04T11:22:33.123Z'::datetime, '2019-07-12T22:33:11.321Z'::datetime) AS "diffInSeconds";

diff --git a/...c/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/datetime/DateDiff.java b/...c/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/datetime/DateDiff.java
@@ -116,12 +116,8 @@ private static int safeInt(long diff) {
         }
 
         private static long diffInMinutes(ZonedDateTime start, ZonedDateTime end) {
-            long secondsDiff = diffInSeconds(start, end);
-            if (secondsDiff > 0) {
-                return (long) Math.ceil(secondsDiff / 60.0d);
-            } else {
-                return (long) Math.floor(secondsDiff / 60.0d);
-            }
+            // Truncate first to minutes (ignore any seconds and sub-seconds fields)
+            return (end.toEpochSecond() / 60) - (start.toEpochSecond() / 60);
         }
 
         private static long diffInHours(ZonedDateTime start, ZonedDateTime end) {

diff --git a/...g/elasticsearch/xpack/sql/expression/function/scalar/datetime/DateDiffProcessorTests.java b/...g/elasticsearch/xpack/sql/expression/function/scalar/datetime/DateDiffProcessorTests.java
@@ -284,6 +284,61 @@ public void testDiffEdgeCases() {
             .makePipe().asProcessor().process(null));
         assertEquals(-436, new DateDiff(Source.EMPTY, l("ww"), dt2, dt1, zoneId)
             .makePipe().asProcessor().process(null));
+
+        dt1 = l(dateTime(1997, 9, 19, 0, 0, 0, 0));
+        dt2 = l(dateTime(2004, 8, 2, 7, 59, 23, 0));
+        assertEquals(60223, new DateDiff(Source.EMPTY, l("hour"), dt1, dt2, UTC)
+            .makePipe().asProcessor().process(null));
+        assertEquals(-60223, new DateDiff(Source.EMPTY, l("hours"), dt2, dt1, UTC)
+            .makePipe().asProcessor().process(null));
+        assertEquals(60223, new DateDiff(Source.EMPTY, l("hh"), dt1, dt2, zoneId)
+            .makePipe().asProcessor().process(null));
+        assertEquals(-60223, new DateDiff(Source.EMPTY, l("hh"), dt2, dt1, zoneId)
+            .makePipe().asProcessor().process(null));
+
+        dt1 = l(dateTime(1997, 9, 19, 0, 0, 0, 0));
+        dt2 = l(dateTime(2004, 8, 2, 7, 59, 59, 999999999));
+        assertEquals(60223, new DateDiff(Source.EMPTY, l("hour"), dt1, dt2, UTC)
+            .makePipe().asProcessor().process(null));
+        assertEquals(-60223, new DateDiff(Source.EMPTY, l("hours"), dt2, dt1, UTC)
+            .makePipe().asProcessor().process(null));
+        assertEquals(60223, new DateDiff(Source.EMPTY, l("hh"), dt1, dt2, zoneId)
+            .makePipe().asProcessor().process(null));
+        assertEquals(-60223, new DateDiff(Source.EMPTY, l("hh"), dt2, dt1, zoneId)
+            .makePipe().asProcessor().process(null));
+
+        dt1 = l(dateTime(2002, 4, 27, 0, 0, 0, 0));
+        dt2 = l(dateTime(2004, 7, 28, 12, 34, 28, 0));
+        assertEquals(1185874, new DateDiff(Source.EMPTY, l("minute"), dt1, dt2, UTC)
+            .makePipe().asProcessor().process(null));
+        assertEquals(-1185874, new DateDiff(Source.EMPTY, l("minutes"), dt2, dt1, UTC)
+            .makePipe().asProcessor().process(null));
+        assertEquals(1185874, new DateDiff(Source.EMPTY, l("mi"), dt1, dt2, zoneId)
+            .makePipe().asProcessor().process(null));
+        assertEquals(-1185874, new DateDiff(Source.EMPTY, l("n"), dt2, dt1, zoneId)
+            .makePipe().asProcessor().process(null));
+
+        dt1 = l(dateTime(1995, 9, 3, 0, 0, 0, 0));
+        dt2 = l(dateTime(2004, 7, 26, 12, 30, 34, 0));
+        assertEquals(4679310, new DateDiff(Source.EMPTY, l("minute"), dt1, dt2, UTC)
+            .makePipe().asProcessor().process(null));
+        assertEquals(-4679310, new DateDiff(Source.EMPTY, l("minutes"), dt2, dt1, UTC)
+            .makePipe().asProcessor().process(null));
+        assertEquals(4679310, new DateDiff(Source.EMPTY, l("mi"), dt1, dt2, zoneId)
+            .makePipe().asProcessor().process(null));
+        assertEquals(-4679310, new DateDiff(Source.EMPTY, l("n"), dt2, dt1, zoneId)
+            .makePipe().asProcessor().process(null));
+
+        dt1 = l(dateTime(1997, 5, 30, 0, 0, 0, 0));
+        dt2 = l(dateTime(2004, 7, 28, 23, 30, 59, 999999999));
+        assertEquals(3768450, new DateDiff(Source.EMPTY, l("minute"), dt1, dt2, UTC)
+            .makePipe().asProcessor().process(null));
+        assertEquals(-3768450, new DateDiff(Source.EMPTY, l("minutes"), dt2, dt1, UTC)
+            .makePipe().asProcessor().process(null));
+        assertEquals(3768450, new DateDiff(Source.EMPTY, l("mi"), dt1, dt2, zoneId)
+            .makePipe().asProcessor().process(null));
+        assertEquals(-3768450, new DateDiff(Source.EMPTY, l("n"), dt2, dt1, zoneId)
+            .makePipe().asProcessor().process(null));
     }
 
     public void testOverflow() {