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: refactor builtin function RAND to be compatible with MySQL #9387

Merged
merged 4 commits into from
Feb 22, 2019

Conversation

XuHuaiyu
Copy link
Contributor

@XuHuaiyu XuHuaiyu commented Feb 21, 2019

What problem does this PR solve?

  1. builtinRandSig/builtinRandWithSeedSig.randGen may cause data race when Project executing parallelly.
  2. Results of rand(N) is not compatible with MySQL as expression: refactor builtin function RAND to be compatible with MySQL #9387 (comment) mentioned.

What is changed and how it works?

  1. Eager evaluate seed for rand() in getFunction.
  2. Check whether args[0] is a constant during getFunction:
    If so, return builtinRandSig rather than builtinRandWithSeedSig;
    If not, evaluate seed each time Rand invoked.

Check List

Tests

integration tests

Code changes

  • Has exported function/method change
  • Has exported variable/fields change

Side effects

N/A

Related changes

  • Need to cherry-pick to the release branch

@XuHuaiyu
Copy link
Contributor Author

/run-all-tests

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.

can we make a copy for each parallel Project worker?

@codecov-io
Copy link

codecov-io commented Feb 21, 2019

Codecov Report

Merging #9387 into master will increase coverage by <.01%.
The diff coverage is 58.82%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #9387      +/-   ##
==========================================
+ Coverage   67.17%   67.17%   +<.01%     
==========================================
  Files         373      373              
  Lines       78292    78298       +6     
==========================================
+ Hits        52590    52599       +9     
+ Misses      20992    20987       -5     
- Partials     4710     4712       +2
Impacted Files Coverage Δ
expression/builtin_math.go 70.16% <58.82%> (+0.63%) ⬆️
store/tikv/scan.go 73.94% <0%> (-3.37%) ⬇️
expression/schema.go 94.53% <0%> (+0.78%) ⬆️
infoschema/infoschema.go 77.63% <0%> (+1.31%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 79a88a0...b865280. Read the comment docs.

expression/builtin_math.go Outdated Show resolved Hide resolved
@XuHuaiyu
Copy link
Contributor Author

@zz-jason
Yes, we can, thus we need to add a Clone function for *expression.EvaluatorSuite. I'll modify it.

@XuHuaiyu XuHuaiyu changed the title expression: add a mutex in builtinRandSig to avoid data race in parallel Proj executor, expression: 1. make rand(N) be compatible with MySQL 2. Add a Clone func for EvaluatorSuite Feb 21, 2019
@XuHuaiyu XuHuaiyu requested review from zz-jason and alivxxx February 21, 2019 05:35
expression/builtin_math.go Outdated Show resolved Hide resolved
expression/builtin_math.go Outdated Show resolved Hide resolved
@XuHuaiyu XuHuaiyu changed the title executor, expression: 1. make rand(N) be compatible with MySQL 2. Add a Clone func for EvaluatorSuite executor, expression: 1. make rand(N) be compatible with MySQL 2. Evaluate seed for rand() when getFunction Feb 22, 2019
@XuHuaiyu XuHuaiyu requested a review from alivxxx February 22, 2019 07:23
@alivxxx
Copy link
Contributor

alivxxx commented Feb 22, 2019

LGTM

@alivxxx alivxxx added the status/LGT1 Indicates that a PR has LGTM 1. label Feb 22, 2019
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

@zz-jason zz-jason changed the title executor, expression: 1. make rand(N) be compatible with MySQL 2. Evaluate seed for rand() when getFunction expression: refactor builtin function RAND to be compatible with MySQL Feb 22, 2019
@zz-jason zz-jason added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Feb 22, 2019
@zz-jason
Copy link
Member

/run-all-tests

@ciscoxll
Copy link
Contributor

/run-all-tests

@zz-jason zz-jason merged commit 8d4db9f into pingcap:master Feb 22, 2019
@zz-jason
Copy link
Member

@XuHuaiyu Please cherry pick this commit to release 2.1 and release 2.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/expression status/LGT2 Indicates that a PR has LGTM 2. type/bugfix This PR fixes a bug.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants