-
Notifications
You must be signed in to change notification settings - Fork 411
feat: Add RESTscan Planning Endpoints and config #2842
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
Conversation
kevinjqliu
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.
Thanks! Here are the corresponding IRC spec entries.
Looks like rest-scan-planning-enabled is a server-side config. Should we check /v1/config response to see if the endpoints are supported by the catalog?
| SNAPSHOT_LOADING_MODE = "snapshot-loading-mode" | ||
| AUTH = "auth" | ||
| CUSTOM = "custom" | ||
| REST_SCAN_PLANNING_ENABLED = "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.
looks like rest-scan-planning-enabled is a catalog server config
https://github.com/apache/iceberg/blob/05998edb8808ae21739072cca17b621925a2081a/core/src/main/java/org/apache/iceberg/rest/RESTCatalogProperties.java#L40-L41
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 dug into the use of rest-scan-planning-enabled, its actually not a catalog server config.
RESTSessionCatalog is the catalog client, and rest-scan-planning-enabled is used along with the checking whether the specific scan planning endpoint is advertised by the server.
https://github.com/apache/iceberg/blob/05998edb8808ae21739072cca17b621925a2081a/core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java#L275-L279
https://github.com/apache/iceberg/blob/05998edb8808ae21739072cca17b621925a2081a/core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java#L510
Yes, I rememebr there was an open PR for the endpoints capability but haven't been able to find it. I'll probably raise a PR to add the endpoints logic. Furthermore, this is currently based off of the merged code in main. |
kevinjqliu
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.
after looking at the java client implementation, this makes sense to me! This PR follows the same PR here
We should also gate on the endpoint's existence but we can address that as a follow up.
|
Created an issue for reading /v1/config's endpoint #2847 |
kevinjqliu
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.
LGTM!
related to #2775
Rationale for this change
This PR adds some of the rest scanning endpoints needed for the synchronous scan planning And the configuration property used to determine scan planning support.
Aligning with the java implementation in apache/iceberg#13400.
Usage
Ideally we only would want to use rest scan planning if the catalog is capable. I remember there was a PR open for this at one point but can't seem to find it.
Are these changes tested?
Are there any user-facing changes?
new property to configure scanning