docs: Fix minor documentation issues#26866
Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideThis PR fixes several Sphinx/docutils warnings in the Presto documentation by correcting code block language annotations, string quoting in Java-labeled blocks, and heading underline lengths/levels so that docs build cleanly without highlighting or title-structure errors. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
4bfdccd to
26395c1
Compare
There was a problem hiding this comment.
Hey - I've found 2 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `presto-docs/src/main/sphinx/presto_cpp/features.rst:56` </location>
<code_context>
* GET: /v1/operation/server/clearCache?type=memory: It clears the memory cache on worker node. Here is an example:
- .. sourcecode:: http
</code_context>
<issue_to_address>
**suggestion (typo):** Consider adding "the" before "worker node" for grammatical correctness.
Change "It clears the memory cache on worker node." to "It clears the memory cache on the worker node." for better readability.
Suggested implementation:
```
* GET: /v1/operation/server/clearCache?type=memory: It clears the memory cache on the worker node. Here is an example:
```
```
* GET: /v1/operation/server/clearCache?type=ssd: It clears the ssd cache on the worker node. Here is an example:
```
</issue_to_address>
### Comment 2
<location> `presto-docs/src/main/sphinx/presto_cpp/features.rst:64` </location>
<code_context>
* GET: /v1/operation/server/clearCache?type=ssd: It clears the ssd cache on worker node. Here is an example:
- .. sourcecode:: http
</code_context>
<issue_to_address>
**suggestion (typo):** Consider capitalizing "SSD" and adding "the" before "worker node".
You could rephrase this sentence to: "It clears the SSD cache on the worker node."
```suggestion
* GET: /v1/operation/server/clearCache?type=ssd: It clears the SSD cache on the worker node. Here is an example:
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
26395c1 to
a46b1ae
Compare
|
|
||
| Stale Data Handling | ||
| """"""""""""""""""" | ||
| ^^^^^^^^^^^^^^^^^^^ |
There was a problem hiding this comment.
One more thing: I suspect this is intentionally meant to be a sub-level. If so, since the document previously defined ~~~~~~ as the sub-level of ^^^^^^, should we use ~~~~~~ here for level consistency? cc: @tdcmeehan
There was a problem hiding this comment.
One more thing: I suspect this is intentionally meant to be a sub-level. If so, since the document previously defined
~~~~~~as the sub-level of^^^^^^, should we use~~~~~~here for level consistency? cc: @tdcmeehan
@hantangwangd, I think that this Stale Data Handling is a subheading of Materialized Views so I think this change is okay. On the other hand, if Stale Data Handling is intended as a subheading of Freshness and Refresh, then I would agree with you.
There was a problem hiding this comment.
Yes, I had intended this to be a sub-heading for Materialized Views, however if we think this should go under Freshness and Refresh I am fine with that too.
There was a problem hiding this comment.
Materialized Views
Thanks @tdcmeehan! This PR sets the Stale Data Handling heading as a subheading of Materialized Views. I don't see a strong reason in the text that this should be changed to be a subheading of Freshness and Refresh.
There was a problem hiding this comment.
Thanks for your feedback and confirmation @tdcmeehan, @steveburnett. I'm completely okey with treating it as a subheading of Materialized Views.
steveburnett
left a comment
There was a problem hiding this comment.
Thanks for the doc! Just Commenting, not Approval today, until we get clarification of the Stale Data Handling heading level.
|
|
||
| Stale Data Handling | ||
| """"""""""""""""""" | ||
| ^^^^^^^^^^^^^^^^^^^ |
There was a problem hiding this comment.
One more thing: I suspect this is intentionally meant to be a sub-level. If so, since the document previously defined
~~~~~~as the sub-level of^^^^^^, should we use~~~~~~here for level consistency? cc: @tdcmeehan
@hantangwangd, I think that this Stale Data Handling is a subheading of Materialized Views so I think this change is okay. On the other hand, if Stale Data Handling is intended as a subheading of Freshness and Refresh, then I would agree with you.
steveburnett
left a comment
There was a problem hiding this comment.
Based on @tdcmeehan's comment, I am approving this PR.
## Description
The PR fixes some minor issues in the documentation.
## Motivation and Context
The following issues have been fixed:
1. Use `sql` lexer for `code-block` instead of `java`.
```
./presto/presto-docs/src/main/sphinx/develop/procedures.rst:155: WARNING: Lexing literal_block "call iceberg.system.expire_snapshots('default', 'test_table');\ncall hive.system.invalidate_directory_list_cache();\n......" as "java" resulted in an error at token: "'". Retrying in relaxed mode. [misc.highlighting_failure]
```
2. Code block `http` was used for `shell` commands.
```
./presto/presto-docs/src/main/sphinx/presto_cpp/features.rst:58: WARNING: Lexing literal_block 'curl -X GET "http://localhost:7777/v1/operation/server/clearCache?type=memory"\n\nCleared memory cache' as "http" resulted in an error at token: 'c'. Retrying in relaxed mode. [misc.highlighting_failure]
./presto/presto-docs/src/main/sphinx/presto_cpp/features.rst:66: WARNING: Lexing literal_block 'curl -X GET "http://localhost:7777/v1/operation/server/clearCache?type=ssd"\n\nCleared ssd cache' as "http" resulted in an error at token: 'c'. Retrying in relaxed mode. [misc.highlighting_failure]
./presto/presto-docs/src/main/sphinx/presto_cpp/features.rst:74: WARNING: Lexing literal_block 'curl -X GET "http://localhost:7777/v1/operation/server/writeSsd"\n\nSucceeded write ssd cache' as "http" resulted in an error at token: 'c'. Retrying in relaxed mode. [misc.highlighting_failure]
```
3. Too short underline.
```
./presto/presto-docs/src/main/sphinx/plugin/native-sidecar-plugin.rst:110: WARNING: Title underline too short.
Expression optimizer
----------------- [docutils]
```
4. Title underline for `Materialized Views` sub-section.
```
./presto/presto-docs/src/main/sphinx/connector/iceberg.rst:2390: CRITICAL: Title level inconsistent:
Stale Data Handling
""""""""""""""""""" [docutils]
```
## Impact
`None`
## Test Plan
Build documentation and check there are no related warning mentioned in
the description:
```shell
presto-docs/build
```
## Contributor checklist
- [ ] Please make sure your submission complies with our [contributing
guide](https://github.com/prestodb/presto/blob/master/CONTRIBUTING.md),
in particular [code
style](https://github.com/prestodb/presto/blob/master/CONTRIBUTING.md#code-style)
and [commit
standards](https://github.com/prestodb/presto/blob/master/CONTRIBUTING.md#commit-standards).
- [ ] PR description addresses the issue accurately and concisely. If
the change is non-trivial, a GitHub Issue is referenced.
- [ ] Documented new properties (with its default value), SQL syntax,
functions, or other functionality.
- [ ] If release notes are required, they follow the [release notes
guidelines](https://github.com/prestodb/presto/wiki/Release-Notes-Guidelines).
- [ ] Adequate tests were added if applicable.
- [ ] CI passed.
- [ ] If adding new dependencies, verified they have an [OpenSSF
Scorecard](https://securityscorecards.dev/#the-checks) score of 5.0 or
higher (or obtained explicit TSC approval for lower scores).
## Release Notes
```
== NO RELEASE NOTE ==
```
Description
The PR fixes some minor issues in the documentation.
Motivation and Context
The following issues have been fixed:
sqllexer forcode-blockinstead ofjava.httpwas used forshellcommands.Materialized Viewssub-section.Impact
NoneTest Plan
Build documentation and check there are no related warning mentioned in the description:
Contributor checklist
Release Notes