From 901a8d1dccbe5087e41523c3195bd8283d496260 Mon Sep 17 00:00:00 2001 From: Marios Trivyzas Date: Fri, 29 Nov 2019 17:06:26 +0100 Subject: [PATCH] SQL: Fix issues with WEEK/ISO_WEEK/DATEDIFF (#49405) Some extended testing with MS-SQL server and H2 (which agree on results) revealed bugs in the implementation of WEEK related extraction and diff functions. Non-iso WEEK seems to be broken since #48209 because of the replacement of Calendar and the change in the ISO rules. ISO_WEEK failed for some edge cases around the January 1st. DATE_DIFF was previously based on non-iso WEEK extraction which seems not to be the case. Fixes: #49376 (cherry picked from commit 54fe7f57289c46bb0905b1418f51a00e8c581560) --- .../sql/qa/src/main/resources/date.csv-spec | 8 ++-- .../main/resources/datetime-interval.csv-spec | 5 +- .../qa/src/main/resources/datetime.csv-spec | 27 +++++++---- .../function/scalar/datetime/DateDiff.java | 17 +++---- .../scalar/datetime/DateTimeProcessor.java | 7 ++- .../datetime/NonIsoDateTimeProcessor.java | 3 +- .../datetime/DateDiffProcessorTests.java | 44 ++++++++++++++++++ .../datetime/DateTimeProcessorTests.java | 46 +++++++++++++++++++ .../NonIsoDateTimeProcessorTests.java | 44 ++++++++++++++++++ 9 files changed, 172 insertions(+), 29 deletions(-) diff --git a/x-pack/plugin/sql/qa/src/main/resources/date.csv-spec b/x-pack/plugin/sql/qa/src/main/resources/date.csv-spec index 46557c77884..828d1105567 100644 --- a/x-pack/plugin/sql/qa/src/main/resources/date.csv-spec +++ b/x-pack/plugin/sql/qa/src/main/resources/date.csv-spec @@ -85,12 +85,12 @@ YEAR(CAST(birth_date AS DATE)) y, birth_date, last_name l FROM "test_emp" WHERE emp_no < 10010 ORDER BY emp_no; d:i | dm:i | dw:i | dy:i | iso_dw:i | w:i |iso_w:i | q:i | y:i | birth_date:ts | l:s -2 |2 |4 |245 |3 |36 |35 |3 |1953 |1953-09-02T00:00:00Z |Facello -2 |2 |3 |154 |2 |23 |22 |2 |1964 |1964-06-02T00:00:00Z |Simmel +2 |2 |4 |245 |3 |36 |36 |3 |1953 |1953-09-02T00:00:00Z |Facello +2 |2 |3 |154 |2 |23 |23 |2 |1964 |1964-06-02T00:00:00Z |Simmel 3 |3 |5 |337 |4 |49 |49 |4 |1959 |1959-12-03T00:00:00Z |Bamford -1 |1 |7 |121 |6 |18 |18 |2 |1954 |1954-05-01T00:00:00Z |Koblick +1 |1 |7 |121 |6 |18 |17 |2 |1954 |1954-05-01T00:00:00Z |Koblick 21 |21 |6 |21 |5 |4 |3 |1 |1955 |1955-01-21T00:00:00Z |Maliniak -20 |20 |2 |110 |1 |17 |16 |2 |1953 |1953-04-20T00:00:00Z |Preusig +20 |20 |2 |110 |1 |17 |17 |2 |1953 |1953-04-20T00:00:00Z |Preusig 23 |23 |5 |143 |4 |21 |21 |2 |1957 |1957-05-23T00:00:00Z |Zielinski 19 |19 |4 |50 |3 |8 |8 |1 |1958 |1958-02-19T00:00:00Z |Kalloufi 19 |19 |7 |110 |6 |16 |16 |2 |1952 |1952-04-19T00:00:00Z |Peac diff --git a/x-pack/plugin/sql/qa/src/main/resources/datetime-interval.csv-spec b/x-pack/plugin/sql/qa/src/main/resources/datetime-interval.csv-spec index 3a01c7e6565..9bb89408923 100644 --- a/x-pack/plugin/sql/qa/src/main/resources/datetime-interval.csv-spec +++ b/x-pack/plugin/sql/qa/src/main/resources/datetime-interval.csv-spec @@ -313,7 +313,7 @@ SELECT birth_date, MAX(hire_date) - INTERVAL 1 YEAR AS f FROM test_emp GROUP BY ; monthOfDatePlusInterval_And_GroupBy -SELECT WEEK_OF_YEAR(birth_date + INTERVAL 25 YEAR) x, COUNT(*) c FROM test_emp GROUP BY x HAVING c >= 3 ORDER BY c DESC; +SELECT WEEK_OF_YEAR(birth_date + INTERVAL 25 YEAR) x, COUNT(*) c FROM test_emp GROUP BY x HAVING c >= 3 ORDER BY c DESC, x ASC; x:i | c:l ---------------+--------------- @@ -324,8 +324,7 @@ null |10 30 |4 40 |4 45 |4 -1 |3 -8 |3 +8 |3 21 |3 28 |3 32 |3 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 index df94a0d25c0..16550c3e914 100644 --- a/x-pack/plugin/sql/qa/src/main/resources/datetime.csv-spec +++ b/x-pack/plugin/sql/qa/src/main/resources/datetime.csv-spec @@ -110,6 +110,15 @@ SELECT WEEK(birth_date) week, birth_date FROM test_emp ORDER BY WEEK(birth_date) 44 |1961-11-02T00:00:00.000Z ; +weekOfYearVsIsoWeekOfYearEdgeCases +SELECT ISO_WEEK_OF_YEAR('2005-01-01T00:00:00.000Z'::datetime) AS "isow2005", WEEK('2005-01-01T00:00:00.000Z'::datetime) AS "w2005", +ISO_WEEK_OF_YEAR('2007-12-31T00:00:00.000Z'::datetime) AS "isow2007", WEEK('2007-12-31T00:00:00.000Z'::datetime) AS "w2007"; + + isow2005 | w2005 | isow2007 | w2007 +---------------+---------------+---------------+--------------- +53 |1 |1 |53 +; + weekOfYearWithFilter SELECT WEEK(birth_date) week, birth_date FROM test_emp WHERE WEEK(birth_date) > 50 OR WEEK(birth_date) < 4 ORDER BY WEEK(birth_date) DESC, birth_date DESC; @@ -319,7 +328,7 @@ DATEDIFF('milliseconds', '2019-09-04'::date, '2019-09-06'::date) as diff_millis, diff_year | diff_quarter | diff_month | diff_week | diff_day | diff_hours | diff_min | diff_sec | diff_millis | diff_mcsec | diff_nsec -----------+--------------+------------+-----------+----------+------------+----------+-----------+-------------+------------+---------- -9 | -91 | 269 | -611 | 11683 | -64248 | 1676160 | -14083200 | 172800000 | 0 | 0 +9 | -91 | 269 | -610 | 11683 | -64248 | 1676160 | -14083200 | 172800000 | 0 | 0 ; selectDateDiffWithField @@ -331,13 +340,13 @@ FROM test_emp WHERE emp_no >= 10032 AND emp_no <= 10042 ORDER BY 1; emp_no | birth_date | hire_date | diff_year | diff_quarter | diff_month | diff_week | diff_day | diff_min | diff_sec ---------+--------------------------+--------------------------+------------+--------------+------------+-----------+----------+-----------+---------- -10032 | 1960-08-09 00:00:00.000Z | 1990-06-20 00:00:00.000Z | 30 | -119 | 358 | -1559 | 10907 | -15706080 | 942364800 -10033 | 1956-11-14 00:00:00.000Z | 1987-03-18 00:00:00.000Z | 31 | -121 | 364 | -1584 | 11081 | -15956640 | 957398400 +10032 | 1960-08-09 00:00:00.000Z | 1990-06-20 00:00:00.000Z | 30 | -119 | 358 | -1558 | 10907 | -15706080 | 942364800 +10033 | 1956-11-14 00:00:00.000Z | 1987-03-18 00:00:00.000Z | 31 | -121 | 364 | -1583 | 11081 | -15956640 | 957398400 10034 | 1962-12-29 00:00:00.000Z | 1988-09-21 00:00:00.000Z | 26 | -103 | 309 | -1343 | 9398 | -13533120 | 811987200 -10035 | 1953-02-08 00:00:00.000Z | 1988-09-05 00:00:00.000Z | 35 | -142 | 427 | -1857 | 12993 | -18709920 | 1122595200 -10036 | 1959-08-10 00:00:00.000Z | 1992-01-03 00:00:00.000Z | 33 | -130 | 389 | -1691 | 11834 | -17040960 | 1022457600 -10037 | 1963-07-22 00:00:00.000Z | 1990-12-05 00:00:00.000Z | 27 | -109 | 329 | -1429 | 9998 | -14397120 | 863827200 -10038 | 1960-07-20 00:00:00.000Z | 1989-09-20 00:00:00.000Z | 29 | -116 | 350 | -1523 | 10654 | -15341760 | 920505600 +10035 | 1953-02-08 00:00:00.000Z | 1988-09-05 00:00:00.000Z | 35 | -142 | 427 | -1856 | 12993 | -18709920 | 1122595200 +10036 | 1959-08-10 00:00:00.000Z | 1992-01-03 00:00:00.000Z | 33 | -130 | 389 | -1690 | 11834 | -17040960 | 1022457600 +10037 | 1963-07-22 00:00:00.000Z | 1990-12-05 00:00:00.000Z | 27 | -109 | 329 | -1428 | 9998 | -14397120 | 863827200 +10038 | 1960-07-20 00:00:00.000Z | 1989-09-20 00:00:00.000Z | 29 | -116 | 350 | -1522 | 10654 | -15341760 | 920505600 10039 | 1959-10-01 00:00:00.000Z | 1988-01-19 00:00:00.000Z | 29 | -113 | 339 | -1477 | 10337 | -14885280 | 893116800 10040 | null | 1993-02-14 00:00:00.000Z | null | null | null | null | null | null | null 10041 | null | 1989-11-12 00:00:00.000Z | null | null | null | null | null | null | null @@ -451,8 +460,8 @@ SELECT count(*) as count, DATE_DIFF('weeks', birth_date, hire_date) diff FROM te count | diff ---------+------ 10 | null -1 | 1121 -1 | 1124 +1 | 1120 +1 | 1123 1 | 1168 1 | 1196 ; diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/datetime/DateDiff.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/datetime/DateDiff.java index 8dddd9343e0..4c7def3d1b5 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/datetime/DateDiff.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/datetime/DateDiff.java @@ -27,7 +27,8 @@ import java.util.function.BiFunction; import static org.elasticsearch.common.logging.LoggerMessageFormat.format; import static org.elasticsearch.xpack.sql.expression.TypeResolutions.isDate; import static org.elasticsearch.xpack.sql.expression.TypeResolutions.isString; -import static org.elasticsearch.xpack.sql.expression.function.scalar.datetime.NonIsoDateTimeProcessor.NonIsoDateTimeExtractor; +import static org.elasticsearch.xpack.sql.util.DateUtils.DAY_IN_MILLIS; +import static org.elasticsearch.xpack.sql.util.DateUtils.UTC; public class DateDiff extends ThreeArgsDateTimeFunction { @@ -41,15 +42,11 @@ public class DateDiff extends ThreeArgsDateTimeFunction { DAYOFYEAR((start, end) -> safeInt(diffInDays(start, end)), "dy", "y"), DAY(DAYOFYEAR::diff, "days", "dd", "d"), WEEK((start, end) -> { - int extraWeek = NonIsoDateTimeExtractor.WEEK_OF_YEAR.extract(end) - - NonIsoDateTimeExtractor.WEEK_OF_YEAR.extract(start) == 0 ? 0 : 1; - long diffWeeks = diffInDays(start, end) / 7; - if (diffWeeks < 0) { - diffWeeks -= extraWeek; - } else { - diffWeeks += extraWeek; - } - return safeInt(diffWeeks); + long startInDays = start.toInstant().toEpochMilli() / DAY_IN_MILLIS - + DatePart.Part.WEEKDAY.extract(start.withZoneSameInstant(UTC)); + long endInDays = end.toInstant().toEpochMilli() / DAY_IN_MILLIS - + DatePart.Part.WEEKDAY.extract(end.withZoneSameInstant(UTC)); + return safeInt((endInDays - startInDays) / 7); }, "weeks", "wk", "ww"), WEEKDAY(DAYOFYEAR::diff, "weekdays", "dw"), HOUR((start, end) -> safeInt(diffInHours(start, end)), "hours", "hh"), diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/datetime/DateTimeProcessor.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/datetime/DateTimeProcessor.java index d0f7b5d9afc..758c1e0cd6f 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/datetime/DateTimeProcessor.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/datetime/DateTimeProcessor.java @@ -13,6 +13,7 @@ import java.time.OffsetTime; import java.time.ZoneId; import java.time.ZonedDateTime; import java.time.temporal.ChronoField; +import java.time.temporal.WeekFields; import java.util.Objects; public class DateTimeProcessor extends BaseDateTimeProcessor { @@ -36,7 +37,11 @@ public class DateTimeProcessor extends BaseDateTimeProcessor { } public int extract(ZonedDateTime dt) { - return dt.get(field); + if (field == ChronoField.ALIGNED_WEEK_OF_YEAR) { + return dt.get(WeekFields.ISO.weekOfWeekBasedYear()); + } else { + return dt.get(field); + } } public int extract(OffsetTime time) { diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/datetime/NonIsoDateTimeProcessor.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/datetime/NonIsoDateTimeProcessor.java index 785a815a45c..f6f0f2ba6e1 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/datetime/NonIsoDateTimeProcessor.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/datetime/NonIsoDateTimeProcessor.java @@ -10,7 +10,6 @@ import org.elasticsearch.common.io.stream.StreamInput; import org.elasticsearch.common.io.stream.StreamOutput; import java.io.IOException; -import java.time.DayOfWeek; import java.time.ZoneId; import java.time.ZonedDateTime; import java.time.temporal.ChronoField; @@ -28,7 +27,7 @@ public class NonIsoDateTimeProcessor extends BaseDateTimeProcessor { return dayOfWeek == 8 ? 1 : dayOfWeek; }), WEEK_OF_YEAR(zdt -> { - return zdt.get(WeekFields.of(DayOfWeek.SUNDAY, 1).weekOfWeekBasedYear()); + return zdt.get(WeekFields.SUNDAY_START.weekOfYear()); }); private final Function apply; diff --git a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/expression/function/scalar/datetime/DateDiffProcessorTests.java b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/expression/function/scalar/datetime/DateDiffProcessorTests.java index 77531991793..19d329cc424 100644 --- a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/expression/function/scalar/datetime/DateDiffProcessorTests.java +++ b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/expression/function/scalar/datetime/DateDiffProcessorTests.java @@ -274,6 +274,17 @@ public class DateDiffProcessorTests extends AbstractSqlWireSerializingTestCase