Copilot commented on code in PR #342:
URL: https://github.com/apache/arrow-dotnet/pull/342#discussion_r3160951519
##########
src/Apache.Arrow.Operations/VariantJson/VariantJsonConverter.cs:
##########
@@ -243,10 +243,7 @@ internal static void WriteValue(Utf8JsonWriter writer,
VariantValue value)
writer.WriteNumberValue(value.AsDecimal());
break;
case VariantPrimitiveType.Decimal16:
- if (value.IsSqlDecimalStorage)
- writer.WriteRawValue(value.AsSqlDecimal().ToString());
- else
- writer.WriteNumberValue(value.AsDecimal());
+ writer.WriteRawValue(value.AsSqlDecimal().ToString());
Review Comment:
`SqlDecimal.ToString()` is culture-sensitive; writing it via
`Utf8JsonWriter.WriteRawValue(...)` can produce invalid JSON in locales that
use `,` as the decimal separator. Please format the number using an invariant
culture (and/or a fixed format) before writing raw JSON, so Decimal16
serialization is always valid JSON regardless of current culture.
```suggestion
writer.WriteRawValue(value.AsSqlDecimal().ToSqlString().Value);
```
##########
src/Apache.Arrow.Operations/Shredding/VariantShredder.cs:
##########
@@ -283,8 +283,8 @@ internal static object ExtractTypedValue(VariantValue
value, ShredType shredType
case ShredType.Float: return value.AsFloat();
case ShredType.Double: return value.AsDouble();
case ShredType.Decimal4:
- case ShredType.Decimal8:
- case ShredType.Decimal16: return value.AsDecimal();
+ case ShredType.Decimal8: return value.AsDecimal();
+ case ShredType.Decimal16: return value.AsSqlDecimal().Value;
Review Comment:
For `ShredType.Decimal16`, `ExtractTypedValue` returns
`value.AsSqlDecimal().Value` (a `decimal`). This will throw for Decimal16
values outside `System.Decimal` range, preventing shredding of valid 38-digit
decimals. Consider returning a `SqlDecimal` for `Decimal16` typed values and
ensuring the typed_value column builder path appends `SqlDecimal` (the Arrow
decimal builders support `Append(SqlDecimal)`).
```suggestion
case ShredType.Decimal16: return value.AsSqlDecimal();
```
##########
src/Apache.Arrow.Scalars/Variant/VariantValue.cs:
##########
@@ -109,17 +109,12 @@ public static VariantValue FromDecimal4(decimal value) =>
public static VariantValue FromDecimal8(decimal value) =>
new VariantValue(VariantPrimitiveType.Decimal8, (object)value);
- /// <summary>Creates a Decimal16 variant value.</summary>
- public static VariantValue FromDecimal16(decimal value) =>
- new VariantValue(VariantPrimitiveType.Decimal16, (object)value);
-
/// <summary>
- /// Creates a Decimal16 variant value from a <see cref="SqlDecimal"/>,
always
- /// producing <see cref="VariantPrimitiveType.Decimal16"/>. Values
exceeding
- /// <see cref="decimal"/> range are stored as <see cref="SqlDecimal"/>.
- /// Use this when the target type is known (e.g. materializing a
Decimal16
- /// shredded column); use <see cref="FromSqlDecimal(SqlDecimal)"/>
when you
- /// want the smallest decimal type that fits the value.
+ /// Creates a Decimal16 variant value. Decimal16 covers the full
Parquet
+ /// 38-digit decimal range, which exceeds the <see cref="decimal"/>
96-bit
+ /// mantissa, so the API uses <see cref="SqlDecimal"/> uniformly. Use
+ /// <see cref="FromSqlDecimal(SqlDecimal)"/> when you want the smallest
+ /// decimal type that fits the value.
/// </summary>
public static VariantValue FromDecimal16(SqlDecimal value)
{
Review Comment:
`FromDecimal16(decimal)` was removed/changed to `FromDecimal16(SqlDecimal)`,
but there are still call sites passing `decimal` that will no longer compile
(e.g., `VariantReader.MaterializePrimitive` calls
`VariantValue.FromDecimal16(d16)` and
`VariantUnshredder.CreateVariantFromTyped` calls
`VariantValue.FromDecimal16((decimal)typedValue)`, plus several tests still
call `FromDecimal16(…)` with `decimal`). Please update those call sites to
construct a `SqlDecimal` (or route through `FromDecimal` / `FromSqlDecimal` as
appropriate) so the solution builds cleanly.
##########
src/Apache.Arrow.Scalars/Variant/VariantValue.cs:
##########
@@ -109,17 +109,12 @@ public static VariantValue FromDecimal4(decimal value) =>
public static VariantValue FromDecimal8(decimal value) =>
new VariantValue(VariantPrimitiveType.Decimal8, (object)value);
- /// <summary>Creates a Decimal16 variant value.</summary>
- public static VariantValue FromDecimal16(decimal value) =>
- new VariantValue(VariantPrimitiveType.Decimal16, (object)value);
-
/// <summary>
- /// Creates a Decimal16 variant value from a <see cref="SqlDecimal"/>,
always
- /// producing <see cref="VariantPrimitiveType.Decimal16"/>. Values
exceeding
- /// <see cref="decimal"/> range are stored as <see cref="SqlDecimal"/>.
- /// Use this when the target type is known (e.g. materializing a
Decimal16
- /// shredded column); use <see cref="FromSqlDecimal(SqlDecimal)"/>
when you
- /// want the smallest decimal type that fits the value.
+ /// Creates a Decimal16 variant value. Decimal16 covers the full
Parquet
+ /// 38-digit decimal range, which exceeds the <see cref="decimal"/>
96-bit
+ /// mantissa, so the API uses <see cref="SqlDecimal"/> uniformly. Use
+ /// <see cref="FromSqlDecimal(SqlDecimal)"/> when you want the smallest
+ /// decimal type that fits the value.
/// </summary>
public static VariantValue FromDecimal16(SqlDecimal value)
{
Review Comment:
`FromDecimal16(SqlDecimal)` still converts some inputs to `System.Decimal`
internally (via `value.Value` when the high 32 bits are zero).
`SqlDecimal.Value` can throw when the value isn’t representable as `decimal`
(e.g., scale > 28), which would make `FromDecimal16` reject valid precision-38
Decimal16 values. Consider guarding that conversion (e.g., try/catch or a
representability check) and falling back to storing a normalized `SqlDecimal`
when needed.
##########
src/Apache.Arrow.Scalars/Variant/VariantValue.cs:
##########
@@ -182,7 +176,7 @@ public static VariantValue FromSqlDecimal(SqlDecimal value)
{
return FromDecimal8(d);
}
- return FromDecimal16(d);
+ return new VariantValue(VariantPrimitiveType.Decimal16, (object)d);
}
Review Comment:
`FromSqlDecimal` converts via `decimal d = value.Value;` whenever `data[3]
== 0`. This can throw for `SqlDecimal` values that don't fit `System.Decimal`
due to scale/precision constraints even though they still fit in 96 bits. To
keep `FromSqlDecimal` robust for Decimal16 inputs, treat “doesn’t fit
System.Decimal” as a Decimal16/SqlDecimal-storage case (similar to the `data[3]
!= 0` path) instead of unconditionally reading `Value`.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
