lexer: replace version comments with feature-ids mechanism #777

tangenta · 2020-03-16T15:50:53Z

What problem does this PR solve?

Executable special comments like /*T!40000 xxx */ are not suitable for TiDB. Due to the nature of branch-release, new features can be cherry-picked to different major versions TiDB. As a result, a TiDB with a higher version may not support the features in a TiDB with a lower version.

To solve this problem, this PR gives a mechanism to manage the features that need to support forward compatibility:

For the features that need to be downgrade compatible, assign a unique identifier to each of them, called feature-id.
The parsers in different release branches maintain their own featureMap, which contains a set of currently supported feature-ids.
The pattern /*T![feature_id] xxx */ is used to wrap the key part during logical synchronization.
To decide whether a statement with special comments should be parsed, parser can search feature_id in featureMap.
In rare cases, a comment can have multiple feature ids like /*T![feature_id1,feature_id2,...] xxx */. It can only be parsed if all feature IDs in the comment can be found in the featureMap.

What is changed and how it works?

Add scanFeatureIDs() to scan feature ids in special comments.

Check List

Tests

Unit test
Integration test

Code changes

Has exported variable/fields change

Side effects

Related changes

Need to cherry-pick to the release branch
Need to update the documentation

codecov · 2020-03-16T15:54:44Z

Codecov Report

Merging #777 into master will increase coverage by 0.05%.
The diff coverage is 92.45%.

@@            Coverage Diff             @@
##           master     #777      +/-   ##
==========================================
+ Coverage   78.05%   78.11%   +0.05%     
==========================================
  Files          40       40              
  Lines       14669    14703      +34     
==========================================
+ Hits        11450    11485      +35     
+ Misses       2537     2536       -1     
  Partials      682      682

lexer.go

lexer_test.go

misc.go

tiancaiamao · 2020-03-18T03:03:41Z

The parser should not care about the detail of what /*T!40000 xxx */ means.
It just parses that value as an opaque data, and return it to TiDB.

If we change to FeaturesMap that means the code bind with the parser.
It's no longer a black box for the parser.

AilinKid

Rest LGTM

AilinKid · 2020-03-23T03:17:49Z

lexer.go

+
+func (s *Scanner) scanFeatureIDs() (featureIDs []string) {
+	pos := s.r.pos()
+	const init, expectAlpha, alpha = 0, 1, 2


state name should more clear?

tangenta · 2020-03-23T03:53:14Z

@tiancaiamao Do you have any suggestions to deal with the problem mentioned in description?

How about putting the FeaturesMap into a part of the parser driver? Let users decide what should be parsed.

tiancaiamao · 2020-03-23T06:08:44Z

Do you have any suggestions to deal with the problem mentioned in the description?

The idea sounds good. I just have concerns about the compatibility issue.
When the TiDB uses this new feature, what happens if the customer rolling updates from an old cluster?

How about putting the FeaturesMap into a part of the parser driver? Let users decide what should be parsed.

Much better.

tangenta · 2020-03-23T07:13:14Z

IMO, it doesn't affect rolling updates.

Currently, only auto_random feature is using the special comments like /*T!40000 xxx */. I think compatibility is not the problem, because it is an experimental feature :)

In later TiDB versions, the syntax or semantics of AUTO_RANDOM might change.
https://pingcap.com/docs/dev/reference/sql/attributes/auto-random/

kennytm · 2020-03-23T07:53:52Z

in the worst case we could support both /*T!12345 ... */ and /*T![abc,def] ... */ since the two syntax are mutually exclusive.

misc.go

tiancaiamao · 2020-03-23T13:27:05Z

If none of our users is actually using /*T!40000 xxx */, it doesn't affect rolling updates.

lexer.go

tiancaiamao · 2020-03-25T02:49:00Z

lexer.go

