Fokko commented on code in PR #941:
URL: https://github.com/apache/iceberg-python/pull/941#discussion_r2154822875
##########
pyiceberg/table/__init__.py:
##########
@@ -635,15 +651,18 @@ def delete(
if isinstance(delete_filter, str):
delete_filter = _parse_row_filter(delete_filter)
- with
self.update_snapshot(snapshot_properties=snapshot_properties).delete() as
delete_snapshot:
+ with self.update_snapshot(snapshot_properties=snapshot_properties,
branch=branch).delete() as delete_snapshot:
delete_snapshot.delete_by_predicate(delete_filter, case_sensitive)
# Check if there are any files that require an actual rewrite of a
data file
if delete_snapshot.rewrites_needed is True:
bound_delete_filter = bind(self.table_metadata.schema(),
delete_filter, case_sensitive)
preserve_row_filter =
_expression_to_complementary_pyarrow(bound_delete_filter)
- files = self._scan(row_filter=delete_filter,
case_sensitive=case_sensitive).plan_files()
+ if branch is None:
+ files = self._scan(row_filter=delete_filter,
case_sensitive=case_sensitive).plan_files()
+ else:
+ files = self._scan(row_filter=delete_filter,
case_sensitive=case_sensitive).use_ref(branch).plan_files()
Review Comment:
Maybe in a subsequent PR we can pass in the ref to the constructor 👍
##########
pyiceberg/table/__init__.py:
##########
@@ -774,12 +797,24 @@ def upsert(
matched_predicate = upsert_util.create_match_filter(df, join_cols)
# We must use Transaction.table_metadata for the scan. This includes
all uncommitted - but relevant - changes.
- matched_iceberg_table = DataScan(
- table_metadata=self.table_metadata,
- io=self._table.io,
- row_filter=matched_predicate,
- case_sensitive=case_sensitive,
- ).to_arrow()
+ if branch is None:
+ matched_iceberg_table = DataScan(
+ table_metadata=self.table_metadata,
+ io=self._table.io,
+ row_filter=matched_predicate,
+ case_sensitive=case_sensitive,
+ ).to_arrow()
+ else:
+ matched_iceberg_table = (
+ DataScan(
+ table_metadata=self.table_metadata,
+ io=self._table.io,
+ row_filter=matched_predicate,
+ case_sensitive=case_sensitive,
+ )
+ .use_ref(branch)
+ .to_arrow()
+ )
Review Comment:
I think this is not the correct usage of the builder pattern, instead you
want to do something like:
```python
if branch is not None:
scan.use_ref(branch)
```
However, I think it also makes sense to just pass this into the constructor
as mentioned in another comment 👍
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
