[GitHub] [flink] slinkydeveloper commented on a change in pull request #19325: [FLINK-26943][table] Add DATE_ADD supported in SQL & Table API

[GitBox <gi...@apache.org>]

slinkydeveloper commented on a change in pull request #19325:
URL: https://github.com/apache/flink/pull/19325#discussion_r840500139



##########
File path: flink-table/flink-table-planner/src/main/scala/org/apache/flink/table/planner/expressions/time.scala
##########
@@ -300,3 +300,25 @@ case class ToTimestampLtz(
 
   override private[flink] def resultType = SqlTimeTypeInfo.TIMESTAMP
 }
+
+case class DateAdd(

Review comment:
       This stack is deprecated, please remove this code. You need to add the type inference and return type inference to `BuiltInFunctionDefinitions`,

##########
File path: flink-table/flink-table-planner/src/main/java/org/apache/flink/table/planner/expressions/converter/converters/CustomizedConverters.java
##########
@@ -48,6 +48,7 @@
         CONVERTERS.put(
                 BuiltInFunctionDefinitions.TEMPORAL_OVERLAPS, new TemporalOverlapsConverter());
         CONVERTERS.put(BuiltInFunctionDefinitions.TIMESTAMP_DIFF, new TimestampDiffConverter());
+        CONVERTERS.put(BuiltInFunctionDefinitions.DATE_ADD, new DateAddConvertor());

Review comment:
       I think you don't need this if you use the new stack

##########
File path: flink-table/flink-table-planner/src/test/scala/org/apache/flink/table/planner/expressions/TemporalTypesTest.scala
##########
@@ -1703,6 +1703,39 @@ class TemporalTypesTest extends ExpressionTestBase {
         " Supported form(s): 'TIMESTAMPDIFF(<ANY>, <DATETIME>, <DATETIME>)'")
   }
 
+  @Test
+  def testValidDateAdd(): Unit = {

Review comment:
       Please add the tests to the new test stack (`BuiltInFunctionTestBase`)

##########
File path: flink-table/flink-table-common/src/main/java/org/apache/flink/table/utils/DateTimeUtils.java
##########
@@ -1689,6 +1690,24 @@ public static int subtractMonths(long t0, long t1) {
         return x;
     }
 
+    public static String dateAdd(String startDate, int daysNumber) {
+        long startMillSecond;
+        try {
+            startMillSecond = FORMATTER_CACHE.get(DATE_FORMAT_STRING).parse(startDate).getTime();
+        } catch (ParseException e) {
+            try {
+                startMillSecond =
+                        FORMATTER_CACHE.get(TIMESTAMP_FORMAT_STRING).parse(startDate).getTime();
+            } catch (ParseException parseException) {
+                return null;

Review comment:
       These methods should fail, rather than returning null. You can then use `fallibleMethodGen` in the codegen




-- 
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: issues-unsubscribe@flink.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org