Skip to content
This repository has been archived by the owner on Nov 24, 2023. It is now read-only.

*: fix cannot convert type: decimal.Decimal #375

Merged
merged 2 commits into from
Nov 27, 2019

Conversation

csuzhangxc
Copy link
Member

@csuzhangxc csuzhangxc commented Nov 26, 2019

What problem does this PR solve?

in golang v1.13, the Decimal decompose interface is introduced, see:

in shopspring/decimal@75bb2cb, it implements the Decimal decompose interface.

when executing a statement with decimal type columns, a possible execution path is:

  1. https://github.com/golang/go/blob/cc8838d645/src/database/sql/sql.go#L2275
  2. https://github.com/golang/go/blob/cc8838d645/src/database/sql/sql.go#L1515
  3. https://github.com/golang/go/blob/cc8838d645/src/database/sql/convert.go#L177
  4. https://github.com/go-sql-driver/mysql/blob/v1.4.1/connection_go18.go#L196
  5. https://github.com/go-sql-driver/mysql/blob/v1.4.1/statement.go#L141
  6. https://github.com/golang/go/blob/cc8838d645/src/database/sql/driver/types.go#L184

the code block

case decimalDecompose:
	return true

will cause the skip of the value fetching (convert decimal.Decimal to string) in https://github.com/go-sql-driver/mysql/blob/v1.4.1/statement.go#L146.

then continue the execution flow:

  1. https://github.com/golang/go/blob/cc8838d645/src/database/sql/sql.go#L1538
  2. https://github.com/golang/go/blob/cc8838d645/src/database/sql/sql.go#L2435
  3. https://github.com/golang/go/blob/cc8838d645/src/database/sql/ctxutil.go#L65
  4. https://github.com/go-sql-driver/mysql/blob/v1.4.1/connection_go18.go#L142
  5. https://github.com/go-sql-driver/mysql/blob/v1.4.1/statement.go#L53
  6. https://github.com/go-sql-driver/mysql/blob/v1.4.1/packets.go#L1071

another useful link golang/go#34315.

What is changed and how it works?

some methods can be used to fix this problem:

  1. chose a shopspring/decimal version without Decimal decompose interface implementation
  2. downgrade golang to v1.12

I chose the first one, and update shopspring/decimal to shopspring/decimal@b054a8d. (revert Decimal decompose interface implementation)

go get -u github.com/shopspring/decimal@b054a8dfd10d5e651f48ab20c6c9bee309805368
go mod tidy

Check List

Tests

  • Unit test
  • Integration test

Related changes

  • Need to cherry-pick to the release branch

@csuzhangxc csuzhangxc added priority/important Major change, requires approval from ≥2 primary reviewers status/PTAL This PR is ready for review. Add this label back after committing new changes type/bug-fix Bug fix needs-cherry-pick-release-1.0 This PR should be cherry-picked to release-1.0. Remove this label after cherry-picked to release-1.0 labels Nov 26, 2019
@csuzhangxc
Copy link
Member Author

/run-all-tests tidb=release-3.0

@codecov
Copy link

codecov bot commented Nov 26, 2019

Codecov Report

Merging #375 into master will not change coverage.
The diff coverage is n/a.

@@             Coverage Diff             @@
##             master       #375   +/-   ##
===========================================
  Coverage   57.7346%   57.7346%           
===========================================
  Files           162        162           
  Lines         16368      16368           
===========================================
  Hits           9450       9450           
  Misses         5994       5994           
  Partials        924        924

@csuzhangxc
Copy link
Member Author

@WangXiangUSTC @amyangfei PTAL

@csuzhangxc csuzhangxc added this to the v1.0.3 milestone Nov 26, 2019
Copy link
Contributor

@amyangfei amyangfei left a comment

Choose a reason for hiding this comment

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

LGTM

@amyangfei amyangfei added status/LGT1 One reviewer already commented LGTM and removed status/PTAL This PR is ready for review. Add this label back after committing new changes labels Nov 26, 2019
@csuzhangxc
Copy link
Member Author

/run-all-tests tidb=release-3.0

2 similar comments
@csuzhangxc
Copy link
Member Author

/run-all-tests tidb=release-3.0

@csuzhangxc
Copy link
Member Author

/run-all-tests tidb=release-3.0

@WangXiangUSTC
Copy link
Contributor

LGTM

@WangXiangUSTC WangXiangUSTC added status/LGT2 Two reviewers already commented LGTM, ready for merge and removed status/LGT1 One reviewer already commented LGTM labels Nov 27, 2019
@csuzhangxc csuzhangxc merged commit 26ad5b8 into pingcap:master Nov 27, 2019
@csuzhangxc csuzhangxc deleted the fix-decimal branch November 27, 2019 03:11
@you06
Copy link

you06 commented Nov 27, 2019

/run-cherry-picker

@sre-bot
Copy link

sre-bot commented Nov 27, 2019

cherry pick to release-1.0 in PR #376

@sre-bot sre-bot added already-cherry-pick-1.0 The related PR is already cherry-picked to release-1.0. Add this label once the PR is cherry-picked and removed needs-cherry-pick-release-1.0 This PR should be cherry-picked to release-1.0. Remove this label after cherry-picked to release-1.0 labels Nov 27, 2019
@ericsyh ericsyh removed this from the v1.0.3 milestone Dec 3, 2019
lichunzhu pushed a commit to lichunzhu/dm that referenced this pull request Apr 6, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
already-cherry-pick-1.0 The related PR is already cherry-picked to release-1.0. Add this label once the PR is cherry-picked priority/important Major change, requires approval from ≥2 primary reviewers status/LGT2 Two reviewers already commented LGTM, ready for merge type/bug-fix Bug fix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants