Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix(sink): construct java.sql.time/date object with a long type #13651

Merged
merged 6 commits into from
Dec 4, 2023
Merged

fix(sink): construct java.sql.time/date object with a long type #13651

merged 6 commits into from
Dec 4, 2023

Conversation

xuefengze
Copy link
Contributor

I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.

What's changed and what's your intention?

resolve #13637

Checklist

  • I have written necessary rustdoc comments
  • I have added necessary unit tests and integration tests
  • I have added test labels as necessary. See details.
  • I have added fuzzing tests or opened an issue to track them. (Optional, recommended for new SQL features Sqlsmith: Sql feature generation #7934).
  • My PR contains breaking changes. (If it deprecates some features, please create a tracking issue to remove them in the future).
  • All checks passed in ./risedev check (or alias, ./risedev c)
  • My PR changes performance-critical code. (Please run macro/micro-benchmarks and show the results.)
  • My PR contains critical fixes that are necessary to be merged into the latest release. (Please check out the details)

Documentation

  • My PR needs documentation updates. (Please use the Release note section below to summarize the impact on users)

Release note

If this PR includes changes that directly affect users or other significant modifications relevant to the community, kindly draft a release note to provide a concise summary of these changes. Please prioritize highlighting the impact these changes will have on users.

@xuefengze xuefengze requested a review from a team as a code owner November 27, 2023 02:55
@xuefengze xuefengze requested a review from lmatz November 27, 2023 02:56
@lmatz lmatz requested a review from xiangjinwu November 27, 2023 03:01
lmatz
lmatz approved these changes Nov 27, 2023
View reviewed changes
Copy link
Contributor

@lmatz lmatz left a comment

Choose a reason for hiding this comment

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

LGTM as we discussed

@xiangjinwu
Copy link
Contributor

xiangjinwu commented Nov 27, 2023 •
edited
Loading

Not familiar about current implementation in Java. But FYI:

  • In RisingWave, time / timestamp / timestamptz has a precision of microseconds, neither milli nor nano.
  • When possible, prefer "new" classes in java.time.* since Java 8 (doc) over legacy ones in java.sql.*.
    • date -> LocalDate
    • time -> LocalTime
    • timestamp -> LocalDateTime
    • timestamptz -> OffsetDateTime

@lmatz lmatz requested a review from wenym1 November 27, 2023 05:01
wenym1
wenym1 reviewed Nov 27, 2023
View reviewed changes
src/jni_core/src/lib.rs Outdated Show resolved Hide resolved
src/jni_core/src/lib.rs Outdated Show resolved Hide resolved
java/connector-node/risingwave-sink-es-7/src/main/java/com/risingwave/connector/EsSink.java Outdated
// construct java.sql.Time/Timestamp with milliseconds time value.
// it will use system timezone by default, so we have to set timezone manually
case TIME:
SimpleDateFormat tDfm = new SimpleDateFormat("HH:mm:ss.SSS");
Copy link
Contributor

Choose a reason for hiding this comment

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

The formatter can be stored as a static field to avoid repeatedly creating the objects.

Copy link
Contributor Author

@xuefengze xuefengze Nov 28, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

SimpleDateFormatter aren’t thread-safe. Using a static formatter seems to lead to incorrect data and test failure. https://buildkite.com/risingwavelabs/pull-request/builds/36192#018c1499-1f24-4950-a41a-d703aaaec4e2

Copy link
Contributor

Choose a reason for hiding this comment

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

That's one of the many reasons to prefer java.time.* But yes it may need a prior refactor for other remote sinks as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

SimpleDateFormatter aren’t thread-safe. Using a static formatter seems to lead to incorrect data and test failure. https://buildkite.com/risingwavelabs/pull-request/builds/36192#018c1499-1f24-4950-a41a-d703aaaec4e2

If so we can store them as a ordinary field instead of a static field and create an instance for each sink writer, since within a single sink writer it's single threaded.

@xuefengze
Copy link
Contributor Author

Not familiar about current implementation in Java. But FYI:

* In RisingWave, time / timestamp / timestamptz has a precision of microseconds, neither milli nor nano.

* When possible, prefer "new" classes in `java.time.*` since Java 8 ([doc](https://www.oracle.com/technical-resources/articles/java/jf14-date-time.html)) over [legacy](https://mail.openjdk.org/pipermail/discuss/2022-May/006077.html) ones in `java.sql.*`.
  
  * `date` -> `LocalDate`
  * `time` -> `LocalTime`
  * `timestamp` -> `LocalDateTime`
  * `timestamptz` -> `OffsetDateTime`

It seems that all remote sinks use java.sql.Time\Date\Timestamp, and their precision is in milliseconds.

wenym1
wenym1 approved these changes Nov 28, 2023
View reviewed changes
Copy link
Contributor

@wenym1 wenym1 left a comment

Choose a reason for hiding this comment

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

LGTM.

Are the changes in this PR covered by any CI pipeline?

@xiangjinwu xiangjinwu mentioned this pull request Nov 28, 2023
Closed
@xuefengze
Copy link
Contributor Author

Are the changes in this PR covered by any CI pipeline?

I have updated the es_sink e2e test.

@wenym1
Copy link
Contributor

wenym1 commented Dec 4, 2023

@xuefengze Can we merge this PR?

@xuefengze
Copy link
Contributor Author

@xuefengze Can we merge this PR?

Need review from Cargo.lock?

hzxa21
hzxa21 approved these changes Dec 4, 2023
View reviewed changes
Copy link
Collaborator

@hzxa21 hzxa21 left a comment

Choose a reason for hiding this comment

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

LGTM for Cargo.lock

@xuefengze xuefengze added this pull request to the merge queue Dec 4, 2023
Merged via the queue into risingwavelabs:main with commit d7d56bd Dec 4, 2023
26 of 27 checks passed
@xuefengze xuefengze deleted the fix_time_init branch December 4, 2023 08:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wenym1 wenym1 wenym1 approved these changes

@lmatz lmatz lmatz approved these changes

@hzxa21 hzxa21 hzxa21 approved these changes

@xiangjinwu xiangjinwu Awaiting requested review from xiangjinwu

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

for Date and Time, their minimal required precision is millisecond
5 participants
@xuefengze @xiangjinwu @wenym1 @hzxa21 @lmatz