Skip to content

Commit

Permalink
Use coef: i128 in decimal (#1017)
Browse files Browse the repository at this point in the history
* Bump up the number of bits from 64 to 128

Before we were using `coef: u64` as that was the
largest type that was also guarunteed to be
positive. This left half the bits unused, however.
This change makes it so we're using all available
bits. Even though technically we could make an
invalid ExDecimal after this change, it's not a
practical concern.

* Increase precision max to 38 in generator

This reverts an old change that was a work around
for the `coef: u64` thing.

* Remove misleading comment

* Fix linting
  • Loading branch information
billylanchantin authored Nov 15, 2024
1 parent 5608c27 commit c07ec04
Show file tree
Hide file tree
Showing 3 changed files with 31 additions and 12 deletions.
18 changes: 10 additions & 8 deletions native/explorer/src/datatypes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -569,23 +569,25 @@ impl Literal for ExTime {
#[module = "Decimal"]
pub struct ExDecimal {
pub sign: i8,
pub coef: u64,
// `coef` is a positive, arbitrary precision integer on the Elixir side.
// It's convenient to represent it here as a signed `i128` because that's
// what the Decimal dtype expects. While you could technically create an
// `ExDecimal` struct with a negative `coef`, it's not a practical concern.
pub coef: i128,
pub exp: i64,
}

impl ExDecimal {
pub fn new(signed_coef: i128, scale: usize) -> Self {
Self {
coef: signed_coef
.abs()
.try_into()
.expect("signed coef is too large for u64"),
coef: signed_coef.abs(),
sign: if signed_coef >= 0 { 1 } else { -1 },
exp: -(scale as i64),
}
}

pub fn signed_coef(self) -> i128 {
self.sign as i128 * self.coef as i128
self.sign as i128 * self.coef
}

pub fn scale(self) -> usize {
Expand All @@ -600,9 +602,9 @@ impl Literal for ExDecimal {
fn lit(self) -> Expr {
Expr::Literal(LiteralValue::Decimal(
if self.sign.is_positive() {
self.coef.into()
self.coef
} else {
-(self.coef as i128)
-self.coef
},
usize::try_from(-(self.exp)).expect("exponent should fit an usize"),
))
Expand Down
17 changes: 17 additions & 0 deletions test/explorer/series_test.exs
Original file line number Diff line number Diff line change
Expand Up @@ -507,6 +507,23 @@ defmodule Explorer.SeriesTest do
Series.from_list([[Decimal.new("3.21")], []], dtype: {:list, {:decimal, 3, 2}})
end

test "decimal precision boundary" do
# Biggest number representable by an i128.
max_i128 = 2 ** 127 - 1
biggest = Decimal.new(max_i128)
too_big = Decimal.new(max_i128 + 1)

# Can make a series using the biggest allowed decimal.
s1 = Series.from_list([biggest])
assert [^biggest] = Series.to_list(s1)

# Can't make a series using a number bigger than the biggest allowed decimal.
assert_raise RuntimeError,
"Generic Error: cannot decode a valid decimal from term;" <>
" check that `coef` fits into an `i128`. error: throw(<term>)",
fn -> Series.from_list([too_big]) end
end

test "mixing dates and integers with `:date` dtype" do
s = Series.from_list([1, nil, ~D[2024-06-13]], dtype: :date)

Expand Down
8 changes: 4 additions & 4 deletions test/support/generator.ex
Original file line number Diff line number Diff line change
Expand Up @@ -295,8 +295,8 @@ defmodule Explorer.Generator do
date: constant(:date),
datetime: tuple({constant(:datetime), time_unit(), constant("Etc/UTC")}),
decimal:
bind(integer(0..19), fn scale ->
bind(integer((scale + 1)..20), fn precision ->
bind(integer(0..37), fn scale ->
bind(integer((scale + 1)..38), fn precision ->
tuple({constant(:decimal), constant(precision), constant(scale)})
end)
end),
Expand Down Expand Up @@ -397,7 +397,7 @@ defmodule Explorer.Generator do
|> map(&elem(&1, 1))
end

@max_u64 2 ** 64 - 1
@max_i128 2 ** 127 - 1
@spec datetime(pos_integer(), pos_integer()) :: gen(Decimal.t())
defp decimal(precision, scale) do
# Smallest integer with `(scale + 1)` digits.
Expand All @@ -407,7 +407,7 @@ defmodule Explorer.Generator do

tuple({
one_of([constant("-"), constant("+")]),
integer(min_scale..min(max_precision, @max_u64))
integer(min_scale..min(max_precision, @max_i128))
})
|> map(fn {sign, coef} ->
# By construction, we are guaranteed that precision > scale.
Expand Down

0 comments on commit c07ec04

Please sign in to comment.