Skip to content

Commit

Permalink
fix(Spanner): Fix ToDecimal truncation for SpannerNumeric.
Browse files Browse the repository at this point in the history 
Towards #8285
  • Loading branch information
@amanda-tarafa
amanda-tarafa committed Jun 20, 2022
1 parent e011bf6 commit 2be4686
Show file tree
Hide file tree
Showing 2 changed files with 163 additions and 48 deletions.
97 changes: 64 additions & 33 deletions apis/Google.Cloud.Spanner.Data/Google.Cloud.Spanner.Data.Tests/V1/SpannerNumericTest.cs
View file
Expand Up @@ -50,20 +50,39 @@ public class SpannerNumericTest

[Theory]
[InlineData("1.123456789123", "1.123456789")]
[InlineData("-1.123456789123", "-1.123456789")]

// Validation that we actually truncate...
[InlineData("1.400000006001", "1.400000006")]
[InlineData("1.400000006499", "1.400000006")]
[InlineData("1.400000006500", "1.400000006")]
[InlineData("1.400000006501", "1.400000006")]
[InlineData("1.400000006901", "1.400000006")]

[InlineData("1.400000007001", "1.400000007")]
[InlineData("1.400000007499", "1.400000007")]
[InlineData("1.400000007500", "1.400000007")]
[InlineData("1.400000007501", "1.400000007")]
[InlineData("1.400000007901", "1.400000007")]

[InlineData("0.000000000000001", "0")]
[InlineData("-0.000000000000001", "0")]
public void ConversionFromDecimal_Lossy(string decimalText, string expectedText)
{
decimal input = decimal.Parse(decimalText, CultureInfo.InvariantCulture);
SpannerNumeric expected = SpannerNumeric.Parse(expectedText);
// It's easier to do this than to vary negation in other ways.
PerformTest(decimalText, expectedText);
PerformTest("-" + decimalText, "-" + expectedText);

SpannerNumeric conversionOutput = (SpannerNumeric)input;
SpannerNumeric fromDecimalOutput = SpannerNumeric.FromDecimal(input, LossOfPrecisionHandling.Truncate);
static void PerformTest(string decimalText, string expectedText)
{
decimal input = decimal.Parse(decimalText, CultureInfo.InvariantCulture);
SpannerNumeric expected = SpannerNumeric.Parse(expectedText);

SpannerNumeric conversionOutput = (SpannerNumeric)input;
SpannerNumeric fromDecimalOutput = SpannerNumeric.FromDecimal(input, LossOfPrecisionHandling.Truncate);

Assert.Equal(expected, conversionOutput);
Assert.Equal(expected, fromDecimalOutput);
Assert.Throws<ArgumentException>(() => SpannerNumeric.FromDecimal(input, LossOfPrecisionHandling.Throw));
Assert.Equal(expected, conversionOutput);
Assert.Equal(expected, fromDecimalOutput);
Assert.Throws<ArgumentException>(() => SpannerNumeric.FromDecimal(input, LossOfPrecisionHandling.Throw));
}
}

[Theory]
Expand Down Expand Up @@ -105,7 +124,11 @@ public void ConversionFromUInt64(ulong input)
[CombinatorialValues(
"79228162514264337593543950335", // decimal.MaxValue
"0.000000001", // Epsilon for numeric
"12345678901234567890.123456789" // Maximum significant digits with 9dps
"12345678901234567890.123456789", // Maximum significant digits with 9dps
"72345678901234567890.123456789", // For scaled values in [-(2^96 - 1), (2^96 - 1)] we can have 29 significant digits.
"8234567890123456789.012345678", // For scaled values outside of [-(2^96 - 1), (2^96 - 1)] we can have 28 significant digits only.
"0.1234",
"12300000000"
)]
string text, LossOfPrecisionHandling handling, bool negate)
{
Expand All @@ -120,37 +143,45 @@ public void ConversionFromUInt64(ulong input)
}

[Theory, CombinatorialData]
public void ToDecimal_Overflow(LossOfPrecisionHandling handling, bool negate)
public void ToDecimal_Overflow([CombinatorialValues(
"79228162514264337593543950336" // decimal.MaxValue + 1
)] string text, LossOfPrecisionHandling handling)
{
string text = "79228162514264337593543950336"; // decimal.MaxValue + 1
SpannerNumeric numeric = SpannerNumeric.Parse(text);
if (negate)
PerformTest(text, handling);
PerformTest("-" + text, handling);

static void PerformTest(string text, LossOfPrecisionHandling handling)
{
numeric = -numeric;
SpannerNumeric numeric = SpannerNumeric.Parse(text);
Assert.Throws<OverflowException>(() => numeric.ToDecimal(handling));
}
Assert.Throws<OverflowException>(() => numeric.ToDecimal(handling));
}

[Theory, CombinatorialData]
public void ToDecimal_LossOfPrecision_Truncate(
[CombinatorialValues(
"79228162514264337593543950335.0000000001", // decimal.MaxValue + epsilon; doesn't count as overflow
"123456789012345678900.123456789" // Simpler example of more significant digits than decimal can handle
)]
string text, bool negate)
[Theory]
[InlineData("79228162514264337593543950335.0000000001", "79228162514264337593543950335")]
[InlineData("0.1234567899", "0.123456789")]
[InlineData("1.1234567899", "1.123456789")]
[InlineData("123456789012345678900.123456780", "123456789012345678900.12345678")]
[InlineData("123456789012345678900.123456785", "123456789012345678900.12345678")]
[InlineData("123456789012345678900.123456789", "123456789012345678900.12345678")]
[InlineData("123456789012345678900.123456770", "123456789012345678900.12345677")]
[InlineData("123456789012345678900.123456775", "123456789012345678900.12345677")]
[InlineData("123456789012345678900.123456779", "123456789012345678900.12345677")]
public void ToDecimal_LossOfPrecision_Truncate(string text, string expectedDecimalText)
{
if (negate)
// It's easier to do this than to vary negation in other ways.
PerformTest(text, expectedDecimalText);
PerformTest("-" + text, "-" + expectedDecimalText);

static void PerformTest(string text, string expectedDecimalText)
{
text = "-" + text;
SpannerNumeric numeric = SpannerNumeric.Parse(text);
decimal actual = numeric.ToDecimal(LossOfPrecisionHandling.Truncate);
Assert.Equal(expectedDecimalText, actual.ToString(CultureInfo.InvariantCulture));
// Conversion via the explicit conversion should do the same thing
decimal actual2 = (decimal) numeric;
Assert.Equal(actual, actual2);
}
SpannerNumeric numeric = SpannerNumeric.Parse(text);
// Decimal.Parse will silently lose precision
decimal expected = decimal.Parse(text, CultureInfo.InvariantCulture);
decimal actual = numeric.ToDecimal(LossOfPrecisionHandling.Truncate);
Assert.Equal(expected, actual);
// Conversion via the explicit conversion should do the same thing
decimal actual2 = (decimal)numeric;
Assert.Equal(expected, actual2);
}

[Theory, CombinatorialData]
Expand Down
114 changes: 99 additions & 15 deletions apis/Google.Cloud.Spanner.V1/Google.Cloud.Spanner.V1/SpannerNumeric.cs
View file
Expand Up @@ -217,7 +217,38 @@ private static string TryParseImpl(string text, out SpannerNumeric value)
/// the value is between -1 and 1 exclusive, a "0" character is included before the decimal point.
/// </summary>
/// <returns>A canonical string representation of this value.</returns>
public override string ToString()
public override string ToString() => ToString((_, _, decimalPlaces) => decimalPlaces);

/// <summary>
/// Returns a canonical string representation of this value. This always uses "." as a decimal point.
/// If the value is between -1 and 1 exclusive, a "0" character is included before the decimal point.
/// Trailing zeros are not included.
/// The number representation will be truncated to the amount of decimal places as returned by
/// <paramref name="decimalPrecisionCalculator"/>.
/// </summary>
/// <param name="decimalPrecisionCalculator">
/// <para>
/// A function to calculate how many decimal places should the
/// representation be truncated to. This number should be greater or equal to zero and less than or equal to
/// the total decimal places in the number.
/// </para>
/// <para>
/// The parameters of this function are:
/// <list type="bullet">
/// <item>
/// The text representation of the the scaled value (scaled to 9 decimal places)
/// Will contain the '-' if the number is negative.
/// </item>
/// <item>
/// The number of integer places in the value. This may be zero.
/// </item>
/// <item>
/// The number of decimal places in the value, not including trailing zeros. This may be zero.
/// </item>
/// </list>
/// </para>
/// </param>
private string ToString(Func<string, int, int, int> decimalPrecisionCalculator)
{
int sign = _integer.Sign;
if (sign == 0)
Expand All @@ -226,7 +257,8 @@ public override string ToString()
}
var integerText = _integer.ToString(CultureInfo.InvariantCulture);

int decimalPlaces = 9;
int scale = 9;
int decimalPlaces = scale;
for (int place = integerText.Length - 1; place > 0 && decimalPlaces > 0; place--)
{
if (integerText[place] != '0')
Expand All @@ -239,9 +271,12 @@ public override string ToString()
int signLength = sign > 0 ? 0 : 1;

// Always have something before the period, even if it's just a 0
int originalIntegerPlaces = Math.Max(integerText.Length - signLength - 9, 0);
int originalIntegerPlaces = Math.Max(integerText.Length - signLength - scale, 0);
int resultIntegerPlaces = Math.Max(originalIntegerPlaces, 1);

// Decimal precision now that we know how many digits are in the integer part.
decimalPlaces = decimalPrecisionCalculator(integerText, originalIntegerPlaces, decimalPlaces);

// Sign, then integer part, then period (maybe), then significant decimals
int resultLength = signLength + resultIntegerPlaces + (decimalPlaces > 0 ? 1 : 0) + decimalPlaces;
var chars = new char[resultLength];
Expand All @@ -263,11 +298,11 @@ public override string ToString()
{
chars[position++] = '.';
int decimalsToCopy = decimalPlaces;

// Fill zeroes if necessary
if (originalIntegerPlaces == 0)
{
int decimalsToFill = 9 - (integerText.Length - signLength);
int decimalsToFill = Math.Min(scale - (integerText.Length - signLength), decimalPlaces);
for (int i = 0; i < decimalsToFill; i++)
{
chars[position++] = '0';
Expand Down Expand Up @@ -379,27 +414,76 @@ public static SpannerNumeric FromDecimal(decimal value, LossOfPrecisionHandling
// Conversions from SpannerNumeric to CLR types.

/// <summary>
/// Converts this value to <see cref="SpannerNumeric"/>,
/// Converts this value to <see cref="decimal"/>,
/// </summary>
/// <remarks>
/// This conversion may silently lose precision, depending on <paramref name="lossOfPrecisionHandling"/>, but
/// will always throw <see cref="OverflowException"/> if value is out of the range of <see cref="Decimal"/>.
/// will always throw <see cref="OverflowException"/> if value is out of the range of <see cref="decimal"/>.
/// </remarks>
/// <param name="lossOfPrecisionHandling">How to handle values with signficant digits that would be lost by the conversion.</param>
/// <exception cref="OverflowException">This value is outside the range of <see cref="Decimal"/>.</exception>
/// <exception cref="OverflowException">This value is outside the range of <see cref="decimal"/>.</exception>
/// <returns>The converted value.</returns>
public decimal ToDecimal(LossOfPrecisionHandling lossOfPrecisionHandling)
{
// FIXME: Awful performance!
string text = ToString();
if (_integer == BigInteger.Zero)
{
return 0m;
}

// Handles overflow
decimal dec = Decimal.Parse(text, CultureInfo.InvariantCulture);
if (lossOfPrecisionHandling == LossOfPrecisionHandling.Throw && this != (SpannerNumeric) dec)
return decimal.Parse(ToString(DecimalPrecision), CultureInfo.InvariantCulture);

int DecimalPrecision(string scaledText, int integerPlaces, int decimalPlaces)
{
// TODO: Is this the right exception to use?
throw new InvalidOperationException("Conversion would lose information");
int decimalPrecision;
// If the number had no decimal places to begin with, then we need not to worry.
// If the number has 29 or more integer places we cannot include any decimal places;
// this may overflow when we convert to decimal but that's fine, let's not handle that here.
if (decimalPlaces == 0 || integerPlaces >= 29)
{
decimalPrecision = 0;
}
// If the number has at most 28 significant digits we can represent it fully.
// Note that this takes of the case where there are no integer places as well,
// since decimal places for SpannerNumeric are at most 9.
else if (integerPlaces + decimalPlaces <= 28)
{
decimalPrecision = decimalPlaces;
}
// integerPlaces + decimalPlaces >= 29 and
// 1 <= integerPlaces <= 28 which means we may have room for some of the decimal places
// but we need to find out if the number can have 28 or 29 significant digits.
else
{
int significantPlaces = 29;
string maxDecimalText = decimal.MaxValue.ToString(CultureInfo.InvariantCulture);
// Let's compare digit by digit to check if our unscaled number is greater than decimal max,
// in which case we can only have 28 significant places.
for (int i = scaledText.StartsWith("-") ? 1 : 0, j = 0; i < scaledText.Length && j < maxDecimalText.Length; i++, j++)
{
char valueDigit = scaledText[i];
char maxValueDigit = maxDecimalText[j];
if (valueDigit > maxValueDigit)
{
significantPlaces = 28;
break;
}
if (valueDigit < maxValueDigit)
{
break;
}
}
// We probably don't need Math.Min here, but it doesn't hurt either.
// Note that since we have at most 28 integer places the result here cannot be negative.
decimalPrecision = Math.Min(significantPlaces - integerPlaces, decimalPlaces);
}
// We truncated but we can't.
if (decimalPrecision < decimalPlaces && lossOfPrecisionHandling == LossOfPrecisionHandling.Throw)
{
throw new InvalidOperationException("Conversion would lose information");
}
return decimalPrecision;
}
return dec;
}

/// <summary>
Expand Down

0 comments on commit 2be4686

Please sign in to comment.