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

planner, executor: fix cast not check error #21064

Closed
wants to merge 13 commits into from
Closed

planner, executor: fix cast not check error #21064

wants to merge 13 commits into from

Conversation

newcworld
Copy link
Contributor

What problem does this PR solve?

Issue Number: close #21063

Problem Summary:

What is changed and how it works?

What's Changed:

the preprocessor will pre check type

How it Works:

the preprocessor will pre check type

Related changes

Check List

Tests

  • Unit test
  • Integration test

Side effects

  • Performance regression
    • Consumes more CPU
      No
    • Consumes more MEM
      No
  • Breaking backward compatibility
    No

Release note

  • preprocessor will precheck to avoid type error

@newcworld newcworld requested review from a team as code owners November 15, 2020 17:29
@newcworld newcworld requested review from XuHuaiyu and removed request for a team November 15, 2020 17:29
@newcworld
Copy link
Contributor Author

PLAT
@blacktear23
@XuHuaiyu

I think this pr can fix issue #21063

@newcworld
Copy link
Contributor Author

/run-all-tests

@newcworld
Copy link
Contributor Author

/run-all-tests

@newcworld
Copy link
Contributor Author

/run-all-tests

@blacktear23
Copy link
Contributor

@pengdaqian2020 I just talk with @bb7133 yesterday. I think we can move some check to parser component, and this will make the fix more easily and fully covered with all statement. What do you think about that?

@newcworld
Copy link
Contributor Author

@pengdaqian2020 I just talk with @bb7133 yesterday. I think we can move some check to parser component, and this will make the fix more easily and fully covered with all statement. What do you think about that?

It looks good

@newcworld
Copy link
Contributor Author

/run-all-tests

@blacktear23
Copy link
Contributor

blacktear23 commented Nov 16, 2020 •
edited
Loading

@pengdaqian2020 I just talk with @bb7133 yesterday. I think we can move some check to parser component, and this will make the fix more easily and fully covered with all statement. What do you think about that?

It looks good

What about implements this validation on parser component?

And there also a bug for convert function:

mysql> select convert('0.0', Decimal(800, 40));
+--------------------------------------------+
| convert('0.0', Decimal(800, 40))           |
+--------------------------------------------+
| 0.0000000000000000000000000000000000000000 |
+--------------------------------------------+
1 row in set (0.00 sec)

@ichn-hu ichn-hu mentioned this pull request Nov 16, 2020
Closed
@newcworld
Copy link
Contributor Author

convert function

the convert function is different types compare error ?

#20128

@newcworld
Copy link
Contributor Author

PLAT pingcap/parser#1099

@newcworld
Copy link
Contributor Author

/run-all-tests

1 similar comment
@newcworld
Copy link
Contributor Author

/run-all-tests

@blacktear23
Copy link
Contributor

@lzmhhh123 PTAL

1 similar comment
@newcworld
Copy link
Contributor Author

@lzmhhh123 PTAL

lzmhhh123
lzmhhh123 reviewed Dec 21, 2020
View reviewed changes
planner/core/preprocess_test.go
Comment on lines +294 to +296
{`select * from t where d = convert(d , decimal(10,20))`, false, errors.New(`[types:1427]For float(M,D), double(M,D) or decimal(M,D), M must be >= D (column '')` + ".")},
{`select * from t where d = convert("d", decimal(10,20))`, false, errors.New(`[types:1427]For float(M,D), double(M,D) or decimal(M,D), M must be >= D (column '')` + ".")},
{`select * from t where d = convert("'d'", decimal(10,20))`, false, errors.New(`[types:1427]For float(M,D), double(M,D) or decimal(M,D), M must be >= D (column '')` + ".")},
Copy link
Contributor

Choose a reason for hiding this comment

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

Why all the columns names are empty when error M bigger than D happened?

@lzmhhh123
Copy link
Contributor

BTW, pls resolve conflicts.

@xiongjiwei
Copy link
Contributor

@pengdaqian2020 hi, any updated?

@ti-chi-bot
Copy link
Member

@pengdaqian2020: PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@ti-chi-bot ti-chi-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 23, 2021
@XuHuaiyu XuHuaiyu removed their request for review June 25, 2021 06:25
@tisonkun
Copy link
Contributor

Closed as the original issue closed.

@pengdaqian2020 thanks for your contribution!

@tisonkun tisonkun closed this Jul 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lzmhhh123 lzmhhh123 lzmhhh123 left review comments

@blacktear23 blacktear23 blacktear23 left review comments

Assignees
No one assigned
Labels
component/expression component/statistics needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. sig/execution SIG execution sig/planner SIG: Planner sig/sql-infra SIG: SQL Infra
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Missing decimal M and D check
6 participants
@newcworld @blacktear23 @lzmhhh123 @xiongjiwei @ti-chi-bot @tisonkun