feat: Add support for rest scan planning #2864
Conversation
| return FileScanTask( | ||
| data_file=data_file, | ||
| delete_files=resolved_deletes, | ||
| residual=rest_task.residual_filter if rest_task.residual_filter else ALWAYS_TRUE, |
There was a problem hiding this comment.
are residual filters bounded in the fs task ?
There was a problem hiding this comment.
Thanks for review @singhpk234! The residual filters from REST are not bound in the normal sense. Currently the residual is only used for the optimize check in count().
The actual row filtering still uses the full row_filter, not the residual. This works correctly but is slightly inefficient.
| Returns: | ||
| PlanningResponse the result of the scan plan request representing the status | ||
| Raises: |
There was a problem hiding this comment.
| Raises: | |
| Raises: |
| def _should_use_rest_planning(self) -> bool: | ||
| """Check if REST scan planning should be used for this scan.""" | ||
| from pyiceberg.catalog.rest import RestCatalog | ||
|
|
||
| if not isinstance(self.catalog, RestCatalog): | ||
| return False | ||
| return self.catalog.is_rest_scan_planning_enabled() |
There was a problem hiding this comment.
I would be inclined to create a method on the Catalog, eg:
@property
@abstractmethod
def use_server_side_planning(self, identifier: str | Identifier) -> bool:
"""Support for Server Side Planning"""
Have the MetastoreCatalog implement it, and return False. And rename is_rest_scan_planning_enabled to support_server_side_planning. Now we have to go though multiple jumps.
This would also clean up _plan_files_rest below.
| # REST content-type to DataFileContent | ||
| CONTENT_TYPE_MAP: dict[str, DataFileContent] = { | ||
| "data": DataFileContent.DATA, | ||
| "position-deletes": DataFileContent.POSITION_DELETES, | ||
| "equality-deletes": DataFileContent.EQUALITY_DELETES, | ||
| } |
There was a problem hiding this comment.
Should we move this to a static method on DataFileContent?
| return False | ||
| return self.catalog.is_rest_scan_planning_enabled() | ||
|
|
||
| def _plan_files_rest(self) -> Iterable[FileScanTask]: |
There was a problem hiding this comment.
nit, To keep the same language:
| def _plan_files_rest(self) -> Iterable[FileScanTask]: | |
| def _plan_files_server_side(self) -> Iterable[FileScanTask]: |
3 participants
related to #2775
Rationale for this change
Adds synchornous client-side support for REST server side scan planning, allowing for scanning if the rest catalog supports it.
This PR cherry-picks and builds on two WIP PRs:
Currently scanning is enable with rest-scan-planning-enabled=true in catalog properties.
TODO: spec handling
Are these changes tested?
Integration tests added with manual testing
Are there any user-facing changes?
yes