wombatu-kun commented on code in PR #19495:
URL: https://github.com/apache/hudi/pull/19495#discussion_r3708871952
##########
hudi-trino/src/test/java/io/trino/plugin/hudi/TestPrefilledColumnValues.java:
##########
@@ -81,9 +81,20 @@ public void testHiveNullPartitionValue()
{
// Trino's HivePartitionKey encodes a null partition value as the
literal string "\N"
PrefilledColumnValues values = prefilledValues(new
HivePartitionKey("pk_string", "\\N"));
+ HiveColumnHandle handle = partitionKey("pk_string", VARCHAR,
HiveType.HIVE_STRING);
- Block block = singleValueBlock(values, partitionKey("pk_string",
VARCHAR, HiveType.HIVE_STRING));
+ Block block = singleValueBlock(values, handle);
assertThat(block.isNull(0)).isTrue();
+
+ // Resolved values are memoized per column, and null is a legitimate
resolved value, so a second
Review Comment:
A null-check memo returns null on every call for a hive-null column too, so
these repeats pass against it just as they do against `containsKey`; the only
variant they rule out is a `ConcurrentHashMap`, which the first read already
rules out. Drop the claim, or add a `@VisibleForTesting` accessor so the test
can assert the entry was actually stored.
##########
hudi-trino/src/test/java/io/trino/plugin/hudi/util/TestHudiAvroSerializer.java:
##########
@@ -41,32 +45,39 @@
class TestHudiAvroSerializer
{
- @Test
- public void testDecimalConverter()
- {
- HudiAvroSerializer.AvroDecimalConverter converter = new
HudiAvroSerializer.AvroDecimalConverter();
-
- assertThat(converter.convert(10, 2,
unscaledBytes("123.45"))).isEqualTo(new BigDecimal("123.45"));
- // Same (precision, scale) again: served from the cached schema
- assertThat(converter.convert(10, 2,
unscaledBytes("-0.07"))).isEqualTo(new BigDecimal("-0.07"));
- // Same precision, different scale, and vice versa: must not collide
in the cache
- assertThat(converter.convert(10, 4,
unscaledBytes("123.4567"))).isEqualTo(new BigDecimal("123.4567"));
- assertThat(converter.convert(18, 2,
unscaledBytes("9999999999999999.99"))).isEqualTo(new
BigDecimal("9999999999999999.99"));
- assertThat(converter.convert(5, 0, unscaledBytes("42"))).isEqualTo(new
BigDecimal("42"));
- }
-
- @Test
- public void testAppendShortDecimalFromAvroFixed()
+ /**
+ * A short decimal is stored in Trino as the unscaled value, which is
exactly what Avro writes into the
+ * fixed bytes, so the read is a plain big-endian two's complement decode.
The cases below pin the parts
+ * that decode gets wrong if it is ever rewritten: sign extension for
negatives, scale 0, and the
+ * full-width value at the maximum short-decimal precision.
+ */
+ @ParameterizedTest
+ @MethodSource("shortDecimals")
+ public void testAppendShortDecimalFromAvroFixed(int precision, int scale,
String value, long expectedUnscaled)
{
- DecimalType type = DecimalType.createDecimalType(10, 2);
- byte[] bytes = unscaledBytes("123.45");
+ DecimalType type = DecimalType.createDecimalType(precision, scale);
+ byte[] bytes = unscaledBytes(value);
Review Comment:
`unscaledBytes` returns the minimal encoding, but Avro's
`DecimalConversion.toFixed` left-pads to the schema's fixed size (0xFF for
negatives), so a real decimal(10,2) is five bytes and no case here runs
sign-extended. Size the Fixed from the precision and pad the bytes to match,
the way `TestHudiUtilColumnHandles` builds its decimal fixed schema.
--
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]
