fix(iceberg): add_files correctly check duplicates #1395
Conversation
Erigara
force-pushed
the
fix/append_files_check_duplicates
branch
2 times, most recently
from
9813024 to
4dd9e78
Compare
|
I changed my initial implementation so that DELETED manifest entries are skipped.
|
| @@ -379,6 +364,7 @@ mod tests { | |||
|
|
|||
| // Attempt to add the existing Parquet files with fast append. | |||
| let new_tx = fast_append_action | |||
| .with_check_duplicate(false) | |||
There was a problem hiding this comment.
I'm not sure what was initial purpose of the test.
But it looks like to check ability of adding existing parquet files to the table (physically existing not in table).
In which case adding with_check_duplicate(false) here is fine.
There was a problem hiding this comment.
I think this is a wrong test, we should remove it.
Erigara
force-pushed
the
fix/append_files_check_duplicates
branch
3 times, most recently
from
08f5af5 to
a90174a
Compare
| let mut append_action = tx | ||
| .fast_append(None, vec![]) | ||
| .unwrap() | ||
| .with_check_duplicate(false); |
There was a problem hiding this comment.
Why we need this? I think if there exists duplicated files in this test, we should remove it.
There was a problem hiding this comment.
By just looking at the test idk why we are trying to commit the same files twice.
So i agree let's remove it.
| @@ -379,6 +364,7 @@ mod tests { | |||
|
|
|||
| // Attempt to add the existing Parquet files with fast append. | |||
| let new_tx = fast_append_action | |||
| .with_check_duplicate(false) | |||
There was a problem hiding this comment.
I think this is a wrong test, we should remove it.
Erigara
force-pushed
the
fix/append_files_check_duplicates
branch
from
a90174a to
00b7b87
Compare
Which issue does this PR close?
add_filesdoesn't actually check duplicated files #1394.What changes are included in this PR?
file_pathfrom itscanAre these changes tested?