-
Notifications
You must be signed in to change notification settings - Fork 5.5k
fix(docs): Iceberg connector reference in ANALYZE docs #27041
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?
Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideUpdates the ANALYZE SQL documentation to reference the correct Iceberg connector, fixing an outdated or incorrect connector name in the docs. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
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 - I've found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `presto-docs/src/main/sphinx/sql/analyze.rst:23-25` </location>
<code_context>
Currently, this statement is only supported by the
-:ref:`Hive connector <hive_analyze>`.
+:ref:`Hive connector <hive_analyze>` and
+:doc:`Iceberg connector <../connector/iceberg>`.
Examples
</code_context>
<issue_to_address>
**suggestion (typo):** Consider adding "the" before "Iceberg connector" for parallel structure.
This will keep the phrasing grammatically consistent with the existing "the Hive connector" reference.
```suggestion
Currently, this statement is only supported by the
:ref:`Hive connector <hive_analyze>` and
:doc:`the Iceberg connector <../connector/iceberg>`.
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| Currently, this statement is only supported by the | ||
| :ref:`Hive connector <hive_analyze>`. | ||
| :ref:`Hive connector <hive_analyze>` and | ||
| :doc:`Iceberg connector <../connector/iceberg>`. |
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.
suggestion (typo): Consider adding "the" before "Iceberg connector" for parallel structure.
This will keep the phrasing grammatically consistent with the existing "the Hive connector" reference.
| Currently, this statement is only supported by the | |
| :ref:`Hive connector <hive_analyze>`. | |
| :ref:`Hive connector <hive_analyze>` and | |
| :doc:`Iceberg connector <../connector/iceberg>`. | |
| Currently, this statement is only supported by the | |
| :ref:`Hive connector <hive_analyze>` and | |
| :doc:`the Iceberg connector <../connector/iceberg>`. |
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.
Consider adding an example to the Iceberg connector doc that parallels the linked doc in the Hive connector.
|
Please edit the PR Description to set the release note as (including the row of three (`) above and below the text to format it as code) to pass the (not required to merge) failing test. |
|
Hi, quick question — is there a specific place in the Iceberg connector docs or code where the supported ANALYZE syntax/properties are documented? |
|
@Ishaan-2312 actually, Iceberg only supports table-level statistics analyzing and does not support table properties like To confirm this, you can refer to the Hive Connector's implementation of |
@hantangwangd, are you saying that we don't need an example in the Iceberg doc similar to the example linked to in the Hive doc? If that is what you mean, then I am okay with this PR as it is now and I'm willing to approve it. |
|
@steveburnett no, I was just responding to @Ishaan-2312's question. I thought that he wanted to confirm the exact usage of ANALYZE in the Iceberg connector. I also agree with your point — we'd better add a section in the Iceberg connector doc similar to the one in the linked doc in the Hive connector. This would be helpful considering we may later introduce partition-level ANALYZE supports as well. @Ishaan-2312 what's your opinion? |
|
Yeah I definitely think there should be a section in the iceberg connector doc for the working of the ANALYZE section and if you both @steveburnett and @hantangwangd also agree with this then I would be willing to do that work in the next PR. |
Description
Earlier the Iceberg connector did not get linked to a valid page, and this change fixes the issue by correctly mapping it to the Iceberg connector documentation page.
Motivation and Context
The previous documentation link for the Iceberg connector was invalid, which could confuse users trying to navigate to the correct connector documentation. This change ensures the link points to the correct and valid page.
Impact
Documentation-only change. No public API, user-facing behavior, or performance impact.
Test Plan
Verified the updated link points to the correct Iceberg connector documentation page.
Contributor checklist
Release Notes