-		if commentVersion <= CommentCodeCurrentVersion {
+		// in '/*T!', try to match the pattern '/*T![feature1,feature2,...]'.
+		features := s.scanFeatureIDs()
+		if SpecialCommentsController.ContainsAll(features) {
 			s.inBangComment = true
 			return s.scan()
 		}


What happens if the feature id is not included in the list?

It will parse it as normal comments. For now, the behavior is:
FeatureMap: {"xxx", "yyy"}

/*T![xxx,yyy] part_of_sql */ # all the feature ids matched /*T![xxx,zzz] part_of_sql */ # one feature id mismatched /*T![xxx part_of_sql */ # no feature id detected /*T!part_of_sql */ # no feature id detected

are equivalant to

part_of_sql [xxx part_of_sql part_of_sql

respectively.

tiancaiamao · 2020-03-27T05:03:56Z

LGTM
Please fix the integration test.

tangenta · 2020-03-30T03:39:29Z

This PR should be merged together with pingcap/tidb#15412.

* lexer: replace version comments with feature-ids mechanism * tests: retain the original version digit test * fix scanVersionDigits * add SpecialCommentsController to decide what comments can be parsed * only ignore comments with unsupported feature-id * remove debug log

tangenta added type/enhancement New feature or request type/compatibility labels Mar 16, 2020

tangenta requested a review from a team March 16, 2020 15:50

ghost requested review from kennytm and removed request for a team March 16, 2020 15:50

tangenta mentioned this pull request Mar 16, 2020

parser: apply feature-ids special comments to auto_random pingcap/tidb#15412

Merged

tangenta added the status/WIP label Mar 16, 2020

kennytm reviewed Mar 16, 2020

View reviewed changes

lexer.go Outdated Show resolved Hide resolved

lexer.go Outdated Show resolved Hide resolved

lexer.go Outdated Show resolved Hide resolved

lexer_test.go Outdated Show resolved Hide resolved

misc.go Outdated Show resolved Hide resolved

misc.go Outdated Show resolved Hide resolved

AilinKid requested review from AilinKid and tiancaiamao March 17, 2020 06:18

tangenta force-pushed the feature-id branch from 7cdb959 to c623e42 Compare March 19, 2020 11:05

AilinKid reviewed Mar 23, 2020

View reviewed changes

tiancaiamao reviewed Mar 23, 2020

View reviewed changes

misc.go Outdated Show resolved Hide resolved

tangenta added 4 commits March 24, 2020 12:59

lexer: replace version comments with feature-ids mechanism

020dbbc

tests: retain the original version digit test

891055a

fix scanVersionDigits

0993030

add SpecialCommentsController to decide what comments can be parsed

87f486e

tangenta force-pushed the feature-id branch from c528778 to 87f486e Compare March 24, 2020 05:01

kennytm reviewed Mar 24, 2020

View reviewed changes

lexer.go Outdated Show resolved Hide resolved

only ignore comments with unsupported feature-id

8db78c4

tiancaiamao reviewed Mar 25, 2020

View reviewed changes

remove debug log

aa9094e

Merge branch 'master' into feature-id

8e5b208

kennytm approved these changes Mar 30, 2020

View reviewed changes

tangenta removed the status/WIP label Mar 30, 2020

kennytm merged commit 8dce7a4 into pingcap:master Mar 31, 2020

This was referenced Apr 8, 2020

parser: apply feature-ids special comments to auto_random (#15412) pingcap/tidb#16155

Merged

parser: apply feature-ids special comments to auto_random (#15412) pingcap/tidb#16157

Merged

tangenta mentioned this pull request Apr 21, 2020

lexer: replace version comments with feature-ids mechanism (#777) #819

Merged

AilinKid mentioned this pull request Apr 24, 2020

lexer: replace version comments with feature-ids mechanism (#777) #829

Closed

bb7133 pushed a commit that referenced this pull request Apr 24, 2020

lexer: replace version comments with feature-ids mechanism (#777) (#819)

867e000

tangenta added a commit to tangenta/parser that referenced this pull request Apr 28, 2020

lexer: replace version comments with feature-ids mechanism (pingcap#777)

47aee32

tangenta mentioned this pull request Apr 28, 2020

lexer: replace version comments with feature-ids mechanism (#777) #838

Merged

bb7133 pushed a commit that referenced this pull request Apr 28, 2020

lexer: replace version comments with feature-ids mechanism (#777) (#838)

7e52571

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

lexer: replace version comments with feature-ids mechanism #777

lexer: replace version comments with feature-ids mechanism #777

tangenta commented Mar 16, 2020

codecov bot commented Mar 16, 2020 •

edited

Loading

tiancaiamao commented Mar 18, 2020

AilinKid left a comment

AilinKid Mar 23, 2020

tangenta commented Mar 23, 2020

tiancaiamao commented Mar 23, 2020

tangenta commented Mar 23, 2020

kennytm commented Mar 23, 2020

tiancaiamao commented Mar 23, 2020

tiancaiamao Mar 25, 2020

tangenta Mar 25, 2020 •

edited

Loading

tiancaiamao commented Mar 27, 2020

tangenta commented Mar 30, 2020

lexer: replace version comments with feature-ids mechanism #777

lexer: replace version comments with feature-ids mechanism #777

Conversation

tangenta commented Mar 16, 2020

What problem does this PR solve?

What is changed and how it works?

Check List

codecov bot commented Mar 16, 2020 • edited Loading

Codecov Report

tiancaiamao commented Mar 18, 2020

AilinKid left a comment

Choose a reason for hiding this comment

AilinKid Mar 23, 2020

Choose a reason for hiding this comment

tangenta commented Mar 23, 2020

tiancaiamao commented Mar 23, 2020

tangenta commented Mar 23, 2020

kennytm commented Mar 23, 2020

tiancaiamao commented Mar 23, 2020

tiancaiamao Mar 25, 2020

Choose a reason for hiding this comment

tangenta Mar 25, 2020 • edited Loading

Choose a reason for hiding this comment

tiancaiamao commented Mar 27, 2020

tangenta commented Mar 30, 2020

codecov bot commented Mar 16, 2020 •

edited

Loading

tangenta Mar 25, 2020 •

edited

Loading