[GitHub] [dolphinscheduler] EricGao888 commented on a diff in pull request #11667: [Improvement-#11608][task-plugin] New time variables are added to facilitate business development

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

EricGao888 commented on code in PR #11667:
URL: https://github.com/apache/dolphinscheduler/pull/11667#discussion_r977442133


##########
docs/docs/zh/guide/parameter/built-in.md:
##########
@@ -28,23 +28,33 @@
 - 我们定义这种基准变量为 \$[...] 格式的，\$[yyyyMMddHHmmss] 是可以任意分解组合的，比如：\$[yyyyMMdd], \$[HHmmss], \$[yyyy-MM-dd] 等
 
 - 也可以通过以下两种方式：
-
-  1.使用add_months()函数，该函数用于加减月份，
-  第一个入口参数为[yyyyMMdd]，表示返回时间的格式
-  第二个入口参数为月份偏移量，表示加减多少个月
-  * 后 N 年：$[add_months(yyyyMMdd,12*N)]
-  * 前 N 年：$[add_months(yyyyMMdd,-12*N)]
-  * 后 N 月：$[add_months(yyyyMMdd,N)]
-  * 前 N 月：$[add_months(yyyyMMdd,-N)]
-  *******************************************
-  2.直接加减数字
-  在自定义格式后直接“+/-”数字
-  * 后 N 周：$[yyyyMMdd+7*N]
-  * 前 N 周：$[yyyyMMdd-7*N]
-  * 后 N 天：$[yyyyMMdd+N]
-  * 前 N 天：$[yyyyMMdd-N]
-  * 后 N 小时：$[HHmmss+N/24]
-  * 前 N 小时：$[HHmmss-N/24]
-  * 后 N 分钟：$[HHmmss+N/24/60]
-  * 前 N 分钟：$[HHmmss-N/24/60]
-
+    1.使用add_months()函数，该函数用于加减月份，

Review Comment:
   Could u plz add a corresponding English doc for this feature?



##########
docs/docs/zh/guide/parameter/built-in.md:
##########
@@ -28,23 +28,33 @@
 - 我们定义这种基准变量为 \$[...] 格式的，\$[yyyyMMddHHmmss] 是可以任意分解组合的，比如：\$[yyyyMMdd], \$[HHmmss], \$[yyyy-MM-dd] 等
 
 - 也可以通过以下两种方式：
-
-  1.使用add_months()函数，该函数用于加减月份，
-  第一个入口参数为[yyyyMMdd]，表示返回时间的格式
-  第二个入口参数为月份偏移量，表示加减多少个月
-  * 后 N 年：$[add_months(yyyyMMdd,12*N)]
-  * 前 N 年：$[add_months(yyyyMMdd,-12*N)]
-  * 后 N 月：$[add_months(yyyyMMdd,N)]
-  * 前 N 月：$[add_months(yyyyMMdd,-N)]
-  *******************************************
-  2.直接加减数字
-  在自定义格式后直接“+/-”数字
-  * 后 N 周：$[yyyyMMdd+7*N]
-  * 前 N 周：$[yyyyMMdd-7*N]
-  * 后 N 天：$[yyyyMMdd+N]
-  * 前 N 天：$[yyyyMMdd-N]
-  * 后 N 小时：$[HHmmss+N/24]
-  * 前 N 小时：$[HHmmss-N/24]
-  * 后 N 分钟：$[HHmmss+N/24/60]
-  * 前 N 分钟：$[HHmmss-N/24/60]
-
+    1.使用add_months()函数，该函数用于加减月份，
+    第一个入口参数为[yyyyMMdd]，表示返回时间的格式
+    第二个入口参数为月份偏移量，表示加减多少个月

Review Comment:
   BTW, I think there could be a better design. Instead of using `N` and converting it into different time units, I suggest we use `y` for year, `M` or month, `d` for day. For example, replace `$[add_months(yyyyMMdd,12*N)]` with `$[add(yyyyMMdd, y)]`. But this is out of the scope of this PR. I will create a new issue for further discussion.



##########
dolphinscheduler-task-plugin/dolphinscheduler-task-api/src/main/java/org/apache/dolphinscheduler/plugin/task/api/parser/TimePlaceholderUtils.java:
##########
@@ -363,6 +375,92 @@ private static String calculateTime(String expression, Date date) {
         return value;
     }
 
+    /**
+     * get week of year
+     * @param expression expression
+     * @param date       date
+     * @return week of year
+     */
+    public static String calcYearWeek(String expression, Date date) {

Review Comment:
   ```suggestion
       public static String calculateYearWeek(String expression, Date date) {
   ```
   
   `calc` is a wired shorthand. I prefer to use `calculate` here.



##########
dolphinscheduler-task-plugin/dolphinscheduler-task-api/src/main/java/org/apache/dolphinscheduler/plugin/task/api/parser/TimePlaceholderUtils.java:
##########
@@ -363,6 +375,92 @@ private static String calculateTime(String expression, Date date) {
         return value;
     }
 
+    /**
+     * get week of year
+     * @param expression expression
+     * @param date       date
+     * @return week of year
+     */
+    public static String calcYearWeek(String expression, Date date) {
+
+        String dataFormat = expression.substring(YEAR_WEEK.length() + 1, expression.length() - 1);
+
+        String targetDate = "";
+        try {
+
+            if(dataFormat.contains(COMMA)) {
+                String param1 = dataFormat.split(COMMA)[0];
+                String param2 = dataFormat.split(COMMA)[1];
+                dataFormat = param1;
+
+                targetDate = transformYearWeek(date, dataFormat, calculate(param2));
+
+            } else {
+                targetDate = transformYearWeek(date, dataFormat, 1);
+            }
+        } catch (Exception e) {
+            throw new RuntimeException("expression not valid");
+        }
+
+        return targetDate;
+    }
+
+    /**
+     * transform week of year
+     * @param date date
+     * @param format date_format,for example: yyyy-MM-dd / yyyyMMdd
+     * @param weekDay day of week
+     * @return date_string
+     */
+    private static String transformYearWeek(Date date,String format,int weekDay) {

Review Comment:
   ```suggestion
       private static String transformYearWeek(Date date, String format, int weekDay) {
   ```



##########
dolphinscheduler-task-plugin/dolphinscheduler-task-api/src/test/java/org/apache/dolphinscheduler/plugin/task/api/parser/TimePlaceholderUtilsTest.java:
##########
@@ -0,0 +1,118 @@
+/*
+ * 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.dolphinscheduler.plugin.task.api.parser;
+
+import org.apache.dolphinscheduler.spi.enums.CommandType;
+import org.apache.dolphinscheduler.spi.utils.DateUtils;
+
+import java.util.Date;
+import java.util.Map;
+
+import org.junit.Assert;
+import org.junit.Test;
+
+
+public class TimePlaceholderUtilsTest {
+
+    @Test
+    public void placeHolderTimeTest() {
+        Date date = DateUtils.parse("2022-08-26 00:00:00", "yyyy-MM-dd HH:mm:ss");
+
+        Map<String, String> timeParams = BusinessTimeUtils.getBusinessTime(CommandType.COMPLEMENT_DATA, date);
+
+        // this_day test

Review Comment:
   Instead of writing a long method as a test case, I suggest splitting it into multiple short cases and remove the comments. For example, we could have 
   
   ```java
   @Test
       public void timePlaceHolderForThisDay () { ... }
   ```
   
   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: commits-unsubscribe@dolphinscheduler.apache.org

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