Conversation
nineinchnick
left a comment
nineinchnick
left a comment
There was a problem hiding this comment.
Thanks a lot for working on this. I did the first pass and expect at least one more round of reviews, but probably it'll take a while.
| columns, err := rows.Columns() | ||
| require.NoError(t, err, "Failed reading result columns") | ||
|
|
||
| assert.Equal(t, 4, len(columns), "Expected 4 result column") |
There was a problem hiding this comment.
Is this necessary? Wouldn't assert.Equal called on a slice verify this?
There was a problem hiding this comment.
I just copied the test code from another test :-)
There was a problem hiding this comment.
We build another slice later on using that length, so I guess it's safer to verify it...
|
Looks like this would resolve #13 |
JPMoresmau
force-pushed
the
jp/support_times_better
branch
from
fa65ac0 to
bbda22a
Compare
nineinchnick
left a comment
nineinchnick
left a comment
There was a problem hiding this comment.
Just a few nitpicks, almost there!
|
@nineinchnick I think I've merged your suggestions and addressed your comments, let me know. I suppose i can squash the commits once you're fine with my latest changes... |
with specific functions to create Trino data types explicitly.
JPMoresmau
force-pushed
the
jp/support_times_better
branch
from
95331a8 to
d8b108f
Compare
| When reading response rows, the driver supports most Trino data types, except: | ||
| * time and timestamps with precision - all time types are returned as `time.Time` | ||
| * time and timestamps with precision - all time types are returned as `time.Time`. | ||
| All precisions up to nanoseconds (`TIMESTAMP(9)` or `TIME(9)`) are supported (since |
There was a problem hiding this comment.
why exactly are we constraining ourselve to nanosecond precision? Would it be possible to support up to TIME*(12)?
There was a problem hiding this comment.
Because Go's time.Time only supports nanoseconds. To support picoseconds, we'd have to define a custom data type. I'm not sure how much work it would be to make it compatible with time.Time.
There was a problem hiding this comment.
Recent work in the Python client in trinodb/trino-python-client#300 is also limited but to microseconds.
| // Timestamp indicates we want a TimeStamp type WITHOUT a time zone in Trino from a Golang time. | ||
| type trinoTimestamp time.Time | ||
|
|
||
| // Timestamp creates a representation of a Trino Timestamp(9) type. |
There was a problem hiding this comment.
those comments are not really useful? @nineinchnick do we have check which enforces those?
There was a problem hiding this comment.
We don't currently have a linter in the CI that would check for this. The built-in go vet doesn't, we'd have to use staticcheck (recommended after go lint was deprecated) or golangci-lint (which is huge but popular).
Having comments for all exported functions is idiomatic in Go and it would show up in online docs like here: https://pkg.go.dev/github.com/trinodb/trino-go-client@v0.308.0/trino
What would it take to make them more useful? Should it explain why time.Time is not used instead?
There was a problem hiding this comment.
What would it take to make them more useful
Not sure. Presonally I would just drop it. But if we want to follow Go guildelines let's go with it :)
|
Sorry, is anything more needed from me to get that PR approved? |
4 participants
Add support for full precision of subseconds, so
PARAMETRIC_DATETIMEworks properly.Add type wrappers in the manner of
Numericfor the various date/time data types, so we can avoid casts and not leak the accepted formats to the client.