Skip to content
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

performance tuning: change SQL tuning TOC #3858

Merged
merged 6 commits into from Sep 29, 2020
Merged

performance tuning: change SQL tuning TOC #3858

merged 6 commits into from Sep 29, 2020

Conversation

ghost
Copy link

@ghost ghost commented Sep 9, 2020

What is changed, added or deleted? (Required)

This changes the SQL tuning TOC to follow the same structure as in the overview (which I've improved to make clearer - mentioning that SQL is declarative etc).

I am trying to make this change small, with additional content under SQL Tuning > Understanding the Query Execution Plan to follow in separate PRs.

Which TiDB version(s) do your changes apply to? (Required)

  • master (the latest development version)
  • v4.0 (TiDB 4.0 versions)
  • v3.1 (TiDB 3.1 versions)
  • v3.0 (TiDB 3.0 versions)
  • v2.1 (TiDB 2.1 versions)

What is the related PR or file link(s)?

  • This PR is translated from:
  • Other reference link(s):

Do your changes match any of the following descriptions?

  • Delete files (renamed file. Redirects added)
  • Change aliases
  • Have version specific changes
  • Might cause conflicts

@ghost ghost added the needs-cherry-pick-4.0 label Sep 9, 2020
@ghost ghost requested a review from zz-jason September 9, 2020 16:02
Copy link
Member

@zz-jason zz-jason left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ti-srebot ti-srebot added the status/LGT1 Indicates that a PR has LGTM 1. label Sep 9, 2020
@TomShawn TomShawn self-assigned this Sep 10, 2020
@TomShawn TomShawn added size/small Changes of a small size. status/PTAL This PR is ready for reviewing. translation/doing This PR's assignee is translating this PR. labels Sep 10, 2020
TOC.md Outdated Show resolved Hide resolved
Copy link
Contributor

@SunRunAway SunRunAway left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ti-srebot
Copy link
Contributor

@SunRunAway,Thanks for your review. The bot only counts LGTMs from Reviewers and higher roles, but you're still welcome to leave your comments.See the corresponding SIG page for more information. Related SIG: docs(slack).

@SunRunAway
Copy link
Contributor

@lilin90 PTAL

@ti-srebot
Copy link
Contributor

@zz-jason, @SunRunAway, @kissmydb, @lilin90, PTAL.

1 similar comment
@ti-srebot
Copy link
Contributor

@zz-jason, @SunRunAway, @kissmydb, @lilin90, PTAL.

Copy link
Member

@lilin90 lilin90 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kissmydb What do you think of the changes? PTAL~ Thanks!

Comment on lines +114 to +115
+ Understanding the Query Execution Plan
+ [Overview](/explain-overview.md)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Since we don't have multiple topics about query execution plan now, how about keeping one line?
  • Is it a must to change the file name from query-execution-plan.md to explain-overview.md? Because we'll try to avoid changing file names unless it's really necessary, to make it easy to maintain documentation (Let's avoid using too many aliases and try to keep the URL stable if possible).
Suggested change
+ Understanding the Query Execution Plan
+ [Overview](/explain-overview.md)
+ [Understanding the Query Execution Plan](/explain-overview.md)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer to keep it as is. I have PRs for the added content being reviewed in #3862 and #3861 - and will add a third one later today.

Copy link
Member

@lilin90 lilin90 Sep 16, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it. Just to confirm: by "keep it as is", I think you mean keeping your changes, right? And the newly added content in two other PRs will be put under "Understanding the Query Execution Plan"?

Copy link
Author

@ghost ghost Sep 16, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is correct. The TOC I proposed to the SQL team is as follows:

Understanding the Query Execution Plan
  Overview
  Explain Walkthough
  Indexes
  Joins
  Subqueries
  Views
  Partitions

@ghost ghost mentioned this pull request Sep 17, 2020
9 tasks
@ghost ghost mentioned this pull request Sep 17, 2020
9 tasks
@ti-srebot ti-srebot mentioned this pull request Sep 17, 2020
9 tasks
This was referenced Sep 17, 2020
@ti-srebot ti-srebot added the status/LGT2 Indicates that a PR has LGTM 2. label Sep 29, 2020
@lilin90 lilin90 added size/medium Changes of a medium size. and removed size/small Changes of a small size. labels Sep 29, 2020
@lilin90
Copy link
Member

lilin90 commented Sep 29, 2020

/merge

@ti-srebot ti-srebot added the status/can-merge Indicates a PR has been approved by a committer. label Sep 29, 2020
@ti-srebot
Copy link
Contributor

/run-all-tests

@ti-srebot ti-srebot merged commit ebbdd50 into pingcap:master Sep 29, 2020
ti-srebot pushed a commit to ti-srebot/docs that referenced this pull request Sep 29, 2020
Signed-off-by: ti-srebot <ti-srebot@pingcap.com>
@ti-srebot
Copy link
Contributor

cherry pick to release-4.0 in PR #3988

ghost pushed a commit that referenced this pull request Sep 29, 2020
Signed-off-by: ti-srebot <ti-srebot@pingcap.com>

Co-authored-by: Null not nil <67764674+nullnotnil@users.noreply.github.com>
@TomShawn TomShawn added translation/done This PR has been translated from English into Chinese and updated to pingcap/docs-cn in a PR. and removed translation/doing This PR's assignee is translating this PR. labels Oct 12, 2020
@ti-srebot ti-srebot mentioned this pull request Oct 13, 2020
9 tasks
@ti-srebot ti-srebot mentioned this pull request Nov 12, 2020
9 tasks
@ghost ghost mentioned this pull request Nov 12, 2020
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/medium Changes of a medium size. status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2. status/PTAL This PR is ready for reviewing. translation/done This PR has been translated from English into Chinese and updated to pingcap/docs-cn in a PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants