Spec: Clarify time travel implementation in Iceberg #8982
Conversation
github-actions
bot
added
the
Specification
Co-authored-by: Eduard Tudenhoefner <[email protected]>
Co-authored-by: Eduard Tudenhoefner <[email protected]>
emkornfield
changed the title
|
|
||
| ### Point in Time Reads (Time Travel) | ||
|
|
||
| Iceberg supports two types of histories for tables. A history of previous "current snapshots" stored in ["snapshot-log" table metadata](#table-metadata-fields) and [parent-child lineage stored in "snapshots"](#table-metadata-fields). These two histories |
There was a problem hiding this comment.
What is exactly meant by two histories might not be consistent for determining a table state?
There was a problem hiding this comment.
tried to clarify. please let me know if this reads better.
There was a problem hiding this comment.
The discrepancies can be caused by a variety of table operations (e.g. updating the
current-snapshot-idof the table).
might be better to further clarify the discrepancy. this wasn't super clear to me.
There was a problem hiding this comment.
I added more lanugage here, please let me know if it addresses the concerns.
|
@aokolnychyi did my changes address your feedback properly? |
|
@aokolnychyi happy new year! Would you mind taking a second look? |
|
@Fokko @aokolnychyi would you mind taking a look? |
|
@Fokko @aokolnychyi just wanted to ping again to see if you have bandwidth to take another look? |
Fokko
left a comment
Fokko
left a comment
There was a problem hiding this comment.
I have one minor comment, but I think it would be good to get this in
Co-authored-by: Fokko Driesprong <[email protected]>
format/spec.md
Outdated
| Iceberg supports two types of histories for tables. A history of previous "current snapshots" stored in ["snapshot-log" table metadata](#table-metadata-fields) and [parent-child lineage stored in "snapshots"](#table-metadata-fields). These two histories | ||
| might indicate different snapshot IDs for a specific timestamp. The discrepancies can be caused by a variety of table operations (e.g. updating the `current-snapshot-id` of the table). | ||
|
|
||
| When processing point in time queries the Iceberg community has chosen to use "snapshot-log" metadata to lookup the table state |
There was a problem hiding this comment.
nit: maybe For point in time queries, "snapshot-log" metadata are used to lookup the table state at the given point of time?
There was a problem hiding this comment.
I'm not sure we would say "the iceberg community has chosen." If we write it into the spec that's redundant. Also this isn't an implementation note though if we are telling folks to do this.
There was a problem hiding this comment.
I agree with Russell, but I would not block this clarification from going in. @RussellSpitzer, is this a blocker for you? If so could you note it on the vote thread? I'm going to vote +1, but we should wait until you're satisfied.
There was a problem hiding this comment.
I'll rephrase when I move it under snapshots.
There was a problem hiding this comment.
+1 for removing "the Iceberg community has chosen to", the spec should not involve the community, we can just describe what should be done.
There was a problem hiding this comment.
@emkornfield : my comment above is not about the retrieval of the JSON metadata itself (which must be supported), but about the (optional) snapshot-log field in the JSON payload returned by a REST Catalog server.
There was a problem hiding this comment.
This spec is independent of the REST catalog protocol. The protocol covers how to exchange table information covered by this spec, but this spec covers how to track that information and, in this case, recommendations for how that information is used. In the context of your suggestion, this assumes that "when the catalog makes the snapshot history available in the metadata JSON" is all the time. It is always true because this spec defines the metadata JSON.
I think it is valuable to say that engines are encouraged to use the information from snapshot-log for time travel by timestamp so that the results match what a query would have seen at that time. We made that choice for Spark because we think that is what users expect.
There was a problem hiding this comment.
snapshot-log is optional in both v1 and v2 spec: https://iceberg.apache.org/spec/#table-metadata-fields
Given that this PR suggests how engines should process that information so that users can expect similar behaviour from different engines, it would be nice to provide a suggestion of how to deal with missing snapshot-log in a common way too. In other words, how to produce a user-friendly message as opposed to some internal and engine-specific error.
There was a problem hiding this comment.
Added sentence mentioning an informative message being raised to the user about missing metadata. I also tried to rephrased to remove the the iceberg community. If further word-smithing is desired, suggestions are welcome.
There was a problem hiding this comment.
Thanks for the update @emkornfield - current text LGTM 👍
|
|
||
| Note that these requirements apply when writing data to a v2 table. Tables that are upgraded from v1 may contain metadata that does not follow these requirements. Implementations should remain backward-compatible with v1 metadata requirements. | ||
|
|
||
| ## Appendix F: Implementation Notes |
There was a problem hiding this comment.
I am a bit hesitant on adding an "implementation notes" section to the spec.
I am wondering if time travel clarification should be added to the snapshots section of the spec?
There was a problem hiding this comment.
I can move it there.
There was a problem hiding this comment.
I like this approach. I don't think that the spec mandates this time travel behavior, but it is what implementations should do to be consistent with one another. Seems like "Implementation notes" is a good place for this kind of thing.
There was a problem hiding this comment.
@stevenzwu @rdblue @ajantha-bhat not sure if either of you have more strongly held preference on this, happy to do it either way.
There was a problem hiding this comment.
Maybe we should call it Implementation Suggestions, or Suggested Behaviors?
I'm fine with it being a separate section
There was a problem hiding this comment.
I can see Ryan's point. I can withdraw my argument.
There was a problem hiding this comment.
Just wondering why can't we mandate this in spec? because it is optional field? We can use "if exists" while mandating in the spec?
Also, we have implementation suggetions like "File System Tables" and "Metastore tables" in the spec. So, can we have a "Time Travel" section for this?
There was a problem hiding this comment.
@ajantha-bhat I think the two sections you described above are required for writers/readers for proper functionality. As per above this is not required for correctness but allows for consistency on implementation.
ajantha-bhat
left a comment
ajantha-bhat
left a comment
There was a problem hiding this comment.
I have doubts/concerns in using the optional field ("snapshot-log") as source of truth for timetravel instead of the available "snapshots" field.
|
Looks like we have reached consensus, so I'll go ahead and merge this. Thanks for the addition, @emkornfield! |
Co-authored-by: Eduard Tudenhoefner <[email protected]> Co-authored-by: Fokko Driesprong <[email protected]>
Co-authored-by: Eduard Tudenhoefner <[email protected]> Co-authored-by: Fokko Driesprong <[email protected]>
12 participants
Based on mailing list discussion: https://lists.apache.org/thread/o6gyky3t3b94grfr8t1nvkyxrgwz4737