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

Properly encode BigDecimal values whose first digit is 10k #1046

Merged
merged 1 commit into from
Jul 27, 2017

Conversation

sgrif
Copy link
Member

@sgrif sgrif commented Jul 27, 2017

When serializing a numeric column, we need to take a decimal value and
convert it to a list of its digits in base 10k. The serialization code
had a bug when the first digit was exactly 10000, resulting in the
digits being [10000, ...] instead of [1, 0, ...]. This bug only
affected the integral part, as the decimal portion uses different logic
in order ot strip trailing zeroes.

Fixes #1044.

@sgrif sgrif requested review from killercup and Eijebong July 27, 2017 17:18
@sgrif
Copy link
Member Author

sgrif commented Jul 27, 2017

/cc @rubdos

When serializing a numeric column, we need to take a decimal value and
convert it to a list of its digits in base 10k. The serialization code
had a bug when the first digit was exactly `10000`, resulting in the
digits being `[10000, ...]` instead of `[1, 0, ...]`. This bug only
affected the integral part, as the decimal portion uses different logic
in order ot strip trailing zeroes.

Fixes #1044.
Copy link
Member

@Eijebong Eijebong left a comment

Choose a reason for hiding this comment

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

Nice test refactoring :)

LGTM

@sgrif sgrif merged commit 94bdd30 into master Jul 27, 2017
@sgrif sgrif deleted the sg-fix-decimal-encoding branch July 27, 2017 20:28
sgrif added a commit that referenced this pull request Jul 28, 2017
Properly encode `BigDecimal` values whose first digit is 10k
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