Skip to content

Support decimals #146

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
Merged
merged 2 commits into from
Jun 22, 2022
Merged

Support decimals #146

merged 2 commits into from
Jun 22, 2022

Conversation

ligurio
Copy link
Member

@ligurio ligurio commented Mar 7, 2022 •
edited
Loading

@ligurio ligurio marked this pull request as draft March 7, 2022 18:31
@ligurio ligurio force-pushed the ligurio/gh-96-decimal-support branch from a2552c3 to 2e0ebba Compare March 7, 2022 19:45
@ligurio ligurio force-pushed the ligurio/gh-96-decimal-support branch from 2e0ebba to 9096c23 Compare March 21, 2022 09:36
ligurio
ligurio commented Apr 11, 2022
View reviewed changes
@ligurio ligurio force-pushed the ligurio/gh-96-decimal-support branch 7 times, most recently from 7013bd5 to d5ed94a Compare April 21, 2022 11:55
@ligurio ligurio force-pushed the ligurio/gh-96-decimal-support branch 8 times, most recently from 17c2d22 to 7f6189e Compare April 26, 2022 13:21
@ligurio ligurio marked this pull request as ready for review April 27, 2022 16:37
@ligurio ligurio force-pushed the ligurio/gh-96-decimal-support branch 2 times, most recently from ee61c0d to 42c35f2 Compare April 28, 2022 08:56
@ligurio ligurio force-pushed the ligurio/gh-96-decimal-support branch 5 times, most recently from 223e59c to cb82992 Compare May 5, 2022 11:16
DifferentialOrange
DifferentialOrange reviewed May 23, 2022
View reviewed changes
@ligurio ligurio requested a review from Totktonada May 24, 2022 13:38
@ligurio ligurio force-pushed the ligurio/gh-96-decimal-support branch from 1e197fb to cf766bc Compare May 24, 2022 14:24
@Totktonada Totktonada requested review from oleg-jukovec and removed request for Totktonada June 9, 2022 10:21
@oleg-jukovec oleg-jukovec self-assigned this Jun 9, 2022
oleg-jukovec
oleg-jukovec approved these changes Jun 10, 2022
View reviewed changes
Copy link
Collaborator

@oleg-jukovec oleg-jukovec left a comment

Choose a reason for hiding this comment

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

Thank you for the pull request! Please, do rebase (be careful with CHANGELOG.md), resolve the notes and I'll merge it.

@oleg-jukovec oleg-jukovec mentioned this pull request Jun 10, 2022
Merged
DifferentialOrange
DifferentialOrange approved these changes Jun 16, 2022
View reviewed changes
Copy link
Member

@DifferentialOrange DifferentialOrange left a comment

Choose a reason for hiding this comment

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

Should be ok

@ligurio ligurio force-pushed the ligurio/gh-96-decimal-support branch 4 times, most recently from 740a0c9 to a034f5c Compare June 21, 2022 16:40
@ligurio
Copy link
Member Author

ligurio commented Jun 21, 2022 •
edited
Loading

Changes:

  • switched to MarshalMsgpack()/UnmarshalMsgpack() (in a separate commit)
  • added tests that covers encoding/decoding to MsgPack without Tarantool (in a separate commit)

NOTE: New changes are not squashed just for reviewer convenience.

@ligurio ligurio requested a review from oleg-jukovec June 21, 2022 16:46
@oleg-jukovec
Copy link
Collaborator

@Totktonada said I could continue work in the pull request. Is it okay?

@ligurio ligurio force-pushed the ligurio/gh-96-decimal-support branch from a034f5c to 9360806 Compare June 22, 2022 07:59
@oleg-jukovec oleg-jukovec force-pushed the ligurio/gh-96-decimal-support branch from 9360806 to 5e7dc18 Compare June 22, 2022 12:38
ligurio and others added 2 commits June 22, 2022 16:26
@ligurio @oleg-jukovec
decimal: add support decimal type in msgpack
1e1b120 
This patch provides decimal support for all space operations and as
function return result. Decimal type was introduced in Tarantool 2.2.
See more about decimal type in [1] and [2].

According to BCD encoding/decoding specification sign is encoded by
letters: '0x0a', '0x0c', '0x0e', '0x0f' stands for plus, and '0x0b' and
'0x0d' for minus. Tarantool always uses '0x0c' for plus and '0x0d' for
minus. Implementation in Golang follows the same rule and in all test
samples sign encoded by '0x0d' and '0x0c' for simplification.

Because 'c' used by Tarantool.

To use decimal with github.com/shopspring/decimal in msgpack, import
tarantool/decimal submodule.

1. https://www.tarantool.io/en/doc/latest/book/box/data_model/
2. https://www.tarantool.io/en/doc/latest/dev_guide/internals/msgpack_extensions/#the-decimal-type
3. https://github.com/douglascrockford/DEC64/blob/663f562a5f0621021b98bfdd4693571993316174/dec64_test.c#L62-L104
4. https://github.com/shopspring/decimal/blob/v1.3.1/decimal_test.go#L27-L64
5. https://github.com/tarantool/tarantool/blob/60fe9d14c1c7896aa7d961e4b68649eddb4d2d6c/test/unit/decimal.c#L154-L171

Lua snippet for encoding number to MsgPack representation:

local decimal = require('decimal')
local msgpack = require('msgpack')

local function mp_encode_dec(num)
    local dec = msgpack.encode(decimal.new(num))
    return dec:gsub('.', function (c)
        return string.format('%02x', string.byte(c))
    end)
end
print(mp_encode_dec(-12.34)) -- 0xd6010201234d

Follows up tarantool/tarantool#692
Part of #96

Co-authored-by: Oleg Jukovec <[email protected]>
@ligurio @oleg-jukovec
decimal: add fuzzing test
b699fd9 
Fuzzing tests in Golang, see [1] and [2], requires Go 1.18+. However in
CI we use Go 1.13 that fails on running fuzzing tests. To avoid this
fuzzing test has been moved to a separate file an marked with build tag.

1. https://go.dev/doc/tutorial/fuzz
2. https://go.dev/doc/fuzz/

Closes #96

Co-authored-by: Oleg Jukovec <[email protected]>
@oleg-jukovec oleg-jukovec force-pushed the ligurio/gh-96-decimal-support branch from b84525b to b699fd9 Compare June 22, 2022 13:27
@oleg-jukovec
Copy link
Collaborator

I made a lot of changes without consulting with @ligurio , so I added myself as co-author of this commits. This is done so that @ligurio will not be shamed for my changes. I had no intention of appropriating his great work.

@oleg-jukovec oleg-jukovec merged commit 624b728 into master Jun 22, 2022
@oleg-jukovec oleg-jukovec deleted the ligurio/gh-96-decimal-support branch June 22, 2022 13:52
@oleg-jukovec oleg-jukovec removed their assignment Jun 30, 2022
@oleg-jukovec oleg-jukovec removed their request for review June 30, 2022 08:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oleg-jukovec oleg-jukovec oleg-jukovec approved these changes

@funny-falcon funny-falcon funny-falcon approved these changes

@0x501D 0x501D 0x501D approved these changes

@DifferentialOrange DifferentialOrange DifferentialOrange approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support DECIMAL type
5 participants
@ligurio @funny-falcon @oleg-jukovec @0x501D @DifferentialOrange