You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "amitgilad3 (via GitHub)" <gi...@apache.org> on 2024/08/10 14:09:14 UTC
[PR] prevent adding duplicate files [iceberg-python]
amitgilad3 opened a new pull request, #1036:
URL: https://github.com/apache/iceberg-python/pull/1036
This resolves #998 , where duplicate files are added with add_files method, handles 2 cases:
1. Files list is not unique
2. One of the files added is already referenced by current snapshot
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] prevent adding duplicate files [iceberg-python]
Posted by "amitgilad3 (via GitHub)" <gi...@apache.org>.
amitgilad3 commented on code in PR #1036:
URL: https://github.com/apache/iceberg-python/pull/1036#discussion_r1714382905
##########
pyiceberg/table/__init__.py:
##########
@@ -621,6 +621,13 @@ def delete(self, delete_filter: Union[str, BooleanExpression], snapshot_properti
if not delete_snapshot.files_affected and not delete_snapshot.rewrites_needed:
warnings.warn("Delete operation did not match any records")
+ def find_referenced_files(self, file_paths: List[str]) -> list[str]:
Review Comment:
you are right, it makes more sense to move it into the logic into add_files since no one uses it and its on 3 lines if code
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] prevent adding duplicate files [iceberg-python]
Posted by "kevinjqliu (via GitHub)" <gi...@apache.org>.
kevinjqliu commented on code in PR #1036:
URL: https://github.com/apache/iceberg-python/pull/1036#discussion_r1714447553
##########
tests/integration/test_add_files.py:
##########
@@ -732,3 +732,76 @@ def test_add_files_subset_of_schema(spark: SparkSession, session_catalog: Catalo
for column in written_arrow_table.column_names:
for left, right in zip(lhs[column].to_list(), rhs[column].to_list()):
assert left == right
+
+
+@pytest.mark.integration
+def test_add_files_with_duplicate_files_in_file_paths(spark: SparkSession, session_catalog: Catalog, format_version: int) -> None:
+ identifier = f"default.test_table_duplicate_add_files_v{format_version}"
+ tbl = _create_table(session_catalog, identifier, format_version)
+
+ file_paths = [f"s3://warehouse/default/unpartitioned/v{format_version}/test-{i}.parquet" for i in range(5)]
+ file_paths.append(f"s3://warehouse/default/unpartitioned/v{format_version}/test-1.parquet")
+ # write parquet files
+ for file_path in file_paths:
+ fo = tbl.io.new_output(file_path)
+ with fo.create(overwrite=True) as fos:
+ with pq.ParquetWriter(fos, schema=ARROW_SCHEMA) as writer:
+ writer.write_table(ARROW_TABLE)
+
+ # add the parquet files as data files
+ with pytest.raises(ValueError) as exc_info:
+ tbl.add_files(file_paths=file_paths)
+ assert "File paths must be unique" in str(exc_info.value)
Review Comment:
nit: if this test is checking for uniqueness of `file_paths`, perhaps just test specifically for that
something like
```
file_path = ""
file_paths = [file_path, file_path]
# add the parquet files as data files
with pytest.raises(ValueError) as exc_info:
tbl.add_files(file_paths=file_paths)
assert "File paths must be unique" in str(exc_info.value)
```
##########
tests/integration/test_add_files.py:
##########
@@ -732,3 +732,76 @@ def test_add_files_subset_of_schema(spark: SparkSession, session_catalog: Catalo
for column in written_arrow_table.column_names:
for left, right in zip(lhs[column].to_list(), rhs[column].to_list()):
assert left == right
+
+
+@pytest.mark.integration
+def test_add_files_with_duplicate_files_in_file_paths(spark: SparkSession, session_catalog: Catalog, format_version: int) -> None:
+ identifier = f"default.test_table_duplicate_add_files_v{format_version}"
+ tbl = _create_table(session_catalog, identifier, format_version)
+
+ file_paths = [f"s3://warehouse/default/unpartitioned/v{format_version}/test-{i}.parquet" for i in range(5)]
+ file_paths.append(f"s3://warehouse/default/unpartitioned/v{format_version}/test-1.parquet")
+ # write parquet files
+ for file_path in file_paths:
+ fo = tbl.io.new_output(file_path)
+ with fo.create(overwrite=True) as fos:
+ with pq.ParquetWriter(fos, schema=ARROW_SCHEMA) as writer:
+ writer.write_table(ARROW_TABLE)
+
+ # add the parquet files as data files
+ with pytest.raises(ValueError) as exc_info:
+ tbl.add_files(file_paths=file_paths)
+ assert "File paths must be unique" in str(exc_info.value)
+
+
+@pytest.mark.integration
+def test_add_files_that_referenced_by_current_snapshot(
+ spark: SparkSession, session_catalog: Catalog, format_version: int
+) -> None:
+ identifier = f"default.test_table_add_referenced_file_v{format_version}"
+ tbl = _create_table(session_catalog, identifier, format_version)
+
+ file_paths = [f"s3://warehouse/default/unpartitioned/v{format_version}/test-{i}.parquet" for i in range(5)]
+ referenced_file = f"s3://warehouse/default/unpartitioned/v{format_version}/test-1.parquet"
+ # write parquet files
+ for file_path in file_paths:
+ fo = tbl.io.new_output(file_path)
+ with fo.create(overwrite=True) as fos:
+ with pq.ParquetWriter(fos, schema=ARROW_SCHEMA) as writer:
+ writer.write_table(ARROW_TABLE)
+
+ # add the parquet files as data files
+ tbl.add_files(file_paths=file_paths)
+
+ with pytest.raises(ValueError) as exc_info:
+ tbl.add_files(file_paths=[referenced_file])
+ assert f"Cannot add files that are already referenced by table, files: {referenced_file}" in str(exc_info.value)
Review Comment:
nit: its not clear to me how `referenced_file` is "referenced" already
##########
tests/integration/test_add_files.py:
##########
@@ -732,3 +732,76 @@ def test_add_files_subset_of_schema(spark: SparkSession, session_catalog: Catalo
for column in written_arrow_table.column_names:
for left, right in zip(lhs[column].to_list(), rhs[column].to_list()):
assert left == right
+
+
+@pytest.mark.integration
+def test_add_files_with_duplicate_files_in_file_paths(spark: SparkSession, session_catalog: Catalog, format_version: int) -> None:
+ identifier = f"default.test_table_duplicate_add_files_v{format_version}"
+ tbl = _create_table(session_catalog, identifier, format_version)
+
+ file_paths = [f"s3://warehouse/default/unpartitioned/v{format_version}/test-{i}.parquet" for i in range(5)]
+ file_paths.append(f"s3://warehouse/default/unpartitioned/v{format_version}/test-1.parquet")
+ # write parquet files
+ for file_path in file_paths:
+ fo = tbl.io.new_output(file_path)
+ with fo.create(overwrite=True) as fos:
+ with pq.ParquetWriter(fos, schema=ARROW_SCHEMA) as writer:
+ writer.write_table(ARROW_TABLE)
+
+ # add the parquet files as data files
+ with pytest.raises(ValueError) as exc_info:
+ tbl.add_files(file_paths=file_paths)
+ assert "File paths must be unique" in str(exc_info.value)
+
+
+@pytest.mark.integration
+def test_add_files_that_referenced_by_current_snapshot(
+ spark: SparkSession, session_catalog: Catalog, format_version: int
+) -> None:
+ identifier = f"default.test_table_add_referenced_file_v{format_version}"
+ tbl = _create_table(session_catalog, identifier, format_version)
+
+ file_paths = [f"s3://warehouse/default/unpartitioned/v{format_version}/test-{i}.parquet" for i in range(5)]
+ referenced_file = f"s3://warehouse/default/unpartitioned/v{format_version}/test-1.parquet"
+ # write parquet files
+ for file_path in file_paths:
+ fo = tbl.io.new_output(file_path)
+ with fo.create(overwrite=True) as fos:
+ with pq.ParquetWriter(fos, schema=ARROW_SCHEMA) as writer:
+ writer.write_table(ARROW_TABLE)
+
+ # add the parquet files as data files
+ tbl.add_files(file_paths=file_paths)
+
+ with pytest.raises(ValueError) as exc_info:
+ tbl.add_files(file_paths=[referenced_file])
+ assert f"Cannot add files that are already referenced by table, files: {referenced_file}" in str(exc_info.value)
+
+
+@pytest.mark.integration
+def test_add_files_that_referenced_by_current_snapshot_with_check_duplicate_files_false(
Review Comment:
nit: split this into 2 tests. one for the happy path, another for `check_duplicate_files=False`.
What happens when `check_duplicate_files=False` is set and we add files already referenced, does this leave the table in an inconsistent (bad) state?
##########
tests/integration/test_add_files.py:
##########
@@ -732,3 +732,47 @@ def test_add_files_subset_of_schema(spark: SparkSession, session_catalog: Catalo
for column in written_arrow_table.column_names:
for left, right in zip(lhs[column].to_list(), rhs[column].to_list()):
assert left == right
+
+
+@pytest.mark.integration
+def test_add_files_with_duplicate_files_in_file_paths(spark: SparkSession, session_catalog: Catalog, format_version: int) -> None:
Review Comment:
nit: I think the `format_version: int` parameterized is missing
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] prevent adding duplicate files [iceberg-python]
Posted by "sungwy (via GitHub)" <gi...@apache.org>.
sungwy commented on code in PR #1036:
URL: https://github.com/apache/iceberg-python/pull/1036#discussion_r1714372318
##########
pyiceberg/table/__init__.py:
##########
@@ -621,6 +621,13 @@ def delete(self, delete_filter: Union[str, BooleanExpression], snapshot_properti
if not delete_snapshot.files_affected and not delete_snapshot.rewrites_needed:
warnings.warn("Delete operation did not match any records")
+ def find_referenced_files(self, file_paths: List[str]) -> list[str]:
Review Comment:
We want to be intentional about introducing public methods. Since this is just used by `add_files`, could we make this method private? The type notation of `list` in the return type also needs to be `typing.List` until we deprecate python3.8 support:
```suggestion
def _find_referenced_files(self, file_paths: List[str]) -> List[str]:
```
An alternative is to just keep this logic within `add_files`, since this method isn't reused and is just 2 or 3 lines of code
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] prevent adding duplicate files [iceberg-python]
Posted by "amitgilad3 (via GitHub)" <gi...@apache.org>.
amitgilad3 commented on code in PR #1036:
URL: https://github.com/apache/iceberg-python/pull/1036#discussion_r1714418419
##########
pyiceberg/table/__init__.py:
##########
@@ -630,7 +630,20 @@ def add_files(self, file_paths: List[str], snapshot_properties: Dict[str, str] =
Raises:
FileNotFoundError: If the file does not exist.
+ ValueError: Raises a ValueError given file_paths contains duplicate files
+ ValueError: Raises a ValueError given file_paths already referenced by table
"""
+ if len(file_paths) != len(set(file_paths)):
+ raise ValueError("File paths must be unique")
+
+ import pyarrow.compute as pc
+
+ expr = pc.field("file_path").isin(file_paths)
+ referenced_files = [file["file_path"] for file in self._table.inspect.files().filter(expr).to_pylist()]
+
+ if referenced_files:
+ raise ValueError(f"Cannot add files that are already referenced by table, files: {', '.join(referenced_files)}")
Review Comment:
i also added the flag to the table api and added tests to make sure the flag works with False
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] prevent adding duplicate files [iceberg-python]
Posted by "kevinjqliu (via GitHub)" <gi...@apache.org>.
kevinjqliu commented on code in PR #1036:
URL: https://github.com/apache/iceberg-python/pull/1036#discussion_r1713025181
##########
pyiceberg/table/__init__.py:
##########
@@ -630,7 +648,15 @@ def add_files(self, file_paths: List[str], snapshot_properties: Dict[str, str] =
Raises:
FileNotFoundError: If the file does not exist.
+ ValueError: Raises a ValueError in case file_paths is not unique
Review Comment:
nit: given file_paths contains duplicate files
##########
pyiceberg/table/__init__.py:
##########
@@ -621,6 +621,24 @@ def delete(self, delete_filter: Union[str, BooleanExpression], snapshot_properti
if not delete_snapshot.files_affected and not delete_snapshot.rewrites_needed:
warnings.warn("Delete operation did not match any records")
+ def has_duplicates(self, file_paths: List[str]) -> bool:
+ unique_files = set(file_paths)
+ return len(unique_files) != len(file_paths)
+
+ def find_referenced_files(self, file_paths: List[str]) -> list[str]:
Review Comment:
I guess we only need to worry about the current snapshot.
If a data file existed in previous snapshots, but not in the current, we can still add the file.
##########
pyiceberg/table/__init__.py:
##########
@@ -621,6 +621,24 @@ def delete(self, delete_filter: Union[str, BooleanExpression], snapshot_properti
if not delete_snapshot.files_affected and not delete_snapshot.rewrites_needed:
warnings.warn("Delete operation did not match any records")
+ def has_duplicates(self, file_paths: List[str]) -> bool:
Review Comment:
nit: static method. may be better if inlined
##########
pyiceberg/table/__init__.py:
##########
@@ -621,6 +621,24 @@ def delete(self, delete_filter: Union[str, BooleanExpression], snapshot_properti
if not delete_snapshot.files_affected and not delete_snapshot.rewrites_needed:
warnings.warn("Delete operation did not match any records")
+ def has_duplicates(self, file_paths: List[str]) -> bool:
+ unique_files = set(file_paths)
+ return len(unique_files) != len(file_paths)
+
+ def find_referenced_files(self, file_paths: List[str]) -> list[str]:
Review Comment:
FYI there's also the `table.inspect.files()` API
https://py.iceberg.apache.org/api/#files
which returns all data files in the current snapshot
##########
pyiceberg/table/__init__.py:
##########
@@ -630,7 +648,15 @@ def add_files(self, file_paths: List[str], snapshot_properties: Dict[str, str] =
Raises:
FileNotFoundError: If the file does not exist.
+ ValueError: Raises a ValueError in case file_paths is not unique
+ ValueError: Raises a ValueError in case file already referenced in table
Review Comment:
nit: given file_paths already referenced by table
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] prevent adding duplicate files [iceberg-python]
Posted by "sungwy (via GitHub)" <gi...@apache.org>.
sungwy commented on code in PR #1036:
URL: https://github.com/apache/iceberg-python/pull/1036#discussion_r1714392026
##########
pyiceberg/table/__init__.py:
##########
@@ -630,7 +630,20 @@ def add_files(self, file_paths: List[str], snapshot_properties: Dict[str, str] =
Raises:
FileNotFoundError: If the file does not exist.
+ ValueError: Raises a ValueError given file_paths contains duplicate files
+ ValueError: Raises a ValueError given file_paths already referenced by table
"""
+ if len(file_paths) != len(set(file_paths)):
+ raise ValueError("File paths must be unique")
+
+ import pyarrow.compute as pc
+
+ expr = pc.field("file_path").isin(file_paths)
+ referenced_files = [file["file_path"] for file in self._table.inspect.files().filter(expr).to_pylist()]
+
+ if referenced_files:
+ raise ValueError(f"Cannot add files that are already referenced by table, files: {', '.join(referenced_files)}")
Review Comment:
so my suggestion is something like:
`def add_files(self, file_paths: List[str], snapshot_properties: Dict[str, str] = EMPTY_DICT, check_duplicate_files: bool = True) -> None:`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] prevent adding duplicate files [iceberg-python]
Posted by "sungwy (via GitHub)" <gi...@apache.org>.
sungwy commented on PR #1036:
URL: https://github.com/apache/iceberg-python/pull/1036#issuecomment-2292280939
@amitgilad3 - this looks almost ready to merge. @kevinjqliu 's made some great suggestions here, so I'm thinking that we can take another round of reviews once we take a pass through adopting those review comments
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] prevent adding duplicate files [iceberg-python]
Posted by "sungwy (via GitHub)" <gi...@apache.org>.
sungwy commented on PR #1036:
URL: https://github.com/apache/iceberg-python/pull/1036#issuecomment-2306881850
> Hey @sungwy , fixed all the comments and tests pass is there anything else or can we merge?
I will take another pass at it today. Thank you @amitgilad3 !
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] prevent adding duplicate files [iceberg-python]
Posted by "sungwy (via GitHub)" <gi...@apache.org>.
sungwy commented on code in PR #1036:
URL: https://github.com/apache/iceberg-python/pull/1036#discussion_r1714391548
##########
pyiceberg/table/__init__.py:
##########
@@ -630,7 +630,20 @@ def add_files(self, file_paths: List[str], snapshot_properties: Dict[str, str] =
Raises:
FileNotFoundError: If the file does not exist.
+ ValueError: Raises a ValueError given file_paths contains duplicate files
+ ValueError: Raises a ValueError given file_paths already referenced by table
"""
+ if len(file_paths) != len(set(file_paths)):
+ raise ValueError("File paths must be unique")
+
+ import pyarrow.compute as pc
+
+ expr = pc.field("file_path").isin(file_paths)
+ referenced_files = [file["file_path"] for file in self._table.inspect.files().filter(expr).to_pylist()]
+
+ if referenced_files:
+ raise ValueError(f"Cannot add files that are already referenced by table, files: {', '.join(referenced_files)}")
Review Comment:
Just one more suggestion here @amitgilad3 - should we make this behavior the default, but have a boolean flag available to disable it in the `add_files` API?
A similar [spark procedure](https://iceberg.apache.org/docs/1.5.1/spark-procedures/#add_files) has an equivalent flag `check_duplicate_files` and I feel that it could be useful because inspecting the files table requires us to read all of the active manifest files. If the user already knows that they won't be running into this issue, I think it would be useful for them to be able to disable this check.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] prevent adding duplicate files [iceberg-python]
Posted by "sungwy (via GitHub)" <gi...@apache.org>.
sungwy commented on code in PR #1036:
URL: https://github.com/apache/iceberg-python/pull/1036#discussion_r1715220578
##########
tests/integration/test_add_files.py:
##########
@@ -732,3 +732,76 @@ def test_add_files_subset_of_schema(spark: SparkSession, session_catalog: Catalo
for column in written_arrow_table.column_names:
for left, right in zip(lhs[column].to_list(), rhs[column].to_list()):
assert left == right
+
+
+@pytest.mark.integration
+def test_add_files_with_duplicate_files_in_file_paths(spark: SparkSession, session_catalog: Catalog, format_version: int) -> None:
Review Comment:
```suggestion
@pytest.mark.integration
@pytest.mark.parametrize("format_version", [1, 2])
def test_add_files_with_duplicate_files_in_file_paths(spark: SparkSession, session_catalog: Catalog, format_version: int) -> None:
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] prevent adding duplicate files [iceberg-python]
Posted by "amitgilad3 (via GitHub)" <gi...@apache.org>.
amitgilad3 commented on code in PR #1036:
URL: https://github.com/apache/iceberg-python/pull/1036#discussion_r1720268289
##########
tests/integration/test_add_files.py:
##########
@@ -732,3 +732,76 @@ def test_add_files_subset_of_schema(spark: SparkSession, session_catalog: Catalo
for column in written_arrow_table.column_names:
for left, right in zip(lhs[column].to_list(), rhs[column].to_list()):
assert left == right
+
+
+@pytest.mark.integration
+def test_add_files_with_duplicate_files_in_file_paths(spark: SparkSession, session_catalog: Catalog, format_version: int) -> None:
+ identifier = f"default.test_table_duplicate_add_files_v{format_version}"
+ tbl = _create_table(session_catalog, identifier, format_version)
+
+ file_paths = [f"s3://warehouse/default/unpartitioned/v{format_version}/test-{i}.parquet" for i in range(5)]
+ file_paths.append(f"s3://warehouse/default/unpartitioned/v{format_version}/test-1.parquet")
+ # write parquet files
+ for file_path in file_paths:
+ fo = tbl.io.new_output(file_path)
+ with fo.create(overwrite=True) as fos:
+ with pq.ParquetWriter(fos, schema=ARROW_SCHEMA) as writer:
+ writer.write_table(ARROW_TABLE)
+
+ # add the parquet files as data files
+ with pytest.raises(ValueError) as exc_info:
+ tbl.add_files(file_paths=file_paths)
+ assert "File paths must be unique" in str(exc_info.value)
+
+
+@pytest.mark.integration
+def test_add_files_that_referenced_by_current_snapshot(
+ spark: SparkSession, session_catalog: Catalog, format_version: int
+) -> None:
+ identifier = f"default.test_table_add_referenced_file_v{format_version}"
+ tbl = _create_table(session_catalog, identifier, format_version)
+
+ file_paths = [f"s3://warehouse/default/unpartitioned/v{format_version}/test-{i}.parquet" for i in range(5)]
+ referenced_file = f"s3://warehouse/default/unpartitioned/v{format_version}/test-1.parquet"
+ # write parquet files
+ for file_path in file_paths:
+ fo = tbl.io.new_output(file_path)
+ with fo.create(overwrite=True) as fos:
+ with pq.ParquetWriter(fos, schema=ARROW_SCHEMA) as writer:
+ writer.write_table(ARROW_TABLE)
+
+ # add the parquet files as data files
+ tbl.add_files(file_paths=file_paths)
+
+ with pytest.raises(ValueError) as exc_info:
+ tbl.add_files(file_paths=[referenced_file])
+ assert f"Cannot add files that are already referenced by table, files: {referenced_file}" in str(exc_info.value)
+
+
+@pytest.mark.integration
+def test_add_files_that_referenced_by_current_snapshot_with_check_duplicate_files_false(
Review Comment:
So when you set check_duplicate_files to False you are essentially taking responsibility for scenarios where duplicate files can be added, but the default is to validate
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] prevent adding duplicate files [iceberg-python]
Posted by "sungwy (via GitHub)" <gi...@apache.org>.
sungwy merged PR #1036:
URL: https://github.com/apache/iceberg-python/pull/1036
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] prevent adding duplicate files [iceberg-python]
Posted by "amitgilad3 (via GitHub)" <gi...@apache.org>.
amitgilad3 commented on code in PR #1036:
URL: https://github.com/apache/iceberg-python/pull/1036#discussion_r1714382905
##########
pyiceberg/table/__init__.py:
##########
@@ -621,6 +621,13 @@ def delete(self, delete_filter: Union[str, BooleanExpression], snapshot_properti
if not delete_snapshot.files_affected and not delete_snapshot.rewrites_needed:
warnings.warn("Delete operation did not match any records")
+ def find_referenced_files(self, file_paths: List[str]) -> list[str]:
Review Comment:
you are right, it makes more sense to move the logic into add_files since no one uses it and its on 3 lines if code
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] prevent adding duplicate files [iceberg-python]
Posted by "amitgilad3 (via GitHub)" <gi...@apache.org>.
amitgilad3 commented on PR #1036:
URL: https://github.com/apache/iceberg-python/pull/1036#issuecomment-2294096857
Hey @sungwy + @kevinjqliu , again thanks for all the help and guidance , i went over all the comments and fixed them
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] prevent adding duplicate files [iceberg-python]
Posted by "amitgilad3 (via GitHub)" <gi...@apache.org>.
amitgilad3 commented on PR #1036:
URL: https://github.com/apache/iceberg-python/pull/1036#issuecomment-2306566373
Hey @sungwy , fixed all the comments and tests pass is there anything else or can we merge?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org