Do we want to allow V2 writers and readers to populate the referenced data file? I generally don't mind but we will have to continue to persist the bounds anyway to not silently break existing integrations that rely on reconstructing the referenced path from the bounds. Writing the field in addition to the bounds will make things only worse and require additional work to implement.

I think we should allow it. We know that it is better to use referenced_data_file than to use the bounds, so it makes sense to allow it. People may not choose to use it if it is duplication.

I agree. The problem, however, is that we won't switch to referenced_data_file anywhere in the library. Persisting both values won't make sense as it will increase the disk footprint and memory pressure. We can't stop writing the bounds too, as it may break external connectors like Trino.

Not sure why we need this requirement, how would it be possible for the first 2 conditions be true but not this one?

Yeah, I debated whether to keep this or not and ended up deciding that it is helpful. The second requirement should also be impossible if the first requirement is true.

I kept both of these because I want people reading the spec to understand that these can be relied on in the scan planning algorithm. If this only said that the file_path must match, then implementers may think that they need to consider all deletion vectors without first filtering by partition and data sequence number.

Using the correct spec and partition for deletion vectors is important so that we can skip them using partition predicates, just like we skip position delete files today.

Correct, this is how the design doc proposes it.

does this condition apply to eq delete application as well?

@szehon-ho, this does not apply to equality deletes.

This is a fantastic question to consider. It would be great if we could add a rule like this one for equality deletes because writers that produce positional deletes will almost always apply equality deletes and encode them in the new DVs.

However, there's at least one big issue: concurrent commits would require re-scanning data. Imagine a writer is nearly done with a MERGE that uses DVs and has created a DV with all of the positions previously deleted by both positional and equality delete files. While the metadata update and commit is happening, a new equality delete comes in and commits first. If we were to require that the DV of the MERGE commit replaces the new equality deletes, the MERGE job would potentially need to go re-scan data files in order to find deleted row positions. That could be a very expensive operation because the equality delete could apply to an entire partition of data.

There is also a similar cost for maintaining position deletes, but in that case we have the metadata to load and union DVs quickly, as opposed to scanning potentially hundreds of files for newly deleted records at commit time. Because position deletes are much more targeted, there are fewer false positives (one goal of this update!) and the work to merge them is lower.

This is indeed an interesting option to consider. If we add a way to annotate a DV with the data sequence of the scan, we can ignore all equality deletes prior to that without requiring re-scanning data during retries.

nit: I think this paragraph and the next one are somewhat redundant with the puffin spec (or if they do provide additional information, I think consolidating in puffin makes sense, since the operations would like change for a v2 bitmap).

Isn't the context important here? I think it makes sense to have it in both places because they describe the same thing.

I'd say it is helpful to have it here.

I don't feel strong, but I think all of the details on optimization and lookup probably belong as an implementation detail in puffin. As stated there isn't enough information to actually implement reading the file, and if the underlying structure changes these sections would become irrelevant. I think the high level logic, is that deletion vectors represent row IDs that should be deleted the presence of a row number in the vector indicates the row should not be read.

Is there a technical reason to force deprecation here?

Yes! Deletion vectors are much better for creating a stream of changes from tables. They are also just faster in general so we want to switch over to using them.

There are many optimizations enabled by knowing there is a DV for a data file, this was one of the points in the design doc.

On the flip side a counter argument, is that requriing DVs means more work for writers that want to support some portions of V3 specification but not upgrade to DVs (despite them being better). It seems other places iceberg has taken the stand that optimizations are not required for writers (e.g. sequence number inheritance).

optimizations are not required for writers (e.g. sequence number inheritance).

Could you elaborate? Not sure I got.

My understanding is that Snapshot/Sequence number inheritance is only turned on via a configuration flag for Iceberg tables for writers. It is not required that writers "must" make snapshot id null for new files added. Similarly, I'm asking that if it really needs to be made a requirement that DVs must be written, even though they are better.

Actually, Iceberg is fairly strict about writing correct and well-maintained metadata, but we are careful to be backward-compatible. Sequence number and snapshot ID inheritance are required in v2. I think what you're referring to here is that we made snapshot ID inheritance available in v1 with a flag to enable it (and break compatibility with some readers).

I guess what I'm referring to in this case might have been considered a bug that was addressed. But the spec has never mandated inheritance "must" be used.

I think the question still stands on whether from a spec perspective this should be mandated ("must") or ("should"). I guess I'm OK with "must" given clarification below on existing delete files are present but it still feels like we might be overly strict.

The snapshot and sequence number inheritance was always required in V2 tables from the spec perspective, it just took us a bit of time to get rid of the legacy V1 logic everywhere. The flag was there only to enable that behavior in V1 tables, which could break older readers. That's why it was safe to merge the referenced PR as it simply aligned the implementation with the spec.

We know the new representation is better. I think it is OK to be strict in this case. We were very flexible with the design of equality deletes. For instance, we allowed adding any number of global equality delete files. We thought implementations would be smart and would not misuse this concept. Yet, we have seen so many cases where tables were unreadable after engines produces a ton of equality deletes without ever compacting them. I think the new layout forces implementations to provide the best possible behavior for position deletes as well as enables various optimizations like CDC generation mentioned by Ryan.