Skip to content

SQL Server timestamp support (as datetime2)#6654

Merged
findepi merged 2 commits intotrinodb:masterfrom
nineinchnick:jwas/sqlserver-timestamp
Feb 3, 2021
Merged

SQL Server timestamp support (as datetime2)#6654
findepi merged 2 commits intotrinodb:masterfrom
nineinchnick:jwas/sqlserver-timestamp

Conversation

@nineinchnick
Copy link
Member

@nineinchnick nineinchnick commented Jan 20, 2021 •
edited
Loading

Tests were copied from PostgreSQL's connector tests and adjusted for max precision of 7. Write functions don't do any rounding. In case the local precision is higher than 7, rounding is done by SQL Server.

Ref:

@cla-bot cla-bot bot added the cla-signed label Jan 20, 2021
findepi
findepi reviewed Jan 20, 2021
View reviewed changes
@nineinchnick nineinchnick force-pushed the jwas/sqlserver-timestamp branch 2 times, most recently from caafcd5 to 89463d9 Compare January 21, 2021 10:18
@nineinchnick nineinchnick force-pushed the jwas/sqlserver-timestamp branch from 89463d9 to 5accc5b Compare January 21, 2021 12:13
@losipiuk
Copy link
Member

losipiuk commented Jan 21, 2021

maven-checks are red

@nineinchnick nineinchnick force-pushed the jwas/sqlserver-timestamp branch 3 times, most recently from 2655f76 to 39bb01f Compare January 21, 2021 15:26
findepi
findepi reviewed Jan 25, 2021
View reviewed changes
Copy link
Member

@findepi findepi left a comment

Choose a reason for hiding this comment

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

see also CI red

@nineinchnick nineinchnick force-pushed the jwas/sqlserver-timestamp branch from 39bb01f to 99b7bb0 Compare January 26, 2021 12:30
losipiuk
losipiuk approved these changes Jan 26, 2021
View reviewed changes
Copy link
Member

@losipiuk losipiuk left a comment

Choose a reason for hiding this comment

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

LGTM

@nineinchnick nineinchnick force-pushed the jwas/sqlserver-timestamp branch from 99b7bb0 to aa993e6 Compare January 26, 2021 13:09
@nineinchnick
Copy link
Member Author

nineinchnick commented Jan 26, 2021

I've noticed the pipeline failed but I'll wait with pushing code style fixes just in case I get more comments :-)

@nineinchnick nineinchnick force-pushed the jwas/sqlserver-timestamp branch 3 times, most recently from ca6c799 to 0cdbb14 Compare January 29, 2021 13:49
@nineinchnick nineinchnick requested a review from findepi January 29, 2021 13:52
findepi
findepi reviewed Jan 29, 2021
View reviewed changes
plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/StandardColumnMappings.java Outdated
Copy link
Member

@findepi findepi Jan 29, 2021

Choose a reason for hiding this comment

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

inline

Copy link
Member Author

@nineinchnick nineinchnick Feb 1, 2021

Choose a reason for hiding this comment

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

Done! But I also removed the tests, is that ok? I guess tests for the whole read and write functions would require more cases.

findepi
findepi reviewed Jan 29, 2021
View reviewed changes
@nineinchnick nineinchnick force-pushed the jwas/sqlserver-timestamp branch 3 times, most recently from bcb11fe to bb7464f Compare February 1, 2021 14:49
@nineinchnick nineinchnick requested a review from findepi February 1, 2021 15:09
findepi
findepi reviewed Feb 1, 2021
View reviewed changes
@nineinchnick nineinchnick force-pushed the jwas/sqlserver-timestamp branch 2 times, most recently from 3fcd500 to f318473 Compare February 1, 2021 15:49
findepi
findepi approved these changes Feb 1, 2021
View reviewed changes
Copy link
Member

@findepi findepi left a comment

Choose a reason for hiding this comment

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

Good job!

@nineinchnick nineinchnick force-pushed the jwas/sqlserver-timestamp branch from f318473 to 922b3af Compare February 1, 2021 15:54
@findepi
Copy link
Member

findepi commented Feb 2, 2021

@nineinchnick please see the build

@nineinchnick nineinchnick force-pushed the jwas/sqlserver-timestamp branch from 922b3af to f21a724 Compare February 2, 2021 09:45
@nineinchnick nineinchnick force-pushed the jwas/sqlserver-timestamp branch from f21a724 to bc81cc9 Compare February 2, 2021 13:37
@nineinchnick
Copy link
Member Author

nineinchnick commented Feb 2, 2021

@findepi all green!

@findepi findepi merged commit 150a17a into trinodb:master Feb 3, 2021
@findepi
Copy link
Member

findepi commented Feb 3, 2021

Merged, thanks!

@findepi findepi added this to the 352 milestone Feb 3, 2021
@findepi
Copy link
Member

findepi commented Feb 3, 2021

@nineinchnick can you please update type mapping table in https://trino.io/docs/current/connector/sqlserver.html ?

@findepi findepi added the needs-docs This pull request requires changes to the documentation label Feb 3, 2021
@findepi findepi mentioned this pull request Feb 3, 2021
Closed
10 tasks
@nineinchnick nineinchnick mentioned this pull request Feb 3, 2021
Merged
@nineinchnick nineinchnick deleted the jwas/sqlserver-timestamp branch February 3, 2021 11:29
@findepi findepi mentioned this pull request Feb 16, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@losipiuk losipiuk losipiuk approved these changes

@hashhar hashhar hashhar left review comments

@findepi findepi findepi approved these changes

Assignees

No one assigned

Labels

cla-signed needs-docs This pull request requires changes to the documentation

Milestone

352

Development

Successfully merging this pull request may close these issues.

4 participants

@nineinchnick @losipiuk @findepi @hashhar