Skip to content

AVRO-4037: [C++] Add local-timestamp-millis, local-timestamp-micros logical types #3053

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
Merged
martin-g merged 4 commits into from
Aug 19, 2024
Merged

AVRO-4037: [C++] Add local-timestamp-millis, local-timestamp-micros logical types #3053

martin-g merged 4 commits into from
Aug 19, 2024

Conversation

@glywk
Copy link
Contributor

@glywk glywk commented Jul 28, 2024 •
edited by martin-g
Loading

AVRO-4037

What is the purpose of the change

This pull request adds support of the following logical types as defined since specification 1.10.0:

  • local-timestamp-millis
  • local-timestamp-micros

Verifying this change

This change added tests and can be verified as follows:

  • Extended interop tests to verify consistent valid schema names between SDKs

Documentation

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

@github-actions github-actions bot added the C++ Pull Requests for C++ binding label Jul 28, 2024
Gerrit0
Gerrit0 approved these changes Jul 30, 2024
View reviewed changes
Copy link
Contributor

@Gerrit0 Gerrit0 left a comment

Choose a reason for hiding this comment

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

LGTM!

KalleOlaviNiemitalo
KalleOlaviNiemitalo reviewed Jul 30, 2024
View reviewed changes
Copy link
Contributor

@KalleOlaviNiemitalo KalleOlaviNiemitalo left a comment

Choose a reason for hiding this comment

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

I worried a bit about potential bugs in conversions between Avro timestamps and std::chrono:: types; but AFAICT this library doesn't implement any such conversions, so it can't have any bugs in them.

@glywk
Copy link
Contributor Author

glywk commented Jul 30, 2024

I worried a bit about potential bugs in conversions between Avro timestamps and std::chrono:: types; but AFAICT this library doesn't implement any such conversions, so it can't have any bugs in them.

You are right std::chrono::milliseconds and std::chrono::microseconds can be less large chrono::duration than avro long (int64_t). But it is not the aim of this pull request.

@glywk
Copy link
Contributor Author

glywk commented Aug 5, 2024

@Fokko

Fokko
Fokko approved these changes Aug 5, 2024
View reviewed changes
Copy link
Contributor

@Fokko Fokko left a comment

Choose a reason for hiding this comment

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

This looks good, thanks @glywk for adding this. Thanks @Gerrit0 and @KalleOlaviNiemitalo for the review 🚀

Gerrit0
Gerrit0 suggested changes Aug 5, 2024
View reviewed changes
Copy link
Contributor

@Gerrit0 Gerrit0 left a comment

Choose a reason for hiding this comment

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

Looks like one of the errors still mixes - and _

@glywk
Copy link
Contributor Author

glywk commented Aug 5, 2024

Looks like one of the errors still mixes - and _

Sorry, you are right. It's fixed now.
Thanks for your check.

@glywk glywk force-pushed the cpp_local_timestamp branch from 332b76c to c3c747e Compare August 15, 2024 20:23
@glywk
Copy link
Contributor Author

glywk commented Aug 15, 2024

std::vector definition was not in the right file. It also ease the conan integration (conan-io/conan-center-index#24854)

@glywk glywk mentioned this pull request Aug 15, 2024
Closed
3 tasks
martin-g
martin-g reviewed Aug 16, 2024
View reviewed changes
Copy link
Member

@martin-g martin-g left a comment

Choose a reason for hiding this comment

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

Do we want to also add support for [local-]timestamp-nanos too with this PR ?
#2554

@martin-g
Copy link
Member

martin-g commented Aug 16, 2024

BTW why there is no JIRA for this PR ?
IMO it would be good to have one for the changelog!

@glywk
Copy link
Contributor Author

glywk commented Aug 16, 2024

BTW why there is no JIRA for this PR ? IMO it would be good to have one for the changelog!

I have no access to JIRA to create one. I have requested one but it was rejected. Can you create one for me? I will request a new one for a next time.

@martin-g
Copy link
Member

martin-g commented Aug 16, 2024

Can you create one for me?

Sure!

Do we want to also add support for [local-]timestamp-nanos too with this PR ?
#2554

What about this ?

@martin-g martin-g changed the title [NO JIRA] [C++] Add local-timestamp-millis, local-timestamp-micros logical types AVRO-4037: [C++] Add local-timestamp-millis, local-timestamp-micros logical types Aug 16, 2024
@glywk
Copy link
Contributor Author

glywk commented Aug 16, 2024 •
edited
Loading

Can you create one for me?

Sure!

Do we want to also add support for [local-]timestamp-nanos too with this PR ?
#2554

What about this ?

@martin-g martin-g merged commit 2205c20 into apache:main Aug 19, 2024
@martin-g
Copy link
Member

martin-g commented Aug 19, 2024

Thank you, @glywk !

@martin-g martin-g mentioned this pull request Aug 19, 2024
Merged
@glywk glywk deleted the cpp_local_timestamp branch September 3, 2024 06:57
opwvhk pushed a commit to opwvhk/avro that referenced this pull request Sep 5, 2025
@glywk @KalleOlaviNiemitalo @opwvhk
AVRO-4037: [C++] Add local-timestamp-millis, local-timestamp-micros l…
1eac15a 
…ogical types (apache#3053)

* [C++] Add local-timestamp-millis, local-timestamp-micros logical types as defined since specification 1.10.0

* Align exception comment on logical type names

Co-authored-by: Kalle Olavi Niemitalo <kon@iki.fi>

* [C++] Fix local-timestamp-micros exception comment

* [C++] Fix include vector dependency location

---------

Co-authored-by: Kalle Olavi Niemitalo <kon@iki.fi>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@martin-g martin-g martin-g left review comments

@Fokko Fokko Fokko approved these changes

+2 more reviewers

@KalleOlaviNiemitalo KalleOlaviNiemitalo KalleOlaviNiemitalo left review comments

@Gerrit0 Gerrit0 Gerrit0 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

C++ Pull Requests for C++ binding

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@glywk @martin-g @Fokko @Gerrit0 @KalleOlaviNiemitalo