[GitHub] [arrow] eerhardt commented on a diff in pull request #13732: ARROW-17223: [C#] DecimalArray incorrectly appends values greater than Decimal.MaxValue / 2 and less than Decimal.MinValue / 2

[GitBox <gi...@apache.org>]

eerhardt commented on code in PR #13732:
URL: https://github.com/apache/arrow/pull/13732#discussion_r932336495


##########
csharp/src/Apache.Arrow/DecimalUtility.cs:
##########
@@ -99,7 +99,7 @@ internal static void GetBytes(decimal value, int precision, int scale, int byteW
                 }
                 bigInt = new BigInteger(bigIntBytes);
 #else
-            byte[] bigIntBytes = new byte[12];
+            byte[] bigIntBytes = new byte[13];

Review Comment:
   (nit) while we are in here, can we fix the existing whitespace issue?
   
   ```suggestion
                   byte[] bigIntBytes = new byte[13];
   ```



##########
csharp/test/Apache.Arrow.Tests/Decimal128ArrayTests.cs:
##########
@@ -97,6 +97,22 @@ public void AppendLargeDecimal()
                     Assert.Equal(-large, array.GetValue(1));
                 }
 
+                [Fact]
+                public void AppendMaxAndMinDecimal()
+                {
+                    // Assert
+                    var builder = new Decimal128Array.Builder(new Decimal128Type(29, 0));
+                    
+                    // Act
+                    builder.Append(Decimal.MaxValue);

Review Comment:
   I don't think it would hurt to also test a value slightly less than max, and slightly more than min as well.
   
   ```suggestion
                       builder.Append(Decimal.MaxValue);
                       builder.Append(Decimal.MaxValue - 10);
                       builder.Append(Decimal.MinValue + 10);
   ```



##########
csharp/test/Apache.Arrow.Tests/Decimal256ArrayTests.cs:
##########
@@ -97,6 +97,22 @@ public void AppendLargeDecimal()
                     Assert.Equal(-large, array.GetValue(1));
                 }
 
+                [Fact]
+                public void AppendMaxAndMinDecimal()
+                {
+                    // Assert
+                    var builder = new Decimal128Array.Builder(new Decimal128Type(29, 0));

Review Comment:
   This is the Decimal256 tests, but we are using Decimal128 here
   
   ```suggestion
                       var builder = new Decimal256Array.Builder(new Decimal128Type(29, 0));
   ```



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org