Add milliseconds timestamps support #6
Conversation
kryvashek
changed the title
ole-baranov
left a comment
ole-baranov
left a comment
There was a problem hiding this comment.
Thanks for the request! A few nits from me.
crates/catalog/sql/src/catalog.rs
Outdated
| assert_eq!( | ||
| err, | ||
| "Unexpected => No such table: TableIdent { namespace: NamespaceIdent([\"a\"]), name: \"tbl1\" }" | ||
| "TableNotFound => No such table: TableIdent { namespace: NamespaceIdent([\"a\"]), name: \"tbl1\" }" |
There was a problem hiding this comment.
Is it possible to refer to some enum here? ErrorKind, maybe?
There was a problem hiding this comment.
It is possible to use ErrorKind here, but it will kinda break the idea of the test since this enum is used to produce an error text being checked. In other words it seems somewhat meaningless to check whether the text produced by some routine equals... to the text, produced by that same routine. (=
There was a problem hiding this comment.
Used direct kind asserting, as well as message.
| /// Time of day in microsecond precision, without date or timezone. | ||
| Time, | ||
| /// Timestamp in millisecond precision, without timezone | ||
| TimestampMs, |
There was a problem hiding this comment.
This timestamp is not a part of the spec. Do we really need it? Will other iceberg-compatible libraries be able to read our new data type?
There was a problem hiding this comment.
We agreed with @ole-baranov to try to force those changes into upstream (and spec therefore).
There was a problem hiding this comment.
It will be discussed in the related thread, I've mentioned both of you there.
What changes are included in this PR?
This PR adds support for millisecond-based timestamps into Iceberg and related code, included in the repo (catalogs).
Also some tests were fixed in accordance with previously made changes.
Are these changes tested?
Changes are tested with a couple of supplemented accordingly unit tests.