Skip to content

Commit

Permalink
SQL: Fix the inconsistent behaviour of ISO_WEEK_YEAR() (#68758) (#68924)
Browse files Browse the repository at this point in the history 
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
  • Loading branch information
Andras Palinkas committed Feb 11, 2021
1 parent 933822f commit 20ae28a
Show file tree
Hide file tree
Showing 8 changed files with 94 additions and 66 deletions.
38 changes: 32 additions & 6 deletions x-pack/plugin/sql/qa/server/src/main/resources/datetime.csv-spec
View file
Original file line number Diff line number Diff line change
Expand Up @@ -131,6 +131,31 @@ SELECT WEEK(birth_date) week, birth_date FROM test_emp ORDER BY WEEK(birth_date)
44 |1961-11-02T00:00:00.000Z
;

isoWeekOfYear
schema::birth_date:ts|iso_week:i|week:i
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;

birth_date | iso_week | week
------------------------+---------------+---------------
1955-01-21T00:00:00.000Z|3 |4
1953-01-23T00:00:00.000Z|4 |4
1958-01-21T00:00:00.000Z|4 |4
1959-01-27T00:00:00.000Z|5 |5
1956-02-12T00:00:00.000Z|6 |7
1953-02-08T00:00:00.000Z|6 |7
1960-02-20T00:00:00.000Z|7 |8
;

isoWeekOfYearFilterEquality
SELECT ISO_WEEK_OF_YEAR(CONCAT(CONCAT('2021-01-22T14:26:06.', (salary % 2)::text), 'Z')::datetime) AS iso_week
FROM test_emp WHERE iso_week = 3 LIMIT 2;

iso_week:i
---------------
3
3
;

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";
Expand Down Expand Up @@ -1209,24 +1234,25 @@ null |null |10
dateTimeAggByIsoWeekOfYear
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;

iso_week:i | week:i
iso_week:i | week:i
---------------+---------------
1 |2
2 |2
3 |4
4 |4
4 |5
5 |5
6 |7
7 |7
7 |8
8 |8
8 |9
9 |9
10 |11
12 |12
14 |14
14 |15
15 |15
15 |16
16 |16
16 |17
17 |17
17 |18
18 |18
;

Expand Down
16 changes: 2 additions & 14 deletions ...ava/org/elasticsearch/xpack/sql/expression/function/scalar/datetime/DateTimeFunction.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,6 @@
import org.elasticsearch.xpack.sql.expression.function.scalar.datetime.DateTimeProcessor.DateTimeExtractor;

import java.time.ZoneId;
import java.time.ZonedDateTime;
import java.time.temporal.ChronoField;
import java.time.temporal.Temporal;

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

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

public static Integer dateTimeChrono(ZonedDateTime dateTime, String tzId, String chronoName) {
ZonedDateTime zdt = dateTime.withZoneSameInstant(ZoneId.of(tzId));
return dateTimeChrono(zdt, ChronoField.valueOf(chronoName));
}

protected static Integer dateTimeChrono(Temporal dateTime, ChronoField field) {
return Integer.valueOf(dateTime.get(field));
}

@Override
public ScriptTemplate asScript() {
ParamsBuilder params = paramsBuilder();

ScriptTemplate script = super.asScript();
String template = formatTemplate("{sql}.dateTimeChrono(" + script.template() + ", {}, {})");
String template = formatTemplate("{sql}.dateTimeExtract(" + script.template() + ", {}, {})");
params.script(script.params())
.variable(zoneId().getId())
.variable(extractor.chronoField().name());
.variable(extractor.name());

return new ScriptTemplate(template, params.build(), dataType());
}
Expand Down
36 changes: 17 additions & 19 deletions ...va/org/elasticsearch/xpack/sql/expression/function/scalar/datetime/DateTimeProcessor.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,11 +10,12 @@
import org.elasticsearch.common.io.stream.StreamOutput;

import java.io.IOException;
import java.time.OffsetTime;
import java.time.ZoneId;
import java.time.ZonedDateTime;
import java.time.temporal.ChronoField;
import java.time.temporal.WeekFields;
import java.time.temporal.IsoFields;
import java.time.temporal.Temporal;
import java.time.temporal.TemporalField;
import java.util.Objects;

public class DateTimeProcessor extends BaseDateTimeProcessor {
Expand All @@ -28,30 +29,18 @@ public enum DateTimeExtractor {
MINUTE_OF_HOUR(ChronoField.MINUTE_OF_HOUR),
MONTH_OF_YEAR(ChronoField.MONTH_OF_YEAR),
SECOND_OF_MINUTE(ChronoField.SECOND_OF_MINUTE),
ISO_WEEK_OF_YEAR(ChronoField.ALIGNED_WEEK_OF_YEAR),
ISO_WEEK_OF_YEAR(IsoFields.WEEK_OF_WEEK_BASED_YEAR),
YEAR(ChronoField.YEAR);

private final ChronoField field;
private final TemporalField field;

DateTimeExtractor(ChronoField field) {
DateTimeExtractor(TemporalField field) {
this.field = field;
}

public int extract(ZonedDateTime dt) {
if (field == ChronoField.ALIGNED_WEEK_OF_YEAR) {
return dt.get(WeekFields.ISO.weekOfWeekBasedYear());
} else {
return dt.get(field);
}
}

public int extract(OffsetTime time) {

public int extract(Temporal time) {
return time.get(field);
}

public ChronoField chronoField() {
return field;
}
}

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

public static Integer doProcess(ZonedDateTime dateTime, String tzId, String extractorName) {
ZonedDateTime zdt = dateTime.withZoneSameInstant(ZoneId.of(tzId));
return doProcess(zdt, extractorName);
}

protected static Integer doProcess(Temporal dateTime, String extractorName) {
return DateTimeExtractor.valueOf(extractorName).extract(dateTime);
}

@Override
public int hashCode() {
return Objects.hash(extractor, zoneId());
Expand Down
7 changes: 0 additions & 7 deletions ...in/java/org/elasticsearch/xpack/sql/expression/function/scalar/datetime/TimeFunction.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,23 +12,16 @@
import org.elasticsearch.xpack.ql.tree.Source;
import org.elasticsearch.xpack.sql.expression.function.scalar.datetime.DateTimeProcessor.DateTimeExtractor;

import java.time.OffsetTime;
import java.time.ZoneId;
import java.time.temporal.ChronoField;

import static org.elasticsearch.xpack.sql.expression.SqlTypeResolutions.isDateOrTime;
import static org.elasticsearch.xpack.sql.util.DateUtils.asTimeAtZone;

public abstract class TimeFunction extends DateTimeFunction {

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

public static Integer dateTimeChrono(OffsetTime time, String tzId, String chronoName) {
return dateTimeChrono(asTimeAtZone(time, ZoneId.of(tzId)), ChronoField.valueOf(chronoName));
}

@Override
protected TypeResolution resolveType() {
return isDateOrTime(field(), sourceText(), Expressions.ParamOrdinal.DEFAULT);
Expand Down
4 changes: 4 additions & 0 deletions ...n/java/org/elasticsearch/xpack/sql/expression/function/scalar/datetime/TimeProcessor.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,10 @@ public Object process(Object input) {
private Object doProcess(OffsetTime time) {
return extractor().extract(time);
}

public static Integer doProcess(OffsetTime dateTime, String tzId, String extractorName) {
return DateTimeProcessor.doProcess(asTimeAtZone(dateTime, ZoneId.of(tzId)), extractorName);
}

@Override
public int hashCode() {
Expand Down
32 changes: 25 additions & 7 deletions .../elasticsearch/xpack/sql/expression/function/scalar/whitelist/InternalSqlScriptUtils.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,13 +15,13 @@
import org.elasticsearch.xpack.sql.expression.function.scalar.datetime.DateDiffProcessor;
import org.elasticsearch.xpack.sql.expression.function.scalar.datetime.DatePartProcessor;
import org.elasticsearch.xpack.sql.expression.function.scalar.datetime.DateTimeFormatProcessor.Formatter;
import org.elasticsearch.xpack.sql.expression.function.scalar.datetime.DateTimeFunction;
import org.elasticsearch.xpack.sql.expression.function.scalar.datetime.DateTimeParseProcessor.Parser;
import org.elasticsearch.xpack.sql.expression.function.scalar.datetime.DateTimeProcessor;
import org.elasticsearch.xpack.sql.expression.function.scalar.datetime.DateTruncProcessor;
import org.elasticsearch.xpack.sql.expression.function.scalar.datetime.NamedDateTimeProcessor.NameExtractor;
import org.elasticsearch.xpack.sql.expression.function.scalar.datetime.NonIsoDateTimeProcessor.NonIsoDateTimeExtractor;
import org.elasticsearch.xpack.sql.expression.function.scalar.datetime.DateTimeParseProcessor.Parser;
import org.elasticsearch.xpack.sql.expression.function.scalar.datetime.QuarterProcessor;
import org.elasticsearch.xpack.sql.expression.function.scalar.datetime.TimeFunction;
import org.elasticsearch.xpack.sql.expression.function.scalar.datetime.TimeProcessor;
import org.elasticsearch.xpack.sql.expression.function.scalar.geo.GeoProcessor;
import org.elasticsearch.xpack.sql.expression.function.scalar.geo.StDistanceProcessor;
import org.elasticsearch.xpack.sql.expression.function.scalar.geo.StWkttosqlProcessor;
Expand Down Expand Up @@ -217,18 +217,36 @@ public static Number sqrt(Number value) {
public static Number tan(Number value) {
return MathOperation.TAN.apply(value);
}



//
// Date/Time functions
//
//
@Deprecated
public static Integer dateTimeChrono(Object dateTime, String tzId, String chronoName) {
if (dateTime == null || tzId == null || chronoName == null) {
String extractorName = null;
switch (chronoName) {
case "DAY_OF_WEEK":
extractorName = "ISO_DAY_OF_WEEK";
break;
case "ALIGNED_WEEK_OF_YEAR":
extractorName = "ISO_WEEK_OF_YEAR";
break;
default:
extractorName = chronoName;
}
return dateTimeExtract(dateTime, tzId, extractorName);
}

public static Integer dateTimeExtract(Object dateTime, String tzId, String extractorName) {
if (dateTime == null || tzId == null || extractorName == null) {
return null;
}
if (dateTime instanceof OffsetTime) {
return TimeFunction.dateTimeChrono((OffsetTime) dateTime, tzId, chronoName);
return TimeProcessor.doProcess((OffsetTime) dateTime, tzId, extractorName);
}
return DateTimeFunction.dateTimeChrono(asDateTime(dateTime), tzId, chronoName);
return DateTimeProcessor.doProcess(asDateTime(dateTime), tzId, extractorName);
}

public static String dayName(Object dateTime, String tzId) {
Expand Down
1 change: 1 addition & 0 deletions x-pack/plugin/sql/src/main/resources/org/elasticsearch/xpack/sql/plugin/sql_whitelist.txt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -125,6 +125,7 @@ class org.elasticsearch.xpack.sql.expression.function.scalar.whitelist.InternalS
# Date/Time functions
#
Integer dateTimeChrono(Object, String, String)
Integer dateTimeExtract(Object, String, String)
String dayName(Object, String)
Integer dayOfWeek(Object, String)
String monthName(Object, String)
Expand Down

0 comments on commit 20ae28a

Please sign in to comment.