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

*: refine the behavior of StrToInt and StrToFloat and support convert JSON to date, time and timestamp (#17902) #18159

Merged
merged 3 commits into from
Jul 28, 2020

Conversation

ti-srebot
Copy link
Contributor

cherry-pick #17902 to release-4.0


What problem does this PR solve?

Issue Number: close #17898

Problem Summary:

TiDB don't distinguish cast(explicit CAST function or implicit cast) from cast(when casting into a column type, CastValue()).
We try to distinguish them according to if the statement is a select statement or an insert statement.
However, it's not correct, especially when the virtual generated column comes out. We can't make sure the value inserted and read are the same.
So we can't depend on if the type of the statement. We need to keep a principle. If some values are converted to a column type, then use CastValue(). Otherwise, don't use it.

Here is an example:
We didn't support converted a JSON to date. When we had a virtual generated column and it needed to be converted from a JSON to date, we used CastJsonAsDuration to do that. This is wrong. So I supported convert JSON to date, time, and timestamp within CastValue(), though their logics are quite similar.

Finally, I correct some tests. If you test these tests in MySQL and previously TiDB, you will see that the warnings of TiDB are wrong.

What is changed and how it works?

  1. refine the behavior of StrToInt and StrToFloat. Handling them differently according to the function call them.
  2. implement convert JSON to date, time, and timestamp.
  3. correct some tests.

Related changes

  • Need to cherry-pick to the release branch

Check List

Tests

  • Unit test

Side effects

Release note

  • refine the behavior of StrToInt and StrToFloat and support convert JSON to date, time and timestamp

@ti-srebot
Copy link
Contributor Author

/run-all-tests

@Reminiscent
Copy link
Contributor

LGTM

@ti-srebot
Copy link
Contributor Author

@qw4990, @hanfei1991, PTAL.

@wjhuang2016
Copy link
Member

/run-all-tests

@ti-srebot
Copy link
Contributor Author

@qw4990, @hanfei1991, PTAL.

2 similar comments
@ti-srebot
Copy link
Contributor Author

@qw4990, @hanfei1991, PTAL.

@ti-srebot
Copy link
Contributor Author

@qw4990, @hanfei1991, PTAL.

@zz-jason zz-jason modified the milestones: v4.0.2, v4.0.3 Jul 10, 2020
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

@ti-srebot
Copy link
Contributor Author

@qw4990,Thanks for your review.

@ti-srebot ti-srebot added the status/LGT1 Indicates that a PR has LGTM 1. label Jul 10, 2020
@qw4990
Copy link
Contributor

qw4990 commented Jul 10, 2020

@wjhuang2016 This PR has two LGTs, please fix the CI.

@wjhuang2016
Copy link
Member

/run-all-tests

@ti-srebot
Copy link
Contributor Author

@qw4990, @hanfei1991, PTAL.

1 similar comment
@ti-srebot
Copy link
Contributor Author

@qw4990, @hanfei1991, PTAL.

@winoros winoros modified the milestones: v4.0.3, v4.0.4 Jul 15, 2020
@ti-srebot
Copy link
Contributor Author

@qw4990, @hanfei1991, PTAL.

@imtbkcat imtbkcat modified the milestones: v4.0.4, v4.0.5 Jul 28, 2020
Copy link
Member

@zz-jason zz-jason 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 status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Jul 28, 2020
@zz-jason
Copy link
Member

/merge

@ti-srebot ti-srebot added the status/can-merge Indicates a PR has been approved by a committer. label Jul 28, 2020
@ti-srebot
Copy link
Contributor Author

Your auto merge job has been accepted, waiting for:

  • 18368
  • 17926
  • 17823
  • 18531
  • 18667
  • 18806
  • 18413
  • 18434
  • 18280
  • 18121
  • 18122
  • 18027
  • 18234
  • 18529
  • 18727
  • 18583
  • 18513
  • 17972
  • 17231
  • 18692
  • 17988
  • 18683

@ti-srebot
Copy link
Contributor Author

/run-all-tests

@ti-srebot ti-srebot merged commit 889eb98 into pingcap:release-4.0 Jul 28, 2020
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. type/4.0-cherry-pick
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants