Spec: add nanosecond timestamp types #8683

jacobmarble · 2023-09-29T18:37:56Z

This change embodies this design doc:
https://docs.google.com/document/d/1bE1DcEGNzZAMiVJSZ0X1wElKLNkT9kRkk0hDlfkXzvU/edit

My IDE reformatted the Markdown tables edited. I'm happy to revert those if needed.

I also resorted the tables of types, so that date/time types land after other primitive types, and before nested types.

jacobmarble · 2023-10-05T05:13:59Z

@Fokko - friendly reminder to review

rdblue · 2023-10-05T23:33:05Z

@jacobmarble, I was talking with @danielcweeks a couple days ago about v3 additions and we came up with a good reason to add timestamp_ms and timestamptz_ms. Currently, type promotion is limited to very specific changes, but we think that we will need to open that up in v3. As long as we are open to promoting types, it makes sense to let people promote long to timestamp of some kind.

Now that we are adding timestamps with different units, this gives us a way to fix a long-standing problem: older data that used millisecond timestamps as longs couldn't be read as timestamps because we had no way to know the unit of the long (millis or micros). Now that we have separate types, we can use the type to carry that information, so that you can promote a long to timestamp_ms or timestamp and we know how to interpret the value.

What do you think? If you agree, then we should probably add ms types at the same time.

format/spec.md

jacobmarble · 2023-10-05T23:42:43Z

What do you think? If you agree, then we should probably add ms types at the same time.

Sounds good to me. I'll add milliseconds to this PR.

jacobmarble · 2023-10-11T17:17:33Z

In the Iceberg community sync today, it was decided that we will not be adding millisecond timestamps after all.

Current status of this PR:

millisecond timestamps removed
note added to v3 Appendix
rebased and squashed
type promotion not included
- someone with better context will follow up in a separate issue

@Fokko do you think we're ready to merge?

Fokko · 2023-10-11T17:33:55Z

@jacobmarble Yes, this looks good to me. Thanks for joining the community sync. Could you avoid reformatting the table? History is especially important in the spec.

Helps apache#8657 This change embodies this design doc: https://docs.google.com/document/d/1bE1DcEGNzZAMiVJSZ0X1wElKLNkT9kRkk0hDlfkXzvU/edit

jacobmarble · 2023-10-11T19:52:05Z

@jacobmarble Yes, this looks good to me. Thanks for joining the community sync. Could you avoid reformatting the table? History is especially important in the spec.

Done.

format/spec.md

rdblue · 2023-10-16T15:31:43Z

format/spec.md

+Notes:
+
+1. Avro type annotation `adjust-to-utc` is an Iceberg convention; default value is `false` if not present.
+2. Avro logical type `timestamp-nanos` is an Iceberg convention; the Avro specification does not define this type.


Is this necessary? Shouldn't we just go add this type to Avro, or use an Avro version?

In the Google doc I already suggested adding this to the Avro spec: https://docs.google.com/document/d/1bE1DcEGNzZAMiVJSZ0X1wElKLNkT9kRkk0hDlfkXzvU/edit?disco=AAAA5YbF34Y

Let me sent out an email on the Avro devlist

@Fokko please drop a link to Avro devlist post when you have a moment. The mailing list link on the Avro community page is just an email address, can't find the web UI.

Ah, we should update that page, that's not helping anyone. You can easily see the devlist on ponymail: https://lists.apache.org/[email protected]

On the bottom-left you can subscribe to the devlist. You can find my message here: https://lists.apache.org/thread/s985tc2m6bj5lvck017q06zkt3dg1f31 or the PR apache/avro#2554. Feel free to chime in anywhere.

@rdblue @Fokko I agree that we should ask this type be added to Avro.

It will be months before this change is added to Avro, then supported by dependent projects. Do you propose that:

We wait for the Avro spec to change before modifying the Iceberg spec?

Adjust the Iceberg spec language to reflect the intention to switch to the Avro type when it becomes available?

Since most Iceberg implementations have their own Avro readers, I think we're fine with running this in parallel. I don't see any huge issues on the Avro side (maybe some discussion around the physical type), but I would not consider that blocking.

@Fokko I understand; do I need to change this PR, then?

It depends on what comes out of the Avro PR, but I think we're good for now.

rdblue · 2023-10-16T15:34:53Z

format/spec.md

