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

Allow constructing a microsoft.sql.DateTimeOffset instance from a java.time.OffsetDateTime value #2340

Conversation

arashi01
Copy link
Contributor

Operates similarly to existing constructor however avoids additional expense of creating Timestamp instances and increased timezone related error potential, by directly converting java.time.OffsetDateTime instances.

Reverse of existing DateTimeOffset.getOffsetDateTime() method.

Rounds precision to closest 100ns matching existing semantics of Timestamp based constructor. Values where nanos are within 50ns of the next second are rounded upwards to the next second; as per current semantics.

Supercedes #2339

@tkyc tkyc force-pushed the DateTimeOffset_construct_from_OffsetDateTime branch from e675f92 to 1ebf504 Compare February 26, 2024 23:45
@tkyc
Copy link
Member

tkyc commented Feb 26, 2024

FYI, I did a force push to fix the pipeline issue.

@arashi01
Copy link
Contributor Author

Also just noticed the second unit test (valueOfOffsetDateTimeRounding()) can cause a false test failure if the test is run at any time with a 59 seconds value, since it uses OffsetDateTime.now() for it's seed data.

Will fix it now. Rather than give the test value a wholly static value I think it's probably better to just fix the "seconds" value, similar to the "nanos" value.

@arashi01 arashi01 force-pushed the DateTimeOffset_construct_from_OffsetDateTime branch from 1ebf504 to ae407c4 Compare February 27, 2024 00:30
@arashi01
Copy link
Contributor Author

Fixed the test. Fixed the test value to OffsetDateTime.now().withSecond(58).

@lilgreenbird lilgreenbird added this to In progress in MSSQL JDBC via automation Feb 27, 2024
@tkyc tkyc force-pushed the DateTimeOffset_construct_from_OffsetDateTime branch from ae407c4 to 859c32d Compare February 27, 2024 00:43
@arashi01
Copy link
Contributor Author

@tkyc
Copy link
Member

tkyc commented Feb 27, 2024

@arashi01

My mistake, could you push again? I had to do another force push which overwrote your last commit.

@tkyc tkyc force-pushed the DateTimeOffset_construct_from_OffsetDateTime branch from 859c32d to 2ec7f00 Compare February 27, 2024 01:16
@arashi01 arashi01 force-pushed the DateTimeOffset_construct_from_OffsetDateTime branch from 2ec7f00 to ae407c4 Compare February 27, 2024 01:26
@lilgreenbird
Copy link
Member

the reason the force-push was done by @tkyc was to get rid of the previous commits which was tied to the old versions of the pipelines for some reason and we wanted to remove CI-mssql-jdbc pipeline from the PR validation check. But now it seems with your latest force-push this came right back :(
Well I guess we can just ignore the failures on this particular pipeline.

@lilgreenbird lilgreenbird moved this from In progress to Under Peer Review in MSSQL JDBC Feb 27, 2024
Copy link
Member

@lilgreenbird lilgreenbird left a comment

Choose a reason for hiding this comment

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

pls use formatter to format all files

MSSQL JDBC automation moved this from Under Peer Review to In progress Feb 27, 2024
@arashi01 arashi01 force-pushed the DateTimeOffset_construct_from_OffsetDateTime branch from ae407c4 to 46bbb6d Compare February 27, 2024 13:55
@arashi01
Copy link
Contributor Author

pls use formatter to format all files

Done. Although used Intellij's Eclipse code style import support to use the mssql-jdbc_formatter.xml. If theres still an issue I'll install Eclipse and rerun.

Copy link

codecov bot commented Feb 27, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 50.20%. Comparing base (cfb018a) to head (9395bcc).

❗ Current head 9395bcc differs from pull request most recent head 46bbb6d. Consider uploading reports for the commit 46bbb6d to get more accurate results

Additional details and impacted files
@@              Coverage Diff              @@
##               main    #2340       +/-   ##
=============================================
- Coverage     73.60%   50.20%   -23.41%     
+ Complexity     6082     3793     -2289     
=============================================
  Files           142      143        +1     
  Lines         33123    33149       +26     
  Branches       5623     5630        +7     
=============================================
- Hits          24380    16641     -7739     
- Misses         6234    14122     +7888     
+ Partials       2509     2386      -123     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@lilgreenbird lilgreenbird moved this from In progress to Under Peer Review in MSSQL JDBC Feb 27, 2024
@lilgreenbird lilgreenbird added this to the 12.7.0 milestone Feb 28, 2024
@Jeffery-Wasty Jeffery-Wasty merged commit 26803d2 into microsoft:main Mar 15, 2024
17 checks passed
MSSQL JDBC automation moved this from Under Peer Review to Closed/Merged PRs Mar 15, 2024
Copy link
Contributor

@barryw-mssql barryw-mssql left a comment

Choose a reason for hiding this comment

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

Looks fine

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
MSSQL JDBC
  
Closed/Merged PRs
Development

Successfully merging this pull request may close these issues.

None yet

5 participants