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

util/chunk: fix incorrect result when set duration to MutRow #8725

Merged
merged 9 commits into from
Dec 19, 2018
Merged

util/chunk: fix incorrect result when set duration to MutRow #8725

merged 9 commits into from
Dec 19, 2018

Conversation

winoros
Copy link
Member

@winoros winoros commented Dec 18, 2018 •
edited by c4pt0r
Loading

What problem does this PR solve?

The duration column's len in MutRow is 16 while it's 8 in chunk.
So it will meet incorrect result when you append multiple rows from MutRow to chunk then read them.

What is changed and how it works?

This pr only fixes the column len.
It may be refactored in future pr.

Check List

Tests

  • Unit test
  • Integration test

Code changes

  • Has exported function/method change

Related changes

  • Need to cherry-pick to the release branch
  • Need to be included in the release note fix incorrect result when union scan meets duration

This change is Reviewable

@winoros winoros requested review from zz-jason and XuHuaiyu December 18, 2018 04:23
@winoros
Copy link
Member Author

winoros commented Dec 18, 2018

/run-all-tests tidb-test=pr/704

alivxxx
alivxxx reviewed Dec 18, 2018
View reviewed changes
Copy link
Contributor

@alivxxx alivxxx left a comment

Choose a reason for hiding this comment

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

LGTM

@winoros winoros mentioned this pull request Dec 18, 2018
Merged
@winoros winoros added the type/bugfix This PR fixes a bug. label Dec 18, 2018
@jackysp
Copy link
Member

jackysp commented Dec 18, 2018

/run-all-tests tidb-test=pr/704

jackysp
jackysp previously approved these changes Dec 18, 2018
View reviewed changes
Copy link
Member

@jackysp jackysp left a comment

Choose a reason for hiding this comment

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

LGTM

@zhouqiang-cl
Copy link
Contributor

/run-integration-ddl-test -tidb-test=pr/705

XuHuaiyu
XuHuaiyu reviewed Dec 19, 2018
View reviewed changes
util/chunk/mutrow_test.go Outdated Show resolved Hide resolved
@XuHuaiyu XuHuaiyu dismissed jackysp’s stale review December 19, 2018 02:39

Unit test seems to be wrong?

@eurekaka eurekaka mentioned this pull request Dec 19, 2018
Merged
@zz-jason
Copy link
Member

Seems we forget to modify the implementation in MutRow when we refactor the Duration data representation in Chunk.

Is it safe to discard the Fsp info in the original Duration value? In other places, that info is stored in the executor's schema, so it can be removed when appended into Chunk, maybe we have to check this.

@XuHuaiyu
Copy link
Contributor

fsp in chunk was removed in #7043.

XuHuaiyu
XuHuaiyu reviewed Dec 19, 2018
View reviewed changes
Copy link
Contributor

@XuHuaiyu XuHuaiyu left a comment

Choose a reason for hiding this comment

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

LGTM

@winoros
Copy link
Member Author

winoros commented Dec 19, 2018 •
edited
Loading

@XuHuaiyu He means that delete Fsp from struct Duration.

There're also other ways:

  • Refactor SetXXX of MutRow, use chunk's AppendXXX directly.
  • Add unit test to check that the appended bytes value in MutRow is the same with Chunk's append method.

@winoros winoros added the status/LGT2 Indicates that a PR has LGTM 2. label Dec 19, 2018
eurekaka
eurekaka approved these changes Dec 19, 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

@winoros winoros merged commit f4f8b3d into pingcap:master Dec 19, 2018
@winoros winoros deleted the mut-row-decimal-bug branch December 19, 2018 06:58
winoros added a commit to winoros/tidb that referenced this pull request Dec 19, 2018
@winoros
util/chunk: fix incorrect result when set duration to MutRow (pingc…
9fa4365 
…ap#8725)
@winoros winoros mentioned this pull request Dec 19, 2018
Merged
winkyao pushed a commit that referenced this pull request Dec 20, 2018
@winoros @winkyao
util/chunk: fix incorrect result when set duration to MutRow (#8725) (
f8d7b03 
#8746)
yu34po pushed a commit to yu34po/tidb that referenced this pull request Jan 2, 2019
@winoros @yu34po
util/chunk: fix incorrect result when set duration to MutRow (pingc…
09ec8d8 
…ap#8725)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alivxxx alivxxx alivxxx left review comments

@XuHuaiyu XuHuaiyu XuHuaiyu left review comments

@eurekaka eurekaka eurekaka approved these changes

@jackysp jackysp jackysp left review comments

@zz-jason zz-jason Awaiting requested review from zz-jason

Assignees
No one assigned
Labels
status/LGT2 Indicates that a PR has LGTM 2. type/bugfix This PR fixes a bug.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@winoros @jackysp @zhouqiang-cl @zz-jason @XuHuaiyu @eurekaka @alivxxx