-
Notifications
You must be signed in to change notification settings - Fork 421
feat: Add support for rest scan planning #2864
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
pyiceberg/table/__init__.py
Outdated
| 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Fokko
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a few comments, would be good to get those cleaned up. Apart from that, this looks great to me! Thanks @geruh for working on this, very exciting to see this being added 👍
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