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

Support parsing pgwire timestamps as they are sent by lib/pq #5877

Merged
merged 1 commit into from Apr 5, 2016

Conversation

danhhz
Copy link
Contributor

@danhhz danhhz commented Apr 5, 2016

Closes #5612.

This change is Reviewable

@danhhz
Copy link
Contributor Author

danhhz commented Apr 5, 2016

After this lands, I'm going to revert cockroachdb/pq@a7769c7 as it was the wrong fix and is unnecessary (sorry, I should have gotten the entire context before committing anything).

@dt
Copy link
Member

dt commented Apr 5, 2016

Review status: 0 of 2 files reviewed at latest revision, 1 unresolved discussion.


sql/pgwire/types.go, line 255 [r1] (raw file):
Is this exercised by any tests currently?


Comments from Reviewable

@danhhz
Copy link
Contributor Author

danhhz commented Apr 5, 2016

Review status: 0 of 2 files reviewed at latest revision, 1 unresolved discussion.


sql/pgwire/types.go, line 255 [r1] (raw file):
Nope, could be dead code for all I know. I was just keeping it because it allows roundtripping the timestamp bytes that CockroachDB sends back. I could add a test for the round trip though.


Comments from Reviewable

@petermattis
Copy link
Collaborator

Review status: 0 of 2 files reviewed at latest revision, 2 unresolved discussions.


sql/pgwire/types_test.go, line 26 [r1] (raw file):
You can define this variable inside of TestParseTs.


Comments from Reviewable

@petermattis
Copy link
Collaborator

Review status: 0 of 2 files reviewed at latest revision, 3 unresolved discussions.


sql/pgwire/types_test.go, line 31 [r1] (raw file):
Add a test for the format accepted by pq.ParseTimestamp?


Comments from Reviewable

@petermattis
Copy link
Collaborator

:lgtm:


Review status: 0 of 2 files reviewed at latest revision, 3 unresolved discussions.


Comments from Reviewable

@danhhz
Copy link
Contributor Author

danhhz commented Apr 5, 2016

Review status: 0 of 2 files reviewed at latest revision, 3 unresolved discussions, all commit checks successful.


sql/pgwire/types_test.go, line 26 [r1] (raw file):
Done.


sql/pgwire/types_test.go, line 31 [r1] (raw file):
Done.


Comments from Reviewable

@danhhz danhhz merged commit dd41a66 into cockroachdb:master Apr 5, 2016
@danhhz danhhz deleted the pgwire_ts branch April 5, 2016 22:29
@danhhz danhhz restored the pgwire_ts branch April 5, 2016 22:33
@danhhz danhhz deleted the pgwire_ts branch April 6, 2016 18: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.

None yet

3 participants