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

expression: support sequence function #14731

Merged
merged 10 commits into from
Feb 17, 2020
Merged

Conversation

AilinKid
Copy link
Contributor

What problem does this PR solve?

mysql> create sequence seq
Query OK, 0 rows affected (0.00 sec)

mysql> select nextval(seq)
+--------------+
| nextval(seq) |
+--------------+
|            1 |
+--------------+
1 row in set (0.001 sec)

mysql> select lastval(seq)
+--------------+
| lastval(seq) |
+--------------+
|            1 |
+--------------+
1 row in set (0.001 sec)

mysql> select setval(seq, 100)
+------------------+
| setval(seq, 100) |
+------------------+
|              100 |
+------------------+
1 row in set (0.000 sec)

What is changed and how it works?

1: support sequence function nextval, lastval, setval.
2: implement sequenceState which could be used to cache sequence lastval.
3: introduce SequenceSchema interface to do the sequence schema check in expression package. (If we use information schema, that will cause import cycle problem)

Check List

Tests

  • Unit test
  • Integration test

Code changes

  • Has introduce SequenceSchema interface in util package.

Related changes

  • Need to update the documentation

Release note

  • support sequence function.

@AilinKid AilinKid requested a review from a team as a code owner February 11, 2020 09:11
@ghost ghost requested review from SunRunAway and XuHuaiyu and removed request for a team February 11, 2020 09:11
@AilinKid AilinKid added this to the v4.0.0-beta.1 milestone Feb 11, 2020
@AilinKid AilinKid removed the request for review from SunRunAway February 11, 2020 09:29
@AilinKid
Copy link
Contributor Author

/run-unit-test

@AilinKid
Copy link
Contributor Author

/build

1 similar comment
@AilinKid
Copy link
Contributor Author

/build

Copy link
Contributor

@crazycs520 crazycs520 left a comment

Choose a reason for hiding this comment

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

No test for those feature?

@AilinKid
Copy link
Contributor Author

AilinKid commented Feb 16, 2020

No test for those feature?

The sequence allcator is in next PR, the code here is the code in expression package.
The function logic test will be in the PR too (put it together will be > 500).
So it will be coming soon!

PTAL @crazycs520

@Deardrops Deardrops self-requested a review February 17, 2020 02:29
sessionctx/variable/sequence_state.go Outdated Show resolved Hide resolved
sessionctx/variable/sequence_state.go Outdated Show resolved Hide resolved
expression/builtin_info.go Outdated Show resolved Hide resolved
type SequenceState struct {
mu sync.Mutex
// latestValueMap caches the last value obtained by nextval() for each sequence id.
latestValueMap map[int64]int64
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider sync.Map and avoid use sync.Mutex manually?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe the map[int64]int64 is more directly to know what kind of the k&v it is.
Sync.map will store the interface, and transform it, maybe too heavy for int64.

@Deardrops
Copy link
Contributor

LGTM

@Deardrops Deardrops added the status/LGT1 Indicates that a PR has LGTM 1. label Feb 17, 2020
Copy link
Contributor

@crazycs520 crazycs520 left a comment

Choose a reason for hiding this comment

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

LGTM

@crazycs520
Copy link
Contributor

/run-all-tests

@AilinKid AilinKid added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Feb 17, 2020
@AilinKid
Copy link
Contributor Author

/merge

@sre-bot sre-bot added the status/can-merge Indicates a PR has been approved by a committer. label Feb 17, 2020
@sre-bot
Copy link
Contributor

sre-bot commented Feb 17, 2020

/run-all-tests

@sre-bot sre-bot merged commit a22ab8f into pingcap:master Feb 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/expression status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants