hudi-agent commented on code in PR #19542:
URL: https://github.com/apache/hudi/pull/19542#discussion_r3725927884
##########
hudi-hadoop-common/src/test/java/org/apache/hudi/common/util/TestAvroOrcUtils.java:
##########
@@ -113,4 +136,226 @@ public void testLocalTimestampConvertedToLong() {
assertEquals(TypeDescription.Category.LONG,
orcLocalTimestampMicros.getCategory(),
"LocalTimestampMicros should convert to ORC Long (preserving old
behavior)");
}
+
+ @Test
+ public void testPrimitiveValuesRoundTripThroughColumnVectors() {
+ assertEquals(true,
roundTrip(HoodieSchema.create(HoodieSchemaType.BOOLEAN), true));
+ assertEquals(12, roundTrip(HoodieSchema.create(HoodieSchemaType.INT), 12));
+ assertEquals(34L, roundTrip(HoodieSchema.create(HoodieSchemaType.LONG),
34L));
+ assertEquals(1.25f, roundTrip(HoodieSchema.create(HoodieSchemaType.FLOAT),
1.25f));
+ assertEquals(2.5d, roundTrip(HoodieSchema.create(HoodieSchemaType.DOUBLE),
2.5d));
+ assertEquals("hoodie",
roundTrip(HoodieSchema.create(HoodieSchemaType.STRING), new
Utf8("hoodie")).toString());
+ assertEquals(42, roundTrip(HoodieSchema.createDate(), 42));
+ assertEquals(1_700_000_000_123L,
roundTrip(HoodieSchema.createTimestampMillis(), 1_700_000_000_123L));
+ assertEquals(1_700_000_000_123_456L,
+ roundTrip(HoodieSchema.createTimestampMicros(),
1_700_000_000_123_456L));
+
+ ByteBuffer binary = (ByteBuffer) roundTrip(
+ HoodieSchema.create(HoodieSchemaType.BYTES), ByteBuffer.wrap(new
byte[] {1, 2, 3}));
+ assertArrayEquals(new byte[] {1, 2, 3}, toByteArray(binary));
+ }
+
+ @Test
+ public void testComplexValuesRoundTripThroughColumnVectors() {
+ HoodieSchema arraySchema =
HoodieSchema.createArray(HoodieSchema.create(HoodieSchemaType.INT));
+ assertEquals(Arrays.asList(1, 2, 3), roundTrip(arraySchema,
Arrays.asList(1, 2, 3)));
+
+ HoodieSchema mapSchema =
HoodieSchema.createMap(HoodieSchema.create(HoodieSchemaType.LONG));
+ Map<String, Long> values = new LinkedHashMap<>();
+ values.put("first", 10L);
+ values.put("second", 20L);
+ assertEquals(values, roundTrip(mapSchema, values));
+
+ HoodieSchema recordSchema = HoodieSchema.createRecord("nested", null,
null, Arrays.asList(
+ HoodieSchemaField.of("name",
HoodieSchema.create(HoodieSchemaType.STRING), null, null),
+ HoodieSchemaField.of("count",
HoodieSchema.create(HoodieSchemaType.INT), null, null)));
+ GenericRecord record = new GenericData.Record(recordSchema.toAvroSchema());
+ record.put("name", "record-name");
+ record.put("count", 7);
+ GenericRecord converted = (GenericRecord) roundTrip(recordSchema, record);
+ assertEquals("record-name", converted.get("name").toString());
+ assertEquals(7, converted.get("count"));
+
+ HoodieSchema unionSchema = HoodieSchema.createUnion(Arrays.asList(
+ HoodieSchema.create(HoodieSchemaType.INT),
HoodieSchema.create(HoodieSchemaType.STRING)));
+ assertEquals(9, roundTrip(unionSchema, 9));
+ assertEquals("union-value", roundTrip(unionSchema,
"union-value").toString());
+ }
+
+ @Test
+ public void testDecimalEnumAndFixedValuesRoundTripThroughColumnVectors() {
+ HoodieSchema decimalSchema = HoodieSchema.createDecimal(10, 2);
+ ByteBuffer decimalBytes = (ByteBuffer) roundTrip(decimalSchema, new
BigDecimal("1234.50"));
+ assertEquals(new BigInteger("123450"), new
BigInteger(toByteArray(decimalBytes)));
+
+ HoodieSchema fixedDecimalSchema =
HoodieSchema.parse("{\"type\":\"fixed\",\"name\":\"amount\","
+ +
"\"size\":8,\"logicalType\":\"decimal\",\"precision\":12,\"scale\":2}");
+ BigDecimal fixedValue = new BigDecimal("9876.50");
+ GenericData.Fixed fixed = (GenericData.Fixed) new
Conversions.DecimalConversion().toFixed(
+ fixedValue, fixedDecimalSchema.toAvroSchema(),
fixedDecimalSchema.toAvroSchema().getLogicalType());
+ GenericData.Fixed convertedFixed = (GenericData.Fixed)
roundTrip(fixedDecimalSchema, fixed);
+ assertEquals(fixedValue, new Conversions.DecimalConversion().fromFixed(
+ convertedFixed, fixedDecimalSchema.toAvroSchema(),
fixedDecimalSchema.toAvroSchema().getLogicalType()));
+
+ HoodieSchema enumSchema = HoodieSchema.parse(
+
"{\"type\":\"enum\",\"name\":\"status\",\"symbols\":[\"OPEN\",\"CLOSED\"]}");
+ GenericData.EnumSymbol open = new
GenericData.EnumSymbol(enumSchema.toAvroSchema(), "OPEN");
+ assertEquals(open, roundTrip(enumSchema, open));
+ }
+
+ @Test
+ public void testNullResizeAndRepeatingVectors() {
+ HoodieSchema schema = HoodieSchema.create(HoodieSchemaType.LONG);
+ TypeDescription type = AvroOrcUtils.createOrcSchema(schema);
+ VectorizedRowBatch batch =
TypeDescription.createStruct().addField("value", type).createRowBatch();
+ ColumnVector vector = batch.cols[0];
+
+ int expandedPosition = vector.isNull.length;
+ AvroOrcUtils.addToVector(type, vector, schema, null, expandedPosition);
+ assertFalse(vector.noNulls);
+ assertTrue(vector.isNull[expandedPosition]);
+ assertNull(AvroOrcUtils.readFromVector(type, vector, schema,
expandedPosition));
+
+ ((LongColumnVector) vector).vector[0] = 99;
+ vector.isNull[0] = false;
+ vector.isRepeating = true;
+ assertEquals(99L, AvroOrcUtils.readFromVector(type, vector, schema, 17));
+ }
+
+ @Test
+ public void testCreateSchemaCoversEveryOrcCategory() {
+ TypeDescription orcSchema = TypeDescription.fromString("struct<"
+ +
"boolean_field:boolean,byte_field:tinyint,short_field:smallint,int_field:int,long_field:bigint,"
+ +
"float_field:float,double_field:double,string_field:string,char_field:char(5),varchar_field:varchar(8),"
+ +
"date_field:date,timestamp_field:timestamp,binary_field:binary,decimal_field:decimal(12,3),"
+ +
"list_field:array<int>,map_field:map<string,bigint>,union_field:uniontype<int,string>>");
+
+ HoodieSchema schema = AvroOrcUtils.createSchema(orcSchema);
+ assertEquals(HoodieSchemaType.RECORD, schema.getType());
+ assertEquals(17, schema.getFields().size());
+ assertEquals(HoodieSchemaType.BOOLEAN,
schema.getField("boolean_field").get().schema().getType());
+ assertEquals(HoodieSchemaType.BYTES,
schema.getField("binary_field").get().schema().getType());
+ assertEquals(HoodieSchemaType.ARRAY,
schema.getField("list_field").get().schema().getType());
+ assertEquals(HoodieSchemaType.MAP,
schema.getField("map_field").get().schema().getType());
+ assertEquals(HoodieSchemaType.UNION,
schema.getField("union_field").get().schema().getType());
+ }
+
+ @Test
+ public void testCreateSchemaWithDefaultsAndNestedNamespaces() {
+ TypeDescription orcSchema = TypeDescription.fromString(
+
"struct<id:bigint,nested:struct<name:string,active:boolean>,amount:decimal(8,2),tags:array<string>>");
+
+ HoodieSchema nullable = AvroOrcUtils.createSchemaWithDefaultValue(
+ orcSchema, "root_record", "org.apache.hudi.test", true);
+ assertEquals("root_record", nullable.getName());
+ assertTrue(nullable.getFields().stream().allMatch(field ->
field.schema().getType() == HoodieSchemaType.UNION));
+
+ HoodieSchema required = AvroOrcUtils.createSchemaWithDefaultValue(
+ orcSchema, "root_record", "org.apache.hudi.test", false);
+ assertTrue(required.getFields().stream().noneMatch(field ->
field.schema().getType() == HoodieSchemaType.UNION));
+ assertEquals("nested",
required.getField("nested").get().schema().getName());
+ }
+
+ @Test
+ public void testCachingOfByteReferencesDoesNotCopyInput() {
+ HoodieSchema schema = HoodieSchema.create(HoodieSchemaType.BYTES);
+ TypeDescription type = AvroOrcUtils.createOrcSchema(schema);
+ VectorizedRowBatch batch =
TypeDescription.createStruct().addField("value", type).createRowBatch();
+ byte[] input = new byte[] {4, 5, 6};
+
+ AvroOrcUtils.addToVector(type, batch.cols[0], schema, input, 0);
+
+ BytesColumnVector vector = (BytesColumnVector) batch.cols[0];
+ assertSame(input, vector.vector[0]);
+ }
+
+ @Test
+ public void testAdditionalOrcVectorRepresentationsAndFailures() {
+ assertEquals((byte) 7, roundTrip(TypeDescription.createByte(),
HoodieSchema.create(HoodieSchemaType.INT), (byte) 7));
+ assertEquals((short) 11, roundTrip(TypeDescription.createShort(),
HoodieSchema.create(HoodieSchemaType.INT), (short) 11));
+ assertEquals("char",
roundTrip(TypeDescription.createChar().withMaxLength(8),
+ HoodieSchema.create(HoodieSchemaType.STRING), "char"));
+ assertEquals("varchar",
roundTrip(TypeDescription.createVarchar().withMaxLength(8),
+ HoodieSchema.create(HoodieSchemaType.STRING), "varchar"));
+
+ HoodieSchema dateSchema = HoodieSchema.create(HoodieSchemaType.INT);
+ assertEquals(1, roundTrip(TypeDescription.createDate(), dateSchema,
Date.valueOf("1970-01-02")));
+ assertEquals(2, roundTrip(TypeDescription.createDate(), dateSchema, new
java.util.Date(2L * 86_400_000L)));
Review Comment:
🤖 This assertion looks timezone-dependent. `new java.util.Date(2 *
86_400_000L)` is midnight UTC on day 2, but for a plain `java.util.Date` the
DATE path goes through `DateWritable.millisToDays`, which adds
`TimeZone.getDefault().getOffset(...)`. In any negative-offset zone (e.g. US
Pacific/Eastern) `(172800000 + negativeOffset)/86400000` truncates to 1, not 2,
so this would fail off-UTC. Could you pin the JVM timezone to UTC here, or use
a `java.sql.Date` (offset-corrected via `dateToDays`, like line 282) instead?
<sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
##########
hudi-hadoop-common/src/test/java/org/apache/hudi/common/util/TestAvroOrcUtils.java:
##########
@@ -113,4 +136,226 @@ public void testLocalTimestampConvertedToLong() {
assertEquals(TypeDescription.Category.LONG,
orcLocalTimestampMicros.getCategory(),
"LocalTimestampMicros should convert to ORC Long (preserving old
behavior)");
}
+
+ @Test
+ public void testPrimitiveValuesRoundTripThroughColumnVectors() {
+ assertEquals(true,
roundTrip(HoodieSchema.create(HoodieSchemaType.BOOLEAN), true));
Review Comment:
🤖 nit: `assertEquals(true, ...)` reads less clearly than `assertTrue(...)`.
The same applies to the boolean assertion pattern throughout this test — could
you swap those to `assertTrue`/`assertFalse` so the intent is immediately
obvious?
<sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
##########
hudi-hadoop-common/src/test/java/org/apache/hudi/hadoop/fs/TestHadoopFSUtils.java:
##########
@@ -255,6 +269,121 @@ public void testFileStatusConversion(String path,
convertedPathInfo, path, length, isDirectory, blockReplication,
blockSize, modificationTime);
}
+ @Test
+ public void testStorageConfigurationCopyAndFileSystemOverloads(@TempDir
java.nio.file.Path tempDir) {
+ Configuration conf = new Configuration(false);
+ conf.set("test.key", "before");
+ StorageConfiguration<Configuration> shared =
HadoopFSUtils.getStorageConf(conf);
+ StorageConfiguration<Configuration> copied =
HadoopFSUtils.getStorageConfWithCopy(conf);
+ conf.set("test.key", "after");
+
+ assertEquals("after", shared.getString("test.key").get());
+ assertEquals("before", copied.getString("test.key").get());
+ assertEquals("file", HadoopFSUtils.getFs(tempDir.toUri().toString(),
shared).getScheme());
+ assertEquals("file", HadoopFSUtils.getFs(tempDir.toUri().toString(),
shared, true).getScheme());
+ assertEquals("file", HadoopFSUtils.getFs(new Path(tempDir.toUri()),
shared).getScheme());
+ assertEquals("file", HadoopFSUtils.getFs(new Path(tempDir.toUri()),
shared, true).getScheme());
+ assertEquals("file", HadoopFSUtils.getFs(new StoragePath(tempDir.toUri()),
conf).getScheme());
+ assertEquals("file", HadoopFSUtils.getFs(tempDir.toString(), conf,
true).getScheme());
+ assertInstanceOf(StorageConfiguration.class,
HadoopFSUtils.getStorageConf());
+ }
+
+ @Test
+ public void testAvroPathPermissionAndStatusConversions() {
+ Path path = new Path("s3://bucket/table/file.parquet");
Review Comment:
🤖 nit: `assertEquals(null, ...)` shows up several times in this method —
could you replace those with `assertNull(...)` (e.g.
`assertNull(HadoopFSUtils.toPath(null))`)? It reads more naturally and gives a
better failure message.
<sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
##########
hudi-hadoop-common/src/test/java/org/apache/hudi/hadoop/fs/TestCachingPath.java:
##########
@@ -0,0 +1,104 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.hudi.hadoop.fs;
+
+import org.apache.hadoop.fs.Path;
+import org.junit.jupiter.api.Test;
+
+import java.net.URI;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertSame;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+class TestCachingPath {
+
+ @Test
+ void testCachesDerivedValues() {
+ CachingPath path = new
CachingPath("s3://bucket/table/partition/file.parquet");
+
+ String name = path.getName();
+ Path parent = path.getParent();
+ String fullPath = path.toString();
+
+ assertSame(name, path.getName());
+ assertSame(parent, path.getParent());
+ assertSame(fullPath, path.toString());
+ assertEquals("file.parquet", name);
+ assertEquals("s3://bucket/table/partition", parent.toString());
+ }
+
+ @Test
+ void testEveryConstructorCreatesEquivalentPath() {
+ Path parent = new Path("file:///tmp/table");
+ Path child = new Path("partition/file.parquet");
+ URI expected = URI.create("file:/tmp/table/partition/file.parquet");
+
+ assertEquals(expected, new CachingPath(parent.toString(),
child.toString()).toUri());
+ assertEquals(expected, new CachingPath(parent, child.toString()).toUri());
+ assertEquals(expected, new CachingPath(parent.toString(), child).toUri());
+ assertEquals(expected, new CachingPath(parent, child).toUri());
+ assertEquals(expected, new CachingPath(expected).toUri());
+ }
+
+ @Test
+ void testWrapAndSubPath() {
+ CachingPath cached = new CachingPath("s3://bucket/table");
+ assertSame(cached, CachingPath.wrap(cached));
+
+ CachingPath wrapped = CachingPath.wrap(new Path("s3://bucket/table"));
+ assertEquals(cached, wrapped);
+ assertEquals("s3://bucket/table/partition/file.parquet",
+ wrapped.subPath("partition/file.parquet").toString());
+ }
+
+ @Test
+ void testUnsafeConcatenationHandlesSeparatorsAndPreservesUriParts() throws
Exception {
+ Path base = new Path(new URI("s3", "bucket", "/table", "version=1",
"fragment"));
+
+ CachingPath expected = CachingPath.concatPathUnsafe(base, "partition");
+ assertEquals("/table/partition", expected.toUri().getPath());
+ assertEquals("version=1", expected.toUri().getQuery());
+ assertEquals("fragment", expected.toUri().getFragment());
+
+ assertEquals("/table/partition", CachingPath.concatPathUnsafe(new
Path("s3://bucket/table/"), "/partition").toUri().getPath());
+ assertEquals("/table/partition", CachingPath.concatPathUnsafe(new
Path("s3://bucket/table/"), "partition").toUri().getPath());
+ assertEquals("/table/partition", CachingPath.concatPathUnsafe(new
Path("s3://bucket/table"), "/partition").toUri().getPath());
+ assertEquals("/table/partition", CachingPath.concatPathUnsafe(new
Path("s3://bucket/table"), new Path("partition")).toUri().getPath());
+ assertThrows(IllegalStateException.class,
+ () -> CachingPath.concatPathUnsafe(base, new
Path("s3://other/absolute")));
+ }
+
+ @Test
+ void testRelativePathAndSchemeRemoval() {
+ CachingPath relative =
CachingPath.createRelativePathUnsafe("partition%2Fvalue/file.parquet");
+ assertTrue(!relative.toUri().isAbsolute());
+ assertEquals("partition%2Fvalue/file.parquet", relative.toString());
Review Comment:
🤖 nit: `assertTrue(!relative.toUri().isAbsolute())` — could you flip this to
`assertFalse(relative.toUri().isAbsolute())`? The negation inside assertTrue
makes the intent a little harder to read at a glance.
<sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
--
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]
