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: function `format` incompatible #9182

Merged
merged 14 commits into from Jan 31, 2019

Conversation

Projects
None yet
7 participants
@tender-boluo
Copy link
Contributor

tender-boluo commented Jan 25, 2019

What problem does this PR solve?

Fixed #8796

What is changed and how it works?

Changed the function evalString in expression/builtin_string.go

Check List

Tests

  • Unit test

tender-boluo added some commits Jan 25, 2019

expression: function `format` incompatible
function `format` incompatible with MySQL for round up
@CLAassistant

This comment has been minimized.

Copy link

CLAassistant commented Jan 25, 2019

CLA assistant check
All committers have signed the CLA.

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Jan 25, 2019

Codecov Report

Merging #9182 into master will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #9182      +/-   ##
==========================================
+ Coverage   67.22%   67.23%   +0.01%     
==========================================
  Files         371      371              
  Lines       77104    77142      +38     
==========================================
+ Hits        51833    51869      +36     
- Misses      20639    20640       +1     
- Partials     4632     4633       +1
Impacted Files Coverage Δ
expression/builtin_string.go 75.89% <100%> (+0.5%) ⬆️
util/filesort/filesort.go 75.54% <0%> (-0.95%) ⬇️
expression/schema.go 94.11% <0%> (-0.85%) ⬇️
executor/executor.go 66.89% <0%> (-0.14%) ⬇️
util/systimemon/systime_mon.go 100% <0%> (+20%) ⬆️

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 4c85822...efd4c06. Read the comment docs.

Show resolved Hide resolved expression/builtin_string.go Outdated
Show resolved Hide resolved expression/builtin_string.go Outdated
Show resolved Hide resolved expression/builtin_string.go Outdated
Show resolved Hide resolved expression/builtin_string.go Outdated
Show resolved Hide resolved expression/builtin_string.go
Show resolved Hide resolved expression/builtin_string.go Outdated
Show resolved Hide resolved expression/builtin_string.go
Show resolved Hide resolved expression/builtin_string.go Outdated
Show resolved Hide resolved expression/builtin_string.go Outdated
Show resolved Hide resolved expression/builtin_string.go Outdated
Show resolved Hide resolved expression/builtin_string.go Outdated
Show resolved Hide resolved expression/builtin_string.go Outdated
@eurekaka
Copy link
Contributor

eurekaka left a comment

LGTM

@xiekeyi98

This comment has been minimized.

Copy link
Member

xiekeyi98 commented Jan 30, 2019

/run-all-tests

Show resolved Hide resolved expression/builtin_string.go Outdated
Show resolved Hide resolved expression/builtin_string.go Outdated
Show resolved Hide resolved expression/builtin_string.go Outdated
Show resolved Hide resolved expression/builtin_string.go Outdated
Show resolved Hide resolved expression/builtin_string.go Outdated
Show resolved Hide resolved expression/builtin_string.go Outdated
@tender-boluo

This comment has been minimized.

Copy link
Contributor Author

tender-boluo commented Jan 31, 2019

All thanks for every talent PingCAPer help me make code better.

I am sorry that my technique is low.
And very thanks for you help me to improve my skill.

Show resolved Hide resolved expression/builtin_string.go Outdated
@zz-jason
Copy link
Member

zz-jason left a comment

LGTM

@zz-jason

This comment has been minimized.

Copy link
Member

zz-jason commented Jan 31, 2019

/run-all-tests

@zz-jason zz-jason merged commit 6398788 into pingcap:master Jan 31, 2019

5 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
idc-jenkins-ci-tidb/build Jenkins job succeeded.
Details
idc-jenkins-ci-tidb/check_dev Jenkins job succeeded.
Details
license/cla Contributor License Agreement is signed.
Details
@zz-jason

This comment has been minimized.

Copy link
Member

zz-jason commented Jan 31, 2019

@tender-boluo Thanks for your contribution! Could you help us to cherry pick this commit to release-2.1 and release-2.0?

@tender-boluo

This comment has been minimized.

Copy link
Contributor Author

tender-boluo commented Jan 31, 2019

Definitely yes!
It is my pleasure.

tender-boluo added a commit to tender-boluo/tidb that referenced this pull request Jan 31, 2019

@tender-boluo

This comment has been minimized.

Copy link
Contributor Author

tender-boluo commented Jan 31, 2019

I am sorry to you @zz-jason .
At beginning , I think this request is easy.
But when I cherry-pick to 2.0(2.1) , and solve the conflict, change the function parameters.
I find I can not make all tests passed.
The error is miscellaneous and I can not solve it at now.
I think it is a little hard to me and I have not enough time to do it.

Could you please assign it to others first, I think I will enjoy my holidays.
If others solve it, It is a good learning opportunity for me.
If it still need to solve this problem in the future, I am happy to solve it again when I have time to do it.

There is what I try, but cannot work well :(
At last but not least , happy Chinese new year to all.

@zz-jason

This comment has been minimized.

Copy link
Member

zz-jason commented Jan 31, 2019

@tender-boluo Okay, I'll try to cherry-pick this commit. Happy Chinese New Year! Have a nice holiday!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment