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) #376

Merged
merged 1 commit into from
Nov 28, 2019

Conversation

sre-bot
Copy link

@sre-bot sre-bot commented Nov 27, 2019

cherry-pick #375 to release-1.0


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

@sre-bot sre-bot added priority/important Major change, requires approval from ≥2 primary reviewers type/bug-fix Bug fix type/cherry-pick This PR is just a cherry-pick (backport) labels Nov 27, 2019
@csuzhangxc
Copy link
Member

/run-all-tests tidb=release-3.0

@codecov
Copy link

codecov bot commented Nov 27, 2019

Codecov Report

❗ No coverage uploaded for pull request base (release-1.0@b99f0b7). Click here to learn what that means.
The diff coverage is n/a.

@@               Coverage Diff                @@
##             release-1.0       #376   +/-   ##
================================================
  Coverage               ?   57.3099%           
================================================
  Files                  ?        158           
  Lines                  ?      16074           
  Branches               ?          0           
================================================
  Hits                   ?       9212           
  Misses                 ?       5956           
  Partials               ?        906

@WangXiangUSTC
Copy link
Contributor

LGTM

@WangXiangUSTC WangXiangUSTC added the status/LGT1 One reviewer already commented LGTM label Nov 27, 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/LGT2 Two reviewers already commented LGTM, ready for merge and removed status/LGT1 One reviewer already commented LGTM labels Nov 28, 2019
@amyangfei amyangfei merged commit 751f8fd into pingcap:release-1.0 Nov 28, 2019
@ericsyh ericsyh added this to the v1.0.3 milestone Dec 3, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
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 type/cherry-pick This PR is just a cherry-pick (backport)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants