-
Notifications
You must be signed in to change notification settings - Fork 5.5k
feat: Distributed Procedure Support Part 2.5/X - dev documentation #26717
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: master
Are you sure you want to change the base?
feat: Distributed Procedure Support Part 2.5/X - dev documentation #26717
Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideAdds developer documentation describing the architecture and implementation of normal and distributed procedures in Presto, and wires it into the main developer docs navigation. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
7c0a466 to
08c3394
Compare
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.
Hey there - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `presto-docs/src/main/sphinx/develop/procedures.rst:77` </location>
<code_context>
+
+.. note::
+
+ * The Java method corresponding to the ``MethodHandle`` have a correspondence with the procedure parameters. Its first parameter must be a session object
+ of type ``ConnectorSession.class``. The subsequent parameters must have a strict one-to-one correspondence in both type and order with the
+ procedure parameters declared in the arguments list.
</code_context>
<issue_to_address>
**issue (typo):** Fix grammar in the sentence about the Java method corresponding to the MethodHandle.
Since the subject is singular ("method"), use "has" instead of "have". You could also rephrase to: "The Java method corresponding to the ``MethodHandle`` must have a one-to-one correspondence with the procedure parameters," which aligns well with the next sentence.
```suggestion
* The Java method corresponding to the ``MethodHandle`` must have a one-to-one correspondence with the procedure parameters. Its first parameter must be a session object
```
</issue_to_address>
### Comment 2
<location> `presto-docs/src/main/sphinx/develop/procedures.rst:433-434` </location>
<code_context>
+ Table icebergTable = procedureContext.getTable().orElseThrow(() -> new VerifyException("No partition data for partitioned table"));
+ IcebergTableHandle tableHandle = layoutHandle.getTable();
+
+ // Performs the preparatory work required when starting the execution of ``rewrite_data_files``,
+ // and encapsulate the necessary information and handling logic within the ``procedureContext``
+ ......
+
</code_context>
<issue_to_address>
**nitpick (typo):** Adjust verb agreement in the comment about preparatory work for `rewrite_data_files`.
Change "and encapsulate" to "and encapsulates" to match "Performs," or rephrase the sentence to avoid the mismatch. This is just a small grammar fix in the comment.
```suggestion
+ // Performs the preparatory work required when starting the execution of ``rewrite_data_files``,
+ // and encapsulates the necessary information and handling logic within the ``procedureContext``
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
steveburnett
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.
Thank you for this lengthy and detailed documentation! It's very good. I made many comments but they are almost all small rephrasing suggestions and formatting fixes, nothing substantial and no major problems. Thanks!
|
@steveburnett thank you so much for the review and detailed feedback. I've addressed your comments. Please take a look when you get a chance. |
steveburnett
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 for the update! Just a couple of nits I missed the first time.
5e2e18a to
0f74c37
Compare
|
Hi @steveburnett, I've addressed your comments. Please take a look when you get a chance. Thanks a lot! |
steveburnett
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! (docs)
Pull updated branch, new local doc build. Looks good, thanks!
Description
This PR add the developer documentation for procedures architecture in Presto, covering the development of both normal procedures and distributed procedures.
Motivation and Context
#26679
prestodb/rfcs#12
Impact
N/A
Test Plan
N/A
Contributor checklist
Release Notes