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.

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

msgpack: support decimals with negative scale #298

Merged
merged 2 commits into from
Jun 30, 2023

Conversation

DifferentialOrange
Copy link
Member

@DifferentialOrange DifferentialOrange commented Jun 29, 2023

Current decimal external type parser do not expect negative scale in decimal payload. Negative scale is a positive exponent. It seems that the only way to obtain positive exponent is to use E-notation since Tarantool library do not truncate trailing zeroes before decimal point:

tarantool> msgpack.encode(decimal.new('1e33')):hex()
---
- c70301d0df1c
...

tarantool> msgpack.encode(decimal.new('1000000000000000000000000000000000')):hex()
---
- c713010001000000000000000000000000000000000c
...

There are two different bugs in current implementation:

  • we support only positive fixint scale and do not expect int 8 [2],
  • we do not expect negative scale so positive exponent will be ignored.

This patch fixes both of them. See also [3].

  1. https://github.com/tarantool/tarantool/blob/ba749e820bf0638aa3f79f266848590f9713c1cf/src/lib/core/decimal.c#L432-L450
  2. https://github.com/msgpack/msgpack/blob/master/spec.md
  3. decimal: incorrect MP_DECIMAL decoding with scale < 0 go-tarantool#314

Current decimal external type parser do not expect negative scale in
decimal payload. Negative scale is a positive exponent. It seems that
the only way to obtain positive exponent is to use E-notation since
Tarantool library do not truncate trailing zeroes before decimal point:

```
tarantool> msgpack.encode(decimal.new('1e33')):hex()
---
- c70301d0df1c
...

tarantool> msgpack.encode(decimal.new('1000000000000000000000000000000000')):hex()
---
- c713010001000000000000000000000000000000000c
...
```

There are two different bugs in current implementation:
- we support only `positive fixint` scale and do not expect `int 8` [2],
- we do not expect negative scale so positive exponent will be ignored.

This patch fixes both of them. See also [3].

1. https://github.com/tarantool/tarantool/blob/ba749e820bf0638aa3f79f266848590f9713c1cf/src/lib/core/decimal.c#L432-L450
2. https://github.com/msgpack/msgpack/blob/master/spec.md
3. tarantool/go-tarantool#314
@DifferentialOrange DifferentialOrange force-pushed the DifferentialOrange/decimal-negative-scale branch from 0767d7e to 086cdca Compare June 29, 2023 12:55
@DifferentialOrange DifferentialOrange changed the title wip msgpack: support decimals with negative scale Jun 29, 2023
@DifferentialOrange DifferentialOrange marked this pull request as ready for review June 29, 2023 12:59
msgpack-python package was deprecated in 2018 in favor of msgpack
package. After previous patch in the patchset, we require `Unpacker`
`tell` handle and msgpack-python 0.4.0 do not yet have it. It seems
to be no reason to support the package which is not supported for the
five years already, so we better drop it.
Copy link
Contributor

@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 patch!

@DifferentialOrange DifferentialOrange merged commit 71f140f into master Jun 30, 2023
@DifferentialOrange DifferentialOrange deleted the DifferentialOrange/decimal-negative-scale branch June 30, 2023 07:23
DifferentialOrange added a commit that referenced this pull request Jun 30, 2023
Overview

  This release inroduces API to request server protocol version and
  feature, as well as introduce decimal bugfix.

Breaking changes

  - Drop `msgpack-python` support. (Package not supported since 2019.)
    Use `msgpack` instead.

Added
  - Allow to require specific server protocol version
    and features (#267).

Fixed
  - Parsing of E-notation Tarantool decimals with positive
    exponent (PR #298).
@DifferentialOrange DifferentialOrange mentioned this pull request Jun 30, 2023
DifferentialOrange added a commit that referenced this pull request Jun 30, 2023
Overview

  This release introduces API to request server protocol version and
  feature, as well as introduce decimal bugfix.

Breaking changes

  - Drop `msgpack-python` support. (Package not supported since 2019.)
    Use `msgpack` instead.

Added
  - Allow to require specific server protocol version
    and features (#267).

Fixed
  - Parsing of E-notation Tarantool decimals with positive
    exponent (PR #298).
DifferentialOrange added a commit that referenced this pull request Jun 30, 2023
Overview

  This release introduces API to request server protocol version and
  feature, as well as introduce decimal bugfix.

Breaking changes

  - Drop `msgpack-python` support. (Package not supported since 2019.)
    Use `msgpack` instead.

Added
  - Allow to require specific server protocol version
    and features (#267).

Fixed
  - Parsing of E-notation Tarantool decimals with positive
    exponent (PR #298).
DifferentialOrange added a commit that referenced this pull request Jun 30, 2023
Overview

  This release introduces API to request server protocol version and
  feature, as well as introduce decimal bugfix.

Breaking changes

  - Drop `msgpack-python` support. (Package not supported since 2019.)
    Use `msgpack` instead.

Added
  - Allow to require specific server protocol version
    and features (#267).

Fixed
  - Parsing of E-notation Tarantool decimals with positive
    exponent (PR #298).
DifferentialOrange added a commit that referenced this pull request Jun 30, 2023
Overview

  This release introduces API to request server protocol version and
  feature, as well as introduce decimal bugfix.

Breaking changes

  - Drop `msgpack-python` support. (Package not supported since 2019.)
    Use `msgpack` instead.

Added
  - Allow to require specific server protocol version
    and features (#267).

Fixed
  - Parsing of E-notation Tarantool decimals with positive
    exponent (PR #298).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants