Skip to content

Commit 20ae28a

Browse files
author
Andras Palinkas
authored
SQL: Fix the inconsistent behaviour of ISO_WEEK_YEAR() (#68758) (#68924)
The `SELECT ISO_WEEK_OF_YEAR(a) AS x FROM test WHERE x=4` query returned with `x=3` results because the `ISO_WEEK_YEAR(a)` in the WHERE clause that turns into a script query and the `ISO_WEEK_YEAR(a)` in the projections that turns into a post-processing on top of the Query DSL results execute different code to calculate the result. This change unifies the different code paths and results in a single method being responsible for the actual calculation. Note: this change impacts the way how all the `DateTimeFunction`s that do the field extraction from a date get translated into a script query. Fixes part of #67872
1 parent 933822f commit 20ae28a

File tree

8 files changed

+94
-66
lines changed

8 files changed

+94
-66
lines changed

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

Lines changed: 32 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -131,6 +131,31 @@ SELECT WEEK(birth_date) week, birth_date FROM test_emp ORDER BY WEEK(birth_date)
131131
44 |1961-11-02T00:00:00.000Z
132132
;
133133

134+
isoWeekOfYear
135+
schema::birth_date:ts|iso_week:i|week:i
136+
SELECT birth_date, IW(birth_date) iso_week, WEEK(birth_date) week FROM test_emp WHERE IW(birth_date) < 8 AND week >2 ORDER BY iso_week;
137+
138+
birth_date | iso_week | week
139+
------------------------+---------------+---------------
140+
1955-01-21T00:00:00.000Z|3 |4
141+
1953-01-23T00:00:00.000Z|4 |4
142+
1958-01-21T00:00:00.000Z|4 |4
143+
1959-01-27T00:00:00.000Z|5 |5
144+
1956-02-12T00:00:00.000Z|6 |7
145+
1953-02-08T00:00:00.000Z|6 |7
146+
1960-02-20T00:00:00.000Z|7 |8
147+
;
148+
149+
isoWeekOfYearFilterEquality
150+
SELECT ISO_WEEK_OF_YEAR(CONCAT(CONCAT('2021-01-22T14:26:06.', (salary % 2)::text), 'Z')::datetime) AS iso_week
151+
FROM test_emp WHERE iso_week = 3 LIMIT 2;
152+
153+
iso_week:i
154+
---------------
155+
3
156+
3
157+
;
158+
134159
weekOfYearVsIsoWeekOfYearEdgeCases
135160
SELECT ISO_WEEK_OF_YEAR('2005-01-01T00:00:00.000Z'::datetime) AS "isow2005", WEEK('2005-01-01T00:00:00.000Z'::datetime) AS "w2005",
136161
ISO_WEEK_OF_YEAR('2007-12-31T00:00:00.000Z'::datetime) AS "isow2007", WEEK('2007-12-31T00:00:00.000Z'::datetime) AS "w2007";
@@ -1209,24 +1234,25 @@ null |null |10
12091234
dateTimeAggByIsoWeekOfYear
12101235
SELECT IW(birth_date) iso_week, WEEK(birth_date) week FROM test_emp WHERE IW(birth_date) < 20 GROUP BY iso_week, week ORDER BY iso_week;
12111236

1212-
iso_week:i | week:i
1237+
iso_week:i | week:i
12131238
---------------+---------------
1214-
1 |2
1239+
2 |2
12151240
3 |4
12161241
4 |4
1217-
4 |5
1242+
5 |5
12181243
6 |7
1219-
7 |7
1244+
7 |8
12201245
8 |8
12211246
8 |9
12221247
9 |9
12231248
10 |11
12241249
12 |12
12251250
14 |14
1226-
14 |15
1251+
15 |15
12271252
15 |16
12281253
16 |16
1229-
16 |17
1254+
17 |17
1255+
17 |18
12301256
18 |18
12311257
;
12321258

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

Lines changed: 2 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -16,9 +16,6 @@
1616
import org.elasticsearch.xpack.sql.expression.function.scalar.datetime.DateTimeProcessor.DateTimeExtractor;
1717

1818
import java.time.ZoneId;
19-
import java.time.ZonedDateTime;
20-
import java.time.temporal.ChronoField;
21-
import java.time.temporal.Temporal;
2219

2320
import static org.elasticsearch.xpack.ql.expression.gen.script.ParamsBuilder.paramsBuilder;
2421

@@ -31,24 +28,15 @@ public abstract class DateTimeFunction extends BaseDateTimeFunction {
3128
this.extractor = extractor;
3229
}
3330

34-
public static Integer dateTimeChrono(ZonedDateTime dateTime, String tzId, String chronoName) {
35-
ZonedDateTime zdt = dateTime.withZoneSameInstant(ZoneId.of(tzId));
36-
return dateTimeChrono(zdt, ChronoField.valueOf(chronoName));
37-
}
38-
39-
protected static Integer dateTimeChrono(Temporal dateTime, ChronoField field) {
40-
return Integer.valueOf(dateTime.get(field));
41-
}
42-
4331
@Override
4432
public ScriptTemplate asScript() {
4533
ParamsBuilder params = paramsBuilder();
4634

4735
ScriptTemplate script = super.asScript();
48-
String template = formatTemplate("{sql}.dateTimeChrono(" + script.template() + ", {}, {})");
36+
String template = formatTemplate("{sql}.dateTimeExtract(" + script.template() + ", {}, {})");
4937
params.script(script.params())
5038
.variable(zoneId().getId())
51-
.variable(extractor.chronoField().name());
39+
.variable(extractor.name());
5240

5341
return new ScriptTemplate(template, params.build(), dataType());
5442
}

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

Lines changed: 17 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -10,11 +10,12 @@
1010
import org.elasticsearch.common.io.stream.StreamOutput;
1111

1212
import java.io.IOException;
13-
import java.time.OffsetTime;
1413
import java.time.ZoneId;
1514
import java.time.ZonedDateTime;
1615
import java.time.temporal.ChronoField;
17-
import java.time.temporal.WeekFields;
16+
import java.time.temporal.IsoFields;
17+
import java.time.temporal.Temporal;
18+
import java.time.temporal.TemporalField;
1819
import java.util.Objects;
1920

2021
public class DateTimeProcessor extends BaseDateTimeProcessor {
@@ -28,30 +29,18 @@ public enum DateTimeExtractor {
2829
MINUTE_OF_HOUR(ChronoField.MINUTE_OF_HOUR),
2930
MONTH_OF_YEAR(ChronoField.MONTH_OF_YEAR),
3031
SECOND_OF_MINUTE(ChronoField.SECOND_OF_MINUTE),
31-
ISO_WEEK_OF_YEAR(ChronoField.ALIGNED_WEEK_OF_YEAR),
32+
ISO_WEEK_OF_YEAR(IsoFields.WEEK_OF_WEEK_BASED_YEAR),
3233
YEAR(ChronoField.YEAR);
3334

34-
private final ChronoField field;
35+
private final TemporalField field;
3536

36-
DateTimeExtractor(ChronoField field) {
37+
DateTimeExtractor(TemporalField field) {
3738
this.field = field;
3839
}
39-
40-
public int extract(ZonedDateTime dt) {
41-
if (field == ChronoField.ALIGNED_WEEK_OF_YEAR) {
42-
return dt.get(WeekFields.ISO.weekOfWeekBasedYear());
43-
} else {
44-
return dt.get(field);
45-
}
46-
}
47-
48-
public int extract(OffsetTime time) {
40+
41+
public int extract(Temporal time) {
4942
return time.get(field);
5043
}
51-
52-
public ChronoField chronoField() {
53-
return field;
54-
}
5544
}
5645

5746
public static final String NAME = "dt";
@@ -86,6 +75,15 @@ public Object doProcess(ZonedDateTime dateTime) {
8675
return extractor.extract(dateTime);
8776
}
8877

78+
public static Integer doProcess(ZonedDateTime dateTime, String tzId, String extractorName) {
79+
ZonedDateTime zdt = dateTime.withZoneSameInstant(ZoneId.of(tzId));
80+
return doProcess(zdt, extractorName);
81+
}
82+
83+
protected static Integer doProcess(Temporal dateTime, String extractorName) {
84+
return DateTimeExtractor.valueOf(extractorName).extract(dateTime);
85+
}
86+
8987
@Override
9088
public int hashCode() {
9189
return Objects.hash(extractor, zoneId());

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

Lines changed: 0 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -12,23 +12,16 @@
1212
import org.elasticsearch.xpack.ql.tree.Source;
1313
import org.elasticsearch.xpack.sql.expression.function.scalar.datetime.DateTimeProcessor.DateTimeExtractor;
1414

15-
import java.time.OffsetTime;
1615
import java.time.ZoneId;
17-
import java.time.temporal.ChronoField;
1816

1917
import static org.elasticsearch.xpack.sql.expression.SqlTypeResolutions.isDateOrTime;
20-
import static org.elasticsearch.xpack.sql.util.DateUtils.asTimeAtZone;
2118

2219
public abstract class TimeFunction extends DateTimeFunction {
2320

2421
TimeFunction(Source source, Expression field, ZoneId zoneId, DateTimeExtractor extractor) {
2522
super(source, field, zoneId, extractor);
2623
}
2724

28-
public static Integer dateTimeChrono(OffsetTime time, String tzId, String chronoName) {
29-
return dateTimeChrono(asTimeAtZone(time, ZoneId.of(tzId)), ChronoField.valueOf(chronoName));
30-
}
31-
3225
@Override
3326
protected TypeResolution resolveType() {
3427
return isDateOrTime(field(), sourceText(), Expressions.ParamOrdinal.DEFAULT);

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

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,10 @@ public Object process(Object input) {
4343
private Object doProcess(OffsetTime time) {
4444
return extractor().extract(time);
4545
}
46+
47+
public static Integer doProcess(OffsetTime dateTime, String tzId, String extractorName) {
48+
return DateTimeProcessor.doProcess(asTimeAtZone(dateTime, ZoneId.of(tzId)), extractorName);
49+
}
4650

4751
@Override
4852
public int hashCode() {

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

Lines changed: 25 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -15,13 +15,13 @@
1515
import org.elasticsearch.xpack.sql.expression.function.scalar.datetime.DateDiffProcessor;
1616
import org.elasticsearch.xpack.sql.expression.function.scalar.datetime.DatePartProcessor;
1717
import org.elasticsearch.xpack.sql.expression.function.scalar.datetime.DateTimeFormatProcessor.Formatter;
18-
import org.elasticsearch.xpack.sql.expression.function.scalar.datetime.DateTimeFunction;
18+
import org.elasticsearch.xpack.sql.expression.function.scalar.datetime.DateTimeParseProcessor.Parser;
19+
import org.elasticsearch.xpack.sql.expression.function.scalar.datetime.DateTimeProcessor;
1920
import org.elasticsearch.xpack.sql.expression.function.scalar.datetime.DateTruncProcessor;
2021
import org.elasticsearch.xpack.sql.expression.function.scalar.datetime.NamedDateTimeProcessor.NameExtractor;
2122
import org.elasticsearch.xpack.sql.expression.function.scalar.datetime.NonIsoDateTimeProcessor.NonIsoDateTimeExtractor;
22-
import org.elasticsearch.xpack.sql.expression.function.scalar.datetime.DateTimeParseProcessor.Parser;
2323
import org.elasticsearch.xpack.sql.expression.function.scalar.datetime.QuarterProcessor;
24-
import org.elasticsearch.xpack.sql.expression.function.scalar.datetime.TimeFunction;
24+
import org.elasticsearch.xpack.sql.expression.function.scalar.datetime.TimeProcessor;
2525
import org.elasticsearch.xpack.sql.expression.function.scalar.geo.GeoProcessor;
2626
import org.elasticsearch.xpack.sql.expression.function.scalar.geo.StDistanceProcessor;
2727
import org.elasticsearch.xpack.sql.expression.function.scalar.geo.StWkttosqlProcessor;
@@ -217,18 +217,36 @@ public static Number sqrt(Number value) {
217217
public static Number tan(Number value) {
218218
return MathOperation.TAN.apply(value);
219219
}
220+
221+
220222

221223
//
222224
// Date/Time functions
223-
//
225+
//
226+
@Deprecated
224227
public static Integer dateTimeChrono(Object dateTime, String tzId, String chronoName) {
225-
if (dateTime == null || tzId == null || chronoName == null) {
228+
String extractorName = null;
229+
switch (chronoName) {
230+
case "DAY_OF_WEEK":
231+
extractorName = "ISO_DAY_OF_WEEK";
232+
break;
233+
case "ALIGNED_WEEK_OF_YEAR":
234+
extractorName = "ISO_WEEK_OF_YEAR";
235+
break;
236+
default:
237+
extractorName = chronoName;
238+
}
239+
return dateTimeExtract(dateTime, tzId, extractorName);
240+
}
241+
242+
public static Integer dateTimeExtract(Object dateTime, String tzId, String extractorName) {
243+
if (dateTime == null || tzId == null || extractorName == null) {
226244
return null;
227245
}
228246
if (dateTime instanceof OffsetTime) {
229-
return TimeFunction.dateTimeChrono((OffsetTime) dateTime, tzId, chronoName);
247+
return TimeProcessor.doProcess((OffsetTime) dateTime, tzId, extractorName);
230248
}
231-
return DateTimeFunction.dateTimeChrono(asDateTime(dateTime), tzId, chronoName);
249+
return DateTimeProcessor.doProcess(asDateTime(dateTime), tzId, extractorName);
232250
}
233251

234252
public static String dayName(Object dateTime, String tzId) {

‎x-pack/plugin/sql/src/main/resources/org/elasticsearch/xpack/sql/plugin/sql_whitelist.txt

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -125,6 +125,7 @@ class org.elasticsearch.xpack.sql.expression.function.scalar.whitelist.InternalS
125125
# Date/Time functions
126126
#
127127
Integer dateTimeChrono(Object, String, String)
128+
Integer dateTimeExtract(Object, String, String)
128129
String dayName(Object, String)
129130
Integer dayOfWeek(Object, String)
130131
String monthName(Object, String)

0 commit comments

Comments
 (0)