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: fix cast invalid string to datetime #26726

Closed
wants to merge 8 commits into from

Conversation

wzru
Copy link
Contributor

@wzru wzru commented Jul 29, 2021

What problem does this PR solve?

Issue Number: close #24997

Problem Summary:

Simplified case:

drop table if exists t1, t2;
create table t1(c1 varbinary(100));
insert into t1 values('e');
create table t2(c1 date);
insert into t2 values('2019-11-10'); 
select * from t1 inner join t2 on t1.c1 <= t2.c1;

MySQL:

mysql> select * from t1 inner join t2 on t1.c1 <= t2.c1;
   
+------------+------------+
| c1         | c1         |
+------------+------------+
| 0x65       | 2019-11-10 |
+------------+------------+
1 row in set, 1 warning (0.00 sec)

TiDB:

mysql> select * from t1 inner join t2 on t1.c1 <= t2.c1;

Empty set, 1 warning (0.00 sec)

What is changed and how it works?

What's Changed:

func (b *builtinCastStringAsTimeSig) vecEvalTime
func (b *builtinCastStringAsTimeSig) evalTime

How it Works:

Check List

Tests

  • Unit test
  • Integration test

Side effects

  • Performance regression: Consumes more CPU

Release note

None

@ti-chi-bot
Copy link
Member

[REVIEW NOTIFICATION]

This pull request has not been approved.

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

The full list of commands accepted by this bot can be found here.

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

@ti-chi-bot ti-chi-bot added release-note-none Denotes a PR that doesn't merit a release note. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jul 29, 2021
@wzru
Copy link
Contributor Author

wzru commented Jul 29, 2021

/cc @guo-shaoge @wshwsh12

@ti-chi-bot ti-chi-bot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jul 29, 2021
@ti-chi-bot ti-chi-bot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jul 29, 2021
@ichn-hu ichn-hu mentioned this pull request Jul 29, 2021
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.

Please add some test cases for your modify.
For example.

MySQL [test]> select cast('e' as date);
+-------------------+
| cast('e' as date) |
+-------------------+
| NULL              |
+-------------------+

@XuHuaiyu
Copy link
Contributor

XuHuaiyu commented Aug 9, 2021

@wzru
Any update for this PR?

@wzru wzru requested a review from wshwsh12 August 11, 2021 07:42
@ti-chi-bot ti-chi-bot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Aug 13, 2021
@ti-chi-bot
Copy link
Member

@wzru: 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.

@@ -1696,8 +1696,6 @@ func (b *builtinCastStringAsTimeSig) vecEvalTime(input *chunk.Chunk, result *chu
if err = handleInvalidTimeError(b.ctx, err); err != nil {
return err
}
result.SetNull(i, true)
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we use b.ctx.GetSessionVars().StrictSQLMode?

@hawkingrei
Copy link
Member

hawkingrei commented Jan 23, 2022

@wzru Do you have time to finish this PR? If there is no reply within a week, I will close the PR and take over the job.

@wzru
Copy link
Contributor Author

wzru commented Jan 23, 2022

@hawkingrei Sorry for that. I don't have time yet. I will close this PR.

@wzru wzru closed this Jan 23, 2022
@hawkingrei
Copy link
Member

@hawkingrei Sorry for that. I don't have time yet. I will close this PR.

Thank you for your contribution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/expression needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. release-note-none Denotes a PR that doesn't merit a release note. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

executor: select with inl_hash_join return inconsistency with MySQL
5 participants