From c07ec0425ae721d6e568749e0ad5f6da955b163e Mon Sep 17 00:00:00 2001
From: Billy Lanchantin <william.lanchantin@cargosense.com>
Date: Thu, 14 Nov 2024 19:04:40 -0500
Subject: [PATCH] Use `coef: i128` in decimal (#1017)

* 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
---
 native/explorer/src/datatypes.rs | 18 ++++++++++--------
 test/explorer/series_test.exs    | 17 +++++++++++++++++
 test/support/generator.ex        |  8 ++++----
 3 files changed, 31 insertions(+), 12 deletions(-)

diff --git a/native/explorer/src/datatypes.rs b/native/explorer/src/datatypes.rs
index 7fabe5dca..78c96e0d9 100644
--- a/native/explorer/src/datatypes.rs
+++ b/native/explorer/src/datatypes.rs
@@ -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 {
@@ -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"),
         ))
diff --git a/test/explorer/series_test.exs b/test/explorer/series_test.exs
index df750af44..1de1cf515 100644
--- a/test/explorer/series_test.exs
+++ b/test/explorer/series_test.exs
@@ -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)
 
diff --git a/test/support/generator.ex b/test/support/generator.ex
index 172fdfcb1..a2a0402e0 100644
--- a/test/support/generator.ex
+++ b/test/support/generator.ex
@@ -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),
@@ -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.
@@ -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.