nssalian commented on code in PR #17002:
URL: https://github.com/apache/iceberg/pull/17002#discussion_r3494439111
##########
spark/v4.0/spark/src/test/java/org/apache/iceberg/spark/variant/TestVariantShredding.java:
##########
@@ -994,6 +994,25 @@ public void testDecimalFallbackAfterBuffer() throws
IOException {
assertThat(rows.get(5)[1]).isEqualTo(new BigDecimal("12.3000"));
}
+ @TestTemplate
+ public void testShreddedLargeIntegerVariantReadBack() {
+ // A 20-digit integer (DECIMAL16, above Long.MAX_VALUE) shreds to a
FIXED_LEN_BYTE_ARRAY
+ // typed_value. Before the fix its declared length (16) exceeded the bytes
written, failing the
Review Comment:
Could we rephrase this so it describes the invariant instead of the history?
Something like:
```
// DECIMAL16 (20-digit integer, above Long.MAX_VALUE) shreds to a
FIXED_LEN_BYTE_ARRAY
// typed_value whose declared length must equal what FixedDecimalWriter emits
```
##########
parquet/src/test/java/org/apache/iceberg/parquet/TestVariantShreddingAnalyzer.java:
##########
@@ -249,6 +250,34 @@ public void testDecimalForExceedingPrecision() {
.isEqualTo(PrimitiveType.PrimitiveTypeName.FIXED_LEN_BYTE_ARRAY);
}
+ @Test
+ public void testDecimalExceedingPrecisionUsesMinimumFixedLength() {
+ DirectAnalyzer analyzer = new DirectAnalyzer();
+
+ // Precision > 18 shreds to FIXED_LEN_BYTE_ARRAY; the declared length must
match what the
+ // writer emits (decimalRequiredBytes), not a hardcoded 16.
+ int expectedPrecision = 20;
+ VariantMetadata meta = Variants.metadata("val");
+ ShreddedObject row = Variants.object(meta);
+ row.put("val", Variants.of(new BigDecimal("12345678901234567890")));
+
+ Type schema = analyzer.analyzeAndCreateSchema(List.of(row), 0);
+ assertThat(schema).isNotNull();
+
+ GroupType valGroup = schema.asGroupType().getType("val").asGroupType();
+ PrimitiveType valPrimitive =
valGroup.getType("typed_value").asPrimitiveType();
+
+ LogicalTypeAnnotation.DecimalLogicalTypeAnnotation decimal =
+ (LogicalTypeAnnotation.DecimalLogicalTypeAnnotation)
+ valPrimitive.getLogicalTypeAnnotation();
+ assertThat(decimal.getPrecision()).isEqualTo(expectedPrecision);
+ assertThat(valPrimitive.getPrimitiveTypeName())
+ .isEqualTo(PrimitiveType.PrimitiveTypeName.FIXED_LEN_BYTE_ARRAY);
+ assertThat(valPrimitive.getTypeLength())
+ .isEqualTo(TypeUtil.decimalRequiredBytes(expectedPrecision))
+ .isLessThan(16);
Review Comment:
I went back to [#14297](https://github.com/apache/iceberg/pull/14297) to see
what was done.
The only test it had was for the FIXED_LEN_BYTE_ARRAY branch happened to
land on precision 38, where decimalRequiredBytes(38) = 16 and the hardcoded 16
looked correct. Here we are adding coverage at precision 20 (9 bytes) and
keeps the precision-38 case, but those are both boundaries of the affected
range.
Could we add one more case at a middle precision, e.g. 28 (12 bytes) or 33
(14 bytes)? Easiest as a @ParameterizedTest taking precision + expected byte
length, or just one extra @Test. One non-boundary case can help prove the fix
is doing the arithmetic correctly.
##########
spark/v4.0/spark/src/test/java/org/apache/iceberg/spark/variant/TestVariantShredding.java:
##########
@@ -554,7 +554,7 @@ public void testDecimalFamilyPromotion() throws IOException
{
field(
"value",
optional(PrimitiveType.PrimitiveTypeName.FIXED_LEN_BYTE_ARRAY)
- .length(16)
+ .length(9) // decimalRequiredBytes(21) = 9, not a hardcoded 16
Review Comment:
Can we clean up this comment as well along with the other similar ones?
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
