Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SQL: Fix the inconsistent behaviour of ISO_WEEK_YEAR() #68758

Merged
merged 7 commits into from
Feb 11, 2021
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 32 additions & 6 deletions x-pack/plugin/sql/qa/server/src/main/resources/datetime.csv-spec
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 @@ -1303,24 +1328,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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note to self: "manual" checking the csv specs sometimes pays off.

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
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
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
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
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
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.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 @@ -221,14 +221,14 @@ public static Number tan(Number value) {
//
// Date/Time functions
//
public static Integer dateTimeChrono(Object dateTime, String tzId, String chronoName) {
if (dateTime == null || tzId == null || chronoName == null) {
public static Integer dateTimeExtract(Object dateTime, String tzId, String extractorName) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have a strong preference regarding the method rename however there is an associated cost for it in backwards compatibility.
When executing across older clusters, the generated query would fail since there's no dateTimeExtract in those versions, only dateTimeChrono.

To avoid this, simply keep dateTimeChrono around and redirect it to dateTimeExtract so its backwards compatible or avoid the method rename in the first place.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, good catch. It is not just a simple method rename, also the possible values of the last parameter changed (from ChronoField enum to DateTimeExtract enum values). I will keep around a @Deprecated dateTimeChrono() for backwards compatibility that will do this translation. I guess we can remove it in the next major version.

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
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,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
Original file line number Diff line number Diff line change
Expand Up @@ -1214,7 +1214,7 @@ public void testTranslateRoundWithOneParameter() {
assertNotNull(groupingContext);
ScriptTemplate scriptTemplate = groupingContext.tail.script();
assertEquals(
"InternalSqlScriptUtils.round(InternalSqlScriptUtils.dateTimeChrono(InternalQlScriptUtils.docValue(doc,params.v0), "
"InternalSqlScriptUtils.round(InternalSqlScriptUtils.dateTimeExtract(InternalQlScriptUtils.docValue(doc,params.v0), "
+ "params.v1, params.v2),params.v3)",
scriptTemplate.toString());
assertEquals("[{v=date}, {v=Z}, {v=YEAR}, {v=null}]", scriptTemplate.params().toString());
Expand All @@ -1239,7 +1239,7 @@ public void testTranslateRoundWithTwoParameters() {
assertNotNull(groupingContext);
ScriptTemplate scriptTemplate = groupingContext.tail.script();
assertEquals(
"InternalSqlScriptUtils.round(InternalSqlScriptUtils.dateTimeChrono(InternalQlScriptUtils.docValue(doc,params.v0), "
"InternalSqlScriptUtils.round(InternalSqlScriptUtils.dateTimeExtract(InternalQlScriptUtils.docValue(doc,params.v0), "
+ "params.v1, params.v2),params.v3)",
scriptTemplate.toString());
assertEquals("[{v=date}, {v=Z}, {v=YEAR}, {v=-2}]", scriptTemplate.params().toString());
Expand Down Expand Up @@ -1578,7 +1578,7 @@ public void testOrderByYear() {
assertThat(eqe.queryContainer().toString().replaceAll("\\s+", ""),
endsWith("\"sort\":[{\"_script\":{\"script\":{\"source\":\"InternalQlScriptUtils.nullSafeSortNumeric("
+
"InternalSqlScriptUtils.dateTimeChrono(InternalQlScriptUtils.docValue(doc,params.v0)," +
"InternalSqlScriptUtils.dateTimeExtract(InternalQlScriptUtils.docValue(doc,params.v0)," +
"params.v1,params.v2))\",\"lang\":\"painless\",\"params\":{\"v0\":\"date\",\"v1\":\"Z\"," +
"\"v2\":\"YEAR\"}},\"type\":\"number\",\"order\":\"asc\"}}]}"));
}
Expand Down Expand Up @@ -1726,7 +1726,7 @@ public void testGroupByCastScalar() {
assertThat(
((EsQueryExec) p).queryContainer().aggs().asAggBuilder().toString()
.replaceAll("\\s+", ""),
endsWith("{\"source\":\"InternalSqlScriptUtils.cast(InternalSqlScriptUtils.abs(InternalSqlScriptUtils.dateTimeChrono" +
endsWith("{\"source\":\"InternalSqlScriptUtils.cast(InternalSqlScriptUtils.abs(InternalSqlScriptUtils.dateTimeExtract" +
"(InternalQlScriptUtils.docValue(doc,params.v0),params.v1,params.v2)),params.v3)\",\"lang\":\"painless\"," +
"\"params\":{\"v0\":\"date\",\"v1\":\"Z\",\"v2\":\"YEAR\",\"v3\":\"LONG\"}},\"missing_bucket\":true," +
"\"value_type\":\"long\",\"order\":\"asc\"}}}]}}}")
Expand All @@ -1743,7 +1743,7 @@ public void testGroupByCastScalarWithAlias() {
assertThat(
((EsQueryExec) p).queryContainer().aggs().asAggBuilder().toString()
.replaceAll("\\s+", ""),
endsWith("{\"source\":\"InternalSqlScriptUtils.cast(InternalSqlScriptUtils.abs(InternalSqlScriptUtils.dateTimeChrono" +
endsWith("{\"source\":\"InternalSqlScriptUtils.cast(InternalSqlScriptUtils.abs(InternalSqlScriptUtils.dateTimeExtract" +
"(InternalQlScriptUtils.docValue(doc,params.v0),params.v1,params.v2)),params.v3)\",\"lang\":\"painless\"," +
"\"params\":{\"v0\":\"date\",\"v1\":\"Z\",\"v2\":\"YEAR\",\"v3\":\"LONG\"}},\"missing_bucket\":true," +
"\"value_type\":\"long\",\"order\":\"asc\"}}}]}}}")
Expand All @@ -1760,7 +1760,7 @@ public void testGroupByCastScalarWithNumericRef() {
assertThat(
((EsQueryExec) p).queryContainer().aggs().asAggBuilder().toString()
.replaceAll("\\s+", ""),
endsWith("{\"source\":\"InternalSqlScriptUtils.cast(InternalSqlScriptUtils.abs(InternalSqlScriptUtils.dateTimeChrono" +
endsWith("{\"source\":\"InternalSqlScriptUtils.cast(InternalSqlScriptUtils.abs(InternalSqlScriptUtils.dateTimeExtract" +
"(InternalQlScriptUtils.docValue(doc,params.v0),params.v1,params.v2)),params.v3)\",\"lang\":\"painless\"," +
"\"params\":{\"v0\":\"date\",\"v1\":\"Z\",\"v2\":\"YEAR\",\"v3\":\"LONG\"}},\"missing_bucket\":true," +
"\"value_type\":\"long\",\"order\":\"asc\"}}}]}}}")
Expand All @@ -1779,7 +1779,7 @@ public void testGroupByConvertScalar() {
assertThat(
((EsQueryExec) p).queryContainer().aggs().asAggBuilder().toString()
.replaceAll("\\s+", ""),
endsWith("{\"source\":\"InternalSqlScriptUtils.cast(InternalSqlScriptUtils.abs(InternalSqlScriptUtils.dateTimeChrono" +
endsWith("{\"source\":\"InternalSqlScriptUtils.cast(InternalSqlScriptUtils.abs(InternalSqlScriptUtils.dateTimeExtract" +
"(InternalQlScriptUtils.docValue(doc,params.v0),params.v1,params.v2)),params.v3)\",\"lang\":\"painless\"," +
"\"params\":{\"v0\":\"date\",\"v1\":\"Z\",\"v2\":\"YEAR\",\"v3\":\"LONG\"}},\"missing_bucket\":true," +
"\"value_type\":\"long\",\"order\":\"asc\"}}}]}}}")
Expand All @@ -1795,7 +1795,7 @@ public void testGroupByConvertScalar() {
assertThat(
((EsQueryExec) p).queryContainer().aggs().asAggBuilder().toString()
.replaceAll("\\s+", ""),
endsWith("{\"source\":\"InternalSqlScriptUtils.dateTimeChrono(" +
endsWith("{\"source\":\"InternalSqlScriptUtils.dateTimeExtract(" +
"InternalQlScriptUtils.docValue(doc,params.v0),params.v1,params.v2)\",\"lang\":\"painless\"," +
"\"params\":{\"v0\":\"date\",\"v1\":\"Z\",\"v2\":\"HOUR_OF_DAY\"}},\"missing_bucket\":true," +
"\"value_type\":\"long\",\"order\":\"asc\"}}}]}}}")
Expand All @@ -1814,7 +1814,7 @@ public void testGroupByConvertScalarWithAlias() {
assertThat(
((EsQueryExec) p).queryContainer().aggs().asAggBuilder().toString()
.replaceAll("\\s+", ""),
endsWith("{\"source\":\"InternalSqlScriptUtils.cast(InternalSqlScriptUtils.abs(InternalSqlScriptUtils.dateTimeChrono" +
endsWith("{\"source\":\"InternalSqlScriptUtils.cast(InternalSqlScriptUtils.abs(InternalSqlScriptUtils.dateTimeExtract" +
"(InternalQlScriptUtils.docValue(doc,params.v0),params.v1,params.v2)),params.v3)\",\"lang\":\"painless\"," +
"\"params\":{\"v0\":\"date\",\"v1\":\"Z\",\"v2\":\"YEAR\",\"v3\":\"LONG\"}},\"missing_bucket\":true," +
"\"value_type\":\"long\",\"order\":\"asc\"}}}]}}}")
Expand All @@ -1829,7 +1829,7 @@ public void testGroupByConvertScalarWithAlias() {
assertThat(
((EsQueryExec) p).queryContainer().aggs().asAggBuilder().toString()
.replaceAll("\\s+", ""),
endsWith("{\"source\":\"InternalSqlScriptUtils.dateTimeChrono(" +
endsWith("{\"source\":\"InternalSqlScriptUtils.dateTimeExtract(" +
"InternalQlScriptUtils.docValue(doc,params.v0),params.v1,params.v2)\",\"lang\":\"painless\"," +
"\"params\":{\"v0\":\"date\",\"v1\":\"Z\",\"v2\":\"MINUTE_OF_HOUR\"}}," +
"\"missing_bucket\":true,\"value_type\":\"long\",\"order\":\"asc\"}}}]}}}")
Expand All @@ -1847,7 +1847,7 @@ public void testGroupByConvertScalarWithNumericRef() {
assertThat(
((EsQueryExec) p).queryContainer().aggs().asAggBuilder().toString()
.replaceAll("\\s+", ""),
endsWith("{\"source\":\"InternalSqlScriptUtils.cast(InternalSqlScriptUtils.abs(InternalSqlScriptUtils.dateTimeChrono" +
endsWith("{\"source\":\"InternalSqlScriptUtils.cast(InternalSqlScriptUtils.abs(InternalSqlScriptUtils.dateTimeExtract" +
"(InternalQlScriptUtils.docValue(doc,params.v0),params.v1,params.v2)),params.v3)\",\"lang\":\"painless\"," +
"\"params\":{\"v0\":\"date\",\"v1\":\"Z\",\"v2\":\"YEAR\",\"v3\":\"LONG\"}},\"missing_bucket\":true," +
"\"value_type\":\"long\",\"order\":\"asc\"}}}]}}}")
Expand Down Expand Up @@ -2149,11 +2149,11 @@ public void testChronoFieldBasedDateTimeFunctionsWithMathIntervalAndGroupBy() {
assertEquals(EsQueryExec.class, p.getClass());
EsQueryExec eqe = (EsQueryExec) p;
assertThat(eqe.queryContainer().toString().replaceAll("\\s+", ""), containsString(
"{\"terms\":{\"script\":{\"source\":\"InternalSqlScriptUtils.dateTimeChrono("
"{\"terms\":{\"script\":{\"source\":\"InternalSqlScriptUtils.dateTimeExtract("
+ "InternalSqlScriptUtils.add(InternalQlScriptUtils.docValue(doc,params.v0),"
+ "InternalSqlScriptUtils.intervalYearMonth(params.v1,params.v2)),params.v3,params.v4)\","
+ "\"lang\":\"painless\",\"params\":{\"v0\":\"date\",\"v1\":\"P1Y\",\"v2\":\"INTERVAL_YEAR\","
+ "\"v3\":\"Z\",\"v4\":\"" + randomFunction.chronoField().name() + "\"}},\"missing_bucket\":true,"
+ "\"v3\":\"Z\",\"v4\":\"" + randomFunction.name() + "\"}},\"missing_bucket\":true,"
+ "\"value_type\":\"long\",\"order\":\"asc\"}}}]}}}}"));
}

Expand Down