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

Convert timestamp columns into Date objects and vice versa #41

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

simong
Copy link
Contributor

@simong simong commented Sep 15, 2015

After the previous PR, I got a taste for it and tried my hand at adding proper timestamp support. Currently it's a bit awkward to deal with timestamp columns. Timestamp values get serialized as numbers, but because timestamps are internally stored as 64 bits the value that's handed to you in JS land is garbage.
You have to resort to passing the timestamp value as a buffer through the timestampAsBlob function and then running that through something like long to get the actual value back out.

This PR allows you to:

  • set columns of type timestamp by passing in a Date object
  • retrieving timestamp columns as Date objects.

This is a breaking API change as timestamp values are no longer coming back as Numbers but proper Date objects. The PR isn't ready to be merged, but I liked to get your thoughts on whether this is a good idea and worth further development. If you're on board I'll refactor some of those timestamp tests and get this ready to be merged into master

@simong
Copy link
Contributor Author

simong commented Oct 5, 2015

@demmer Any feedback/suggestions on this?

@demmer
Copy link
Owner

demmer commented Oct 5, 2015

Hi @simong I'm thinking that we should extend the type hints with optional encodings for things like this where there are multiple ways in which the client may want the data to be encoded in the translation. I started working on this in the context of your other PR #42 and should hopefully have something ready in a couple days.

@demmer
Copy link
Owner

demmer commented Oct 14, 2015

@simong see #44 -- I reworked things a little bit so the encoding of numbers as either a vanilla JS number or a {low: <lowBits>, high: <highBits>} object would be specified by the caller along with the type codes.

This seems like a case in which you would want to define a TIMESTAMP_AS_DATE encoding to override the default behavior of encoding them as raw numbers.

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