You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@dolphinscheduler.apache.org by "iampratap7997-dot (via GitHub)" <gi...@apache.org> on 2026/03/06 05:34:53 UTC
[PR] [Fix-18015] [DST ISSUE] Handle DST issue when calculating yyyyMMdd±N time placeholders [dolphinscheduler]
iampratap7997-dot opened a new pull request, #18026:
URL: https://github.com/apache/dolphinscheduler/pull/18026
<!--Thanks very much for contributing to Apache DolphinScheduler, we are happy that you want to help us improve DolphinScheduler! -->
## Was this PR generated or assisted by AI?
NO (Only the manual test case for America/Los_Angeles timezone was created by AI to verify the fix.)
## Purpose of the pull request
Fixes #18015 DST issue when calculating $[yyyyMMdd±N] time placeholders.
Day-based calculations were previously implemented in minutes, which caused incorrect dates during Daylight Saving Time transitions.
## Brief change log
1- Replaced minute-based day calculations with addDays for expressions like yyyyMMdd+N & yyyyMMdd-N.
2- Ensures correct date calculation in timezones with DST (e.g., America/Los_Angeles).
## Verify this pull request
- The fix was manually verified using America/Los_Angeles timezone around DST transition.
- Unit tests cover existing placeholders (no new tests added, logic change only).
## Pull Request Notice
[Pull Request Notice](https://github.com/apache/dolphinscheduler/blob/dev/docs/docs/en/contribute/join/pull-request.md)
If your pull request contains incompatible change, you should also add it to `docs/docs/en/guide/upgrade/incompatible.md`
--
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
Re: [PR] [Fix-18015] [DST ISSUE] Handle DST issue when calculating yyyyMMdd±N time placeholders [dolphinscheduler]
Posted by "iampratap7997-dot (via GitHub)" <gi...@apache.org>.
iampratap7997-dot commented on PR #18026:
URL: https://github.com/apache/dolphinscheduler/pull/18026#issuecomment-4015447656
HI @Gallardot @SbloodyS @caishunfeng,
Please add the milestone label for this PR .
Thanks,
Divyansh.
--
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
Re: [PR] [Fix-18015] [DST ISSUE] Handle DST issue when calculating yyyyMMdd±N time placeholders [dolphinscheduler]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #18026:
URL: https://github.com/apache/dolphinscheduler/pull/18026#discussion_r2937849945
##########
dolphinscheduler-task-plugin/dolphinscheduler-task-api/src/main/java/org/apache/dolphinscheduler/plugin/task/api/parser/TimePlaceholderUtils.java:
##########
@@ -649,19 +649,43 @@ public static Map.Entry<Date, String> calcMinutes(String expression, Date date)
if (expression.contains("+")) {
int index = expression.lastIndexOf('+');
- if (Character.isDigit(expression.charAt(index + 1))) {
- String addMinuteExpr = expression.substring(index + 1);
- Date targetDate = addMinutes(date, calcMinutes(addMinuteExpr));
+ if (index + 1 < expression.length() && Character.isDigit(expression.charAt(index + 1))) {
+ String addDayExpr = expression.substring(index + 1);
+ int days;
+ try {
+ days = Integer.parseInt(addDayExpr);
+ } catch (NumberFormatException e) {
+ return null;
+ }
+ LocalDate localDate = date.toInstant()
+ .atZone(ZoneId.systemDefault())
+ .toLocalDate()
+ .plusDays(days);
+
+ Date targetDate = Date.from(
+ localDate.atStartOfDay(ZoneId.systemDefault()).toInstant());
Review Comment:
On parse failure this method returns `null`, but `calcTimeExpression` immediately dereferences `resultEntry.getKey()`/`getValue()`, causing an NPE and treating valid expressions as "unsupported" under `ignoreInvalidExpression`. Prefer returning a non-null entry (e.g., `(date, expression)`) or throwing a clear exception so invalid placeholders fail deterministically without an intermediate NPE.
##########
dolphinscheduler-task-plugin/dolphinscheduler-task-api/src/main/java/org/apache/dolphinscheduler/plugin/task/api/parser/TimePlaceholderUtils.java:
##########
@@ -649,19 +649,43 @@ public static Map.Entry<Date, String> calcMinutes(String expression, Date date)
if (expression.contains("+")) {
int index = expression.lastIndexOf('+');
- if (Character.isDigit(expression.charAt(index + 1))) {
- String addMinuteExpr = expression.substring(index + 1);
- Date targetDate = addMinutes(date, calcMinutes(addMinuteExpr));
+ if (index + 1 < expression.length() && Character.isDigit(expression.charAt(index + 1))) {
+ String addDayExpr = expression.substring(index + 1);
+ int days;
+ try {
+ days = Integer.parseInt(addDayExpr);
+ } catch (NumberFormatException e) {
+ return null;
+ }
Review Comment:
`calcMinutes(String expression, Date date)` now assumes the offset after `+`/`-` is a plain integer and uses `Integer.parseInt`. This breaks existing supported placeholders like `$[yyyyMMdd+7*2]` and `$[HHmmss+2/24]` (see `ParameterUtilsTest`), which rely on arithmetic/fraction parsing via the existing `calcMinutes(String minuteExpression)` helper.
##########
dolphinscheduler-task-plugin/dolphinscheduler-task-api/src/main/java/org/apache/dolphinscheduler/plugin/task/api/parser/TimePlaceholderUtils.java:
##########
@@ -18,8 +18,6 @@
package org.apache.dolphinscheduler.plugin.task.api.parser;
import static org.apache.commons.lang3.time.DateUtils.addWeeks;
Review Comment:
The static import for `addDays` was removed, but this class still calls `addDays(...)` in multiple methods (e.g., `calcMonthBegin`, `calcMonthEnd`, `calcCustomDay`, `calcWeekStart/End`). As-is, this file will not compile; either restore the static import or qualify the calls (e.g., `DateUtils.addDays`).
--
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
Re: [PR] [Fix-18015] [DST ISSUE] Handle DST issue when calculating yyyyMMdd±N time placeholders [dolphinscheduler]
Posted by "github-advanced-security[bot] (via GitHub)" <gi...@apache.org>.
github-advanced-security[bot] commented on code in PR #18026:
URL: https://github.com/apache/dolphinscheduler/pull/18026#discussion_r2898528462
##########
dolphinscheduler-task-plugin/dolphinscheduler-task-api/src/main/java/org/apache/dolphinscheduler/plugin/task/api/parser/TimePlaceholderUtils.java:
##########
@@ -649,19 +648,21 @@
if (expression.contains("+")) {
int index = expression.lastIndexOf('+');
- if (Character.isDigit(expression.charAt(index + 1))) {
- String addMinuteExpr = expression.substring(index + 1);
- Date targetDate = addMinutes(date, calcMinutes(addMinuteExpr));
+ if (index + 1 < expression.length() && Character.isDigit(expression.charAt(index + 1))) {
+ String addDayExpr = expression.substring(index + 1);
+ int days = Integer.parseInt(addDayExpr);
Review Comment:
## Missing catch of NumberFormatException
Potential uncaught 'java.lang.NumberFormatException'.
[Show more details](https://github.com/apache/dolphinscheduler/security/code-scanning/5620)
##########
dolphinscheduler-task-plugin/dolphinscheduler-task-api/src/main/java/org/apache/dolphinscheduler/plugin/task/api/parser/TimePlaceholderUtils.java:
##########
@@ -649,19 +648,21 @@
if (expression.contains("+")) {
int index = expression.lastIndexOf('+');
- if (Character.isDigit(expression.charAt(index + 1))) {
- String addMinuteExpr = expression.substring(index + 1);
- Date targetDate = addMinutes(date, calcMinutes(addMinuteExpr));
+ if (index + 1 < expression.length() && Character.isDigit(expression.charAt(index + 1))) {
+ String addDayExpr = expression.substring(index + 1);
+ int days = Integer.parseInt(addDayExpr);
+ Date targetDate = addDays(date, days);
String dateFormat = expression.substring(0, index);
return new AbstractMap.SimpleImmutableEntry<>(targetDate, dateFormat);
}
} else if (expression.contains("-")) {
int index = expression.lastIndexOf('-');
- if (Character.isDigit(expression.charAt(index + 1))) {
- String addMinuteExpr = expression.substring(index + 1);
- Date targetDate = addMinutes(date, 0 - calcMinutes(addMinuteExpr));
+ if (index + 1 < expression.length() && Character.isDigit(expression.charAt(index + 1))) {
+ String addDayExpr = expression.substring(index + 1);
+ int days = Integer.parseInt(addDayExpr);
Review Comment:
## Missing catch of NumberFormatException
Potential uncaught 'java.lang.NumberFormatException'.
[Show more details](https://github.com/apache/dolphinscheduler/security/code-scanning/5621)
--
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
Re: [PR] [Fix-18015] [DST ISSUE] Handle DST issue when calculating yyyyMMdd±N time placeholders [dolphinscheduler]
Posted by "SbloodyS (via GitHub)" <gi...@apache.org>.
SbloodyS closed pull request #18026: [Fix-18015] [DST ISSUE] Handle DST issue when calculating yyyyMMdd±N time placeholders
URL: https://github.com/apache/dolphinscheduler/pull/18026
--
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
Re: [PR] [Fix-18015] [DST ISSUE] Handle DST issue when calculating yyyyMMdd±N time placeholders [dolphinscheduler]
Posted by "iampratap7997-dot (via GitHub)" <gi...@apache.org>.
iampratap7997-dot commented on PR #18026:
URL: https://github.com/apache/dolphinscheduler/pull/18026#issuecomment-4031971317
Hi ,
I've fixed the DST issue occurring in the recent test case and also addressed the Copilot issues.
Could you please re-run the tests?
Thanks,
Divyansh
--
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
Re: [PR] [Fix-18015] [DST ISSUE] Handle DST issue when calculating yyyyMMdd±N time placeholders [dolphinscheduler]
Posted by "SbloodyS (via GitHub)" <gi...@apache.org>.
SbloodyS commented on PR #18026:
URL: https://github.com/apache/dolphinscheduler/pull/18026#issuecomment-4196168478
Please check the failed CI. @iampratap7997-dot
--
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