Skip to content

Commit

Permalink
fix(Spanner): Fixes ToDecimal truncation for PgNumeric.
Browse files Browse the repository at this point in the history 
Closes #8285
  • Loading branch information
@amanda-tarafa
amanda-tarafa committed Jun 20, 2022
1 parent 2be4686 commit 3ff15fb
Show file tree
Hide file tree
Showing 2 changed files with 204 additions and 55 deletions.
133 changes: 96 additions & 37 deletions apis/Google.Cloud.Spanner.Data/Google.Cloud.Spanner.Data.Tests/V1/PgNumericTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -311,6 +311,7 @@ public void Parse_Invalid(string input)
[InlineData("NaN", "NaN")]
[InlineData("1.00", "1.00")]
[InlineData("1.000000", "1.000000")]
[InlineData("0.0000000000000", "0.0000000000000")]
[InlineData("99999.999999000", "99999.999999000")]
[InlineData("-0.000001", "-0.000001")]
[InlineData("100", "100")]
Expand All @@ -329,56 +330,114 @@ public void FromDecimal(decimal input, string expectedValue)
Assert.Equal(expectedValue, numeric.ToString());
}

[Theory]
[InlineData("0.00")]
[InlineData("1.00")]
[InlineData("1.01")]
[InlineData("12345.123456000")]
[InlineData("-0.000001")]
[InlineData("1234567891.123456789123")]
[InlineData("-12345678912.12345678912")]
[InlineData("123456789123456789.12345678912")]
[InlineData("12345678912345678912345678.1234567891234567891234")]
[InlineData("9999999999999999999999999999.999999999999999999999999")]
[InlineData("100")]
[InlineData("1.33333333333333333")]
public void ToDecimal(string input)
[Theory, CombinatorialData]
public void ToDecimal_Precise(
[CombinatorialValues(
"79228162514264337593543950335", // decimal.MaxValue
"0.0000000000000000000000000001", // Epsilon for decimal
"7.9228162514264337593543950335", // Maximum significant digits with 28 dps which is the max dps for decimal
"7.2345678901234567890123456789",
"8.234567890123456789012345678",
"0.8234567890123456789012345678",
"0.1234",
"12300000000"
)]
string text, LossOfPrecisionHandling handling, bool negate)
{
var numeric = PgNumeric.Parse(input);
// decimal.Parse will silently lose precision if input has more significant digits than decimal can handle.
decimal expected = decimal.Parse(input, CultureInfo.InvariantCulture);
decimal actual = numeric.ToDecimal(LossOfPrecisionHandling.Truncate);
if (negate)
{
text = "-" + text;
}
PgNumeric numeric = PgNumeric.Parse(text);
decimal expected = decimal.Parse(text, CultureInfo.InvariantCulture);
decimal actual = numeric.ToDecimal(handling);
Assert.Equal(expected, actual);
// Conversion via the explicit conversion should do the same thing.
decimal actual2 = (decimal)numeric;
Assert.Equal(expected, actual2);
}

[Theory, CombinatorialData]
public void ToDecimal_Overflow([CombinatorialValues(
"79228162514264337593543950336", // decimal.MaxValue + 1
"123456789012345678901234567890" // 30 integer places
)] string text, LossOfPrecisionHandling handling)
{
PerformTest(text, handling);
PerformTest("-" + text, handling);

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

[Theory]
[InlineData("123456789012345678900.123456789", "123456789012345678900.12345679")]
[InlineData("79228162514264337593543950335.0000000001", "79228162514264337593543950335")]
public void ToDecimal_LossOfPrecision_Truncate(string input, string expectedValue)
[InlineData("0.1234567891234567891234567899999", "0.1234567891234567891234567899")]
[InlineData("1.1234567891234567891234567899999", "1.1234567891234567891234567899")]
[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")]
// For scaled values in [-(2^96 - 1), (2^96 - 1)] we can have 29 significant digits.
[InlineData("7.2345678901234567890123456789", "7.2345678901234567890123456789")]
// For scaled values outside of [-(2^96 - 1), (2^96 - 1)] we can have 28 significant digits only.
[InlineData("8.2345678901234567890123456789", "8.234567890123456789012345678")]
[InlineData("0.82345678901234567890123456789", "0.8234567890123456789012345678")]
[InlineData("9999999999999999999999999999.999999999999999999999999", "9999999999999999999999999999")]
public void ToDecimal_LossOfPrecision_Truncate(string text, string expectedDecimalText)
{
// Decimal can take upto 29 digits. If input has more significant digits than decimal can handle, it will be truncated and
// there will be loss of precision.
var numeric = PgNumeric.Parse(input);
var convertedDecimalString = numeric.ToDecimal(LossOfPrecisionHandling.Truncate).ToString(CultureInfo.InvariantCulture);
Assert.Equal(expectedValue, convertedDecimalString);
// 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)
{
PgNumeric numeric = PgNumeric.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);
}
}

[Fact]
public void ToDecimal_Overflow()
[Theory]
[InlineData("0.00000000000000000000000000000000000000", "0.0000000000000000000000000000")]
[InlineData("-0.00000000000000000000000000000000000000", "0.0000000000000000000000000000")]
// We test zero on its own as the minus sign is not included on the string representation.
public void ToDecimal_LossOfPrecision_Truncate_Zero(string text, string expectedDecimalText)
{
var numeric = PgNumeric.Parse("99999999999999999999999999999999.9999999999999999999999999999");
Assert.Throws<OverflowException>(() => numeric.ToDecimal(LossOfPrecisionHandling.Truncate));
PgNumeric numeric = PgNumeric.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);
}

