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: incompatibility with MySQL for ADDTIME() #21401

Closed
wants to merge 6 commits into from
Closed

expression: incompatibility with MySQL for ADDTIME() #21401

wants to merge 6 commits into from
+21 −12

Conversation

hidehalo
Copy link
Contributor

@hidehalo hidehalo commented Nov 30, 2020 •
edited
Loading

What problem does this PR solve?

Issue Number: close #19150

What is changed and how it works?

What's Changed:

  • builtinAddStringStringSig eval/vecEval return null when arg1 isn't Duration type (eg: ADDTIME('2020-05-13 14:01:24', '2020-04-29 05:11:19'))

How it Works:

make builtinAddStringStringSig return null when arg1 isn't Duration type (eg: ADDTIME('2020-05-13 14:01:24', '2020-04-29 05:11:19'))

Related changes

Tests

  • Integration tests

Side effects

  • Breaking backward compatibility

Release note

Fix incompatibility with MySQL 8.0 for `ADDTIME(expr1, expr2)` when `expr2` is not `Time`

@hidehalo hidehalo requested a review from a team as a code owner November 30, 2020 14:42
@hidehalo hidehalo requested review from lzmhhh123 and removed request for a team November 30, 2020 14:42
@hidehalo hidehalo closed this Dec 24, 2020
@hidehalo hidehalo reopened this Dec 24, 2020
@ti-challenge-bot
Copy link

There is no reward for this challenge pull request, so you can request a reward from @SunRunAway.

Details

Tip : About reward you can refs to reward-command.

Warning: None

@hidehalo
Copy link
Contributor Author

@qw4990 PTAL

@hidehalo hidehalo requested a review from qw4990 December 25, 2020 03:58
@zz-jason zz-jason requested review from XuHuaiyu and removed request for lzmhhh123 March 15, 2021 13:35
@qw4990 qw4990 requested review from wshwsh12 and removed request for qw4990 June 21, 2021 03:22
wshwsh12
wshwsh12 reviewed Aug 4, 2021
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.

Rest LGTM

types/time.go Outdated Show resolved Hide resolved
expression/builtin_time_vec_generated.go
Comment on lines +407 to +410
if !isDuration(arg1) {
result.AppendNull() // fixed: false
continue
}
Copy link
Contributor

Choose a reason for hiding this comment

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

The file is generated by go generate in expression/generator. We shouldn't edit it by manual.. Maybe we should modify the template in time_vec.go and run make gogenerate to generate the code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that's the way

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@wshwsh12 PTAL, BTW I believe subtime have the same issue, should I fix it in this PR?

Copy link
Contributor

Choose a reason for hiding this comment

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

If the reason is the same, I think we can fix it in this pr.

@ti-chi-bot ti-chi-bot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Aug 12, 2021
@ti-chi-bot ti-chi-bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Aug 12, 2021
hidehalo
hidehalo commented Aug 12, 2021
View reviewed changes
expression/builtin_time.go Outdated Show resolved Hide resolved
@ti-chi-bot ti-chi-bot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Aug 12, 2021
@ti-chi-bot ti-chi-bot removed the do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. label Aug 12, 2021
@wshwsh12
Copy link
Contributor

Test case fails

[2021-08-12T09:34:14.503Z] FAIL: builtin_time_test.go:903: testEvaluatorSuite.TestAddTimeSig
[2021-08-12T09:34:14.503Z] 
[2021-08-12T09:34:14.503Z] builtin_time_test.go:1012:
[2021-08-12T09:34:14.503Z]     c.Assert(len(warnings), Equals, i+1+beforeWarnCnt)
[2021-08-12T09:34:14.503Z] ... obtained int = 8
[2021-08-12T09:34:14.503Z] ... expected int = 9
[2021-08-12T09:34:14.503Z]

hidehalo
hidehalo commented Aug 13, 2021
View reviewed changes
expression/builtin_time.go
@@ -5559,6 +5559,9 @@ func (b *builtinAddStringAndStringSig) evalString(row chunk.Row) (result string,
if isNull || err != nil {
return "", isNull, err
}
if !isDuration(arg1Str) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Test case fails

[2021-08-12T09:34:14.503Z] FAIL: builtin_time_test.go:903: testEvaluatorSuite.TestAddTimeSig
[2021-08-12T09:34:14.503Z] 
[2021-08-12T09:34:14.503Z] builtin_time_test.go:1012:
[2021-08-12T09:34:14.503Z]     c.Assert(len(warnings), Equals, i+1+beforeWarnCnt)
[2021-08-12T09:34:14.503Z] ... obtained int = 8
[2021-08-12T09:34:14.503Z] ... expected int = 9
[2021-08-12T09:34:14.503Z]

Here is the reason, if arg1Str is not a MySQL Time, it just return without error.

hidehalo
hidehalo commented Aug 13, 2021
View reviewed changes
expression/builtin_time.go
@@ -5559,6 +5559,9 @@ func (b *builtinAddStringAndStringSig) evalString(row chunk.Row) (result string,
if isNull || err != nil {
return "", isNull, err
}
if !isDuration(arg1Str) {
return "", true, nil
}
sc := b.ctx.GetSessionVars().StmtCtx
arg1, err = types.ParseDuration(sc, arg1Str, getFsp4TimeAddSub(arg1Str))
Copy link
Contributor Author

@hidehalo hidehalo Aug 13, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I'm more expect here throw a ErrTruncatedWrongVal error, IMO, it should be (in fact no). @wshwsh12 what do you think? more details I was said in Slack. please check it out.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think so. MySQL also doesn't throw a error/warning.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's make sense. Should we remove those two test cases(below)? Or we should throw a error/warning inside func (b *builtinAddStringAndStringSig) evalString(row chunk.Row)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@wshwsh12 PTAL PR #11262, I really feel confuse, would you like to give me some advices? thx!

@wshwsh12 wshwsh12 self-requested a review August 13, 2021 09:58
@hidehalo hidehalo closed this Jan 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@qw4990 qw4990 qw4990 left review comments

@XuHuaiyu XuHuaiyu Awaiting requested review from XuHuaiyu

@wshwsh12 wshwsh12 Awaiting requested review from wshwsh12

Assignees
No one assigned
Labels
component/expression release-note Denotes a PR that will be considered when it comes time to generate release notes. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Incompatibility with MySQL for ADDTIME()
4 participants
@hidehalo @wshwsh12 @qw4990 @ti-chi-bot