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 expression reverse evaluation framework #13738

Merged

Conversation

lzmhhh123
Copy link
Contributor

@lzmhhh123 lzmhhh123 commented Nov 26, 2019

What problem does this PR solve?

A sub PR of #13659
Now, we only support reverse evaluation for expression type int, float, decimal. Because other types' reverse evaluations are very complex.

What is changed and how it works?

Add the framework of the reverse evaluation for expression.

Check List

Tests

  • Unit test
  • Integration test

Code changes

  • Has exported function/method change
  • Has exported variable/fields change
  • Has interface methods change
  • Has persistent data change

Side effects

  • Possible performance regression
  • Increased code complexity

@lzmhhh123 lzmhhh123 requested a review from a team as a code owner November 26, 2019 07:12
@ghost ghost requested review from qw4990 and wshwsh12 and removed request for a team November 26, 2019 07:12

// ReverseEvalDuration evaluates the only one column duration value with given function result.
func (col *Column) ReverseEvalDuration(res types.Datum, rType RoundingType) (val types.Duration, err error) {
return res.GetMysqlDuration(), errors.Errorf("Constant.ReverseEvalDuration() should never be called, please contact the TiDB team for help")
Copy link
Contributor

Choose a reason for hiding this comment

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

Why return error for them?

@lzmhhh123 lzmhhh123 force-pushed the dev/support_reverse_evaluation_framework branch from f07c752 to e8de681 Compare November 26, 2019 11:25
@codecov
Copy link

codecov bot commented Nov 26, 2019

Codecov Report

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

@@             Coverage Diff             @@
##             master     #13738   +/-   ##
===========================================
  Coverage   80.3523%   80.3523%           
===========================================
  Files           480        480           
  Lines        119699     119699           
===========================================
  Hits          96181      96181           
  Misses        16007      16007           
  Partials       7511       7511

@lzmhhh123 lzmhhh123 force-pushed the dev/support_reverse_evaluation_framework branch from 3e841bd to bae37fd Compare November 27, 2019 05:30
@lzmhhh123 lzmhhh123 requested a review from alivxxx November 28, 2019 05:06
@lzmhhh123 lzmhhh123 force-pushed the dev/support_reverse_evaluation_framework branch from 8b5de09 to 185f2e9 Compare November 28, 2019 05:15
Copy link
Contributor

@alivxxx alivxxx left a comment

Choose a reason for hiding this comment

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

Can you add some unit tests?

@lzmhhh123 lzmhhh123 requested a review from alivxxx December 2, 2019 07:56
@lzmhhh123 lzmhhh123 requested a review from a team as a code owner December 4, 2019 03:45
@ghost ghost removed their request for review December 4, 2019 03:45
@lzmhhh123 lzmhhh123 requested a review from alivxxx December 4, 2019 03:45
Copy link
Contributor

@alivxxx alivxxx left a comment

Choose a reason for hiding this comment

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

LGTM

@lzmhhh123 lzmhhh123 requested a review from qw4990 December 5, 2019 03:08
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

@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 Dec 5, 2019
@sre-bot
Copy link
Contributor

sre-bot commented Dec 5, 2019

Your auto merge job has been accepted, waiting for 13827

@sre-bot
Copy link
Contributor

sre-bot commented Dec 5, 2019

/run-all-tests

@sre-bot
Copy link
Contributor

sre-bot commented Dec 5, 2019

@lzmhhh123 merge failed.

@lzmhhh123
Copy link
Contributor Author

/merge

@sre-bot
Copy link
Contributor

sre-bot commented Dec 5, 2019

/run-all-tests

@sre-bot sre-bot merged commit 7de6200 into pingcap:master Dec 5, 2019
@lzmhhh123 lzmhhh123 deleted the dev/support_reverse_evaluation_framework branch December 5, 2019 03:50
XiaTianliang pushed a commit to XiaTianliang/tidb that referenced this pull request Dec 21, 2019
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. type/new-feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants