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: handle duration type infer in least and greatest #22271

Merged
merged 46 commits into from
Jan 18, 2021

Conversation

iosmanthus
Copy link
Contributor

What problem does this PR solve?

Issue Number: close #22200

Problem Summary:

What is changed and how it works?

This pull request compares duration as string while they are formalized which will prevent panicking while encountering a non-handled duration type infer.

Related changes

  • PR to update pingcap/docs/pingcap/docs-cn:
  • Need to cherry-pick to the release branch

Check List

Tests

  • Unit test
  • Integration test
  • Manual test (add detailed scripts or steps below)

Side effects

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

Release note

  • expression: handle duration type infer in least and greatest

iosmanthus and others added 30 commits November 19, 2020 20:51
Signed-off-by: iosmanthus <myosmanthustree@gmail.com>
Signed-off-by: iosmanthus <myosmanthustree@gmail.com>
Signed-off-by: iosmanthus <myosmanthustree@gmail.com>
Signed-off-by: iosmanthus <myosmanthustree@gmail.com>
Signed-off-by: iosmanthus <myosmanthustree@gmail.com>
Signed-off-by: iosmanthus <myosmanthustree@gmail.com>
Signed-off-by: iosmanthus <myosmanthustree@gmail.com>
Signed-off-by: iosmanthus <myosmanthustree@gmail.com>
Signed-off-by: iosmanthus <myosmanthustree@gmail.com>
Signed-off-by: iosmanthus <myosmanthustree@gmail.com>
Signed-off-by: iosmanthus <myosmanthustree@gmail.com>
Signed-off-by: iosmanthus <myosmanthustree@gmail.com>
Signed-off-by: iosmanthus <myosmanthustree@gmail.com>
Signed-off-by: iosmanthus <myosmanthustree@gmail.com>
…reatest

Signed-off-by: iosmanthus <myosmanthustree@gmail.com>
Signed-off-by: iosmanthus <myosmanthustree@gmail.com>
Signed-off-by: iosmanthus <myosmanthustree@gmail.com>
Signed-off-by: iosmanthus <myosmanthustree@gmail.com>
Signed-off-by: iosmanthus <myosmanthustree@gmail.com>
Signed-off-by: iosmanthus <myosmanthustree@gmail.com>
Signed-off-by: iosmanthus <myosmanthustree@gmail.com>
@iosmanthus
Copy link
Contributor Author

this pr:

mysql> SELECT LEAST(TIME'00:00:00.1', TIME'00:00:00.12000');
+-----------------------------------------------+
| LEAST(TIME'00:00:00.1', TIME'00:00:00.12000') |
+-----------------------------------------------+
| 00:00:00.1                                    |
+-----------------------------------------------+
1 row in set (0.00 sec)

MySQL:

mysql> SELECT LEAST(TIME'00:00:00.1', TIME'00:00:00.12000');
+-----------------------------------------------+
| LEAST(TIME'00:00:00.1', TIME'00:00:00.12000') |
+-----------------------------------------------+
| 00:00:00.10000                                |
+-----------------------------------------------+
1 row in set (0.01 sec)

Though is' ok and the same as release 4.0, it's better to make it be compatible with MySQL. If it is hard, I think that's fine.

For now, I think we should consider the bug itself, instead of the compatibility with MySQL. We may fix this behaviour in another pull request.

@iosmanthus
Copy link
Contributor Author

@wjhuang2016 @XuHuaiyu PTAL

Copy link
Member

@wjhuang2016 wjhuang2016 left a comment

Choose a reason for hiding this comment

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

LGTM

@ti-srebot ti-srebot added the status/LGT1 Indicates that a PR has LGTM 1. label Jan 13, 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.

Rest LGTM

expression/builtin_compare.go Outdated Show resolved Hide resolved
expression/builtin_compare.go Outdated Show resolved Hide resolved
expression/builtin_compare.go Outdated Show resolved Hide resolved
types/etc.go Outdated Show resolved Hide resolved
Signed-off-by: iosmanthus <myosmanthustree@gmail.com>
@XuHuaiyu XuHuaiyu requested a review from wshwsh12 January 18, 2021 03:59
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.

LGTM

@ti-srebot ti-srebot removed the status/LGT1 Indicates that a PR has LGTM 1. label Jan 18, 2021
@ti-srebot ti-srebot added the status/LGT2 Indicates that a PR has LGTM 2. label Jan 18, 2021
@wshwsh12
Copy link
Contributor

/merge

@ti-srebot ti-srebot added the status/can-merge Indicates a PR has been approved by a committer. label Jan 18, 2021
@ti-srebot
Copy link
Contributor

/run-all-tests

@ti-srebot ti-srebot merged commit 4658536 into pingcap:master Jan 18, 2021
ti-srebot pushed a commit to ti-srebot/tidb that referenced this pull request Jan 18, 2021
Signed-off-by: ti-srebot <ti-srebot@pingcap.com>
@ti-srebot
Copy link
Contributor

cherry pick to release-4.0 in PR #22421

qw4990 pushed a commit to qw4990/tidb that referenced this pull request Jan 27, 2021
…#22271)

Signed-off-by: iosmanthus <myosmanthustree@gmail.com>
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/bugfix This PR fixes a bug.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

runtime error: invalid memory address or nil pointer dereference
6 participants