NIFI-15308 Time record field converters handle time offset

[turcsanyip]

Summary

NIFI-15308

Tracking

Please complete the following tracking steps prior to pull request creation.

Issue Tracking

• Apache NiFi Jira issue created

Pull Request Tracking

• Pull Request title starts with Apache NiFi Jira issue number, such as NIFI-00000
• Pull Request commit message starts with Apache NiFi Jira issue number, as such NIFI-00000

Pull Request Formatting

• Pull Request based on current revision of the main branch
• Pull Request refers to a feature branch with one commit containing changes

Verification

Please indicate the verification steps performed prior to pull request creation.

Build

• Build completed using ./mvnw clean install -P contrib-check
  • JDK 21
  • JDK 25

Licensing

• New dependencies are compatible with the Apache License 2.0 according to the License Policy
• New dependencies are documented in applicable LICENSE and NOTICE files

Documentation

• Documentation formatting appears as expected in rendered files

[exceptionfactory]

Thanks for addressing this issue @turcsanyip. The implementation in the Field Converter looks good, following the same basic strategy as the converter for LocalDateTime.

Although the TimeZone.setDefault() approach for testing is used in some other modules, it should be possible to avoid that and instead compare expected values.

The ObjectTimestampFieldConverterTest has a getDateTimeZoneOffset method that builds a string with a specific offset based on the current system default timezone, which is one way to evaluate the conversion, without having to call TimeZone.setOffset(). I recommend looking at that approach as opposed to changing the default TimeZone in the test method.

[turcsanyip]

Thanks for your feedback @exceptionfactory. I updated the tests to avoid setting the default time zone at the JVM level.

Actually, I was focusing on the use case where the same input data is processed in environments with different time zone settings. But the issue can also be tested by using the same runtime time zone with different input data offsets. The main point is that the JVM’s system time zone needs to be different from the input data’s offset. It does not matter in what way.

I added several different time zone offsets to the tests in order to cover various cases (not only system default and UTC that can overlap and might not reveal the current issue).

[exceptionfactory]

Thanks for the update @turcsanyip. The tests look better without the TimeZone changes. However, the construction of the expected value is still a bit difficult to follow. In past experience, I have found this to be important when reviewing conversion code and associated expectations.

[exceptionfactory]

The levels of nesting make this declaration rather hard to follow.

On closer analysis, I think a different approach is needed.

The parsing and conversion from LocalTime to ZonedDateTime and back to LocalTime effectively do something similar to what the FieldConverter itself is doing.

The challenge is LocalTime is different based on where it is running. However, the main goal of the comparison should be that the FieldConverter shifts the hours forward or backward, based on the relative offset. The issue is that the number of hours is different based on the JVM system default zone. That's the reason for limiting the test to the offset of the system default zone, versus attempting to test other offsets.

With that background, it would help break up the declaration of each variable, as opposed to nesting the parsing and construction of different values. That should make it somewhat easier to understand how the expected value is being constructed.

I can take a closer look at providing an alternative if that would be helpful.

[turcsanyip]

Time-related code is not easy to read, and I actually spent a lot of time getting familiar with the existing code as well. I created an updated version in which the result of each step is assigned to variables with explicit, descriptive names. I’m wondering what you think of it.

[exceptionfactory]

Thanks for the updates @turcsanyip, I pushed a minor update to the test classes, adjusting the ordering, naming, and breaking out the ZoneOffset variable from the input string. If those changes look good to you, I will proceed with merging.

[turcsanyip]

@exceptionfactory The code looks good. Please go ahead with merging.
The Windows job failed but looks unrelated. I restarted it to be sure.

[exceptionfactory]

Thanks again @turcsanyip! +1 merging