-
Notifications
You must be signed in to change notification settings - Fork 2.9k
Core: Request/Response models and parsers for REST Scan Planning #13004
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
28b5fda to
18de49b
Compare
|
Breaking this change in to 3 logical changes, have all 3 working locally, will send these as we go !
Have couple of things to propose to spec as well, will do it in the Plumbing and Spark Integ phase of the PR |
be65a34 to
d445830
Compare
84b879e to
75db076
Compare
320f764 to
39da580
Compare
fb035f7 to
834fa2b
Compare
core/src/main/java/org/apache/iceberg/rest/RESTObjectMapper.java
Outdated
Show resolved
Hide resolved
amogh-jahagirdar
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 @singhpk234! I think this is close, just one thing that I think should not be public
core/src/main/java/org/apache/iceberg/rest/TableScanResponseContext.java
Outdated
Show resolved
Hide resolved
amogh-jahagirdar
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 @singhpk234! Minor style comment but overall looks good to me. I will hold for a bit in case @danielcweeks or any others have any remaining comments.
core/src/main/java/org/apache/iceberg/rest/RESTSerializers.java
Outdated
Show resolved
Hide resolved
| PartitionData partitionData = null; | ||
| if (jsonNode.has(PARTITION)) { | ||
| partitionData = new PartitionData(spec.partitionType()); | ||
| partitionData = new PartitionData(specsById.get(specId).partitionType()); |
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.
Minor followup: Just noticed this but in the new structure we should probably have a clearer message rather than a potential NPE in case the file somehow references a spec ID that doesn't exist for some reason. It's unlikely but I think it's generally better to have clearer error messages so that if it does happen a user knows what to check rather than having to go deeper in the implementation. We can also probably extract specsById.get(specId) into a separate variable since I see it's called three times.
68774f6 to
75cb595
Compare
core/src/main/java/org/apache/iceberg/RESTFileScanTaskParser.java
Outdated
Show resolved
Hide resolved
core/src/main/java/org/apache/iceberg/RESTFileScanTaskParser.java
Outdated
Show resolved
Hide resolved
Co-authored-by: Rahil <rchertara@gmail.com> Co-authored-by: Prashant <singhpk010696@gmaill.com>
core/src/main/java/org/apache/iceberg/rest/requests/PlanTableScanRequest.java
Outdated
Show resolved
Hide resolved
core/src/main/java/org/apache/iceberg/rest/requests/PlanTableScanRequest.java
Show resolved
Hide resolved
…nor style changes
danielcweeks
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.
Minor comments, but +1. Thanks @singhpk234 !
|
Thanks @singhpk234 @rahil-c for the implementation and patience in following through on this. Thanks @danielcweeks for the review! |
|
Thanks @singhpk234 for pushing this thru, happy to see it finally get merged! |
About the change
Solves #13005
PR for resuming the work for scan planning (previous pr : #11180), presently since the client is not ready it can't be used even if IRC Server (for ex Apache Polaris ) supports it. Scan planning unblocks many use-cases such as inter-op.
This is Part 1 of changes #13004 (comment) have rest 2 in my local will send them over part by part !
Parsers :
[1] ContentFileParser for Delete and DataFile is re-used, there is one API change though that now the fromJson and toJson require specsById, as unless the file is parsed we don;t know which spec it belongs to in rest case
[2] TableScanResponseParser is introduced which is a stateful parser, introduced via #13191
[3] Request (and its corresponding models)
[a] PlanTableScanRequestParser : parser for https://github.com/apache/iceberg/blob/main/open-api/rest-catalog-open-api.yaml#L4370
[b] FetchScanTasksRequestParser : parser for https://github.com/apache/iceberg/blob/main/open-api/rest-catalog-open-api.yaml#L4434
4 Response (and its corresponding models)
[a] FetchPlanningResultResponseParser : parser for https://github.com/apache/iceberg/blob/main/open-api/rest-catalog-open-api.yaml#L4700
[b] FetchScanTasksResponseParser : parser for https://github.com/apache/iceberg/blob/main/open-api/rest-catalog-open-api.yaml#L4707
[c] PlanTableScanResponseParser : parser for https://github.com/apache/iceberg/blob/main/open-api/rest-catalog-open-api.yaml#L4693.
Acknowledgement
I connected with the author i.e @rahil-c to see if he plans to resume it, apparently he doesn't have cycles and was kind enough to allow me to push this to the end.
I just rebased the pr, on the current main and go break the pr into smaller chunks while addressing the pending feedback from review, I have something working locally, will publish them soon
(SideNote : I was one of the reviewers to the PR too, will be interesting to see the other side 😌)
Co-author: @rahil-c