@@ -948,6 +961,7 @@ Lists must use the [3-level representation](https://github.com/apache/parquet-fo
 Notes:

 1. ORC's [TimestampColumnVector](https://orc.apache.org/api/hive-storage-api/org/apache/hadoop/hive/ql/exec/vector/TimestampColumnVector.html) consists of a time field (milliseconds since epoch) and a nanos field (nanoseconds within the second). Hence the milliseconds within the second are reported twice; once in the time field and again in the nanos field. The read adapter should only use milliseconds within the second from one of these fields. The write adapter should also report milliseconds within the second twice; once in the time field and again in the nanos field. ORC writer is expected to correctly consider millis information from one of the fields. More details at https://issues.apache.org/jira/browse/ORC-546
+2. ORC `timestamp` and `timestamp_instant` values store nanosecond precision. Iceberg ORC writers for Iceberg types `timestamp` and `timestamptz` truncate nanoseconds to microseconds.


I think that this should state that Iceberg ORC writers MUST truncate nanoseconds to microseconds. Otherwise someone may store nanosecond values, which would break equality and have implications when casting from timestamp to timestamp_ns.

Done. PTAL 150b6a9

rdblue · 2023-10-16T15:35:37Z

format/spec.md

+| **`timestamp`**    | `hashLong(microsecsFromUnixEpoch(v))`     | `2017-11-16T22:31:08` ￫ `-2047944441`<br />`2017-11-16T22:31:08.000001` ￫ `-1207196810` |
+| **`timestamptz`**  | `hashLong(microsecsFromUnixEpoch(v))`     | `2017-11-16T14:31:08-08:00` ￫ `-2047944441`<br />`2017-11-16T14:31:08.000001-08:00` ￫ `-1207196810` |
+| **`timestamp_ns`** | `hashLong(nanosecsFromUnixEpoch(v))`      | `2017-11-16T22:31:08` ￫ `-737750069`<br />`2017-11-16T22:31:08.000001` ￫ `-976603392`<br />`2017-11-16T22:31:08.000000001` ￫ `-160215926` |
+| **`timestamptz_ns`** | `hashLong(nanosecsFromUnixEpoch(v))`    | `2017-11-16T14:31:08-08:00` ￫ `-737750069`<br />`2017-11-16T14:31:08.000001-08:00` ￫ `-976603392`<br />`2017-11-16T14:31:08.000000001-08:00` ￫ `-160215926` |


Can you please add tests for these cases? We need to make sure that the actual code produces the given values.

Yes. I did generate these values with code tests, which I did not add to this PR. This change is just a spec change, following this comment.

Would you like some code to change in this PR? If so, I could remove the milliseconds details in #8658 then add those commits to this PR.

I've added the unit tests to #8658 : bc50195

rdblue · 2023-10-16T15:37:37Z

format/spec.md

+| **`timestamp`**    | Timestamp, microsecond precision, without timezone                       | [2]                                              |
+| **`timestamptz`**  | Timestamp, microsecond precision, with timezone                          | [2]                                              |
+| **`timestamp_ns`** | Timestamp, nanosecond precision, without timezone                        | [2]                                              |
+| **`timestamptz_ns`** | Timestamp, nanosecond precision, with timezone                         | [2]                                              |


I think that we need to update this table to show that timestamp_ns and timestamptz_ns are only allowed in v3 tables.

Done. PTAL 8f75adb

jacobmarble · 2023-10-24T18:55:35Z

@rdblue @Fokko what concerns remain regarding this change?

rdblue · 2023-10-26T23:08:54Z

format/spec.md

 3. Character strings must be stored as UTF-8 encoded byte arrays.
+4. `timestamp_ns` and `timstamptz_ns` are only supported in v3 tables.


I think that this needs to be a column in the table rather than a footnote. It just isn't very obvious that you can't use the ns types in v2 otherwise. That's my only concern with this PR.

I like Ryan's suggestion of having a full new column. We can expect more types to be added so that a column would make sense to me, instead of adding it to the requirements column.

Ah, I missed that detail. How's this? e81df55

findepi · 2023-10-27T20:20:04Z

@jacobmarble indeed, thanks!
do we expect any type promotions being allowed around the new types being added here?

jacobmarble · 2023-10-28T04:50:22Z

@jacobmarble indeed, thanks! do we expect any type promotions being allowed around the new types being added here?

@findepi when we discussed in the last community meeting, that was discussed. We agreed to scope this PR, and #8657 to just adding the type, and let type promotion be a separate issue.

rdblue · 2023-10-28T19:36:40Z

@findepi, for type promotion we decided to always interpret a promoted long as a millisecond value and the convert to either micros or nanos depending on the Iceberg type. Since we don't have a way to know what a long represents, there are problems no matter what we do and that's the best way to get predictable behavior. Otherwise, type promotion (i.e. timestamp_ms to a higher precision value) could corrupt data values that were previously working.

rdblue · 2023-10-28T19:48:16Z

format/spec.md

+| **`timestamp`**    | Timestamp, microsecond precision, without timezone                       | [2]                                              |
+| **`timestamptz`**  | Timestamp, microsecond precision, with timezone                          | [2]                                              |
+| **`timestamp_ns`** | Timestamp, nanosecond precision, without timezone                        | [2]                                              | [v3](#version-3) |
+| **`timestamptz_ns`** | Timestamp, nanosecond precision, with timezone                         | [2]                                              | [v3](#version-3) |


I don't think this is clear enough. I think the version column should come first and be "Valid from" or similar. "Iceberg version" could be confused with the library version and we need to make it clear what this version means: that you are not allowed to use it in tables until that version.

PTAL. I could also fill in the other rows with v1.

rdblue · 2023-10-31T16:15:44Z

@jacobmarble, I updated the table to be more clear about when types can be used and merge this. Thanks!

Helps apache#8657 This change adds field `ChronoUnit unit` to `TimestampType`, such that `TimestampType` now represents four specified types: - `timestamp` (existing) - `timestamptz` (existing) - `timestamp_ns` (new apache#8683) - `timestamptz_ns` (new apache#8683)

Helps apache#8657 This change adds field `ChronoUnit unit` to `TimestampType`, such that `TimestampType` now represents four specified types: - `timestamp` (existing) - `timestamptz` (existing) - `timestamp_ns` (new apache#8683) - `timestamptz_ns` (new apache#8683) Note that TimestampType.with[out]Zone() are marked as deprecated in this change. In future PRs, I'll remove usage of these static methods.

Helps apache#8657 This change adds field `TimestampType.Unit unit` to `TimestampType`, such that `TimestampType` now represents four specified types: - `timestamp` (existing) - `timestamptz` (existing) - `timestamp_ns` (new apache#8683) - `timestamptz_ns` (new apache#8683) Note that TimestampType.with[out]Zone() are marked as deprecated in this change. In future PRs, I'll remove usage of these static methods.

jacobmarble mentioned this pull request Sep 29, 2023

API: add nanosecond variant of Timestamp type #8658

Closed

Fokko added the Specification Issues that may introduce spec changes. label Oct 2, 2023

rdblue reviewed Oct 5, 2023

View reviewed changes

format/spec.md Outdated Show resolved Hide resolved

jacobmarble requested a review from rdblue October 9, 2023 16:08

jacobmarble changed the title ~~Spec: add types timstamp_ns and timestamptz_ns~~ Spec: add nanosecond and millisecond timestamp types Oct 9, 2023

jacobmarble force-pushed the jgm-timestamp-nanos-spec branch 2 times, most recently from 449c5ff to ed36cfb Compare October 11, 2023 17:13

jacobmarble changed the title ~~Spec: add nanosecond and millisecond timestamp types~~ Spec: add nanosecond timestamp types Oct 11, 2023

Spec: add types timstamp_ns and timestamptz_ns

f6fbf04

Helps apache#8657 This change embodies this design doc: https://docs.google.com/document/d/1bE1DcEGNzZAMiVJSZ0X1wElKLNkT9kRkk0hDlfkXzvU/edit

jacobmarble force-pushed the jgm-timestamp-nanos-spec branch from ed36cfb to f6fbf04 Compare October 11, 2023 19:51

Fokko reviewed Oct 11, 2023

View reviewed changes

format/spec.md Outdated Show resolved Hide resolved

Fokko reviewed Oct 11, 2023

View reviewed changes

format/spec.md Show resolved Hide resolved

chore: integrate review feedback

6e515ec

rdblue reviewed Oct 16, 2023

View reviewed changes

jacobmarble added 2 commits October 16, 2023 10:09

spec: ORC must truncate microsecond timestamps

150b6a9

spec: more clarity that ns timestamps are v3 only

8f75adb

jacobmarble requested review from Fokko and rdblue October 16, 2023 17:24

Fokko approved these changes Oct 20, 2023

View reviewed changes

rdblue reviewed Oct 26, 2023

View reviewed changes

jacobmarble requested a review from rdblue October 27, 2023 16:22

chore: add version column to markdown table

e81df55

rdblue reviewed Oct 28, 2023

View reviewed changes

jacobmarble and others added 2 commits October 28, 2023 16:49

chore: clarify minimum, not only, iceberg version

34e06fd

Clarify version where types were added.

0caa0e8

rdblue approved these changes Oct 31, 2023

View reviewed changes

rdblue merged commit da55503 into apache:main Oct 31, 2023

jacobmarble deleted the jgm-timestamp-nanos-spec branch October 31, 2023 16:23

jacobmarble mentioned this pull request Oct 31, 2023

API: implement types timestamp_ns and timestamptz_ns #8960

Closed

jacobmarble mentioned this pull request Nov 1, 2023

API, Core: implement types timestamp_ns and timestamptz_ns #8971

Closed

jacobmarble mentioned this pull request Nov 8, 2023

API: implement types timestamp_ns and timestamptz_ns #9008

Merged

Fokko mentioned this pull request Nov 13, 2023

Support for TIMESTAMP WITH TIME ZONE with nanosecond precision in Parquet trinodb/trino#13599

Open

findepi mentioned this pull request Nov 15, 2023

Support Iceberg timestamp types with nanosecond precision: timestamp_ns, timestamptz_ns trinodb/trino#19753

Open

alippai mentioned this pull request May 7, 2024

Timestamp precision in the schema of the DeltaTable of the transaction log. delta-io/delta#643

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Spec: add nanosecond timestamp types #8683

Spec: add nanosecond timestamp types #8683

jacobmarble commented Sep 29, 2023

jacobmarble commented Oct 5, 2023

rdblue commented Oct 5, 2023

jacobmarble commented Oct 5, 2023

jacobmarble commented Oct 11, 2023

Fokko commented Oct 11, 2023

jacobmarble commented Oct 11, 2023

rdblue Oct 16, 2023

Fokko Oct 16, 2023

jacobmarble Oct 16, 2023

Fokko Oct 16, 2023

jacobmarble Oct 16, 2023

Fokko Oct 16, 2023

jacobmarble Oct 16, 2023

Fokko Oct 17, 2023

rdblue Oct 16, 2023

jacobmarble Oct 16, 2023

rdblue Oct 16, 2023

jacobmarble Oct 16, 2023

jacobmarble Oct 16, 2023

rdblue Oct 16, 2023

jacobmarble Oct 16, 2023 •

edited

Loading

jacobmarble commented Oct 24, 2023

rdblue Oct 26, 2023

jacobmarble Oct 27, 2023

Fokko Oct 27, 2023

jacobmarble Oct 27, 2023 •

edited

Loading

findepi commented Oct 27, 2023

jacobmarble commented Oct 28, 2023

rdblue commented Oct 28, 2023

rdblue Oct 28, 2023

jacobmarble Oct 28, 2023

rdblue commented Oct 31, 2023

		3. Character strings must be stored as UTF-8 encoded byte arrays.
		4. `timestamp_ns` and `timstamptz_ns` are only supported in v3 tables.

Spec: add nanosecond timestamp types #8683

Spec: add nanosecond timestamp types #8683

Conversation

jacobmarble commented Sep 29, 2023

jacobmarble commented Oct 5, 2023

rdblue commented Oct 5, 2023

jacobmarble commented Oct 5, 2023

jacobmarble commented Oct 11, 2023

Fokko commented Oct 11, 2023

jacobmarble commented Oct 11, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jacobmarble Oct 16, 2023 • edited Loading

Choose a reason for hiding this comment

jacobmarble commented Oct 24, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jacobmarble Oct 27, 2023 • edited Loading

Choose a reason for hiding this comment

findepi commented Oct 27, 2023

jacobmarble commented Oct 28, 2023

rdblue commented Oct 28, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rdblue commented Oct 31, 2023

jacobmarble Oct 16, 2023 •

edited

Loading

jacobmarble Oct 27, 2023 •

edited

Loading