[Append Scan] Extract manifest group planning into separate class #2232
Conversation
| @property | ||
| def partition_filters(self) -> KeyDefaultDict[int, BooleanExpression]: | ||
| return self._manifest_planner.partition_filters |
There was a problem hiding this comment.
Keeping this public method around to not introduce a breaking change to DataScan. On the other hand, the private methods have been moved into ManifestGroupPlanner. Technically, that could still break users subclassing DataScan and calling the removed methods in the subclass (and also users accessing those private methods, but that feels more OK to break than the subclassing case).
I'm not familiar with PyIceberg breaks / deprecations - would it be fine to remove these private methods or is a deprecation cycle still required?
| @@ -2075,6 +1957,160 @@ def count(self) -> int: | |||
| return res | |||
|
|
|||
|
|
|||
| class ManifestGroupPlanner: | |||
There was a problem hiding this comment.
The motivation for a manifest-based file scan task planner comes from the Java-side https://github.com/apache/iceberg/blob/1911c94ea605a3d3f10a1994b046f00a5e9fdceb/core/src/main/java/org/apache/iceberg/BaseIncrementalAppendScan.java#L76-L97 (class here).
There was a problem hiding this comment.
It's also to share this logic between scans like DataScan and IncrementalAppendScan, that both use this flow.
| case_sensitive=self.case_sensitive, | ||
| schema=self.table_metadata.schema(), | ||
| ) | ||
| def _manifest_planner(self) -> ManifestGroupPlanner: |
There was a problem hiding this comment.
This could also be a field on the class set in the constructor. Kept the diff smaller here, but happy to change
Rationale for this change
Split up from incremental append scan work - see #2031 (comment). PyIceberg doesn't support incremental reading of appended data between snapshots, like Spark does.
This PR introduces a
ManifestGroupPlannerto hold the logic of using manifests to plan the files for a table scan. This allows this logic to be re-used across scans. TheIncrementalAppendScanwill also use this (see #2234).Are these changes tested?
N / A
Are there any user-facing changes?
Yes, see #2232 (comment).