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.

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

AVRO-3914: Add nanos support for the Java SDK #2608

Merged
merged 1 commit into from
Dec 12, 2023
Merged

Conversation

Fokko
Copy link
Contributor

@Fokko Fokko commented Dec 4, 2023

What is the purpose of the change

AVRO-3914

Verifying this change

(Please pick one of the following options)

This change is a trivial rework / code cleanup without any test coverage.

(or)

This change is already covered by existing tests, such as (please describe tests).

(or)

This change added tests and can be verified as follows:

(example:)

  • Extended interop tests to verify consistent valid schema names between SDKs
  • Added test that validates that Java throws an AvroRuntimeException on invalid binary data
  • Manually verified the change by building the website and checking the new redirect

Documentation

  • Does this pull request introduce a new feature? (yes / no)
  • If yes, how is the feature documented? (not applicable / docs / JavaDocs / not documented)

@clesaec
Copy link
Contributor

clesaec commented Dec 5, 2023

May need a doc change on time description
Other than that, code proposal is nice.

@Fokko
Copy link
Contributor Author

Fokko commented Dec 5, 2023

Hey @clesaec thanks for the comment, the doc change can be found here: #2554

Copy link
Contributor

@clesaec clesaec left a comment

Choose a reason for hiding this comment

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

LGTM
(to keep link with doc change : #2554)

@martin-g martin-g changed the title AVRO-3914: Add nanos support AVRO-3914: Add nanos support for the Java SDK Dec 7, 2023
@Fokko Fokko merged commit ff32453 into apache:main Dec 12, 2023
16 checks passed
@Fokko Fokko deleted the fd-nanos branch December 12, 2023 08:31
@Fokko
Copy link
Contributor Author

Fokko commented Dec 12, 2023

Moving this forward. Thanks for the review @clesaec

Comment on lines +284 to +289
private static final LocalTimestampMicros LOCAL_TIMESTAMP_NANOS_TYPE = new LocalTimestampMicros();

public static LocalTimestampMicros localTimestampNanos() {
return LOCAL_TIMESTAMP_NANOS_TYPE;
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Shoudln't it be LocalTimestampNanos class here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, you're correct! Do you want to raise the PR?

Copy link
Contributor

Choose a reason for hiding this comment

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

Done at #2706

RanbirK pushed a commit to RanbirK/avro that referenced this pull request May 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Java Pull Requests for Java binding
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants