parser: add new analyze option `SAMPLERATE` #28961

winoros · 2021-10-19T11:23:28Z

What problem does this PR solve?

Issue Number: to #28960

Problem Summary:

Modify parser to support the new analyze option.

What is changed and how it works?

Since the new option needs float value, we not only modified the parser, but also modified the structure of the AnalyzeOption.

Check List

Tests

Unit test

Documentation

Release note

None

ti-chi-bot · 2021-10-19T11:23:30Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

time-and-fate
xuyifangreeneyes

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

The full list of commands accepted by this bot can be found here.

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

planner/core/planbuilder.go

chrysan · 2021-10-20T00:12:55Z

planner/core/planbuilder.go

+				return nil, err
+			}
+			if statsVer == statistics.Version1 && v > 0 {
+				return nil, errors.Errorf("Version 1's statistics doesn't support the SAMPLE RATE option, please set tidb_analyze_version to 2")


Any UT to check if these checks work?

Current we don't actually support the analyze process for sample rate case, will be added in next pr.

Suggested change

return nil, errors.Errorf("Version 1's statistics doesn't support the SAMPLE RATE option, please set tidb_analyze_version to 2")

return nil, errors.Errorf("Version 1's statistics doesn't support the SAMPLERATE option, please set tidb_analyze_version to 2.")

chrysan

LGTM

ti-chi-bot · 2021-10-20T07:45:10Z

@chrysan: Thanks for your review. The bot only counts approvals from reviewers and higher roles in list, but you're still welcome to leave your comments.

In response to this:

LGTM

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository.

kennytm · 2021-10-20T08:09:29Z

Since the new option needs float value, we not only modified the parser, but also modified the structure of the AnalyzeOption.

Technically you could use math.Float64bits/math.Float64frombits to support floats 🙃

winoros · 2021-10-20T08:55:03Z

Technically you could use math.Float64bits/math.Float64frombits to support floats 🙃

@kennytm
Oh you mean, we use it to store the rate as uint64 in the map[Option]uint64 and convert back to float64 when using it?

kennytm · 2021-10-20T09:31:30Z

@winoros yes. though this bit-twiddling hack won't work if in the future you need an option with string value.

time-and-fate

Maybe we should change analyze options from a map into a struct someday.

time-and-fate · 2021-10-21T09:15:27Z

planner/core/planbuilder.go

+				return nil, err
+			}
+			if statsVer == statistics.Version1 && v > 0 {
+				return nil, errors.Errorf("Version 1's statistics doesn't support the SAMPLE RATE option, please set tidb_analyze_version to 2")


Suggested change

return nil, errors.Errorf("Version 1's statistics doesn't support the SAMPLE RATE option, please set tidb_analyze_version to 2")

return nil, errors.Errorf("Version 1's statistics doesn't support the SAMPLERATE option, please set tidb_analyze_version to 2.")

time-and-fate · 2021-10-21T09:15:34Z

planner/core/planbuilder.go

+				return nil, errors.Errorf("Version 1's statistics doesn't support the SAMPLE RATE option, please set tidb_analyze_version to 2")
+			}
+			if v > analyzeOptionLimit[opt.Type].(float64) || v <= 0 {
+				return nil, errors.Errorf("value of analyze option %s should not larger than %f, and greater than 0", ast.AnalyzeOptionString[opt.Type], analyzeOptionLimit[opt.Type])


Suggested change

return nil, errors.Errorf("value of analyze option %s should not larger than %f, and greater than 0", ast.AnalyzeOptionString[opt.Type], analyzeOptionLimit[opt.Type])

return nil, errors.Errorf("value of analyze option %s should not be larger than %f, and should be greater than 0", ast.AnalyzeOptionString[opt.Type], analyzeOptionLimit[opt.Type])

time-and-fate · 2021-10-21T09:16:41Z

planner/core/planbuilder.go

+		switch opt.Type {
+		case ast.AnalyzeOptNumTopN:
+			v := datumValue.GetUint64()
+			if v > analyzeOptionLimit[opt.Type].(uint64) {
 				return nil, errors.Errorf("value of analyze option %s should not larger than %d", ast.AnalyzeOptionString[opt.Type], analyzeOptionLimit[opt.Type])


Suggested change

return nil, errors.Errorf("value of analyze option %s should not larger than %d", ast.AnalyzeOptionString[opt.Type], analyzeOptionLimit[opt.Type])

return nil, errors.Errorf("Value of analyze option %s should not be larger than %d", ast.AnalyzeOptionString[opt.Type], analyzeOptionLimit[opt.Type])

time-and-fate · 2021-10-21T09:19:16Z

planner/core/planbuilder.go

+			optMap[opt.Type] = v
+		default:
+			v := datumValue.GetUint64()
+			if v == 0 || v > analyzeOptionLimit[opt.Type].(uint64) {
 				return nil, errors.Errorf("value of analyze option %s should be positive and not larger than %d", ast.AnalyzeOptionString[opt.Type], analyzeOptionLimit[opt.Type])


Suggested change

return nil, errors.Errorf("value of analyze option %s should be positive and not larger than %d", ast.AnalyzeOptionString[opt.Type], analyzeOptionLimit[opt.Type])

return nil, errors.Errorf("Value of analyze option %s should be positive and not larger than %d.", ast.AnalyzeOptionString[opt.Type], analyzeOptionLimit[opt.Type])

time-and-fate · 2021-10-21T09:21:19Z

Also please resolve the conflicts.

time-and-fate · 2021-10-21T11:49:33Z

planner/core/planbuilder.go

+			}
+			limit := math.Float64frombits(analyzeOptionLimit[opt.Type])
+			if fVal <= 0 || fVal > limit {
+				return nil, errors.Errorf("Value of analyze option %s should not larger than %f, and should be greater than 0", limit, analyzeOptionLimit[opt.Type])


The arguments are wrong.

xuyifangreeneyes · 2021-10-22T04:46:52Z

planner/core/planbuilder.go

@@ -2065,6 +2068,7 @@ var analyzeOptionDefaultV2 = map[ast.AnalyzeOptionType]uint64{
 	ast.AnalyzeOptCMSketchWidth: 2048,
 	ast.AnalyzeOptCMSketchDepth: 5,
 	ast.AnalyzeOptNumSamples:    100000,
+	ast.AnalyzeOptSampleRate:    math.Float64bits(0),


The default value of SAMPLE RATE is 0. When users doesn't set either NUM SAMPLES or SAMPLE RATE, the default values would be used. In that case, will ANALYZE execute with 10000 SAMPLES?

The next pr will change the default config.

xuyifangreeneyes

LGTM

xuyifangreeneyes · 2021-10-22T05:03:05Z

planner/core/planbuilder.go

+			}
+			limit := math.Float64frombits(analyzeOptionLimit[opt.Type])
+			if fVal <= 0 || fVal > limit {
+				return nil, errors.Errorf("Value of analyze option %s should not larger than %f, and should be greater than 0", ast.AnalyzeOptionString[opt.Type], limit)


Suggested change

return nil, errors.Errorf("Value of analyze option %s should not larger than %f, and should be greater than 0", ast.AnalyzeOptionString[opt.Type], limit)

return nil, errors.Errorf("Value of analyze option %s should not be larger than %f, and should be greater than 0", ast.AnalyzeOptionString[opt.Type], limit)

winoros · 2021-10-23T06:07:20Z

/merge

ti-chi-bot · 2021-10-23T06:07:23Z

This pull request has been accepted and is ready to merge.

Commit hash: fb10d8b

tisonkun · 2021-10-24T05:09:45Z

@winoros the GitHub settings require at least 1 approving review which indicate a committer approve (green). You can ask for another committer to approve this PR.

winoros · 2021-10-25T05:58:12Z

/merge

ti-chi-bot · 2021-10-25T05:58:24Z

@winoros: Your PR was out of date, I have automatically updated it for you.

At the same time I will also trigger all tests for you:

/run-all-tests

If the CI test fails, you just re-trigger the test that failed and the bot will merge the PR for you after the CI passes.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository.

winoros added the sig/planner SIG: Planner label Oct 19, 2021

ti-chi-bot added release-note-none Denotes a PR that doesn't merit a release note. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Oct 19, 2021

winoros mentioned this pull request Oct 19, 2021

Tracking issue for supporting analyze with sample rate option #28960

Closed

5 tasks

chrysan suggested changes Oct 20, 2021

View reviewed changes

chrysan approved these changes Oct 20, 2021

View reviewed changes

time-and-fate reviewed Oct 21, 2021

View reviewed changes

ti-chi-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 21, 2021

parser: add new analyze option `SAMPLERATE

74c01cc

winoros force-pushed the sample-rate-parser branch from ec376b9 to 74c01cc Compare October 21, 2021 10:19

time-and-fate reviewed Oct 21, 2021

View reviewed changes

winoros added 2 commits October 21, 2021 20:03

address comments

39eee2b

Merge branch 'master' into sample-rate-parser

e5f2959

ti-chi-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 21, 2021

time-and-fate approved these changes Oct 21, 2021

View reviewed changes

ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Oct 21, 2021

address comments

fb10d8b

xuyifangreeneyes reviewed Oct 22, 2021

View reviewed changes

xuyifangreeneyes approved these changes Oct 22, 2021

View reviewed changes

ti-chi-bot added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Oct 22, 2021

ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Oct 23, 2021

ti-chi-bot added 2 commits October 23, 2021 14:07

Merge branch 'master' into sample-rate-parser

8ed6f8a

Merge branch 'master' into sample-rate-parser

3f75b74

ti-chi-bot added 3 commits October 25, 2021 10:44

Merge branch 'master' into sample-rate-parser

5f468c6

Merge branch 'master' into sample-rate-parser

d25c3dd

Merge branch 'master' into sample-rate-parser

9998dc7

qw4990 approved these changes Oct 25, 2021

View reviewed changes

Merge branch 'master' into sample-rate-parser

754c583

Merge branch 'master' into sample-rate-parser

0777002

ti-chi-bot merged commit b332a08 into pingcap:master Oct 25, 2021

winoros deleted the sample-rate-parser branch October 25, 2021 09:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

parser: add new analyze option `SAMPLERATE` #28961

parser: add new analyze option `SAMPLERATE` #28961

winoros commented Oct 19, 2021

ti-chi-bot commented Oct 19, 2021 •

edited

Loading

chrysan Oct 20, 2021

winoros Oct 20, 2021

time-and-fate Oct 21, 2021

chrysan left a comment

ti-chi-bot commented Oct 20, 2021

kennytm commented Oct 20, 2021

winoros commented Oct 20, 2021 •

edited

Loading

kennytm commented Oct 20, 2021

time-and-fate left a comment

time-and-fate Oct 21, 2021

time-and-fate Oct 21, 2021

time-and-fate Oct 21, 2021

time-and-fate Oct 21, 2021

time-and-fate commented Oct 21, 2021

time-and-fate Oct 21, 2021

xuyifangreeneyes Oct 22, 2021

xuyifangreeneyes Oct 22, 2021

xuyifangreeneyes left a comment

xuyifangreeneyes Oct 22, 2021

winoros commented Oct 23, 2021

ti-chi-bot commented Oct 23, 2021

tisonkun commented Oct 24, 2021

winoros commented Oct 25, 2021

ti-chi-bot commented Oct 25, 2021

	return nil, errors.Errorf("Version 1's statistics doesn't support the SAMPLE RATE option, please set tidb_analyze_version to 2")
	return nil, errors.Errorf("Version 1's statistics doesn't support the SAMPLERATE option, please set tidb_analyze_version to 2.")

	return nil, errors.Errorf("value of analyze option %s should not larger than %f, and greater than 0", ast.AnalyzeOptionString[opt.Type], analyzeOptionLimit[opt.Type])
	return nil, errors.Errorf("value of analyze option %s should not be larger than %f, and should be greater than 0", ast.AnalyzeOptionString[opt.Type], analyzeOptionLimit[opt.Type])

parser: add new analyze option SAMPLERATE #28961

parser: add new analyze option SAMPLERATE #28961

Conversation

winoros commented Oct 19, 2021

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

ti-chi-bot commented Oct 19, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrysan left a comment

Choose a reason for hiding this comment

ti-chi-bot commented Oct 20, 2021

kennytm commented Oct 20, 2021

winoros commented Oct 20, 2021 • edited Loading

kennytm commented Oct 20, 2021

time-and-fate left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

time-and-fate commented Oct 21, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xuyifangreeneyes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

winoros commented Oct 23, 2021

ti-chi-bot commented Oct 23, 2021

tisonkun commented Oct 24, 2021

winoros commented Oct 25, 2021

ti-chi-bot commented Oct 25, 2021

parser: add new analyze option `SAMPLERATE` #28961

parser: add new analyze option `SAMPLERATE` #28961

ti-chi-bot commented Oct 19, 2021 •

edited

Loading

winoros commented Oct 20, 2021 •

edited

Loading