[ https://issues.apache.org/jira/browse/DRILL-8340?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17622157#comment-17622157 ]
ASF GitHub Bot commented on DRILL-8340: --------------------------------------- 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. > Add Additional Date Manipulation Functions (Part 1) > --------------------------------------------------- > > Key: DRILL-8340 > URL: https://issues.apache.org/jira/browse/DRILL-8340 > Project: Apache Drill > Issue Type: Improvement > Components: Functions - Drill > Affects Versions: 1.20.2 > Reporter: Charles Givre > Assignee: Charles Givre > Priority: Major > Fix For: 2.0.0 > > > This PR adds several utility functions to facilitate working with dates and > times. These are modeled after the date/time functionality in MySQL. > Specifically this adds: > * YEARWEEK(<date>): Returns an int of year week. IE (202002) > * TIME_STAMP(<date string>): Converts most anything that looks like a date > string into a timestamp. -- This message was sent by Atlassian Jira (v8.20.10#820010)