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

Proposed PR: configurable time encoding #1126

Open
kalafut opened this issue Jan 27, 2023 · 5 comments
Open

Proposed PR: configurable time encoding #1126

kalafut opened this issue Jan 27, 2023 · 5 comments

Comments

@kalafut
Copy link

kalafut commented Jan 27, 2023

Hi, and thank you for an excellent library that I've used for years!

This driver stores time.Time values as strings:

"2006-01-02 15:04:05.999999999-07:00",
but for compactness and consistency I always prefer to use unix timestamps. This has meant always modifying my Go code to save an int version of whatever time I'm using.

I'd like to propose a new driver configuration parameter, _time_format, that would let one opt into different storage formats. This lets you use time.Time directly but get the encoding you want. I've built a version that allows unix and unix_ms formats, and it is a small (~dozen LOC) change: kalafut/go-sqlite3.

Other formats could be added in the future too, which might be useful (e.g. I noticed #951 (comment)). Note that this is completely opt-in and only affects the way values are written. Reading/parsing is unchanged.

Is this of any interest? If so I can prepare a proper PR with better, consolidated tests, and some docs.

Thanks for taking a look.

@rittneje
Copy link
Collaborator

Reading/parsing is unchanged.

I would think that you would have to change reading/parsing to also consider the custom encoding, or else it will be unable to read what it just wrote, unless it happens to be the same as one of the entries in SQLiteTimestampFormats.

I also wonder if this sort of thing would be better in a driver.Connector implementation (which this library currently lacks) instead of continuing to cram things into the DSN.

@kalafut
Copy link
Author

kalafut commented Jan 28, 2023

I would think that you would have to change reading/parsing

For truly custom formats, that's right. I was envisioning that these would be more of a enum type that would relate to selecting one of the SQLiteTimestampFormats. But even that is a bit more than what I'm most interested in, which is integral timestamp support. These can already be parsed by the driver:

go-sqlite3/sqlite3.go

Lines 2126 to 2140 in 8543684

case columnTimestamp, columnDatetime, columnDate:
var t time.Time
// Assume a millisecond unix timestamp if it's 13 digits -- too
// large to be a reasonable timestamp in seconds.
if val > 1e12 || val < -1e12 {
val *= int64(time.Millisecond) // convert ms to nsec
t = time.Unix(0, val)
} else {
t = time.Unix(val, 0)
}
t = t.UTC()
if rc.s.c.loc != nil {
t = t.In(rc.s.c.loc)
}
dest[i] = t

Today if I have a unix timestamp stored in e.g. a TIMESTAMP column, it is converted just fine. There is just no way to write them without adding another int to the structure, and is what the proposal addresses.

As for the location of the configuration... I'm not partial to it and actually don't know that much about the options available. I only picked the DSN since I was familiar with it and saw other config there. I can take a look at a Connector implementation if you think that is now worth investing in.

@rittneje
Copy link
Collaborator

The other thing to keep in mind is that with what you are proposing, it would affect the entire connection. You might be better served making something that implements driver.Valuer and sql.Scanner so you can explicitly control the specific field rather than relying on the DSN being correct. To that end, maybe this library could expose some typedefs for that.

@kalafut
Copy link
Author

kalafut commented Jan 28, 2023

I started this with experiments using custom types. They work for the database's use, but those same time values are then used lots of other places and it gets tedious bumping into MyTime type not being time.Time, and the conversions then ensue. The friction also extends into external tooling (e.g. https://sqlc.dev).

I don't know if there is any other way to effect per-field configuration.

Well, I'll let you think it over and no biggee if you decide the design doesn't fit and you close this. The fork is pretty easy to keep up and serves my needs.

@markuswustenberg
Copy link

I’ve also run into this issue. I tend to write time in a string format in the highest resolution SQLite natively supports (milliseconds), without truncation of 0 suffixes, for sortability: strftime('%Y-%m-%dT%H:%M:%fZ')

I’ve resorted to a custom type with (de)serialization which embeds a time.Time, but it feels a bit awkward. So a driver solution would be preferable for me as well.

I’ve played with solutions myself. One I liked was having a driver initializer constructor function taking a struct, instead of relying on an init() function and DSN parsing. But I know that’s a breaking change…

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

No branches or pull requests

3 participants