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: vectorized builtinSleepSig #15193

Merged
merged 25 commits into from
Mar 11, 2020

Conversation

ziyi-yan
Copy link
Contributor

@ziyi-yan ziyi-yan commented Mar 6, 2020

UCP #12103

What problem does this PR solve?

Vectorized builtinSleepSig function from issue #12103

What is changed and how it works?

Implemented vecEvalInt method of builtinSleepSig

Check List

Tests

  • Unit test
  • Manual test (add detailed scripts or steps below)
mysql root@localhost:(none)> SELECT SLEEP(100);                                 
+------------+
| SLEEP(100) |
+------------+
| 0          |
+------------+
1 row in set
Time: 100.010s

@ziyi-yan ziyi-yan requested a review from a team as a code owner March 6, 2020 15:13
@sre-bot
Copy link
Contributor

sre-bot commented Mar 6, 2020

Thanks for your contribution. If your PR get merged, you will be rewarded 50 points.

@sre-bot sre-bot added the contribution This PR is from a community contributor. label Mar 6, 2020
@ghost ghost requested review from SunRunAway and XuHuaiyu and removed request for a team March 6, 2020 15:13
@codecov
Copy link

codecov bot commented Mar 7, 2020

Codecov Report

Merging #15193 into master will decrease coverage by 0.0522%.
The diff coverage is n/a.

@@               Coverage Diff               @@
##            master     #15193        +/-   ##
===============================================
- Coverage   80.464%   80.4118%   -0.0523%     
===============================================
  Files          503        503                
  Lines       133692     133361       -331     
===============================================
- Hits        107574     107238       -336     
- Misses       17710      17711         +1     
- Partials      8408       8412         +4

@ziyi-yan
Copy link
Contributor Author

ziyi-yan commented Mar 9, 2020

PTAL @SunRunAway @XuHuaiyu

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.

Hi, Thank you for the PR. I've left some comments.

And it would be nice if you add some unit tests.
Take a look at testEvaluatorSuite.TestSleep and consider adding TestSleepVectorized.
And there should be a test for comparison in builtin_miscellaneous_vec_test.go.

expression/builtin_miscellaneous_vec.go Show resolved Hide resolved
expression/builtin_miscellaneous_vec.go Outdated Show resolved Hide resolved
expression/builtin_miscellaneous_vec.go Outdated Show resolved Hide resolved
expression/builtin_miscellaneous_vec.go Outdated Show resolved Hide resolved
@qw4990 qw4990 self-requested a review March 9, 2020 08:06
@ziyi-yan
Copy link
Contributor Author

ziyi-yan commented Mar 9, 2020

Thanks for your suggestions @SunRunAway! I resolved them and PTAL.

@ziyi-yan ziyi-yan requested a review from SunRunAway March 9, 2020 09:09
expression/builtin_miscellaneous_vec.go Outdated Show resolved Hide resolved
expression/builtin_miscellaneous_vec.go Outdated Show resolved Hide resolved
expression/builtin_miscellaneous_vec.go Outdated Show resolved Hide resolved
expression/builtin_miscellaneous_vec.go Outdated Show resolved Hide resolved
@SunRunAway
Copy link
Contributor

Hi, please consider adding unit tests described here #15193 (review).
Thank you.

@ziyi-yan ziyi-yan force-pushed the vectorize-builtinSleepSig branch from d21aff7 to d0e31e0 Compare March 9, 2020 11:10
@ziyi-yan ziyi-yan force-pushed the vectorize-builtinSleepSig branch from d0e31e0 to 3a63474 Compare March 9, 2020 11:12
@ziyi-yan
Copy link
Contributor Author

ziyi-yan commented Mar 9, 2020

Finally, I got my own unit test passed. PTAL @SunRunAway 😁

expression/builtin_miscellaneous_vec.go Outdated Show resolved Hide resolved
expression/builtin_miscellaneous_vec.go Outdated Show resolved Hide resolved
expression/builtin_miscellaneous_vec.go Outdated Show resolved Hide resolved
expression/builtin_miscellaneous_vec.go Outdated Show resolved Hide resolved
expression/builtin_miscellaneous_vec.go Outdated Show resolved Hide resolved
expression/builtin_miscellaneous_vec.go Outdated Show resolved Hide resolved
expression/builtin_miscellaneous_vec_test.go Show resolved Hide resolved
expression/builtin_miscellaneous_vec_test.go Outdated Show resolved Hide resolved
@ziyi-yan
Copy link
Contributor Author

PTAL @SunRunAway @qw4990

@SunRunAway SunRunAway added status/LGT1 Indicates that a PR has LGTM 1. status/PTAL labels Mar 10, 2020
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

Copy link
Contributor

@qw4990 qw4990 left a comment

Choose a reason for hiding this comment

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

LGTM, WELL DONE!

@qw4990 qw4990 added status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2. labels Mar 11, 2020
@qw4990 qw4990 removed status/LGT1 Indicates that a PR has LGTM 1. status/PTAL labels Mar 11, 2020
@sre-bot
Copy link
Contributor

sre-bot commented Mar 11, 2020

/run-all-tests

@sre-bot sre-bot merged commit 6f6c415 into pingcap:master Mar 11, 2020
@sre-bot
Copy link
Contributor

sre-bot commented Mar 11, 2020

Team ziyi-yan complete task #12103 and get 50 score, currerent score 50.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/expression contribution This PR is from a community contributor. 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