perf(deletes): Implement filtering position delete files by file path

[brgr-s]

Which issue does this PR close?

• Closes Scan planning matches every position delete to every data file in the partition, making planning memory O(data files × delete files) #2935 .

What changes are included in this PR?

The PR introduces (uncomments) pos_deletes_by_path in PopulatedDeleteFileIndex and populates the HashMap. To determine if a position delete file belongs in the map, it uses a helper position_delete_target that investigates

1. is referenced_data_file set? if yes, we know the delete file applies to that path
2. is lower == upper for the reserved field RESERVED_FIELD_ID_DELETE_FILE_PATH? If yes, we know that all entries point to the same path

As a small ergonomic imrpovement it changes eq_deletes_by_partition and pod_deletes_by_partition to take a (i32, Struct) as key, where the i32 is the partition_spec_id of the delete file.

get_deletes_for_data_file can then use pos_deletes_by_path to lookup delete files that match that specific data_file.

PR also adds a debug trace that could be usefull to determine if a read on a table is slow or OOMs (see linked issue) due to a large number of created delete file references.

Are these changes tested?

I have added unit tests.

The change was also tested locally in conjuncture with PR #2620. The task was compacting a merge-on-read table with a huge number of delete files. The test scenario can be reproduced without PR #2620, see Issue #2935.

AI Disclosure

I used AI to investigate how Spark handles delete files, to review my changes and add more tests. I also used AI to write parts of the Issue and double-check the math.

[c-thiel]

I left a few comments. I also see think we have at least two remaining efficiency gaps vs Java for which we should open Issues or directly follow-up PRs:

1. Equality deletes are not pruned by column statistics

Java's canContainEqDeletesForFile skips an equality delete file when the data file's and delete file's bounds for the equality fields don't overlap, or when null counts prove no rows can match. PopulatedDeleteFileIndex::get_deletes_for_data_file only applies the
sequence-number check, so every equality delete in the partition is returned, then loaded and applied.

2. Buckets are linearly scanned instead of binary-searched

Java keeps each bucket sorted by data sequence number and binary-searches the first applicable delete (findStartIndex), giving O(log n + matches). Rust filters linearly per data file.

[brgr-s]

@c-thiel thanks for the review, changes are incoming.

[xanderbailey]

Took a quick look! Thanks for the PR!

[xanderbailey]

Should we be failing hard here? Feels like this could result in a correctness issue of deletes being skipped which is pretty scary to me.

We should, I think, always be inheriting the sequence number but it's not actually demanded by the type system today which is an issue I'd be happy to track separately.

[brgr-s]

This line is basically the pre-existing behaviour, but with an added warning. I added that behaviour back in my last commit, the original proposal was different:

match (delete_seq_num, data_file_seq_num) {
    (Some(delete_seq_num), Some(data_file_seq_num)) => delete_seq_num >= data_file_seq_num,
    _ => true,
}

So no failure or warning, the pos delete file just gets an "applicable" state. In my opinion this is safe: the entries in a delete file key each line to a file and a row. If that row exists in that file, it probably should be deleted. If it doesn't exist, nothing happens.
However, a missing sequence number is a spec violation, I think, so failing hard is an option. It boils down to an implementation problem: new is called in a spawned thread with no error path, so we'd have to wire the failure state through. Considering this, a seperate issue is called for, but the question remains which behaviour is wanted for this PR:

• old, but risk ressurecting rows
• new, probably "more correct", but it is a behaviour change that might be uncalled for in a "performance PR"

[xanderbailey]

Yeah I would typically lean on the side of Postel’s Law: “Be conservative in what you send, be liberal in what you accept" BUT in this case I do find the spec violation a little scary and susceptible to correctness issues.

+1 that this is existing behaviour and can be made as a follow up. Would you be able to create an issue so we can track? I'm happy to take it if you're busy or review if you want to give it a shot.

[brgr-s]

@xanderbailey thanks for taking a look :)