Skip to content

Commit d62579c

Browse files
matrivSivagurunathanV
matriv
authored and
SivagurunathanV
committed
SQL: Fix issues with WEEK/ISO_WEEK/DATEDIFF (elastic#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 elastic#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: elastic#49376
1 parent dde2a06 commit d62579c

File tree

9 files changed

+172
-29
lines changed

9 files changed

+172
-29
lines changed

‎x-pack/plugin/sql/qa/src/main/resources/date.csv-spec

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -85,12 +85,12 @@ YEAR(CAST(birth_date AS DATE)) y,
8585
birth_date, last_name l FROM "test_emp" WHERE emp_no < 10010 ORDER BY emp_no;
8686

8787
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
88-
2 |2 |4 |245 |3 |36 |35 |3 |1953 |1953-09-02T00:00:00Z |Facello
89-
2 |2 |3 |154 |2 |23 |22 |2 |1964 |1964-06-02T00:00:00Z |Simmel
88+
2 |2 |4 |245 |3 |36 |36 |3 |1953 |1953-09-02T00:00:00Z |Facello
89+
2 |2 |3 |154 |2 |23 |23 |2 |1964 |1964-06-02T00:00:00Z |Simmel
9090
3 |3 |5 |337 |4 |49 |49 |4 |1959 |1959-12-03T00:00:00Z |Bamford
91-
1 |1 |7 |121 |6 |18 |18 |2 |1954 |1954-05-01T00:00:00Z |Koblick
91+
1 |1 |7 |121 |6 |18 |17 |2 |1954 |1954-05-01T00:00:00Z |Koblick
9292
21 |21 |6 |21 |5 |4 |3 |1 |1955 |1955-01-21T00:00:00Z |Maliniak
93-
20 |20 |2 |110 |1 |17 |16 |2 |1953 |1953-04-20T00:00:00Z |Preusig
93+
20 |20 |2 |110 |1 |17 |17 |2 |1953 |1953-04-20T00:00:00Z |Preusig
9494
23 |23 |5 |143 |4 |21 |21 |2 |1957 |1957-05-23T00:00:00Z |Zielinski
9595
19 |19 |4 |50 |3 |8 |8 |1 |1958 |1958-02-19T00:00:00Z |Kalloufi
9696
19 |19 |7 |110 |6 |16 |16 |2 |1952 |1952-04-19T00:00:00Z |Peac

‎x-pack/plugin/sql/qa/src/main/resources/datetime-interval.csv-spec

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -313,7 +313,7 @@ SELECT birth_date, MAX(hire_date) - INTERVAL 1 YEAR AS f FROM test_emp GROUP BY
313313
;
314314

315315
monthOfDatePlusInterval_And_GroupBy
316-
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;
316+
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;
317317

318318
x:i | c:l
319319
---------------+---------------
@@ -324,8 +324,7 @@ null |10
324324
30 |4
325325
40 |4
326326
45 |4
327-
1 |3
328-
8 |3
327+
8 |3
329328
21 |3
330329
28 |3
331330
32 |3

‎x-pack/plugin/sql/qa/src/main/resources/datetime.csv-spec

Lines changed: 18 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -110,6 +110,15 @@ SELECT WEEK(birth_date) week, birth_date FROM test_emp ORDER BY WEEK(birth_date)
110110
44 |1961-11-02T00:00:00.000Z
111111
;
112112

113+
weekOfYearVsIsoWeekOfYearEdgeCases
114+
SELECT ISO_WEEK_OF_YEAR('2005-01-01T00:00:00.000Z'::datetime) AS "isow2005", WEEK('2005-01-01T00:00:00.000Z'::datetime) AS "w2005",
115+
ISO_WEEK_OF_YEAR('2007-12-31T00:00:00.000Z'::datetime) AS "isow2007", WEEK('2007-12-31T00:00:00.000Z'::datetime) AS "w2007";
116+
117+
isow2005 | w2005 | isow2007 | w2007
118+
---------------+---------------+---------------+---------------
119+
53 |1 |1 |53
120+
;
121+
113122
weekOfYearWithFilter
114123
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;
115124

@@ -319,7 +328,7 @@ DATEDIFF('milliseconds', '2019-09-04'::date, '2019-09-06'::date) as diff_millis,
319328

320329
diff_year | diff_quarter | diff_month | diff_week | diff_day | diff_hours | diff_min | diff_sec | diff_millis | diff_mcsec | diff_nsec
321330
-----------+--------------+------------+-----------+----------+------------+----------+-----------+-------------+------------+----------
322-
9 | -91 | 269 | -611 | 11683 | -64248 | 1676160 | -14083200 | 172800000 | 0 | 0
331+
9 | -91 | 269 | -610 | 11683 | -64248 | 1676160 | -14083200 | 172800000 | 0 | 0
323332
;
324333

325334
selectDateDiffWithField
@@ -331,13 +340,13 @@ FROM test_emp WHERE emp_no >= 10032 AND emp_no <= 10042 ORDER BY 1;
331340

332341
emp_no | birth_date | hire_date | diff_year | diff_quarter | diff_month | diff_week | diff_day | diff_min | diff_sec
333342
---------+--------------------------+--------------------------+------------+--------------+------------+-----------+----------+-----------+----------
334-
10032 | 1960-08-09 00:00:00.000Z | 1990-06-20 00:00:00.000Z | 30 | -119 | 358 | -1559 | 10907 | -15706080 | 942364800
335-
10033 | 1956-11-14 00:00:00.000Z | 1987-03-18 00:00:00.000Z | 31 | -121 | 364 | -1584 | 11081 | -15956640 | 957398400
343+
10032 | 1960-08-09 00:00:00.000Z | 1990-06-20 00:00:00.000Z | 30 | -119 | 358 | -1558 | 10907 | -15706080 | 942364800
344+
10033 | 1956-11-14 00:00:00.000Z | 1987-03-18 00:00:00.000Z | 31 | -121 | 364 | -1583 | 11081 | -15956640 | 957398400
336345
10034 | 1962-12-29 00:00:00.000Z | 1988-09-21 00:00:00.000Z | 26 | -103 | 309 | -1343 | 9398 | -13533120 | 811987200
337-
10035 | 1953-02-08 00:00:00.000Z | 1988-09-05 00:00:00.000Z | 35 | -142 | 427 | -1857 | 12993 | -18709920 | 1122595200
338-
10036 | 1959-08-10 00:00:00.000Z | 1992-01-03 00:00:00.000Z | 33 | -130 | 389 | -1691 | 11834 | -17040960 | 1022457600
339-
10037 | 1963-07-22 00:00:00.000Z | 1990-12-05 00:00:00.000Z | 27 | -109 | 329 | -1429 | 9998 | -14397120 | 863827200
340-
10038 | 1960-07-20 00:00:00.000Z | 1989-09-20 00:00:00.000Z | 29 | -116 | 350 | -1523 | 10654 | -15341760 | 920505600
346+
10035 | 1953-02-08 00:00:00.000Z | 1988-09-05 00:00:00.000Z | 35 | -142 | 427 | -1856 | 12993 | -18709920 | 1122595200
347+
10036 | 1959-08-10 00:00:00.000Z | 1992-01-03 00:00:00.000Z | 33 | -130 | 389 | -1690 | 11834 | -17040960 | 1022457600
348+
10037 | 1963-07-22 00:00:00.000Z | 1990-12-05 00:00:00.000Z | 27 | -109 | 329 | -1428 | 9998 | -14397120 | 863827200
349+
10038 | 1960-07-20 00:00:00.000Z | 1989-09-20 00:00:00.000Z | 29 | -116 | 350 | -1522 | 10654 | -15341760 | 920505600
341350
10039 | 1959-10-01 00:00:00.000Z | 1988-01-19 00:00:00.000Z | 29 | -113 | 339 | -1477 | 10337 | -14885280 | 893116800
342351
10040 | null | 1993-02-14 00:00:00.000Z | null | null | null | null | null | null | null
343352
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
451460
count | diff
452461
---------+------
453462
10 | null
454-
1 | 1121
455-
1 | 1124
463+
1 | 1120
464+
1 | 1123
456465
1 | 1168
457466
1 | 1196
458467
;

‎x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/datetime/DateDiff.java

Lines changed: 7 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,8 @@
2525
import static org.elasticsearch.common.logging.LoggerMessageFormat.format;
2626
import static org.elasticsearch.xpack.sql.expression.TypeResolutions.isDate;
2727
import static org.elasticsearch.xpack.sql.expression.TypeResolutions.isString;
28-
import static org.elasticsearch.xpack.sql.expression.function.scalar.datetime.NonIsoDateTimeProcessor.NonIsoDateTimeExtractor;
28+
import static org.elasticsearch.xpack.sql.util.DateUtils.DAY_IN_MILLIS;
29+
import static org.elasticsearch.xpack.sql.util.DateUtils.UTC;
2930

3031
public class DateDiff extends ThreeArgsDateTimeFunction {
3132

@@ -39,15 +40,11 @@ public enum Part implements DateTimeField {
3940
DAYOFYEAR((start, end) -> safeInt(diffInDays(start, end)), "dy", "y"),
4041
DAY(DAYOFYEAR::diff, "days", "dd", "d"),
4142
WEEK((start, end) -> {
42-
int extraWeek = NonIsoDateTimeExtractor.WEEK_OF_YEAR.extract(end) -
43-
NonIsoDateTimeExtractor.WEEK_OF_YEAR.extract(start) == 0 ? 0 : 1;
44-
long diffWeeks = diffInDays(start, end) / 7;
45-
if (diffWeeks < 0) {
46-
diffWeeks -= extraWeek;
47-
} else {
48-
diffWeeks += extraWeek;
49-
}
50-
return safeInt(diffWeeks);
43+
long startInDays = start.toInstant().toEpochMilli() / DAY_IN_MILLIS -
44+
DatePart.Part.WEEKDAY.extract(start.withZoneSameInstant(UTC));
45+
long endInDays = end.toInstant().toEpochMilli() / DAY_IN_MILLIS -
46+
DatePart.Part.WEEKDAY.extract(end.withZoneSameInstant(UTC));
47+
return safeInt((endInDays - startInDays) / 7);
5148
}, "weeks", "wk", "ww"),
5249
WEEKDAY(DAYOFYEAR::diff, "weekdays", "dw"),
5350
HOUR((start, end) -> safeInt(diffInHours(start, end)), "hours", "hh"),

‎x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/datetime/DateTimeProcessor.java

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@
1313
import java.time.ZoneId;
1414
import java.time.ZonedDateTime;
1515
import java.time.temporal.ChronoField;
16+
import java.time.temporal.WeekFields;
1617
import java.util.Objects;
1718

1819
public class DateTimeProcessor extends BaseDateTimeProcessor {
@@ -36,7 +37,11 @@ public enum DateTimeExtractor {
3637
}
3738

3839
public int extract(ZonedDateTime dt) {
39-
return dt.get(field);
40+
if (field == ChronoField.ALIGNED_WEEK_OF_YEAR) {
41+
return dt.get(WeekFields.ISO.weekOfWeekBasedYear());
42+
} else {
43+
return dt.get(field);
44+
}
4045
}
4146

4247
public int extract(OffsetTime time) {

‎x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/datetime/NonIsoDateTimeProcessor.java

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,6 @@
1010
import org.elasticsearch.common.io.stream.StreamOutput;
1111

1212
import java.io.IOException;
13-
import java.time.DayOfWeek;
1413
import java.time.ZoneId;
1514
import java.time.ZonedDateTime;
1615
import java.time.temporal.ChronoField;
@@ -28,7 +27,7 @@ public enum NonIsoDateTimeExtractor {
2827
return dayOfWeek == 8 ? 1 : dayOfWeek;
2928
}),
3029
WEEK_OF_YEAR(zdt -> {
31-
return zdt.get(WeekFields.of(DayOfWeek.SUNDAY, 1).weekOfWeekBasedYear());
30+
return zdt.get(WeekFields.SUNDAY_START.weekOfYear());
3231
});
3332

3433
private final Function<ZonedDateTime, Integer> apply;

‎x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/expression/function/scalar/datetime/DateDiffProcessorTests.java

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -274,6 +274,17 @@ public void testDiffEdgeCases() {
274274
assertEquals(-350, new DateDiff(Source.EMPTY, l("ww"), dt2, dt1, zoneId)
275275
.makePipe().asProcessor().process(null));
276276

277+
dt1 = l(dateTime(1988, 1, 2, 0, 0, 0, 0));
278+
dt2 = l(dateTime(1987, 12, 29, 0, 0, 0, 0));
279+
assertEquals(0, new DateDiff(Source.EMPTY, l("week"), dt1, dt2, UTC)
280+
.makePipe().asProcessor().process(null));
281+
assertEquals(0, new DateDiff(Source.EMPTY, l("weeks"), dt2, dt1, UTC)
282+
.makePipe().asProcessor().process(null));
283+
assertEquals(0, new DateDiff(Source.EMPTY, l("wk"), dt1, dt2, zoneId)
284+
.makePipe().asProcessor().process(null));
285+
assertEquals(0, new DateDiff(Source.EMPTY, l("ww"), dt2, dt1, zoneId)
286+
.makePipe().asProcessor().process(null));
287+
277288
dt1 = l(dateTime(1988, 1, 5, 0, 0, 0, 0));
278289
dt2 = l(dateTime(1996, 5, 13, 0, 0, 0, 0));
279290
assertEquals(436, new DateDiff(Source.EMPTY, l("week"), dt1, dt2, UTC)
@@ -285,6 +296,39 @@ public void testDiffEdgeCases() {
285296
assertEquals(-436, new DateDiff(Source.EMPTY, l("ww"), dt2, dt1, zoneId)
286297
.makePipe().asProcessor().process(null));
287298

299+
dt1 = l(dateTime(1999, 8, 20, 0, 0, 0, 0));
300+
dt2 = l(dateTime(1974, 3, 17, 0, 0, 0, 0));
301+
assertEquals(-1326, new DateDiff(Source.EMPTY, l("week"), dt1, dt2, UTC)
302+
.makePipe().asProcessor().process(null));
303+
assertEquals(1326, new DateDiff(Source.EMPTY, l("weeks"), dt2, dt1, UTC)
304+
.makePipe().asProcessor().process(null));
305+
assertEquals(-1326, new DateDiff(Source.EMPTY, l("wk"), dt1, dt2, zoneId)
306+
.makePipe().asProcessor().process(null));
307+
assertEquals(1326, new DateDiff(Source.EMPTY, l("ww"), dt2, dt1, zoneId)
308+
.makePipe().asProcessor().process(null));
309+
310+
dt1 = l(dateTime(1997, 2, 2, 0, 0, 0, 0));
311+
dt2 = l(dateTime(1997, 9, 19, 0, 0, 0, 0));
312+
assertEquals(32, new DateDiff(Source.EMPTY, l("week"), dt1, dt2, UTC)
313+
.makePipe().asProcessor().process(null));
314+
assertEquals(-32, new DateDiff(Source.EMPTY, l("weeks"), dt2, dt1, UTC)
315+
.makePipe().asProcessor().process(null));
316+
assertEquals(32, new DateDiff(Source.EMPTY, l("wk"), dt1, dt2, zoneId)
317+
.makePipe().asProcessor().process(null));
318+
assertEquals(-32, new DateDiff(Source.EMPTY, l("ww"), dt2, dt1, zoneId)
319+
.makePipe().asProcessor().process(null));
320+
321+
dt1 = l(dateTime(1980, 11, 7, 0, 0, 0, 0));
322+
dt2 = l(dateTime(1979, 4, 1, 0, 0, 0, 0));
323+
assertEquals(-83, new DateDiff(Source.EMPTY, l("week"), dt1, dt2, UTC)
324+
.makePipe().asProcessor().process(null));
325+
assertEquals(83, new DateDiff(Source.EMPTY, l("weeks"), dt2, dt1, UTC)
326+
.makePipe().asProcessor().process(null));
327+
assertEquals(-83, new DateDiff(Source.EMPTY, l("wk"), dt1, dt2, zoneId)
328+
.makePipe().asProcessor().process(null));
329+
assertEquals(83, new DateDiff(Source.EMPTY, l("ww"), dt2, dt1, zoneId)
330+
.makePipe().asProcessor().process(null));
331+
288332
dt1 = l(dateTime(1997, 9, 19, 0, 0, 0, 0));
289333
dt2 = l(dateTime(2004, 8, 2, 7, 59, 23, 0));
290334
assertEquals(60223, new DateDiff(Source.EMPTY, l("hour"), dt1, dt2, UTC)

‎x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/expression/function/scalar/datetime/DateTimeProcessorTests.java

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -53,6 +53,29 @@ public void testApply_withTimezoneUTC() {
5353
assertEquals(1, proc.process(dateTime(0L)));
5454
assertEquals(2, proc.process(dateTime(2017, 01, 02, 10, 10)));
5555
assertEquals(31, proc.process(dateTime(2017, 01, 31, 10, 10)));
56+
57+
// Tested against MS-SQL Server and H2
58+
proc = new DateTimeProcessor(DateTimeExtractor.ISO_WEEK_OF_YEAR, UTC);
59+
assertEquals(1, proc.process(dateTime(1988, 1, 5, 0, 0, 0, 0)));
60+
assertEquals(5, proc.process(dateTime(2001, 2, 4, 0, 0, 0, 0)));
61+
assertEquals(6, proc.process(dateTime(1977, 2, 8, 0, 0, 0, 0)));
62+
assertEquals(11, proc.process(dateTime(1974, 3, 17, 0, 0, 0, 0)));
63+
assertEquals(16, proc.process(dateTime(1977, 4, 20, 0, 0, 0, 0)));
64+
assertEquals(16, proc.process(dateTime(1994, 4, 20, 0, 0, 0, 0)));
65+
assertEquals(17, proc.process(dateTime(2002, 4, 27, 0, 0, 0, 0)));
66+
assertEquals(18, proc.process(dateTime(1974, 5, 3, 0, 0, 0, 0)));
67+
assertEquals(22, proc.process(dateTime(1997, 5, 30, 0, 0, 0, 0)));
68+
assertEquals(22, proc.process(dateTime(1995, 6, 4, 0, 0, 0, 0)));
69+
assertEquals(28, proc.process(dateTime(1972, 7, 12, 0, 0, 0, 0)));
70+
assertEquals(30, proc.process(dateTime(1980, 7, 26, 0, 0, 0, 0)));
71+
assertEquals(33, proc.process(dateTime(1998, 8, 12, 0, 0, 0, 0)));
72+
assertEquals(35, proc.process(dateTime(1995, 9, 3, 0, 0, 0, 0)));
73+
assertEquals(37, proc.process(dateTime(1976, 9, 9, 0, 0, 0, 0)));
74+
assertEquals(38, proc.process(dateTime(1997, 9, 19, 0, 0, 0, 0)));
75+
assertEquals(45, proc.process(dateTime(1980, 11, 7, 0, 0, 0, 0)));
76+
assertEquals(53, proc.process(dateTime(2005, 1, 1, 0, 0, 0, 0)));
77+
assertEquals(1, proc.process(dateTime(2007, 12, 31, 0, 0, 0, 0)));
78+
assertEquals(1, proc.process(dateTime(2019, 12, 31, 20, 22, 33, 987654321)));
5679
}
5780

5881
public void testApply_withTimezoneOtherThanUTC() {
@@ -62,6 +85,29 @@ public void testApply_withTimezoneOtherThanUTC() {
6285

6386
proc = new DateTimeProcessor(DateTimeExtractor.DAY_OF_MONTH, zoneId);
6487
assertEquals(1, proc.process(dateTime(2017, 12, 31, 20, 30)));
88+
89+
// Tested against MS-SQL Server and H2
90+
proc = new DateTimeProcessor(DateTimeExtractor.ISO_WEEK_OF_YEAR, UTC);
91+
assertEquals(1, proc.process(dateTime(1988, 1, 5, 0, 0, 0, 0)));
92+
assertEquals(5, proc.process(dateTime(2001, 2, 4, 0, 0, 0, 0)));
93+
assertEquals(6, proc.process(dateTime(1977, 2, 8, 0, 0, 0, 0)));
94+
assertEquals(11, proc.process(dateTime(1974, 3, 17, 0, 0, 0, 0)));
95+
assertEquals(16, proc.process(dateTime(1977, 4, 20, 0, 0, 0, 0)));
96+
assertEquals(16, proc.process(dateTime(1994, 4, 20, 0, 0, 0, 0)));
97+
assertEquals(17, proc.process(dateTime(2002, 4, 27, 0, 0, 0, 0)));
98+
assertEquals(18, proc.process(dateTime(1974, 5, 3, 0, 0, 0, 0)));
99+
assertEquals(22, proc.process(dateTime(1997, 5, 30, 0, 0, 0, 0)));
100+
assertEquals(22, proc.process(dateTime(1995, 6, 4, 0, 0, 0, 0)));
101+
assertEquals(28, proc.process(dateTime(1972, 7, 12, 0, 0, 0, 0)));
102+
assertEquals(30, proc.process(dateTime(1980, 7, 26, 0, 0, 0, 0)));
103+
assertEquals(33, proc.process(dateTime(1998, 8, 12, 0, 0, 0, 0)));
104+
assertEquals(35, proc.process(dateTime(1995, 9, 3, 0, 0, 0, 0)));
105+
assertEquals(37, proc.process(dateTime(1976, 9, 9, 0, 0, 0, 0)));
106+
assertEquals(38, proc.process(dateTime(1997, 9, 19, 0, 0, 0, 0)));
107+
assertEquals(45, proc.process(dateTime(1980, 11, 7, 0, 0, 0, 0)));
108+
assertEquals(53, proc.process(dateTime(2005, 1, 1, 0, 0, 0, 0)));
109+
assertEquals(1, proc.process(dateTime(2007, 12, 31, 0, 0, 0, 0)));
110+
assertEquals(1, proc.process(dateTime(2019, 12, 31, 20, 22, 33, 987654321)));
65111
}
66112

67113
public void testFailOnTime() {

0 commit comments

Comments
 (0)