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.

Sign up for GitHub

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

Jump to bottom

expression: add warnings for logarithm functions #18668

Merged
merged 8 commits into from
Aug 19, 2020
Merged

expression: add warnings for logarithm functions #18668

merged 8 commits into from
Aug 19, 2020

Conversation

dyzsr
Copy link
Contributor

@dyzsr dyzsr commented Jul 17, 2020

What problem does this PR solve?

Issue Number: close #17992

Problem Summary: No warning for invalid usage of function LN()

What is changed and how it works?

What's Changed:

  • Add warnings when logarithm functions are given invalid arguments
  • Add error code (3020) for invalid argument for logarithm

Check List

Tests

  • Unit test

Release note

  • bug fix for logarithm functions

@dyzsr dyzsr requested a review from a team as a code owner July 17, 2020 08:00
@dyzsr dyzsr requested review from wshwsh12 and removed request for a team July 17, 2020 08:00
@dyzsr dyzsr changed the title expression: add warnings for logarithm functions expression: add warnings for logarithm functions (#17992) Jul 17, 2020
@dyzsr
expression: add warnings for Log function
d1b7845 
+ add new errcode: 3020
+ add warnings when logarithm functions are given invalid arguments
@dyzsr
expression: correct test files
5c46ab0 
+ file: builtin_math_test.go
@wshwsh12 wshwsh12 changed the title expression: add warnings for logarithm functions (#17992) expression: add warnings for logarithm functions Jul 24, 2020
wshwsh12
wshwsh12 reviewed Jul 24, 2020
View reviewed changes
Copy link
Contributor

@wshwsh12 wshwsh12 left a comment

Choose a reason for hiding this comment

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

lgtm

@ti-srebot ti-srebot added the status/LGT1 Indicates that a PR has LGTM 1. label Jul 24, 2020
@wshwsh12 wshwsh12 requested a review from a team July 24, 2020 09:48
@codecov
Copy link

codecov bot commented Jul 24, 2020 •
edited
Loading

Codecov Report

Merging #18668 into master will not change coverage.
The diff coverage is n/a.

@@             Coverage Diff             @@
##             master     #18668   +/-   ##
===========================================
  Coverage   79.2618%   79.2618%           
===========================================
  Files           546        546           
  Lines        148268     148268           
===========================================
  Hits         117520     117520           
  Misses        21313      21313           
  Partials       9435       9435

zz-jason
zz-jason approved these changes Jul 28, 2020
View reviewed changes
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

@ti-srebot ti-srebot removed the status/LGT1 Indicates that a PR has LGTM 1. label Jul 28, 2020
@ti-srebot ti-srebot added the status/LGT2 Indicates that a PR has LGTM 2. label Jul 28, 2020
@zz-jason
Copy link
Member

/merge

@ti-srebot ti-srebot added the status/can-merge Indicates a PR has been approved by a committer. label Jul 28, 2020
@ti-srebot
Copy link
Contributor

/run-all-tests

@zz-jason
Copy link
Member

@dyzsr Do we need to cherry-pick this PR to release branches?

@ti-srebot
Copy link
Contributor

@dyzsr merge failed.

@zz-jason
Copy link
Member

/merge

@ti-srebot
Copy link
Contributor

/run-all-tests

@ti-srebot
Copy link
Contributor

@dyzsr merge failed.

@dyzsr
Copy link
Contributor Author

dyzsr commented Jul 29, 2020

@dyzsr Do we need to cherry-pick this PR to release branches?

Maybe not, it's a tiny change and doesn't affect much. But I'm not sure about this.

@zz-jason
Copy link
Member

zz-jason commented Jul 29, 2020 •
edited
Loading

@dyzsr Do we need to cherry-pick this PR to release branches?

Maybe not, it's a tiny change and doesn't affect much. But I'm not sure about this.

It's a bugfix, let's backport it to release branches.

@zz-jason
Copy link
Member

/merge

@ti-srebot
Copy link
Contributor

/run-all-tests

@ti-srebot
Copy link
Contributor

@dyzsr merge failed.

@zz-jason
Copy link
Member

/merge

@ti-srebot
Copy link
Contributor

/run-all-tests

@ti-srebot
Copy link
Contributor

@dyzsr merge failed.

@SunRunAway
Copy link
Contributor

/run-all-tests

@lzmhhh123
Copy link
Contributor

/merge

@ti-srebot
Copy link
Contributor

/run-all-tests

@ti-srebot
Copy link
Contributor

@dyzsr merge failed.

@lzmhhh123
Copy link
Contributor

/run-unit-test

@lzmhhh123 lzmhhh123 merged commit bbc0502 into pingcap:master Aug 19, 2020
ti-srebot pushed a commit to ti-srebot/tidb that referenced this pull request Aug 19, 2020
@dyzsr @ti-srebot
cherry pick pingcap#18668 to release-3.0
3bcb3b6 
Signed-off-by: ti-srebot <[email protected]>
@ti-srebot ti-srebot mentioned this pull request Aug 19, 2020
Merged
@ti-srebot
Copy link
Contributor

cherry pick to release-3.0 in PR #19289

ti-srebot pushed a commit to ti-srebot/tidb that referenced this pull request Aug 19, 2020
@dyzsr @ti-srebot
cherry pick pingcap#18668 to release-3.1
deca940 
Signed-off-by: ti-srebot <[email protected]>
@ti-srebot ti-srebot mentioned this pull request Aug 19, 2020
Closed
@ti-srebot
Copy link
Contributor

cherry pick to release-3.1 in PR #19290

ti-srebot pushed a commit to ti-srebot/tidb that referenced this pull request Aug 19, 2020
@dyzsr @ti-srebot
cherry pick pingcap#18668 to release-4.0
41eb6ea 
Signed-off-by: ti-srebot <[email protected]>
@ti-srebot ti-srebot mentioned this pull request Aug 19, 2020
Merged
@ti-srebot
Copy link
Contributor

cherry pick to release-4.0 in PR #19291

ti-srebot added a commit that referenced this pull request Sep 1, 2020
@ti-srebot
expression: add warnings for logarithm functions (#18668) (#19291)
e190796 
Signed-off-by: ti-srebot <[email protected]>
zz-jason pushed a commit that referenced this pull request Sep 1, 2020
@ti-srebot
expression: add warnings for logarithm functions (#18668) (#19289)
f40c116
@dyzsr dyzsr deleted the issue-17992 branch November 25, 2020 03:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wshwsh12 wshwsh12 wshwsh12 left review comments

@ti-srebot ti-srebot ti-srebot approved these changes

@zz-jason zz-jason zz-jason approved these changes

Assignees
No one assigned
Labels
component/expression status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2. type/bugfix This PR fixes a bug.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

No warning for invalid usage of function LN()
6 participants
@dyzsr @zz-jason @ti-srebot @SunRunAway @lzmhhh123 @wshwsh12