fix(docs): Fix spilling docs#26194
Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideDocumentation for spilling has been updated to correct a typo, refresh session and admin property descriptions, align defaults with code, and improve clarity. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
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/admin/properties-session.rst:150-151` </location>
<code_context>
-footprint to pass at the cost of slower execution times. Currently, spilling is supported only for
-aggregations and joins (inner and outer), so this property will not reduce memory usage required for
-window functions, sorting and other join types.
+footprint to pass at the cost of slower execution times. See :ref:`spill-operations`
+for list of operations that support spilling.
Be aware that this is an experimental feature and should be used with care.
</code_context>
<issue_to_address>
**suggestion (typo):** Add 'a' before 'list' for grammatical correctness.
Change to 'for a list of operations that support spilling.' for correct grammar.
```suggestion
footprint to pass at the cost of slower execution times. See :ref:`spill-operations`
for a list of operations that support spilling.
```
</issue_to_address>
### Comment 2
<location> `presto-docs/src/main/sphinx/admin/properties.rst:255-256` </location>
<code_context>
-footprint to pass at the cost of slower execution times. Currently, spilling is supported only for
-aggregations and joins (inner and outer), so this property will not reduce memory usage required for
-window functions, sorting and other join types.
+footprint to pass at the cost of slower execution times. See :ref:`spill-operations`
+for list of operations that support spilling.
Be aware that this is an experimental feature and should be used with care.
</code_context>
<issue_to_address>
**suggestion (typo):** Add 'a' before 'list' for grammatical correctness.
Change to 'for a list of operations that support spilling.' for correct grammar.
```suggestion
footprint to pass at the cost of slower execution times. See :ref:`spill-operations`
for a list of operations that support spilling.
```
</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.
Please fix the two grammar nits that sourcery-ai commented on. I don't find anything else that needs fixing.
Pull branch, local doc build, review all three pages' changes as built locally.
00cb311 to
4febcea
Compare
|
@steveburnett please check again. |
steveburnett
left a comment
There was a problem hiding this comment.
LGTM! (docs)
Pull updated branch, new local doc build, looks good. Thanks!
hantangwangd
left a comment
There was a problem hiding this comment.
Thanks for the fix! LGTM overall, just have one little point for a quick discussion.
4febcea to
5d1cfb9
Compare
|
@steveburnett @hantangwangd can you both please take a pass again? |
hantangwangd
left a comment
There was a problem hiding this comment.
Thanks @pratyakshsharma, lgtm!
steveburnett
left a comment
There was a problem hiding this comment.
LGTM! (docs)
Pull branch, local doc build, looks good. Thanks!
Description
Fix spilling related docs
Motivation and Context
User facing docs related to spilling were out of date till now and had a typo. This PR fixes them.
Impact
NA
Test Plan
Contributor checklist
Release Notes
Please follow release notes guidelines and fill in the release notes below.