[Theory]
[InlineData("12345678.1234567891234567891234567891")]
[InlineData("123456789012345678900.123456789")]
public void ToDecimal_LossOfPrecision_Throw(string input)
[Theory, CombinatorialData]
public void ToDecimal_LossOfPrecision_Throw(
[CombinatorialValues(
// decimal.MaxValue + epsilon; doesn't count as overflow
"79228162514264337593543950335.00000000000000000000000000000000000001",
// Simpler example of more significant digits than decimal can handle
"123456789012345678900.123456789",
// Even for zero, this loses information as there are more significant digits than decimal can handle.
// Trailing zeroes are significant here because they are relevant for determining precission.
"0.00000000000000000000000000000000000000",
// Or for any number really, this loses information as there are more significant digits than decimal can handle
// Trailing zeroes are significant here because they are relevant for determining precission.
"1.00000000000000000000000000000000000000"
)]
string text, bool negate)
{
var numeric = PgNumeric.Parse(input);
if (negate)
{
text = "-" + text;
}
PgNumeric numeric = PgNumeric.Parse(text);
Assert.Throws<InvalidOperationException>(() => numeric.ToDecimal(LossOfPrecisionHandling.Throw));
}

Expand Down
126 changes: 108 additions & 18 deletions apis/Google.Cloud.Spanner.V1/Google.Cloud.Spanner.V1/PgNumeric.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -275,7 +275,39 @@ private static string TryParseImpl(string text, out PgNumeric value)
/// If 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 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 <see cref="_scale"/> or
/// the empty string for zero.
/// Will contain the '-' if the number is negative. It won't contain the trailing zeros.
/// </item>
/// <item>
/// The number of integer places in the value. This may be zero.
/// </item>
/// <item>
/// The number of decimal places in the value, including trailing zeros. This may be zero.
/// </item>
/// </list>
/// </para>
/// </param>
private string ToString(Func<string, int, int, int> decimalPrecisionCalculator)
{
if (_notANumber)
{
Expand All @@ -285,25 +317,28 @@ public override string ToString()
int sign = _scaledValue.Sign;
if (sign == 0)
{
return _trailingZeros == 0 ? "0" : "0." + new string('0', _trailingZeros);
return _trailingZeros == 0 ? "0" : "0." + new string('0', decimalPrecisionCalculator("", 0, _trailingZeros));
}

string integerText = _scaledValue.ToString(CultureInfo.InvariantCulture);
var integerText = _scaledValue.ToString(CultureInfo.InvariantCulture);

int signLength = sign > 0 ? 0 : 1;

// We can have a scaled value or a non-scaled value, check if its scaled.
if (_scale == 0)
{
return _trailingZeros == 0 ? integerText : integerText + '.' + new string('0', _trailingZeros);
return _trailingZeros == 0 ? integerText : integerText + '.' + new string('0',decimalPrecisionCalculator(integerText, integerText.Length - signLength, _trailingZeros));
}

int signLength = sign > 0 ? 0 : 1;

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

// Sign, then integer part, then period (maybe), then significant decimals, then trailing zeros.
int resultLength = signLength + resultIntegerPlaces + (_scale > 0 ? 1 : 0) + _scale + _trailingZeros;
// Decimal precision now that we know how many digits are in the integer part.
int decimalPlaces = decimalPrecisionCalculator(integerText, originalIntegerPlaces, _scale + _trailingZeros);

// Sign, then integer part, then period (maybe), then allowed decimal places
int resultLength = signLength + resultIntegerPlaces + (_scale > 0 ? 1 : 0) + decimalPlaces;
var chars = new char[resultLength];
int targetPosition = 0;
int sourcePosition = 0;
Expand All @@ -324,15 +359,15 @@ public override string ToString()
// Fill zeroes if necessary.
if (originalIntegerPlaces == 0)
{
int decimalsToFill = _scale - (integerText.Length - signLength);
int decimalsToFill = Math.Min(decimalPlaces, _scale - (integerText.Length - signLength));
for (int i = 0; i < decimalsToFill; i++)
{
chars[targetPosition++] = '0';
}
}

// Now the significant digits in the remaining text.
for (; sourcePosition < integerText.Length; sourcePosition++)
for (; sourcePosition < integerText.Length && targetPosition < chars.Length; sourcePosition++)
{
chars[targetPosition++] = integerText[sourcePosition];
}
Expand Down Expand Up @@ -431,15 +466,70 @@ public static PgNumeric FromDecimal(decimal value)
/// <returns>The converted value.</returns>
public decimal ToDecimal(LossOfPrecisionHandling lossOfPrecisionHandling)
{
// FIXME: Awful performance!
string text = ToString();
// Handles overflow.
decimal dec = Decimal.Parse(text, CultureInfo.InvariantCulture);
if (lossOfPrecisionHandling == LossOfPrecisionHandling.Throw && this != dec)
if (_notANumber)
{
throw new InvalidOperationException("Conversion would lose information");
throw new OverflowException($"NaN cannot be represented as {typeof(decimal).FullName}");
}

// We don't handle zero explicitly as we want to take into account trailing zeros here.

// Handles overflow
return decimal.Parse(ToString(DecimalPrecision), CultureInfo.InvariantCulture);

int DecimalPrecision(string scaledText, int integerPlaces, int decimalPlaces)
{
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.
else if (integerPlaces + decimalPlaces <= 28)
{
decimalPrecision = decimalPlaces;
}
// If there are no integer places we can represent up to 28 decimal places.
else if (integerPlaces == 0)
{
decimalPrecision = Math.Min(decimalPlaces, 28);
}
// 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 3ff15fb

Please sign in to comment.