jnturton commented on code in PR #2689:
URL: https://github.com/apache/drill/pull/2689#discussion_r1001539567
##########
contrib/udfs/src/main/java/org/apache/drill/exec/udfs/DateFunctions.java:
##########
@@ -140,8 +143,77 @@ public void eval() {
java.time.format.DateTimeFormatter formatter =
java.time.format.DateTimeFormatter.ofPattern(format);
java.time.LocalDateTime dateTime =
java.time.LocalDateTime.parse(inputDate, formatter);
- java.time.LocalDateTime td =
org.apache.drill.exec.udfs.NearestDateUtils.getDate(dateTime, intervalString);
+ java.time.LocalDateTime td = DateConversionUtils.getDate(dateTime,
intervalString);
out.value =
td.atZone(java.time.ZoneId.of("UTC")).toInstant().toEpochMilli();
}
}
+
+ @FunctionTemplate(names = {"yearweek","year_week"},
+ scope = FunctionTemplate.FunctionScope.SIMPLE,
+ nulls = FunctionTemplate.NullHandling.NULL_IF_NULL)
+ public static class YearWeekFunction implements DrillSimpleFunc {
+ @Param
+ VarCharHolder inputHolder;
Review Comment:
If Drill's implicit casting is working the way it should be then we should
not need to duplicate this function for varchar inputs.
##########
contrib/udfs/src/main/java/org/apache/drill/exec/udfs/DateFunctions.java:
##########
@@ -140,8 +143,77 @@ public void eval() {
java.time.format.DateTimeFormatter formatter =
java.time.format.DateTimeFormatter.ofPattern(format);
java.time.LocalDateTime dateTime =
java.time.LocalDateTime.parse(inputDate, formatter);
- java.time.LocalDateTime td =
org.apache.drill.exec.udfs.NearestDateUtils.getDate(dateTime, intervalString);
+ java.time.LocalDateTime td = DateConversionUtils.getDate(dateTime,
intervalString);
out.value =
td.atZone(java.time.ZoneId.of("UTC")).toInstant().toEpochMilli();
}
}
+
+ @FunctionTemplate(names = {"yearweek","year_week"},
+ scope = FunctionTemplate.FunctionScope.SIMPLE,
+ nulls = FunctionTemplate.NullHandling.NULL_IF_NULL)
+ public static class YearWeekFunction implements DrillSimpleFunc {
+ @Param
+ VarCharHolder inputHolder;
+
+ @Output
+ IntHolder out;
+
+ @Override
+ public void setup() {
+ // noop
+ }
+
+ @Override
+ public void eval() {
+ String input =
org.apache.drill.exec.expr.fn.impl.StringFunctionHelpers.toStringFromUTF8(inputHolder.start,
inputHolder.end, inputHolder.buffer);
+ java.time.LocalDateTime dt =
org.apache.drill.exec.udfs.DateUtilFunctions.getTimestampFromString(input);
+ int week = dt.get(java.time.temporal.IsoFields.WEEK_OF_WEEK_BASED_YEAR);
+ int year = dt.getYear();
+ out.value = (year * 100) + week;
+ }
+ }
+
+ @FunctionTemplate(names = {"yearweek","year_week"},
Review Comment:
I admit that I don't know all of the detail but I understand that because
year_week(x) = 100*year(x) + week(x) we have the opportunity to provide "no
code" implementation of this function in terms of existing functions by making
an addition to DrillConvertletTable.
##########
contrib/udfs/src/test/java/org/apache/drill/exec/udfs/TestDateUtils.java:
##########
@@ -0,0 +1,77 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.drill.exec.udfs;
+
+import org.junit.Test;
+
+import java.time.LocalDate;
+import java.time.LocalDateTime;
+
+import static org.junit.Assert.assertEquals;
+
+public class TestDateUtils {
+
+ @Test
+ public void testDateFromString() {
+ LocalDate testDate = LocalDate.of(2022, 3,14);
+ LocalDate badDate = LocalDate.of(1970, 1, 1);
+ assertEquals(testDate, DateUtilFunctions.getDateFromString("2022-03-14"));
+ assertEquals(testDate, DateUtilFunctions.getDateFromString("3/14/2022"));
+ assertEquals(testDate, DateUtilFunctions.getDateFromString("14/03/2022",
true));
+ assertEquals(testDate, DateUtilFunctions.getDateFromString("2022/3/14"));
+
+ // Test bad dates
+ assertEquals(badDate, DateUtilFunctions.getDateFromString(null));
+ assertEquals(badDate, DateUtilFunctions.getDateFromString("1975-13-56"));
+ assertEquals(badDate, DateUtilFunctions.getDateFromString("1975-1s"));
Review Comment:
Is there a reason we aren't using null to represent invalid and missing
dates? This seems like a perfect fit for it to me.
##########
contrib/udfs/src/test/java/org/apache/drill/exec/udfs/TestNearestDateFunctions.java:
##########
@@ -149,7 +149,7 @@ public void testReadException() throws Exception {
run(query);
fail();
} catch (DrillRuntimeException e) {
- assertTrue(e.getMessage().contains("[BAD_DATE] is not a valid time
statement. Expecting: " +
Arrays.asList(NearestDateUtils.TimeInterval.values())));
+ assertTrue(e.getMessage().contains("[BAD_DATE] is not a valid time
statement. Expecting: " +
Arrays.asList(DateConversionUtils.TimeInterval.values())));
Review Comment:
Is TestNearestDateFunctions still a good name for this class given the
renamings made in this PR?
##########
contrib/udfs/src/main/java/org/apache/drill/exec/udfs/DateFunctions.java:
##########
@@ -140,8 +143,77 @@ public void eval() {
java.time.format.DateTimeFormatter formatter =
java.time.format.DateTimeFormatter.ofPattern(format);
java.time.LocalDateTime dateTime =
java.time.LocalDateTime.parse(inputDate, formatter);
- java.time.LocalDateTime td =
org.apache.drill.exec.udfs.NearestDateUtils.getDate(dateTime, intervalString);
+ java.time.LocalDateTime td = DateConversionUtils.getDate(dateTime,
intervalString);
out.value =
td.atZone(java.time.ZoneId.of("UTC")).toInstant().toEpochMilli();
}
}
+
+ @FunctionTemplate(names = {"yearweek","year_week"},
+ scope = FunctionTemplate.FunctionScope.SIMPLE,
+ nulls = FunctionTemplate.NullHandling.NULL_IF_NULL)
+ public static class YearWeekFunction implements DrillSimpleFunc {
+ @Param
+ VarCharHolder inputHolder;
+
+ @Output
+ IntHolder out;
+
+ @Override
+ public void setup() {
+ // noop
+ }
+
+ @Override
+ public void eval() {
+ String input =
org.apache.drill.exec.expr.fn.impl.StringFunctionHelpers.toStringFromUTF8(inputHolder.start,
inputHolder.end, inputHolder.buffer);
+ java.time.LocalDateTime dt =
org.apache.drill.exec.udfs.DateUtilFunctions.getTimestampFromString(input);
+ int week = dt.get(java.time.temporal.IsoFields.WEEK_OF_WEEK_BASED_YEAR);
+ int year = dt.getYear();
+ out.value = (year * 100) + week;
+ }
+ }
+
+ @FunctionTemplate(names = {"yearweek","year_week"},
+ scope = FunctionTemplate.FunctionScope.SIMPLE,
+ nulls = FunctionTemplate.NullHandling.NULL_IF_NULL)
+ public static class YearWeekFromDateFunction implements DrillSimpleFunc {
+ @Param
+ DateHolder inputHolder;
+
+ @Output
+ IntHolder out;
+
+ @Override
+ public void setup() {
+ // noop
+ }
+
+ @Override
+ public void eval() {
+ out.value =
org.apache.drill.exec.udfs.DateUtilFunctions.getYearWeek(inputHolder.value);
+ }
+ }
+
+ @FunctionTemplate(names = {"time_stamp", "timestamp"},
Review Comment:
Timestamp is one word everywhere else in Drill. I vote for sticking to that
and for a more descriptive name like parse_timestamp or to_timestamp_auto
(continuing our existing to_timestamp) for this function. I guess extending
to_timestamp is also an option and clutters the namespace less e.g.
`to_timestamp('1970-01-01', 'auto')` could apply the format detection logic
here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscr...@drill.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
