(I’m not sure if the lineage proposal document is still active for discussion, hence posting this comment here. If it is preferred, I can move this discussion there.)

The term row lineage has limited scope -- it refers to a "sequence number" which indicates when a row was created or modified. However, it does not reference the source of a modified row. I.e. it does not provide details about evolution of a row (the original row which was modified). Is this definition of "evolution" sufficient?

Also, the proposal document mentions that the impact on reads should be minimal. If there are any available metrics, specially increase in data file size, it might be beneficial to include them in this document so that users can understand the impact of enabling this feature.

Every "row_id" can be used to track the creation of a row by checking the "row_id" high water mark of each snapshot in the table history. This allows a user (with sufficent snapshot history) to determine when any particular row was initially added to the table. The second field last-updated-seq points to the update in which the row was last modified.

Together these allow you determine when a row was made and when it was last changed. The origin of a modified row is always the row with the exact same _row_id in the commit before last-updated-seq.

Impact on read should be 0 since these columns do not need to actually be materialized by scans. Impact on merge statements/copy statements should be slightly increased because more data has to go through the compute engine although this will differ in efficiency based on the engine.

On file size this should be relatively low impact but we can do some benchmarks once the reference implementation is done. For use cases without row-level-updates it would be very very cheap since any materialized row_id and last-updated-sequence values should be either very very similar (and compressible) or identical.

Do we allow to disable row lineage for a table? If it is allowed, should we rewrite the manifest files and data files when we disable it?

@rdblue also has some thoughts on this. I don't have a problem with enabling and disabling and having slightly odd behavior when that happens. I think it's a pretty unlikely situation to have it flip back and forth, for users who do that they can expect some odd events.

I don't think that we should allow disabling it. That would create strange situations and we would not be able to trust the metadata. There may be a way to handle this, but I don't want to block the initial feature taking the time to design it.

I would assume we would treat any commits in the gap the same as we would treat writes before tracking was on.

A related question: if we revert the table to a snapshot before enabling the row lineage, should we disable row lineage? If not, what about next_row_id?

Can we be more explicit in the spec about disabling row lineage is not allowed at this moment? I think the engine/catalog should guard it so that users won't accidentally disable it.

A table snapshot reversion is the same as resetting current-snapshot. In this case we don't change next_row_id and we would not disable row_lineage. Turning on row_lineage is not a snapshot operation. I'll write that explicitly.

The same clean-up question here. Do we rewrite data files in case of disabling row lineage or we disallow disabling?

I think disabling is allowed it just means we stop changing any of the metadata and lineage information may be possibly lost by clients which don't support row-lineage. I don't think we need to prevent this.

Not related to this PR, but a general v3 question: Should we make refs required in v3?

@aokolnychyi, @danielcweeks, @jackye1995, @RussellSpitzer, @flyrain, what do you think?

I don't think there are any negatives to that. What was the reason it wasn't required in V3?

What was the reason it wasn't required in V3?

@RussellSpitzer Do you mean V2? I think it was largely because branching/tagging came after the voting/adoption of V2 and so for compatibility purposes it needed to be optional for writers.

I'm +1 on making it required in V3 though. I think in general it's good to standardize on fields in newer format versions, when those fields are fairly adopted in the previous version and it's not that much of a metadata overhead to write it or maintain (for a user not using branches/tags the worst case is a just a mapping of main to the details of main). It makes it easier for developers to assume which metadata properties exist or not.

What was the reason it wasn't required in V3?

@RussellSpitzer Do you mean V2? I think it was largely because branching/tagging came after the voting/adoption of V2 and so for compatibility purposes it needed to be optional for writers.

I'm +1 on making it required in V3 though. I think in general it's good to standardize on fields in newer format versions, when those fields are fairly adopted in the previous version and it's not that much of a metadata overhead to write it or maintain (for a user not using branches/tags the worst case is a just a mapping of main to the details of main). It makes it easier for developers to assume which metadata properties exist or not.

I am wondering if we should assign sequence number regardless of if row-lineage is enabled or not.

The goal here is to not make it a requirement for engines which are not compatible with row-lineage.

Do we need to engine side changes for the last_updated_seq, or it can be done completely in the iceberg-core layer?

It's a change in the underlying parquet files so all engines will need to participate.

Any chance we can avoid unnecessary changes? I usually use a text editor. I believe there are ways to disable auto-formatting in IntelliJ as well.

written to a new file -> copied to a new file or something like below written to a different data file?

I think I disagree with the "copied" language. When "the row" is written (even if it is modified) the row ID should be preserved. To me, "copied" implies that the row is not modified, opening a question of how to handle row modification.

If we allow writers to skip these fields in new data files, does it mean we loose the ability to distinguish a data file added prior to enabling row lineage and a data file with all new rows? Do we even care about that?

Probably not, we still have first_row_id on the data file. Never mind.

such files must set both column -> shouldn't this be the reader's responsibility?

Yes, let me reword that

readers should treat both columns as if they exist and are set to null for all rows

Clarifying, if we are also saying here, that _last_updated_sequence_number and _row_id are reserved column names in a table created with v3 spec.

There are no reserved column names, only IDs. And this does update the table of reserved IDs.

When a user does INSERT OVERWRITE of an entire partition / table, some rows might be overwritten implicitly, as the operation is not copy-on-write, unlike MERGE INTO or UPDATE. For such cases, are we saying the rows are treated as new rows, and existing row _row_id or _last_updated_sequence_number will not be carried forward?

Yes. INSERT OVERWRITE is an INSERT and the rows should be treated as new rows.

I know there are cases here where users have historically built patterns around INSERT OVERWRITE when MERGE was not available. For example, the read-union-overwrite pattern was heavily used at Netflix to add new data to existing partitions. The problem is that engines can't detect the intent and carry row information through. These patterns also can't be optimized by engines, so I think the best choice is to use the INSERT semantics here.

In addition, the Iceberg community has discouraged using INSERT OVERWRITE for years because of the challenges with implicit data overwrites. Implicitly overwriting a directory of data means that the physical layout needs to implicitly align with writes. That's not a good pattern to use.

+1, examples made it easy to follow.

For completeness, should we add line on expected behavior if disabling row lineage after enabling it for some time.

It is not possible to disable row lineage after enabling it.

Should we call that out in the spec? I know it is merged already. May be a followup PR?