You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by GitBox <gi...@apache.org> on 2022/10/27 18:51:07 UTC
[GitHub] [iceberg] dhruv-pratap opened a new pull request, #6069: Python: TableScan Plan files API implementation without row-level filters evaluation.
dhruv-pratap opened a new pull request, #6069:
URL: https://github.com/apache/iceberg/pull/6069
Taking a first dig at TableScan Plan Files API. The implementation at present evaluates partition filters but does not evaluate row-level filters at the moment as it requires Projections.
The API is exposed by the CLI tool and lists out all the data files that would need to be scanned in order to perform the scan plan.
<img width="1912" alt="image" src="https://user-images.githubusercontent.com/11399509/198372507-7f4a9b8a-bd3d-44d8-a534-234bacb0bba0.png">
High-level changes done as part of this PR:
1. Add `new_scan()` API to the `Table` interface.
2. Add `TableScan` interface with just `plan_files` abstract method.
3. Add `FileScanTask` model that represents a scan task over the existing `DataFile` model.
4. Add `DataTableScan` class that extends `TableScan` and implements `plan_files` API to return a collection of `FileScanTasks` using the existing `_ManifestEvalVisitor` to perform partition pruning.
--
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
[GitHub] [iceberg] rdblue commented on a diff in pull request #6069: Python: TableScan Plan files API implementation without residual evaluation
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #6069:
URL: https://github.com/apache/iceberg/pull/6069#discussion_r1014903928
##########
python/pyiceberg/table/scan.py:
##########
@@ -0,0 +1,103 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+import itertools
+from abc import ABC, abstractmethod
+from typing import Iterable, List, Optional
+
+from pydantic import Field
+
+from pyiceberg.expressions import AlwaysTrue, BooleanExpression
+from pyiceberg.expressions.visitors import manifest_evaluator
+from pyiceberg.io import FileIO
+from pyiceberg.manifest import DataFile, ManifestFile
+from pyiceberg.table import PartitionSpec, Snapshot, Table
+from pyiceberg.utils.iceberg_base_model import IcebergBaseModel
+
+ALWAYS_TRUE = AlwaysTrue()
Review Comment:
I'd prefer not adding these constants everywhere. Just use `AlwaysTrue()` in place of this. It's a singleton.
--
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
[GitHub] [iceberg] dhruv-pratap commented on pull request #6069: Python: TableScan Plan files API implementation without residual evaluation
Posted by GitBox <gi...@apache.org>.
dhruv-pratap commented on PR #6069:
URL: https://github.com/apache/iceberg/pull/6069#issuecomment-1322841113
> @dhruv-pratap, we've been working on the list lately and I think the remaining items are: (4) add `MetricsEvalVisitor` to prune by column stats, (5) add `ResidualEvalVisitor` to produce residuals (LOW priority), and (8) expose `plan_files` via the CLI.
>
> For the last one, we may just want to add filters to the existing `files` command?
@rdblue We discussed this more in [this](https://github.com/apache/iceberg/pull/6069#discussion_r1012044761) thread. We can extend `files` with three options:
* `files --snapshot=all` - The current files behavior
* `files --snapshot=current` - Files under the current snapshot
* `files --snapshot=<snapshot_id>` - Files under the snapshot_id specified
Adding `expression` option would be a bigger undertaking though, as it would require an expression parser. Not sure if we want to prioritize that at this point for the CLI.
--
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
[GitHub] [iceberg] rdblue commented on pull request #6069: Python: TableScan Plan files API implementation without residual evaluation
Posted by GitBox <gi...@apache.org>.
rdblue commented on PR #6069:
URL: https://github.com/apache/iceberg/pull/6069#issuecomment-1304927324
@Fokko, @dhruv-pratap, I posted an alternative scan API in a draft as #6131. Please take a look. That behaves like this one and allows you to specify optional arguments when creating a scan, but also allows the same syntax that Java uses.
--
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
[GitHub] [iceberg] dhruv-pratap commented on a diff in pull request #6069: Python: TableScan Plan files API implementation without row-level filters evaluation.
Posted by GitBox <gi...@apache.org>.
dhruv-pratap commented on code in PR #6069:
URL: https://github.com/apache/iceberg/pull/6069#discussion_r1009562735
##########
python/pyiceberg/cli/output.py:
##########
@@ -14,9 +14,11 @@
# KIND, either express or implied. See the License for the
# specific language governing permissions and limitations
# under the License.
+from __future__ import annotations
Review Comment:
I like Optional[str] too, but I think for some reason when I run `make lint` locally it turns it into the later format i.e. `str | None ` Do we need to tweak the linter config to enforce one format over the other?
--
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
[GitHub] [iceberg] Fokko commented on a diff in pull request #6069: Python: TableScan Plan files API implementation without row-level filters evaluation.
Posted by GitBox <gi...@apache.org>.
Fokko commented on code in PR #6069:
URL: https://github.com/apache/iceberg/pull/6069#discussion_r1009574021
##########
python/pyiceberg/cli/output.py:
##########
@@ -14,9 +14,11 @@
# KIND, either express or implied. See the License for the
# specific language governing permissions and limitations
# under the License.
+from __future__ import annotations
Review Comment:
This is because the import statement. PyUpgrade will automatically rewrite the imports: https://github.com/asottile/pyupgrade#pep-604-typing-rewrites We can change this in another PR.
--
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
[GitHub] [iceberg] rdblue commented on pull request #6069: Python: TableScan Plan files API implementation without residual evaluation
Posted by GitBox <gi...@apache.org>.
rdblue commented on PR #6069:
URL: https://github.com/apache/iceberg/pull/6069#issuecomment-1374903916
> Adding expression option would be a bigger undertaking though, as it would require an expression parser. Not sure if we want to prioritize that at this point for the CLI.
We added an expression parser a few weeks ago, so this would be unblocked. I'm all for updating the CLI with your suggestions.
--
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
[GitHub] [iceberg] Fokko commented on pull request #6069: Python: TableScan Plan files API implementation without residual evaluation
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on PR #6069:
URL: https://github.com/apache/iceberg/pull/6069#issuecomment-1454114897
Hey @dhruv-pratap. I'm closing this draft as this has already been implemented. Feel free to open a new PR if you feel something 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
[GitHub] [iceberg] Fokko commented on a diff in pull request #6069: Python: TableScan Plan files API implementation without row-level filters evaluation.
Posted by GitBox <gi...@apache.org>.
Fokko commented on code in PR #6069:
URL: https://github.com/apache/iceberg/pull/6069#discussion_r1009344258
##########
python/pyiceberg/cli/output.py:
##########
@@ -14,9 +14,11 @@
# KIND, either express or implied. See the License for the
# specific language governing permissions and limitations
# under the License.
+from __future__ import annotations
Review Comment:
As a community, we probably should make a decision on this one. Sometimes we use it, but not all the time. Personally I find `Optional[str]` easier to read than `str | None`
##########
python/pyiceberg/table/__init__.py:
##########
@@ -60,33 +60,40 @@ def sort_order(self) -> SortOrder:
sort_order for sort_order in self.metadata.sort_orders if sort_order.order_id == self.metadata.default_sort_order_id
)
- def sort_orders(self) -> Dict[int, SortOrder]:
+ def sort_orders(self) -> dict[int, SortOrder]:
"""Return a dict of the sort orders of this table"""
return {sort_order.order_id: sort_order for sort_order in self.metadata.sort_orders}
def location(self) -> str:
"""Return the table's base location."""
return self.metadata.location
- def current_snapshot(self) -> Optional[Snapshot]:
+ def current_snapshot(self) -> Snapshot | None:
"""Get the current snapshot for this table, or None if there is no current snapshot."""
if snapshot_id := self.metadata.current_snapshot_id:
return self.snapshot_by_id(snapshot_id)
return None
- def snapshot_by_id(self, snapshot_id: int) -> Optional[Snapshot]:
+ def snapshot_by_id(self, snapshot_id: int) -> Snapshot | None:
"""Get the snapshot of this table with the given id, or None if there is no matching snapshot."""
try:
return next(snapshot for snapshot in self.metadata.snapshots if snapshot.snapshot_id == snapshot_id)
except StopIteration:
return None
- def snapshot_by_name(self, name: str) -> Optional[Snapshot]:
+ def snapshot_by_name(self, name: str) -> Snapshot | None:
"""Returns the snapshot referenced by the given name or null if no such reference exists."""
if ref := self.metadata.refs.get(name):
return self.snapshot_by_id(ref.snapshot_id)
return None
- def history(self) -> List[SnapshotLogEntry]:
+ def history(self) -> list[SnapshotLogEntry]:
"""Get the snapshot history of this table."""
return self.metadata.snapshot_log
+
+ def new_scan(self, io: FileIO, snapshot_id: int | None = None, expression: BooleanExpression | None = None):
+ """Create a new scan for this table."""
+ from pyiceberg.table.scan import DataTableScan
+
+ snapshot = self.snapshot_by_id(snapshot_id) if snapshot_id is not None else None
Review Comment:
What do you think of setting the current `snapshot_id` if it hasn't been passed in explicitly?
```python
snapshot_id = snapshot_id or self.metadata.current_snapshot_id
snapshot = self.snapshot_by_id(snapshot_id)
```
##########
python/pyiceberg/table/scan.py:
##########
@@ -0,0 +1,115 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+from __future__ import annotations
+
+import itertools
+from abc import ABC, abstractmethod
+from typing import Iterable
+
+from pyiceberg.expressions import AlwaysTrue, BooleanExpression
+from pyiceberg.expressions.visitors import manifest_evaluator
+from pyiceberg.io import FileIO
+from pyiceberg.manifest import DataFile, ManifestFile
+from pyiceberg.table import PartitionSpec, Snapshot, Table
+
+
+class FileScanTask(ABC):
+ """A scan task over a range of bytes in a single data file."""
+
+ manifest: ManifestFile
+ data_file: DataFile
+ residual: BooleanExpression | None
+ spec: PartitionSpec | None
+
+ def __init__(
+ self,
+ manifest: ManifestFile,
+ data_file: DataFile,
+ spec: PartitionSpec | None = None,
+ residual: BooleanExpression | None = None,
+ ):
+ self.manifest = manifest
+ self.data_file = data_file
+ self.spec = spec
+ self.residual = residual
+
+ @property
+ def start(self) -> int:
+ return 0
+
+ @property
+ def length(self) -> int:
+ return self.data_file.file_size_in_bytes
+
+
+class TableScan(ABC):
+ """API for configuring a table scan."""
+
+ table: Table
+ snapshot: Snapshot
+ expression: BooleanExpression
+
+ def __init__(self, table: Table, snapshot: Snapshot | None = None, expression: BooleanExpression | None = None):
+ self.table = table
+ self.expression = AlwaysTrue() if expression is None else expression
+ if snapshot is None:
+ snapshot = table.current_snapshot()
+ if snapshot is not None:
+ self.snapshot = snapshot
+ else:
+ raise ValueError("Unable to resolve to a Snapshot to use for the table scan.")
+
+ @abstractmethod
+ def plan_files(self) -> Iterable[FileScanTask]:
+ """Plan tasks for this scan where each task reads a single file.
+
+ Returns:
+ Table: a tuple of tasks scanning entire files required by this scan
+ """
+
+
+class DataTableScan(TableScan):
+ """API for configuring a table scan."""
+
+ io: FileIO
+
+ def __init__(self, io: FileIO, table: Table, snapshot: Snapshot | None = None, expression: BooleanExpression | None = None):
Review Comment:
How about passing in the schema and partition-spec, rather than pulling in the whole table object?
##########
python/pyiceberg/table/__init__.py:
##########
@@ -60,33 +60,40 @@ def sort_order(self) -> SortOrder:
sort_order for sort_order in self.metadata.sort_orders if sort_order.order_id == self.metadata.default_sort_order_id
)
- def sort_orders(self) -> Dict[int, SortOrder]:
+ def sort_orders(self) -> dict[int, SortOrder]:
"""Return a dict of the sort orders of this table"""
return {sort_order.order_id: sort_order for sort_order in self.metadata.sort_orders}
def location(self) -> str:
"""Return the table's base location."""
return self.metadata.location
- def current_snapshot(self) -> Optional[Snapshot]:
+ def current_snapshot(self) -> Snapshot | None:
"""Get the current snapshot for this table, or None if there is no current snapshot."""
if snapshot_id := self.metadata.current_snapshot_id:
return self.snapshot_by_id(snapshot_id)
return None
- def snapshot_by_id(self, snapshot_id: int) -> Optional[Snapshot]:
+ def snapshot_by_id(self, snapshot_id: int) -> Snapshot | None:
"""Get the snapshot of this table with the given id, or None if there is no matching snapshot."""
try:
return next(snapshot for snapshot in self.metadata.snapshots if snapshot.snapshot_id == snapshot_id)
except StopIteration:
return None
- def snapshot_by_name(self, name: str) -> Optional[Snapshot]:
+ def snapshot_by_name(self, name: str) -> Snapshot | None:
"""Returns the snapshot referenced by the given name or null if no such reference exists."""
if ref := self.metadata.refs.get(name):
return self.snapshot_by_id(ref.snapshot_id)
return None
- def history(self) -> List[SnapshotLogEntry]:
+ def history(self) -> list[SnapshotLogEntry]:
"""Get the snapshot history of this table."""
return self.metadata.snapshot_log
+
+ def new_scan(self, io: FileIO, snapshot_id: int | None = None, expression: BooleanExpression | None = None):
Review Comment:
What do you think of setting it to AlwaysTrue by default?
```suggestion
def new_scan(self, io: FileIO, snapshot_id: int | None = None, expression: BooleanExpression = AlwaysTrue()):
```
##########
python/pyiceberg/expressions/visitors.py:
##########
@@ -613,6 +613,8 @@ def visit_or(self, left_result: bool, right_result: bool) -> bool:
def manifest_evaluator(
partition_spec: PartitionSpec, schema: Schema, partition_filter: BooleanExpression, case_sensitive: bool = True
) -> Callable[[ManifestFile], bool]:
- partition_schema = Schema(*partition_spec.partition_type(schema))
+ partition_type = partition_spec.partition_type(schema)
+ partition_schema = Schema(*partition_type.fields)
+ partition_filter = partition_filter if partition_filter is not None else AlwaysTrue()
Review Comment:
Technically this should not happen. The `partition_filter` is non-optional. A shorter way of writing this is:
```suggestion
partition_filter = partition_filter or AlwaysTrue()
```
--
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
[GitHub] [iceberg] Fokko commented on a diff in pull request #6069: Python: TableScan Plan files API implementation without row-level filters evaluation.
Posted by GitBox <gi...@apache.org>.
Fokko commented on code in PR #6069:
URL: https://github.com/apache/iceberg/pull/6069#discussion_r1009574626
##########
python/pyiceberg/cli/console.py:
##########
@@ -361,3 +361,24 @@ def table(ctx: Context, identifier: str, property_name: str): # noqa: F811
ctx.exit(1)
else:
raise NoSuchPropertyException(f"Property {property_name} does not exist on {identifier}")
+
+
+@run.group()
+def scan():
+ """Create a table scan."""
+
+
+@scan.command("table")
+@click.argument("identifier")
+@click.pass_context
+@catch_exception()
+def scan_table(ctx: Context, identifier: str):
Review Comment:
I think the later one is more flexible, but then we should remove the files because they have so much overlap
--
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
[GitHub] [iceberg] Fokko closed pull request #6069: Python: TableScan Plan files API implementation without residual evaluation
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko closed pull request #6069: Python: TableScan Plan files API implementation without residual evaluation
URL: https://github.com/apache/iceberg/pull/6069
--
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
[GitHub] [iceberg] dhruv-pratap commented on pull request #6069: Python: TableScan Plan files API implementation without residual evaluation
Posted by GitBox <gi...@apache.org>.
dhruv-pratap commented on PR #6069:
URL: https://github.com/apache/iceberg/pull/6069#issuecomment-1305942424
In retrospect, I think this is becoming too large of a PR and would benefit from breaking down into smaller tasks. I'm going to go ahead and close this PR and if you guys are onboard I can create issues for the below:
1. Add `ScanTask` interface
2. Add `FileScanTask` class that extends `ScanTask` interface
3. Fix ManifestEvalVisitor eq #6117
4. Add MetricsEvalVisitor that evaluates an expression on a DataFile to test whether rows in the file may match.
5. Add ResidualEvalVisitor that evaluates the residuals for an expression the partitions in the given PartitionSpec.
6. Add `TableScan` interface. @rdblue just picked this up in #6131
7. Add `DataTableScan` class that extends implements `plan_files()` API
8. Expose plan_files API as scan subcommand via CLI.
CC: @Fokko @rdblue @samredai
--
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
[GitHub] [iceberg] dhruv-pratap commented on a diff in pull request #6069: Python: TableScan Plan files API implementation without row-level filters evaluation.
Posted by GitBox <gi...@apache.org>.
dhruv-pratap commented on code in PR #6069:
URL: https://github.com/apache/iceberg/pull/6069#discussion_r1012034942
##########
python/pyiceberg/cli/output.py:
##########
@@ -14,9 +14,11 @@
# KIND, either express or implied. See the License for the
# specific language governing permissions and limitations
# under the License.
+from __future__ import annotations
Review Comment:
I've reverted to not use `from __future__ import annotations` in a recent commit.
--
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
[GitHub] [iceberg] rdblue commented on a diff in pull request #6069: Python: TableScan Plan files API implementation without residual evaluation
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #6069:
URL: https://github.com/apache/iceberg/pull/6069#discussion_r1014895867
##########
python/pyiceberg/expressions/visitors.py:
##########
@@ -526,7 +528,7 @@ def visit_equal(self, term: BoundTerm, literal: Literal[Any]) -> bool:
if lower > literal.value:
return ROWS_CANNOT_MATCH
- upper = _from_byte_buffer(term.ref().field.field_type, field.lower_bound)
+ upper = _from_byte_buffer(term.ref().field.field_type, field.upper_bound)
Review Comment:
@Fokko, can you check why tests didn't catch this case? We should ensure this is tested properly and separate the fix into its own PR.
--
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
[GitHub] [iceberg] rdblue commented on a diff in pull request #6069: Python: TableScan Plan files API implementation without residual evaluation
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #6069:
URL: https://github.com/apache/iceberg/pull/6069#discussion_r1014903669
##########
python/tests/table/test_scan.py:
##########
@@ -0,0 +1,177 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+# pylint:disable=redefined-outer-name
+import glob
+import json
+import os
+import tempfile
+from typing import Tuple
+
+import pytest
+from fastavro import reader, writer
+
+from pyiceberg.expressions import (
+ BooleanExpression,
+ EqualTo,
+ IsNull,
+ Reference,
+)
+from pyiceberg.expressions.literals import literal
+from pyiceberg.serializers import FromInputFile
+from pyiceberg.table import Table
+from tests.conftest import LocalFileIO
+from tests.io.test_io import LocalInputFile
+
+
+@pytest.fixture
+def temperatures_table() -> Table:
Review Comment:
Yes, this should generate a sample table rather than putting binaries in the repo.
Initially, we can build all of this based on just Avro and add Parquet tests later.
--
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
[GitHub] [iceberg] rdblue commented on pull request #6069: Python: TableScan Plan files API implementation without residual evaluation
Posted by GitBox <gi...@apache.org>.
rdblue commented on PR #6069:
URL: https://github.com/apache/iceberg/pull/6069#issuecomment-1322809905
@dhruv-pratap, we've been working on the list lately and I think the remaining items are: (4) add `MetricsEvalVisitor` to prune by column stats, (5) add `ResidualEvalVisitor` to produce residuals (LOW priority), and (8) expose `plan_files` via the CLI.
For the last one, we may just want to add filters to the existing `files` command?
--
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
[GitHub] [iceberg] Fokko commented on a diff in pull request #6069: Python: TableScan Plan files API implementation without row-level filters evaluation.
Posted by GitBox <gi...@apache.org>.
Fokko commented on code in PR #6069:
URL: https://github.com/apache/iceberg/pull/6069#discussion_r1009343180
##########
python/pyiceberg/cli/console.py:
##########
@@ -361,3 +361,24 @@ def table(ctx: Context, identifier: str, property_name: str): # noqa: F811
ctx.exit(1)
else:
raise NoSuchPropertyException(f"Property {property_name} does not exist on {identifier}")
+
+
+@run.group()
+def scan():
+ """Create a table scan."""
+
+
+@scan.command("table")
+@click.argument("identifier")
+@click.pass_context
+@catch_exception()
+def scan_table(ctx: Context, identifier: str):
Review Comment:
I like that we can easily visualize the files. I think we should merge this one with the files command since they are so similar. WDYT?
```
➜ python git:(master) ✗ pyiceberg --verbose true --catalog fokko files hive_test.h1
Snapshots: fokko.hive_test.h1
└── Snapshot 4362649644256929116, schema 0:
s3://bucket/41246f47-e1bc-465e-89b4-6d039a9d55dc/67ad77ac-d90b-4472-b1e9-672d91a1ac41/metadata/snap-4362649644256929116-1-63e88d4d-1b30-4cae-b7a0-f53b0b54758a.avro
├── Manifest: s3://bucket/41246f47-e1bc-465e-89b4-6d039a9d55dc/67ad77ac-d90b-4472-b1e9-672d91a1ac41/metadata/63e88d4d-1b30-4cae-b7a0-f53b0b54758a-m0.avro
│ └── Datafile:
│ s3://bucket/41246f47-e1bc-465e-89b4-6d039a9d55dc/67ad77ac-d90b-4472-b1e9-672d91a1ac41/data/0bfca5ff//00000-0-dweeks_20220901095219_f181f97a-43ce-4aa6-8cec-6956d83b7a42-job_lo
│ cal2002913683_0002-00001.parquet
└── Manifest: s3://bucket/41246f47-e1bc-465e-89b4-6d039a9d55dc/67ad77ac-d90b-4472-b1e9-672d91a1ac41/metadata/508fd9ac-520f-47d8-900f-00f1dda051ac-m0.avro
└── Datafile: s3://bucket/41246f47-e1bc-465e-89b4-6d039a9d55dc/67ad77ac-d90b-4472-b1e9-672d91a1ac41/data/3d030bcf/00000-0-44897130-c494-4dca-a164-e874c1e37c36-00001.parquet
```
--
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
[GitHub] [iceberg] rdblue commented on a diff in pull request #6069: Python: TableScan Plan files API implementation without residual evaluation
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #6069:
URL: https://github.com/apache/iceberg/pull/6069#discussion_r1014895768
##########
python/pyiceberg/expressions/visitors.py:
##########
@@ -517,7 +519,7 @@ def visit_equal(self, term: BoundTerm, literal: Literal[Any]) -> bool:
pos = term.ref().accessor.position
field = self.partition_fields[pos]
- if field.lower_bound is None:
+ if field.lower_bound is None or field.upper_bound is None:
Review Comment:
If one is `None` then both should be, so this shouldn't have any effect. But it's fine to update this.
--
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
[GitHub] [iceberg] Fokko commented on a diff in pull request #6069: Python: TableScan Plan files API implementation without residual evaluation
Posted by GitBox <gi...@apache.org>.
Fokko commented on code in PR #6069:
URL: https://github.com/apache/iceberg/pull/6069#discussion_r1015087384
##########
python/pyiceberg/expressions/visitors.py:
##########
@@ -517,7 +519,7 @@ def visit_equal(self, term: BoundTerm, literal: Literal[Any]) -> bool:
pos = term.ref().accessor.position
field = self.partition_fields[pos]
- if field.lower_bound is None:
+ if field.lower_bound is None or field.upper_bound is None:
Review Comment:
This is required for the linter since we're the variable below.
--
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
[GitHub] [iceberg] dhruv-pratap commented on a diff in pull request #6069: Python: TableScan Plan files API implementation without row-level filters evaluation.
Posted by GitBox <gi...@apache.org>.
dhruv-pratap commented on code in PR #6069:
URL: https://github.com/apache/iceberg/pull/6069#discussion_r1009610760
##########
python/pyiceberg/table/scan.py:
##########
@@ -0,0 +1,115 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+from __future__ import annotations
+
+import itertools
+from abc import ABC, abstractmethod
+from typing import Iterable
+
+from pyiceberg.expressions import AlwaysTrue, BooleanExpression
+from pyiceberg.expressions.visitors import manifest_evaluator
+from pyiceberg.io import FileIO
+from pyiceberg.manifest import DataFile, ManifestFile
+from pyiceberg.table import PartitionSpec, Snapshot, Table
+
+
+class FileScanTask(ABC):
+ """A scan task over a range of bytes in a single data file."""
+
+ manifest: ManifestFile
+ data_file: DataFile
+ residual: BooleanExpression | None
+ spec: PartitionSpec | None
+
+ def __init__(
+ self,
+ manifest: ManifestFile,
+ data_file: DataFile,
+ spec: PartitionSpec | None = None,
+ residual: BooleanExpression | None = None,
+ ):
+ self.manifest = manifest
+ self.data_file = data_file
+ self.spec = spec
+ self.residual = residual
+
+ @property
+ def start(self) -> int:
+ return 0
+
+ @property
+ def length(self) -> int:
+ return self.data_file.file_size_in_bytes
+
+
+class TableScan(ABC):
+ """API for configuring a table scan."""
+
+ table: Table
+ snapshot: Snapshot
+ expression: BooleanExpression
+
+ def __init__(self, table: Table, snapshot: Snapshot | None = None, expression: BooleanExpression | None = None):
+ self.table = table
+ self.expression = AlwaysTrue() if expression is None else expression
+ if snapshot is None:
+ snapshot = table.current_snapshot()
+ if snapshot is not None:
+ self.snapshot = snapshot
+ else:
+ raise ValueError("Unable to resolve to a Snapshot to use for the table scan.")
+
+ @abstractmethod
+ def plan_files(self) -> Iterable[FileScanTask]:
+ """Plan tasks for this scan where each task reads a single file.
+
+ Returns:
+ Table: a tuple of tasks scanning entire files required by this scan
+ """
+
+
+class DataTableScan(TableScan):
+ """API for configuring a table scan."""
+
+ io: FileIO
+
+ def __init__(self, io: FileIO, table: Table, snapshot: Snapshot | None = None, expression: BooleanExpression | None = None):
Review Comment:
I'm not sold on that, it would kind of be exposing internal implementation details of `DataTableScan`. Besides, the `DataTableScan` would always be created using the API `table.new_scan()` so the table(self) object would always be available to pass on.
--
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
[GitHub] [iceberg] dhruv-pratap commented on a diff in pull request #6069: Python: TableScan Plan files API implementation without row-level filters evaluation.
Posted by GitBox <gi...@apache.org>.
dhruv-pratap commented on code in PR #6069:
URL: https://github.com/apache/iceberg/pull/6069#discussion_r1009560503
##########
python/pyiceberg/cli/console.py:
##########
@@ -361,3 +361,24 @@ def table(ctx: Context, identifier: str, property_name: str): # noqa: F811
ctx.exit(1)
else:
raise NoSuchPropertyException(f"Property {property_name} does not exist on {identifier}")
+
+
+@run.group()
+def scan():
+ """Create a table scan."""
+
+
+@scan.command("table")
+@click.argument("identifier")
+@click.pass_context
+@catch_exception()
+def scan_table(ctx: Context, identifier: str):
Review Comment:
Not sure I follow you @Fokko . Do you mean make `scan` a sub-command of `files` command? Or just have `files` command use the `table.new_scan().plan_files()` API underneath?
If it's the later, I like that.
--
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
[GitHub] [iceberg] rdblue commented on a diff in pull request #6069: Python: TableScan Plan files API implementation without residual evaluation
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #6069:
URL: https://github.com/apache/iceberg/pull/6069#discussion_r1014904027
##########
python/pyiceberg/table/scan.py:
##########
@@ -0,0 +1,103 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+import itertools
+from abc import ABC, abstractmethod
+from typing import Iterable, List, Optional
+
+from pydantic import Field
+
+from pyiceberg.expressions import AlwaysTrue, BooleanExpression
+from pyiceberg.expressions.visitors import manifest_evaluator
+from pyiceberg.io import FileIO
+from pyiceberg.manifest import DataFile, ManifestFile
+from pyiceberg.table import PartitionSpec, Snapshot, Table
+from pyiceberg.utils.iceberg_base_model import IcebergBaseModel
+
+ALWAYS_TRUE = AlwaysTrue()
+
+
+class FileScanTask(IcebergBaseModel):
+ """A scan task over a range of bytes in a single data file."""
+
+ manifest: ManifestFile = Field()
+ data_file: DataFile = Field()
+ _residual: BooleanExpression = Field()
+ spec: PartitionSpec = Field()
+ start: int = Field(default=0)
+
+ @property
+ def length(self) -> int:
+ return self.data_file.file_size_in_bytes
+
+
+class TableScan(ABC):
+ """API for configuring a table scan."""
+
+ table: Table
+ snapshot: Snapshot
+ expression: BooleanExpression
+
+ def __init__(self, table: Table, snapshot: Optional[Snapshot] = None, expression: BooleanExpression = ALWAYS_TRUE):
+ self.table = table
+ self.expression = expression or ALWAYS_TRUE
+ if resolved_snapshot := snapshot or table.current_snapshot():
+ self.snapshot = resolved_snapshot
+ else:
+ raise ValueError("Unable to resolve to a Snapshot to use for the table scan.")
+
+ @abstractmethod
+ def plan_files(self) -> Iterable[FileScanTask]:
Review Comment:
In Java, there are several different types of tasks. I'd recommend using the same pattern and not requiring this to be `FileScanTask`. Introduce an abstract `ScanTask` and return an iterable of that instead.
--
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
[GitHub] [iceberg] dhruv-pratap commented on a diff in pull request #6069: Python: TableScan Plan files API implementation without residual evaluation
Posted by GitBox <gi...@apache.org>.
dhruv-pratap commented on code in PR #6069:
URL: https://github.com/apache/iceberg/pull/6069#discussion_r1015579816
##########
python/tests/table/test_scan.py:
##########
@@ -0,0 +1,177 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+# pylint:disable=redefined-outer-name
+import glob
+import json
+import os
+import tempfile
+from typing import Tuple
+
+import pytest
+from fastavro import reader, writer
+
+from pyiceberg.expressions import (
+ BooleanExpression,
+ EqualTo,
+ IsNull,
+ Reference,
+)
+from pyiceberg.expressions.literals import literal
+from pyiceberg.serializers import FromInputFile
+from pyiceberg.table import Table
+from tests.conftest import LocalFileIO
+from tests.io.test_io import LocalInputFile
+
+
+@pytest.fixture
+def temperatures_table() -> Table:
Review Comment:
Alright, let me take a look at that example, reverse engineer the binaries, and remove it from the repo.
--
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
[GitHub] [iceberg] Fokko commented on pull request #6069: Python: TableScan Plan files API implementation without residual evaluation
Posted by GitBox <gi...@apache.org>.
Fokko commented on PR #6069:
URL: https://github.com/apache/iceberg/pull/6069#issuecomment-1306148401
Hey @dhruv-pratap that makes a lot of sense. Maybe we should create issues on the list you mentioned above, to make sure that we're aligned on who's working on what. Smaller PRs make it much easier to get stuff merged.
--
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
[GitHub] [iceberg] rdblue commented on a diff in pull request #6069: Python: TableScan Plan files API implementation without residual evaluation
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #6069:
URL: https://github.com/apache/iceberg/pull/6069#discussion_r1014903835
##########
python/pyiceberg/table/scan.py:
##########
@@ -0,0 +1,103 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+import itertools
+from abc import ABC, abstractmethod
+from typing import Iterable, List, Optional
+
+from pydantic import Field
+
+from pyiceberg.expressions import AlwaysTrue, BooleanExpression
+from pyiceberg.expressions.visitors import manifest_evaluator
+from pyiceberg.io import FileIO
+from pyiceberg.manifest import DataFile, ManifestFile
+from pyiceberg.table import PartitionSpec, Snapshot, Table
+from pyiceberg.utils.iceberg_base_model import IcebergBaseModel
+
+ALWAYS_TRUE = AlwaysTrue()
+
+
+class FileScanTask(IcebergBaseModel):
+ """A scan task over a range of bytes in a single data file."""
+
+ manifest: ManifestFile = Field()
+ data_file: DataFile = Field()
+ _residual: BooleanExpression = Field()
+ spec: PartitionSpec = Field()
+ start: int = Field(default=0)
+
+ @property
+ def length(self) -> int:
+ return self.data_file.file_size_in_bytes
+
+
+class TableScan(ABC):
Review Comment:
Why does this API not match the Java API? Is there something more pythonic about this?
--
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
[GitHub] [iceberg] dhruv-pratap commented on a diff in pull request #6069: Python: TableScan Plan files API implementation without residual evaluation
Posted by GitBox <gi...@apache.org>.
dhruv-pratap commented on code in PR #6069:
URL: https://github.com/apache/iceberg/pull/6069#discussion_r1015576539
##########
python/pyiceberg/table/scan.py:
##########
@@ -0,0 +1,103 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+import itertools
+from abc import ABC, abstractmethod
+from typing import Iterable, List, Optional
+
+from pydantic import Field
+
+from pyiceberg.expressions import AlwaysTrue, BooleanExpression
+from pyiceberg.expressions.visitors import manifest_evaluator
+from pyiceberg.io import FileIO
+from pyiceberg.manifest import DataFile, ManifestFile
+from pyiceberg.table import PartitionSpec, Snapshot, Table
+from pyiceberg.utils.iceberg_base_model import IcebergBaseModel
+
+ALWAYS_TRUE = AlwaysTrue()
+
+
+class FileScanTask(IcebergBaseModel):
+ """A scan task over a range of bytes in a single data file."""
+
+ manifest: ManifestFile = Field()
+ data_file: DataFile = Field()
+ _residual: BooleanExpression = Field()
+ spec: PartitionSpec = Field()
+ start: int = Field(default=0)
+
+ @property
+ def length(self) -> int:
+ return self.data_file.file_size_in_bytes
+
+
+class TableScan(ABC):
+ """API for configuring a table scan."""
+
+ table: Table
+ snapshot: Snapshot
+ expression: BooleanExpression
+
+ def __init__(self, table: Table, snapshot: Optional[Snapshot] = None, expression: Optional[BooleanExpression] = ALWAYS_TRUE):
+ self.table = table
+ self.expression = expression or ALWAYS_TRUE
+ if resolved_snapshot := snapshot or table.current_snapshot():
+ self.snapshot = resolved_snapshot
+ else:
+ raise ValueError("Unable to resolve to a Snapshot to use for the table scan.")
+
+ @abstractmethod
+ def plan_files(self) -> Iterable[FileScanTask]:
+ """Plan tasks for this scan where each task reads a single file.
+
+ Returns:
+ Table: a tuple of tasks scanning entire files required by this scan
+ """
+
+
+class DataTableScan(TableScan):
+ """API for configuring a table scan."""
+
+ io: FileIO
+
+ def __init__(
+ self, io: FileIO, table: Table, snapshot: Optional[Snapshot] = None, expression: Optional[BooleanExpression] = ALWAYS_TRUE
+ ):
+ self.io = io
+ super().__init__(table, snapshot, expression)
Review Comment:
Will make the change.
--
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
[GitHub] [iceberg] dhruv-pratap commented on a diff in pull request #6069: Python: TableScan Plan files API implementation without row-level filters evaluation.
Posted by GitBox <gi...@apache.org>.
dhruv-pratap commented on code in PR #6069:
URL: https://github.com/apache/iceberg/pull/6069#discussion_r1012039167
##########
python/pyiceberg/cli/console.py:
##########
@@ -361,3 +361,24 @@ def table(ctx: Context, identifier: str, property_name: str): # noqa: F811
ctx.exit(1)
else:
raise NoSuchPropertyException(f"Property {property_name} does not exist on {identifier}")
+
+
+@run.group()
+def scan():
+ """Create a table scan."""
+
+
+@scan.command("table")
+@click.argument("identifier")
+@click.pass_context
+@catch_exception()
+def scan_table(ctx: Context, identifier: str):
Review Comment:
The only difference being `files` at present lists all snapshots. The table scan `plan_files()` will only list the snapshot that is being used for the scan.
--
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
[GitHub] [iceberg] dhruv-pratap commented on pull request #6069: Python: TableScan Plan files API implementation without row-level filters evaluation.
Posted by GitBox <gi...@apache.org>.
dhruv-pratap commented on PR #6069:
URL: https://github.com/apache/iceberg/pull/6069#issuecomment-1299226027
Added tests for table scan with partition filters. Also, added logic to copy and rewrite a table's metadata along with its data. This logic can be used to stage test table in a temporary directory to perform scan operations on it. This seems to be a necessity in absence of write-path APIs to stage test table for testing read-path APIs.
Happy to listen to suggestions if there are any alternatives to test these.
--
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
[GitHub] [iceberg] dhruv-pratap commented on a diff in pull request #6069: Python: TableScan Plan files API implementation without residual evaluation
Posted by GitBox <gi...@apache.org>.
dhruv-pratap commented on code in PR #6069:
URL: https://github.com/apache/iceberg/pull/6069#discussion_r1015651935
##########
python/pyiceberg/table/scan.py:
##########
@@ -0,0 +1,103 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+import itertools
+from abc import ABC, abstractmethod
+from typing import Iterable, List, Optional
+
+from pydantic import Field
+
+from pyiceberg.expressions import AlwaysTrue, BooleanExpression
+from pyiceberg.expressions.visitors import manifest_evaluator
+from pyiceberg.io import FileIO
+from pyiceberg.manifest import DataFile, ManifestFile
+from pyiceberg.table import PartitionSpec, Snapshot, Table
+from pyiceberg.utils.iceberg_base_model import IcebergBaseModel
+
+ALWAYS_TRUE = AlwaysTrue()
+
+
+class FileScanTask(IcebergBaseModel):
+ """A scan task over a range of bytes in a single data file."""
+
+ manifest: ManifestFile = Field()
+ data_file: DataFile = Field()
+ _residual: BooleanExpression = Field()
+ spec: PartitionSpec = Field()
+ start: int = Field(default=0)
+
+ @property
+ def length(self) -> int:
+ return self.data_file.file_size_in_bytes
+
+
+class TableScan(ABC):
Review Comment:
Just wanted to build the API incrementally and add things as and when needed, otherwise it becomes a bloated PR.
--
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
[GitHub] [iceberg] dhruv-pratap commented on a diff in pull request #6069: Python: TableScan Plan files API implementation without row-level filters evaluation.
Posted by GitBox <gi...@apache.org>.
dhruv-pratap commented on code in PR #6069:
URL: https://github.com/apache/iceberg/pull/6069#discussion_r1009566930
##########
python/pyiceberg/table/__init__.py:
##########
@@ -60,33 +60,40 @@ def sort_order(self) -> SortOrder:
sort_order for sort_order in self.metadata.sort_orders if sort_order.order_id == self.metadata.default_sort_order_id
)
- def sort_orders(self) -> Dict[int, SortOrder]:
+ def sort_orders(self) -> dict[int, SortOrder]:
"""Return a dict of the sort orders of this table"""
return {sort_order.order_id: sort_order for sort_order in self.metadata.sort_orders}
def location(self) -> str:
"""Return the table's base location."""
return self.metadata.location
- def current_snapshot(self) -> Optional[Snapshot]:
+ def current_snapshot(self) -> Snapshot | None:
"""Get the current snapshot for this table, or None if there is no current snapshot."""
if snapshot_id := self.metadata.current_snapshot_id:
return self.snapshot_by_id(snapshot_id)
return None
- def snapshot_by_id(self, snapshot_id: int) -> Optional[Snapshot]:
+ def snapshot_by_id(self, snapshot_id: int) -> Snapshot | None:
"""Get the snapshot of this table with the given id, or None if there is no matching snapshot."""
try:
return next(snapshot for snapshot in self.metadata.snapshots if snapshot.snapshot_id == snapshot_id)
except StopIteration:
return None
- def snapshot_by_name(self, name: str) -> Optional[Snapshot]:
+ def snapshot_by_name(self, name: str) -> Snapshot | None:
"""Returns the snapshot referenced by the given name or null if no such reference exists."""
if ref := self.metadata.refs.get(name):
return self.snapshot_by_id(ref.snapshot_id)
return None
- def history(self) -> List[SnapshotLogEntry]:
+ def history(self) -> list[SnapshotLogEntry]:
"""Get the snapshot history of this table."""
return self.metadata.snapshot_log
+
+ def new_scan(self, io: FileIO, snapshot_id: int | None = None, expression: BooleanExpression | None = None):
+ """Create a new scan for this table."""
+ from pyiceberg.table.scan import DataTableScan
+
+ snapshot = self.snapshot_by_id(snapshot_id) if snapshot_id is not None else None
Review Comment:
That does make sense. Will amend.
--
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
[GitHub] [iceberg] dhruv-pratap commented on a diff in pull request #6069: Python: TableScan Plan files API implementation without row-level filters evaluation.
Posted by GitBox <gi...@apache.org>.
dhruv-pratap commented on code in PR #6069:
URL: https://github.com/apache/iceberg/pull/6069#discussion_r1012034138
##########
python/pyiceberg/table/__init__.py:
##########
@@ -60,33 +60,40 @@ def sort_order(self) -> SortOrder:
sort_order for sort_order in self.metadata.sort_orders if sort_order.order_id == self.metadata.default_sort_order_id
)
- def sort_orders(self) -> Dict[int, SortOrder]:
+ def sort_orders(self) -> dict[int, SortOrder]:
"""Return a dict of the sort orders of this table"""
return {sort_order.order_id: sort_order for sort_order in self.metadata.sort_orders}
def location(self) -> str:
"""Return the table's base location."""
return self.metadata.location
- def current_snapshot(self) -> Optional[Snapshot]:
+ def current_snapshot(self) -> Snapshot | None:
"""Get the current snapshot for this table, or None if there is no current snapshot."""
if snapshot_id := self.metadata.current_snapshot_id:
return self.snapshot_by_id(snapshot_id)
return None
- def snapshot_by_id(self, snapshot_id: int) -> Optional[Snapshot]:
+ def snapshot_by_id(self, snapshot_id: int) -> Snapshot | None:
"""Get the snapshot of this table with the given id, or None if there is no matching snapshot."""
try:
return next(snapshot for snapshot in self.metadata.snapshots if snapshot.snapshot_id == snapshot_id)
except StopIteration:
return None
- def snapshot_by_name(self, name: str) -> Optional[Snapshot]:
+ def snapshot_by_name(self, name: str) -> Snapshot | None:
"""Returns the snapshot referenced by the given name or null if no such reference exists."""
if ref := self.metadata.refs.get(name):
return self.snapshot_by_id(ref.snapshot_id)
return None
- def history(self) -> List[SnapshotLogEntry]:
+ def history(self) -> list[SnapshotLogEntry]:
"""Get the snapshot history of this table."""
return self.metadata.snapshot_log
+
+ def new_scan(self, io: FileIO, snapshot_id: int | None = None, expression: BooleanExpression | None = None):
+ """Create a new scan for this table."""
+ from pyiceberg.table.scan import DataTableScan
+
+ snapshot = self.snapshot_by_id(snapshot_id) if snapshot_id is not None else None
Review Comment:
Amended in a recent commit.
--
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
[GitHub] [iceberg] dhruv-pratap commented on a diff in pull request #6069: Python: TableScan Plan files API implementation without row-level filters evaluation.
Posted by GitBox <gi...@apache.org>.
dhruv-pratap commented on code in PR #6069:
URL: https://github.com/apache/iceberg/pull/6069#discussion_r1012044761
##########
python/pyiceberg/cli/console.py:
##########
@@ -361,3 +361,24 @@ def table(ctx: Context, identifier: str, property_name: str): # noqa: F811
ctx.exit(1)
else:
raise NoSuchPropertyException(f"Property {property_name} does not exist on {identifier}")
+
+
+@run.group()
+def scan():
+ """Create a table scan."""
+
+
+@scan.command("table")
+@click.argument("identifier")
+@click.pass_context
+@catch_exception()
+def scan_table(ctx: Context, identifier: str):
Review Comment:
Maybe we can make files with three options:
- `files --snapshot=all` - The current files behavior
- `files --snapshot=current` - Files under the current snapshot
- `files --snapshot=<snapshot_id>` - Files under the snapshot_id specified
--
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
[GitHub] [iceberg] Fokko commented on a diff in pull request #6069: Python: TableScan Plan files API implementation without residual evaluation
Posted by GitBox <gi...@apache.org>.
Fokko commented on code in PR #6069:
URL: https://github.com/apache/iceberg/pull/6069#discussion_r1014404025
##########
python/pyiceberg/cli/output.py:
##########
@@ -49,6 +52,10 @@ def describe_table(self, table: Table) -> None:
def files(self, table: Table, io: FileIO, history: bool) -> None:
...
+ @abstractmethod
+ def scan_plan_files(self, plan_files: Iterable[FileScanTask], snapshot_id: int | None = None) -> None:
Review Comment:
```suggestion
def scan_plan_files(self, plan_files: Iterable[FileScanTask], snapshot_id: Optional[int] = None) -> None:
```
##########
python/pyiceberg/cli/output.py:
##########
@@ -182,7 +202,7 @@ def _out(self, d: Any) -> None:
def exception(self, ex: Exception) -> None:
self._out({"type": ex.__class__.__name__, "message": str(ex)})
- def identifiers(self, identifiers: List[Identifier]) -> None:
+ def identifiers(self, identifiers: list[Identifier]) -> None:
Review Comment:
```suggestion
def identifiers(self, identifiers: List[Identifier]) -> None:
```
##########
python/pyiceberg/table/__init__.py:
##########
@@ -90,3 +92,15 @@ def snapshot_by_name(self, name: str) -> Optional[Snapshot]:
def history(self) -> List[SnapshotLogEntry]:
"""Get the snapshot history of this table."""
return self.metadata.snapshot_log
+
+ def new_scan(self, io: FileIO, snapshot_id: Optional[int] = None, expression: Optional[BooleanExpression] = ALWAYS_TRUE):
Review Comment:
```suggestion
def new_scan(self, io: FileIO, snapshot_id: Optional[int] = None, expression:BooleanExpression = ALWAYS_TRUE):
```
Since we default to `AlwaysTrue`, I think we can set this as required
##########
python/pyiceberg/cli/console.py:
##########
@@ -361,3 +361,24 @@ def table(ctx: Context, identifier: str, property_name: str): # noqa: F811
ctx.exit(1)
else:
raise NoSuchPropertyException(f"Property {property_name} does not exist on {identifier}")
+
+
+@run.group()
+def scan():
+ """Create a table scan."""
+
+
+@scan.command("table")
+@click.argument("identifier")
+@click.pass_context
+@catch_exception()
+def scan_table(ctx: Context, identifier: str):
Review Comment:
Why not move it to the scan subcommand? This way we don't have similar commands
##########
python/pyiceberg/cli/output.py:
##########
@@ -66,7 +73,7 @@ def spec(self, spec: PartitionSpec) -> None:
...
@abstractmethod
- def uuid(self, uuid: Optional[UUID]) -> None:
+ def uuid(self, uuid: UUID | None) -> None:
Review Comment:
```suggestion
def uuid(self, uuid: Optional[UUID]) -> None:
```
##########
python/pyiceberg/cli/output.py:
##########
@@ -146,6 +153,19 @@ def files(self, table: Table, io: FileIO, history: bool) -> None:
manifest_tree.add(f"Datafile: {manifest_entry.data_file.file_path}")
Console().print(snapshot_tree)
+ def scan_plan_files(self, plan_files: Iterable[FileScanTask], snapshot_id: int | None = None) -> None:
+ snapshot_tree = Tree(f"Snapshot: {snapshot_id}")
+
+ manifest_dict = {}
+ for file in plan_files:
+ if file.manifest.manifest_path not in manifest_dict:
Review Comment:
Nit: Should we move `file.manifest.manifest_path` in a local variable, since we use it three times?
##########
python/pyiceberg/expressions/visitors.py:
##########
@@ -436,8 +436,10 @@ def __init__(self, partition_struct_schema: Schema, partition_filter: BooleanExp
self.partition_filter = bind(partition_struct_schema, rewrite_not(partition_filter), case_sensitive)
def eval(self, manifest: ManifestFile) -> bool:
+ print(f"Evaluating ManifestFile = {manifest} with partition_filter={self.partition_filter}")
Review Comment:
I think this is still around from the debugging? Maybe convert it into a log statement?
##########
python/pyiceberg/cli/output.py:
##########
@@ -38,7 +41,7 @@ def exception(self, ex: Exception) -> None:
...
@abstractmethod
- def identifiers(self, identifiers: List[Identifier]) -> None:
+ def identifiers(self, identifiers: list[Identifier]) -> None:
Review Comment:
```suggestion
def identifiers(self, identifiers: List[Identifier]) -> None:
```
##########
python/pyiceberg/cli/output.py:
##########
@@ -88,7 +95,7 @@ def exception(self, ex: Exception) -> None:
else:
Console(stderr=True).print(ex)
- def identifiers(self, identifiers: List[Identifier]) -> None:
+ def identifiers(self, identifiers: list[Identifier]) -> None:
Review Comment:
```suggestion
def identifiers(self, identifiers: List[Identifier]) -> None:
```
##########
python/pyiceberg/cli/output.py:
##########
@@ -200,8 +220,11 @@ def schema(self, schema: Schema) -> None:
def files(self, table: Table, io: FileIO, history: bool) -> None:
pass
+ def scan_plan_files(self, plan_files: Iterable[FileScanTask], snapshot_id: int | None = None) -> None:
+ pass
+
def spec(self, spec: PartitionSpec) -> None:
print(spec.json())
- def uuid(self, uuid: Optional[UUID]) -> None:
+ def uuid(self, uuid: UUID | None) -> None:
Review Comment:
```suggestion
def uuid(self, uuid: Optional[UUID]) -> None:
```
##########
python/pyiceberg/expressions/visitors.py:
##########
@@ -613,6 +615,9 @@ def visit_or(self, left_result: bool, right_result: bool) -> bool:
def manifest_evaluator(
partition_spec: PartitionSpec, schema: Schema, partition_filter: BooleanExpression, case_sensitive: bool = True
) -> Callable[[ManifestFile], bool]:
- partition_schema = Schema(*partition_spec.partition_type(schema))
+ partition_type = partition_spec.partition_type(schema)
+ partition_schema = Schema(*partition_type.fields)
+ partition_filter = partition_filter or AlwaysTrue()
Review Comment:
Since `partition_filter` cannot be `None` according to the type, this statement doesn't have any effect
##########
python/pyiceberg/cli/output.py:
##########
@@ -164,7 +184,7 @@ def schema(self, schema: Schema) -> None:
def spec(self, spec: PartitionSpec) -> None:
Console().print(str(spec))
- def uuid(self, uuid: Optional[UUID]) -> None:
+ def uuid(self, uuid: UUID | None) -> None:
Review Comment:
```suggestion
def uuid(self, uuid: Optional[UUID]) -> None:
```
##########
python/pyiceberg/table/__init__.py:
##########
@@ -90,3 +92,15 @@ def snapshot_by_name(self, name: str) -> Optional[Snapshot]:
def history(self) -> List[SnapshotLogEntry]:
"""Get the snapshot history of this table."""
return self.metadata.snapshot_log
+
+ def new_scan(self, io: FileIO, snapshot_id: Optional[int] = None, expression: Optional[BooleanExpression] = ALWAYS_TRUE):
+ """Create a new scan for this table."""
+ from pyiceberg.table.scan import DataTableScan
+
+ if not (use_snapshot := snapshot_id or self.metadata.current_snapshot_id):
+ raise ValueError("Unable to resolve a snapshot to use for this scan.")
+
+ if not (snapshot := self.snapshot_by_id(use_snapshot)):
Review Comment:
Same thing here, the walrus operator doesn't add anything here
```suggestion
if not self.snapshot_by_id(use_snapshot):
```
##########
python/pyiceberg/expressions/visitors.py:
##########
@@ -517,7 +519,7 @@ def visit_equal(self, term: BoundTerm, literal: Literal[Any]) -> bool:
pos = term.ref().accessor.position
field = self.partition_fields[pos]
- if field.lower_bound is None:
+ if field.lower_bound is None or field.upper_bound is None:
Review Comment:
Great catch! Thanks!
##########
python/pyiceberg/table/scan.py:
##########
@@ -0,0 +1,103 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+import itertools
+from abc import ABC, abstractmethod
+from typing import Iterable, List, Optional
+
+from pydantic import Field
+
+from pyiceberg.expressions import AlwaysTrue, BooleanExpression
+from pyiceberg.expressions.visitors import manifest_evaluator
+from pyiceberg.io import FileIO
+from pyiceberg.manifest import DataFile, ManifestFile
+from pyiceberg.table import PartitionSpec, Snapshot, Table
+from pyiceberg.utils.iceberg_base_model import IcebergBaseModel
+
+ALWAYS_TRUE = AlwaysTrue()
+
+
+class FileScanTask(IcebergBaseModel):
+ """A scan task over a range of bytes in a single data file."""
+
+ manifest: ManifestFile = Field()
+ data_file: DataFile = Field()
+ _residual: BooleanExpression = Field()
+ spec: PartitionSpec = Field()
+ start: int = Field(default=0)
+
+ @property
+ def length(self) -> int:
+ return self.data_file.file_size_in_bytes
+
+
+class TableScan(ABC):
+ """API for configuring a table scan."""
+
+ table: Table
+ snapshot: Snapshot
+ expression: BooleanExpression
+
+ def __init__(self, table: Table, snapshot: Optional[Snapshot] = None, expression: Optional[BooleanExpression] = ALWAYS_TRUE):
+ self.table = table
+ self.expression = expression or ALWAYS_TRUE
+ if resolved_snapshot := snapshot or table.current_snapshot():
+ self.snapshot = resolved_snapshot
+ else:
+ raise ValueError("Unable to resolve to a Snapshot to use for the table scan.")
+
+ @abstractmethod
+ def plan_files(self) -> Iterable[FileScanTask]:
+ """Plan tasks for this scan where each task reads a single file.
+
+ Returns:
+ Table: a tuple of tasks scanning entire files required by this scan
+ """
+
+
+class DataTableScan(TableScan):
+ """API for configuring a table scan."""
+
+ io: FileIO
+
+ def __init__(
+ self, io: FileIO, table: Table, snapshot: Optional[Snapshot] = None, expression: Optional[BooleanExpression] = ALWAYS_TRUE
+ ):
+ self.io = io
+ super().__init__(table, snapshot, expression)
Review Comment:
Nit: Convention is to first call the super, and then do the rest
##########
python/pyiceberg/table/scan.py:
##########
@@ -0,0 +1,103 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+import itertools
+from abc import ABC, abstractmethod
+from typing import Iterable, List, Optional
+
+from pydantic import Field
+
+from pyiceberg.expressions import AlwaysTrue, BooleanExpression
+from pyiceberg.expressions.visitors import manifest_evaluator
+from pyiceberg.io import FileIO
+from pyiceberg.manifest import DataFile, ManifestFile
+from pyiceberg.table import PartitionSpec, Snapshot, Table
+from pyiceberg.utils.iceberg_base_model import IcebergBaseModel
+
+ALWAYS_TRUE = AlwaysTrue()
+
+
+class FileScanTask(IcebergBaseModel):
+ """A scan task over a range of bytes in a single data file."""
+
+ manifest: ManifestFile = Field()
+ data_file: DataFile = Field()
+ _residual: BooleanExpression = Field()
+ spec: PartitionSpec = Field()
+ start: int = Field(default=0)
+
+ @property
+ def length(self) -> int:
+ return self.data_file.file_size_in_bytes
+
+
+class TableScan(ABC):
+ """API for configuring a table scan."""
+
+ table: Table
+ snapshot: Snapshot
+ expression: BooleanExpression
+
+ def __init__(self, table: Table, snapshot: Optional[Snapshot] = None, expression: Optional[BooleanExpression] = ALWAYS_TRUE):
Review Comment:
Since we provide a default here, maybe we should just make it mandatory
```suggestion
def __init__(self, table: Table, snapshot: Optional[Snapshot] = None, expression: BooleanExpression = ALWAYS_TRUE):
```
##########
python/pyiceberg/table/__init__.py:
##########
@@ -90,3 +92,15 @@ def snapshot_by_name(self, name: str) -> Optional[Snapshot]:
def history(self) -> List[SnapshotLogEntry]:
"""Get the snapshot history of this table."""
return self.metadata.snapshot_log
+
+ def new_scan(self, io: FileIO, snapshot_id: Optional[int] = None, expression: Optional[BooleanExpression] = ALWAYS_TRUE):
+ """Create a new scan for this table."""
+ from pyiceberg.table.scan import DataTableScan
+
+ if not (use_snapshot := snapshot_id or self.metadata.current_snapshot_id):
Review Comment:
The walrus operator doesn't add any value here, I would suggest removing it
```suggestion
if not snapshot_id and not self.metadata.current_snapshot_id:
```
##########
python/tests/table/test_scan.py:
##########
@@ -0,0 +1,177 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+# pylint:disable=redefined-outer-name
+import glob
+import json
+import os
+import tempfile
+from typing import Tuple
+
+import pytest
+from fastavro import reader, writer
+
+from pyiceberg.expressions import (
+ BooleanExpression,
+ EqualTo,
+ IsNull,
+ Reference,
+)
+from pyiceberg.expressions.literals import literal
+from pyiceberg.serializers import FromInputFile
+from pyiceberg.table import Table
+from tests.conftest import LocalFileIO
+from tests.io.test_io import LocalInputFile
+
+
+@pytest.fixture
+def temperatures_table() -> Table:
Review Comment:
I'm hesitant to keep actual binaries in the repository. We already generate the manifest list and manifest itself: https://github.com/apache/iceberg/blob/b215c4861a0c967f8d2a2b0444697d7877d33992/python/tests/conftest.py#L952-L967 How would you feel about generating some Parquet files too?
##########
python/pyiceberg/cli/output.py:
##########
@@ -14,9 +14,11 @@
# KIND, either express or implied. See the License for the
# specific language governing permissions and limitations
# under the License.
+from __future__ import annotations
Review Comment:
It should have been fixed now https://github.com/apache/iceberg/pull/6114 is in. Delayed loading of the annotations (that's what the import does), can be helpful sometime :)
--
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