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

fix: proleptic java.time support #1539

Merged
merged 1 commit into from Aug 27, 2019

Conversation

@marschall
Copy link
Contributor

commented Aug 3, 2019

Make the java.time support proleptic by not going through
TimestampUtils.toJavaSecs when in binary mode.

Fixes #1534

All Submissions:

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same update/change?

Changes to Existing Features:

  • Does this break existing behaviour? For timestamps in the Julian calendar different java.time objects are returned. Code relying on the old (buggy) behavior will break.
  • Have you added an explanation of what your changes do and why you'd like us to include them? Old behaviour is wrong.
  • Have you written new tests for your core changes, as applicable?
  • Have you successfully run tests with your changes locally?
@AppVeyorBot

This comment has been minimized.

Copy link

commented Aug 3, 2019

@marschall

This comment has been minimized.

Copy link
Contributor Author

commented Aug 3, 2019

Some comments about the change:

  • I wanted the keep the existing #toJavaSecs and #toPgSecs logic for the old data types. That's why I factored out the binary parsing.
  • Binding does not go through TimestampUtils#toPgSecs because binary binding for java.time types is currently not implemented. I would like to address this in a different change.
  • SetObject310Test had a bug where it would ignore the second column
  • I skip the tests on anything before 8.4. Writing test that also work on older versions would be harder because
    • timestamp and timestamptz support in generate_series was added in 8.4
    • CTE support in added in 8.4

@marschall marschall force-pushed the marschall:proleptic-java.time branch from 45e214a to a6ad4d4 Aug 3, 2019

@AppVeyorBot

This comment has been minimized.

Copy link

commented Aug 3, 2019

@codecov-io

This comment has been minimized.

Copy link

commented Aug 3, 2019

Codecov Report

Merging #1539 into master will increase coverage by 0.03%.
The diff coverage is 70.83%.

@@             Coverage Diff              @@
##             master    #1539      +/-   ##
============================================
+ Coverage     68.94%   68.97%   +0.03%     
- Complexity     3956     3972      +16     
============================================
  Files           179      179              
  Lines         16491    16546      +55     
  Branches       2678     2688      +10     
============================================
+ Hits          11369    11412      +43     
- Misses         3877     3887      +10     
- Partials       1245     1247       +2

@marschall marschall force-pushed the marschall:proleptic-java.time branch from a6ad4d4 to fe7c12d Aug 3, 2019

@AppVeyorBot

This comment has been minimized.

Copy link

commented Aug 3, 2019

@marschall marschall force-pushed the marschall:proleptic-java.time branch from fe7c12d to a6736c8 Aug 3, 2019

@AppVeyorBot

This comment has been minimized.

Copy link

commented Aug 3, 2019

fix: proleptic java.time support
Make the java.time support proleptic by not going through
TimestampUtils.toJavaSecs when in binary mode.

Fixes #1534

@marschall marschall force-pushed the marschall:proleptic-java.time branch from a6736c8 to 04ad8c0 Aug 10, 2019

@AppVeyorBot

This comment has been minimized.

Copy link

commented Aug 10, 2019

@davecramer davecramer merged commit 60fa6d3 into pgjdbc:master Aug 27, 2019

3 checks passed

codecov/project 68.97% (+0.03%) compared to 0600990
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.