NIFI-15973 Fix issues caused by the use of deprecated GregorianCalendar#11283
NIFI-15973 Fix issues caused by the use of deprecated GregorianCalendar#11283Kamilkime wants to merge 4 commits into
Conversation
2592587 to
d7a238f
Compare
exceptionfactory
left a comment
There was a problem hiding this comment.
Thanks for describing the issues with the current Date and Timestamp conversion @Kamilkime. This pull request highlights the subtle behavior of different time classes.
I noted some recommendations related to the unit test methods for readability and clear construction.
Regarding the functional change, it appears reasonable from one perspective, since date conversion of all modern values are unchanged. I would expect this to cover many use cases without concern.
The key question is whether the change in behavior for ancient dates is acceptable. At minimum, the nuance should be commented in the Converter classes that the described approach uses the Proleptic Gregorian Calendar System, avoid the implicit use of deprecated Date conversion methods. Declaring the intermediate values would also helpful emphasize the behavioral adjustment.
| final Instant expected = LocalDateTime.of(1, 1, 1, 12, 0, 0).atZone(ZoneId.systemDefault()).toInstant(); | ||
| assertEquals(expected, timestamp.toInstant()); |
There was a problem hiding this comment.
Unit tests should assert expected Timestamp values, not values converted to Instant objects.
There was a problem hiding this comment.
Changed to Timestamp assertions
| @Test | ||
| public void testConvertStringYearOneIsProlepticGregorian() { | ||
| final LocalDateTime result = converter.convertField("0001-01-01 12:00:00", Optional.of("yyyy-MM-dd HH:mm:ss"), FIELD_NAME); | ||
| assertEquals(LocalDateTime.of(1, 1, 1, 12, 0, 0), result); |
There was a problem hiding this comment.
The expected value should be declared separately instead of inline instance creation for clarity.
There was a problem hiding this comment.
Extracted expected variable
| final LocalDate localDate = (value instanceof LocalDate ld) ? ld : LocalDate.ofEpochDay((int) value); | ||
| return new java.sql.Date(localDate.atStartOfDay(ZoneId.systemDefault()).toInstant().toEpochMilli()); |
There was a problem hiding this comment.
The inline ternary and cast is difficult to read, and the multiple levels of conversion to create the new java.sql.Date() should be split out and declared.
There was a problem hiding this comment.
Used an if statement for the localDate value, and extracted zonedDate
| @Test | ||
| void testConvertFieldSqlDateModernYear() { | ||
| final LocalDate localDate = LocalDate.of(2025, 5, 25); | ||
| final Date sqlDate = new Date(localDate.atStartOfDay(ZoneId.systemDefault()).toInstant().toEpochMilli()); |
There was a problem hiding this comment.
Intermediate values should be declared for readability.
There was a problem hiding this comment.
Extracted zonedDate
Kamilkime
left a comment
There was a problem hiding this comment.
Thanks @exceptionfactory, I addressed your comments, please let me know if it looks good now
| @Test | ||
| void testConvertFieldSqlDateModernYear() { | ||
| final LocalDate localDate = LocalDate.of(2025, 5, 25); | ||
| final Date sqlDate = new Date(localDate.atStartOfDay(ZoneId.systemDefault()).toInstant().toEpochMilli()); |
There was a problem hiding this comment.
Extracted zonedDate
| @Test | ||
| public void testConvertStringYearOneIsProlepticGregorian() { | ||
| final LocalDateTime result = converter.convertField("0001-01-01 12:00:00", Optional.of("yyyy-MM-dd HH:mm:ss"), FIELD_NAME); | ||
| assertEquals(LocalDateTime.of(1, 1, 1, 12, 0, 0), result); |
There was a problem hiding this comment.
Extracted expected variable
| final LocalDate localDate = (value instanceof LocalDate ld) ? ld : LocalDate.ofEpochDay((int) value); | ||
| return new java.sql.Date(localDate.atStartOfDay(ZoneId.systemDefault()).toInstant().toEpochMilli()); |
There was a problem hiding this comment.
Used an if statement for the localDate value, and extracted zonedDate
| final Instant expected = LocalDateTime.of(1, 1, 1, 12, 0, 0).atZone(ZoneId.systemDefault()).toInstant(); | ||
| assertEquals(expected, timestamp.toInstant()); |
There was a problem hiding this comment.
Changed to Timestamp assertions
Summary
NIFI-15973
Avoid Julian-calendar shift when converting
java.sql.Date/java.sql.Timestampvalues tojava.timetypes (and back) in NiFi's record framework.java.sql.Timestamp#toLocalDateTime,java.sql.Timestamp#valueOf(LocalDateTime),java.sql.Date#toLocalDate, andjava.sql.Date#valueOf(LocalDate)all route through GregorianCalendar, which applies Julian-calendar semantics for years before 1582-10-15. As a result, a value whose epoch milliseconds represent year 0001 in the proleptic Gregorian calendar (the calendarjava.time, Avro, BigQuery, or Snowflake all use) is rendered as year 0000 — about a 2-day backward shift. Downstream sinks that validate against an inclusive year-1-to-9999 range (e.g., the Snowflake Ingest SDK) then reject the row:Timestamp out of representable inclusive range of years between 1 and 9999, ... value:0000-12-30T10:47Z.The fix replaces every Julian-aware bridge between
java.sql.*andjava.time.*with an Instant-based one, which preserves the proleptic-Gregorian instant the source actually carried.Tests:
Compatibility:
Backwards compatible. Instant-based conversion and Julian-aware Timestamp / Date conversion agree for every date from 1582-10-15 onward, so all values produced after the Gregorian cutover (i.e., everything the vast majority of users have ever ingested) are unchanged. Pre-cutover values — including the 1582-10-05 → 1582-10-14 gap that Julian had but proleptic Gregorian does not — now match the proleptic-Gregorian instant the source actually carried, which is what
java.time, Avro, and downstream sinks already assume.No public API signatures change, the fix is purely internal to the existing converter implementations.
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