NIFI-15900 Add optional timezone to plusDuration and minusDuration for DST-aware calendar arithmetic#11296
Conversation
pvillard31
left a comment
pvillard31
left a comment
There was a problem hiding this comment.
Please rebase on top of main before submitting a PR so that the GitHub checks can run.
rakesh-rsky
force-pushed
the
fix/NIFI-15900-dst-aware-plusDuration
branch
from
1cd92a2 to
3dba077
Compare
| * <pre> | ||
| * ${date:toDate('dd-MM-yyyy'):plusDuration('1 week'):format('dd-MM-yyyy')} | ||
| * ${date:toDate('dd-MM-yyyy'):plusDuration('2 years')} | ||
| * ${date:toDate('dd-MM-yyyy', 'America/Chicago'):plusDuration('1 day', 'America/Chicago'):format('dd-MM-yyyy', 'America/Chicago')} |
There was a problem hiding this comment.
The expression language guide documents plusDuration and minusDuration with only the amount argument. Should we update expression-language-guide.adoc to document the new optional timezone argument and add a DST example?
| if (timeZoneEvaluator != null) { | ||
| final String tz = timeZoneEvaluator.evaluate(evaluationContext).getValue(); | ||
| if (tz != null) { | ||
| zoneId = ZoneId.of(tz); |
There was a problem hiding this comment.
The amount literal is validated in the constructor, but the timezone is only resolved at evaluate time. Should we validate a literal timezone at construction so an invalid zone fails fast like an invalid amount?
…r DST-aware calendar arithmetic
When adding or subtracting calendar units (days, weeks, months, years) across DST
boundaries, the result should preserve the wall-clock time rather than adding a
fixed number of seconds. Previously, both functions always used the JVM system
default timezone when converting the Date to a ZonedDateTime, which produced
incorrect results when the desired timezone differed from the system default.
Changes:
- Add optional timezone parameter to plusDuration and minusDuration functions
e.g. plusDuration('1 day', 'America/New_York')
- Update ANTLR parser grammar to allow 1 or 2 arguments for these functions
- AbstractDateArithmeticEvaluator uses the provided timezone (or system default)
when performing calendar arithmetic
- Add DST boundary test cases covering spring-forward and fall-back scenarios
rakesh-rsky
force-pushed
the
fix/NIFI-15900-dst-aware-plusDuration
branch
from
3dba077 to
23214dc
Compare
|
@pvillard31 you are right I have updated expression-language-guide.adoc for both plusDuration and minusDuration to document the new optional timezone argument, added a DST example row to each example table, and added a DST boundary scenario to each Calendar-Aware Behavior table. Also added fail-fast validation for literal timezone arguments in the constructor of AbstractDateArithmeticEvaluator, mirroring the existing pattern used for the amount literal via DateAmountParser.validate. For dynamic (non-literal) timezone expressions, the ZoneId.of() call in evaluate() is now also wrapped to throw AttributeExpressionLanguageException consistently. Added test coverage for both cases in TestQuery. |
pvillard31
left a comment
pvillard31
left a comment
There was a problem hiding this comment.
Thanks, latest LGTM, merging
2 participants
NIFI-15900 Add optional timezone to plusDuration and minusDuration for DST-aware calendar arithmetic
When adding or subtracting calendar units (days, weeks, months, years) across DST boundaries, the result should preserve the wall-clock time rather than adding a fixed number of seconds. Previously, both functions always used the JVM system default timezone when converting the Date to a ZonedDateTime, which produced incorrect results when the desired timezone differed from the system default.
Changes:
Summary
NIFI-15900
Tracking
Please complete the following tracking steps prior to pull request creation.
Issue Tracking
Pull Request Tracking
NIFI-00000NIFI-00000VerifiedstatusPull Request Formatting
mainbranchVerification
Please indicate the verification steps performed prior to pull request creation.
Build
./mvnw clean install -P contrib-checkLicensing
LICENSEandNOTICEfilesDocumentation