Skip to content

Conversation

raminqaf
Copy link
Contributor

@raminqaf raminqaf commented Sep 1, 2025

What is the purpose of the change

Add precision supports for the TIME function. Currently, Flink only takes precision 0 and ignores any other type of precision.

Brief change log

  • Pass precision to TIME funciton.
  • Fixed the TIME conversion formula in JsonToRowDataConverters.convertToTime() from localTime.toNanoOfDay() * 1000_000 to localTime.toNanoOfDay() / 1000_000
  • Enhanced test coverage in JsonRowDataSerDeSchemaTest.testSerDe() to include comprehensive testing of all supported data types
  • Updated test assertions to use correct field indexing and proper row size

Verifying this change

This change is already covered by existing tests, such as:

  • JsonRowDataSerDeSchemaTest.testSerDe() - Enhanced to comprehensively test TIME type conversion for both JsonParser and non-JsonParser deserializers
  • The test now verifies correct handling of TIME(0) and TIME(3) precision levels
  • Added test coverage for the previously failing scenario that caused the DateTimeException

This change added tests and can be verified as follows:

  • The enhanced testSerDe() method now tests the complete round-trip serialization/deserialization for TIME types
  • Verified that the fix resolves the DateTimeException: Invalid value for HourOfDay error
  • Confirmed that both isJsonParser=true and isJsonParser=false code paths work correctly
  • Manually verified that TIME values like "12:12:43" and "12:12:43.123" are properly converted without precision loss

Does this pull request potentially affect one of the following parts:

  • Dependencies (does it add or upgrade a dependency): no
  • The public API, i.e., is any changed class annotated with @Public(Evolving): no
  • The serializers: yes - affects JSON format serialization/deserialization of TIME types
  • The runtime per-record code paths (performance sensitive): no - this is format-specific conversion logic
  • Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Kubernetes/Yarn, ZooKeeper: no
  • The S3 file system connector: no

Documentation

  • Does this pull request introduce a new feature? no
  • If yes, how is the feature documented? not applicable - this is a bug fix for existing functionality

@raminqaf raminqaf changed the title [FLINK-17224] Support precision of TIME type [FLINK-17224][table] Support precision of TIME type Sep 1, 2025
@raminqaf
Copy link
Contributor Author

raminqaf commented Sep 1, 2025

The TimeLongConvert converts the time to millisecond precision. I was wondering if we need to change this to TimeLongConverter implements DataStructureConverter<Long, Long> so that we internally support nanosecond precision.

@raminqaf raminqaf marked this pull request as ready for review September 1, 2025 10:28
@flinkbot
Copy link
Collaborator

flinkbot commented Sep 1, 2025

CI report:

Bot commands The @flinkbot bot supports the following commands:
  • @flinkbot run azure re-run the last Azure build

@@ -266,7 +266,7 @@ private int convertToTime(JsonParser jsonNode) throws IOException {
LocalTime localTime = parsedTime.query(TemporalQueries.localTime());

// get number of milliseconds of the day
return localTime.toSecondOfDay() * 1000;
return (int) (localTime.toNanoOfDay() / 1000_000);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we change the way that the json format parses times, then the output will change for existing users, which has implications to the logic of their processing. I suggest that this way of parsing be enabled by a new json format configuration option - so it can be knowingly adopted.

@raminqaf
Copy link
Contributor Author

raminqaf commented Sep 1, 2025

A similar PR exists:
#22775

@github-actions github-actions bot added the community-reviewed PR has been reviewed by the community. label Sep 1, 2025
Comment on lines +859 to +861
TIME(5),
DEFAULT_TIME,
LocalDateTime.of(1970, 1, 1, 12, 34, 56, 123_000_000))
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are not really supporting TIME(5). As far as I understood, the time converters are storing the time as integer and not as long. We lose the precession (>3) there.

@github-actions github-actions bot added community-reviewed PR has been reviewed by the community. and removed community-reviewed PR has been reviewed by the community. labels Sep 2, 2025
@github-actions github-actions bot added community-reviewed PR has been reviewed by the community. and removed community-reviewed PR has been reviewed by the community. labels Sep 5, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community-reviewed PR has been reviewed by the community.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants