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,planner: fix to preserve the precision information of a timestamp-typed column in the plan cache #8619

Merged
merged 3 commits into from
Dec 10, 2018
Merged

expression,planner: fix to preserve the precision information of a timestamp-typed column in the plan cache #8619

merged 3 commits into from
Dec 10, 2018

Conversation

dbjoa
Copy link
Contributor

@dbjoa dbjoa commented Dec 7, 2018 •
edited
Loading

What problem does this PR solve?

Fix #8615

What is changed and how it works?

When cloning FieldType.Tp from the source, FieldType.Decimal should be cloned too because it can be used as Fsp in Timestamp type.

Check List

Tests

  • Unit test

Code changes

  • Has exported function/method change

Side effects

  • None

Related changes

  • Need to cherry-pick to the release branch

This change is Reviewable

@sre-bot
Copy link
Contributor

sre-bot commented Dec 7, 2018

Hi contributor, thanks for your PR.

This patch needs to be approved by someone of admins. They should reply with "/ok-to-test" to accept this PR for running test automatically.

@dbjoa
Copy link
Contributor Author

dbjoa commented Dec 7, 2018

/run-all-tests

@dbjoa
Copy link
Contributor Author

dbjoa commented Dec 7, 2018

/run-common-test

1 similar comment
@dbjoa
Copy link
Contributor Author

dbjoa commented Dec 7, 2018

/run-common-test

@dbjoa
Copy link
Contributor Author

dbjoa commented Dec 7, 2018

/run-all-tests

@dbjoa dbjoa changed the title expression,planner: FileType.Flen/.Decimal should be cloned expression,planner: FileType.Decimal should be cloned for Timestamp's precision Dec 7, 2018
@dbjoa
Copy link
Contributor Author

dbjoa commented Dec 7, 2018

/run-common-test

@dbjoa
Copy link
Contributor Author

dbjoa commented Dec 7, 2018

/run-all-tests

@dbjoa
Copy link
Contributor Author

dbjoa commented Dec 8, 2018

/run-unit-test

@dbjoa
Copy link
Contributor Author

dbjoa commented Dec 8, 2018

/run-sqllogic-test

@dbjoa dbjoa changed the title expression,planner: FileType.Decimal should be cloned for Timestamp's precision expression,planner: fix to preserve the precision information of a timestamp-typed column in the plan cache Dec 8, 2018
@dbjoa
Copy link
Contributor Author

dbjoa commented Dec 8, 2018

@eurekaka, @zz-jason PTAL

@shenli shenli added contribution This PR is from a community contributor. sig/planner SIG: Planner labels Dec 10, 2018
eurekaka
eurekaka reviewed Dec 10, 2018
View reviewed changes
expression/constant.go Outdated Show resolved Hide resolved
@winoros winoros mentioned this pull request Dec 10, 2018
Merged
@dbjoa
Copy link
Contributor Author

dbjoa commented Dec 10, 2018

/run-all-tests

eurekaka
eurekaka reviewed Dec 10, 2018
View reviewed changes
Copy link
Contributor

@eurekaka eurekaka left a comment

Choose a reason for hiding this comment

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

LGTM

@eurekaka eurekaka added the status/LGT1 Indicates that a PR has LGTM 1. label Dec 10, 2018
XuHuaiyu
XuHuaiyu reviewed Dec 10, 2018
View reviewed changes
types/field_type.go Show resolved Hide resolved
expression/constant.go Outdated Show resolved Hide resolved
@dbjoa
Copy link
Contributor Author

dbjoa commented Dec 10, 2018

/run-all-tests

@dbjoa
Copy link
Contributor Author

dbjoa commented Dec 10, 2018

/run-all-tests

zz-jason
zz-jason approved these changes Dec 10, 2018
View reviewed changes
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

@zz-jason zz-jason added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Dec 10, 2018
@zz-jason zz-jason merged commit 18b33fb into pingcap:master Dec 10, 2018
@zz-jason
Copy link
Member

@dbjoa Could you please help us cherry-pick this commit to release-2.1 and release-2.0?

@dbjoa
Copy link
Contributor Author

dbjoa commented Dec 10, 2018

@zz-jason Unfortunately, we couldn't. This is because the PR has dependency on the previous PR #8105, and release-2.1 and release-2.0 should include the previous PR.

@dbjoa dbjoa mentioned this pull request Dec 11, 2018
Merged
zz-jason pushed a commit that referenced this pull request Dec 12, 2018
@dbjoa @zz-jason
planner,types: refactor the PR #8619 by using parser/types/FieldType.…
64cd9b2 
…Clone() (#8641)
iamzhoug37 pushed a commit to iamzhoug37/tidb that referenced this pull request Dec 13, 2018
@dbjoa @iamzhoug37
expression,planner: fix to preserve the precision information of a ti…
9da9002 
…mestamp-typed column in the plan cache (pingcap#8619)
iamzhoug37 pushed a commit to iamzhoug37/tidb that referenced this pull request Dec 13, 2018
@dbjoa @iamzhoug37
planner,types: refactor the PR pingcap#8619 by using parser/types/Fie…
f23e306 
…ldType.Clone() (pingcap#8641)
xhebox pushed a commit to xhebox/tidb that referenced this pull request Sep 28, 2021
@dbjoa @ngaut
[parser] refactor the PR pingcap#8619 by introducing the FieldType.Cl…
f78c90e 
…one() member function (pingcap#77)
xhebox pushed a commit to xhebox/tidb that referenced this pull request Oct 8, 2021
@dbjoa @xhebox
[parser] refactor the PR pingcap#8619 by introducing the FieldType.Cl…
1a0e34f 
…one() member function (pingcap#77)
ti-chi-bot pushed a commit that referenced this pull request Oct 9, 2021
@dbjoa @ti-chi-bot
[parser] refactor the PR #8619 by introducing the FieldType.Clone() m…
3fb7739 
…ember function (#77)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eurekaka eurekaka eurekaka left review comments

@winoros winoros winoros left review comments

@XuHuaiyu XuHuaiyu XuHuaiyu left review comments

@alivxxx alivxxx alivxxx left review comments

@zz-jason zz-jason zz-jason approved these changes

Assignees
No one assigned
Labels
contribution This PR is from a community contributor. sig/planner SIG: Planner status/LGT2 Indicates that a PR has LGTM 2.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@dbjoa @sre-bot @zz-jason @eurekaka @winoros @XuHuaiyu @alivxxx @shenli