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: open CAST string as real push down switcher #14323

Merged
merged 7 commits into from Jan 6, 2020
Merged

expression: open CAST string as real push down switcher #14323

merged 7 commits into from Jan 6, 2020

Conversation

lonng
Copy link
Contributor

@lonng lonng commented Jan 2, 2020

Signed-off-by: Lonng heng@lonng.org

What problem does this PR solve?

This PR opens the push-down switcher of the CAST string as real.

Additional, this PR fixes the #14326.

What is changed and how it works?

The tikv/tikv#6390 has fix the inconsistent with TiDB.

Check List

Tests

  • Unit test
  • Integration test

Signed-off-by: Lonng <heng@lonng.org>
@lonng lonng requested a review from a team as a code owner January 2, 2020 09:44
Signed-off-by: Lonng <heng@lonng.org>
Signed-off-by: Lonng <heng@lonng.org>
@lonng lonng requested a review from a team as a code owner January 2, 2020 10:03
@lonng
Copy link
Contributor Author

lonng commented Jan 2, 2020

/run-integration-copr-test tikv=pr/6390

@ghost ghost requested review from qw4990, XuHuaiyu, eurekaka and francis0407 and removed request for a team January 2, 2020 11:27
@lonng
Copy link
Contributor Author

lonng commented Jan 3, 2020

/run-integration-copr-test tikv=pr/6390

Signed-off-by: Lonng <heng@lonng.org>
Signed-off-by: Lonng <heng@lonng.org>
@lonng
Copy link
Contributor Author

lonng commented Jan 3, 2020

/run-integration-copr-test tikv=pr/6390

@lonng
Copy link
Contributor Author

lonng commented Jan 3, 2020

/run-all-tests tikv=pr/6390

@lonng lonng changed the title [DNM] expression: open CAST string as real push down switcher expression: open CAST string as real push down switcher Jan 3, 2020
if isNull || err != nil {
return 0, isNull, err
a, isLHSNull, err := s.args[0].EvalReal(s.ctx, row)
if err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why change this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All of the expressions which have two arguments in TiDB have this pattern, they have inconsistent behavior with MySQL.

create table t (a varchar(10));
insert into t values ('2007');
select * from t where NULL + pow(7020, a);

The above statement should report an error.

Copy link
Contributor

@XuHuaiyu XuHuaiyu 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

@lonng
Copy link
Contributor Author

lonng commented Jan 3, 2020

Note: this PR should be merged with tikv/tikv#6390 together.

@ngaut ngaut added the status/LGT2 Indicates that a PR has LGTM 2. label Jan 4, 2020
@lonng
Copy link
Contributor Author

lonng commented Jan 6, 2020

/run-all-tests tikv=pr/6390

@lonng lonng merged commit 0adab37 into pingcap:master Jan 6, 2020
@lonng lonng deleted the open-cast-string-real branch January 6, 2020 03:28
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants