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

feat: implement serde for DATE #7718

Merged
merged 3 commits into from
Jun 25, 2021
Merged

Conversation

jzaralim
Copy link
Contributor

Description

Implement serde for DATE. DATE gets serialized to an integer number of days since 1970-01-01, and deserialized to java.sql.Date

Testing done

unit and QTT tests

Reviewer checklist

  • Ensure docs are updated if necessary. (eg. if a user visible feature is being added or changed).
  • Ensure relevant issues are linked (description should include text like "Fixes #")

@@ -148,4 +149,18 @@ public static Time returnTimeOrThrow(final long time) {
+ TimeUnit.DAYS.toMillis(1) + ".");
}
}

public static Date getDateFromEpochDays(final long days) {
return new Date(TimeUnit.DAYS.toMillis(days));
Copy link
Member

Choose a reason for hiding this comment

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

Are there issues if days is negative?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope, added some negative dates to QTT tests

Copy link
Member

@spena spena left a comment

Choose a reason for hiding this comment

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

The code looks good. I just left a comment about negative numbers. Check that and fix it if necessary.

@spena
Copy link
Member

spena commented Jun 25, 2021

Looks good then. Thanks for the test

@jzaralim jzaralim merged commit a94f1f2 into confluentinc:master Jun 25, 2021
@jzaralim jzaralim deleted the date-serde branch June 25, 2021 22:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants