voonhous commented on code in PR #19253:
URL: https://github.com/apache/hudi/pull/19253#discussion_r3881618959
##########
hudi-hadoop-common/src/test/java/org/apache/hudi/io/hadoop/TestHoodieHadoopIOFactory.java:
##########
@@ -61,9 +68,50 @@ public void testGetFileFormatUtils() throws IOException {
assertTrue(ioFactory.getFileFormatUtils(HoodieFileFormat.ORC) instanceof
OrcUtils);
assertTrue(ioFactory.getFileFormatUtils(HoodieFileFormat.HFILE)
instanceof HFileUtils);
assertTrue(ioFactory.getFileFormatUtils(HoodieFileFormat.LANCE)
instanceof LanceUtils);
+ assertTrue(ioFactory.getFileFormatUtils(HoodieFileFormat.VORTEX)
instanceof VortexUtils);
assertThrows(
UnsupportedOperationException.class,
() -> ioFactory.getFileFormatUtils(HoodieFileFormat.HOODIE_LOG));
}
}
+
+ /**
+ * {@link HoodieIOFactory#getFileFormatUtils(StoragePath)} maps the file
extension to a format
+ * through its own if-chain before delegating to the {@link
HoodieFileFormat} switch in
+ * {@link HoodieHadoopIOFactory#getFileFormatUtils(HoodieFileFormat)}. A
format added to one
+ * dispatch point but missed in the other only fails at runtime, so this
sweeps every enum value
+ * (never a hardcoded list) and requires both entry points to agree: the
same utils class, or
+ * {@link UnsupportedOperationException} from both. Any other exception type
propagates.
+ */
+ @ParameterizedTest
+ @EnumSource(HoodieFileFormat.class)
+ void testGetFileFormatUtilsEntryPointsAgreeForEveryFormat(HoodieFileFormat
format) throws IOException {
+ try (HoodieStorage storage = newStorage()) {
+ HoodieIOFactory ioFactory = new HoodieHadoopIOFactory(storage);
+ StoragePath path = new StoragePath("file:///a/b" +
format.getFileExtension());
+ Option<Class<?>> byFormat = fileFormatUtilsClass(() ->
ioFactory.getFileFormatUtils(format));
+ Option<Class<?>> byPath = fileFormatUtilsClass(() ->
ioFactory.getFileFormatUtils(path));
+ assertEquals(byFormat, byPath, () -> String.format(
Review Comment:
This only catches one direction: `getFileFormatUtils(StoragePath)` delegates
each arm into `getFileFormatUtils(HoodieFileFormat)`
(`HoodieIOFactory.java:108-121`), so a case missing from the switch, or a new
enum value wired into neither site, yields `empty == empty` and passes. That
second shape is how VORTEX shipped in #19182 (every site but
`CommonClientUtils`). Could we exclude `HOODIE_LOG` from the `@EnumSource` and
let a missing case throw instead of being swallowed, so a format with no utils
at all fails too?
##########
hudi-client/hudi-client-common/src/test/java/org/apache/hudi/utils/TestCommonClientUtils.java:
##########
@@ -110,6 +116,47 @@ private static Stream<Arguments>
provideWriteVersionNativeLogExpectations() {
);
}
+ /**
+ * Every base file format must map to a log block type: a case missing from
the
+ * {@code getLogBlockType} switch only surfaces as a {@code HoodieException}
on the first MOR
+ * log write, which is what happened for VORTEX (apache/hudi#19252). Sweeps
the enum (never a
+ * hardcoded list); HOODIE_LOG is the log format itself and can never be a
base file format.
+ */
+ // TODO: drop the VORTEX exclusion once apache/hudi#19252 adds the case (see
testGetLogBlockTypeForVortex).
+ @ParameterizedTest
+ @EnumSource(value = HoodieFileFormat.class, mode = EnumSource.Mode.EXCLUDE,
names = {"HOODIE_LOG", "VORTEX"})
+ void testGetLogBlockTypeMapsEveryBaseFileFormat(HoodieFileFormat format) {
+ assertNotNull(
Review Comment:
`getLogBlockType` never returns null, so this only fails on a throw; `HFILE
-> HFILE_DATA_BLOCK` is asserted by no test on master. #19252 adds a
value-pinned `testGetLogBlockType` table to this class in this same hunk, so
the two PRs will conflict. Could we resolve the expected type through a switch
with a failing `default` (VORTEX arm included, `AVRO_DATA_BLOCK`) and
`assertEquals` it, keeping only the `@EnumSource` exclusion until #19252 lands?
##########
hudi-client/hudi-client-common/src/test/java/org/apache/hudi/utils/TestCommonClientUtils.java:
##########
@@ -110,6 +116,47 @@ private static Stream<Arguments>
provideWriteVersionNativeLogExpectations() {
);
}
+ /**
+ * Every base file format must map to a log block type: a case missing from
the
+ * {@code getLogBlockType} switch only surfaces as a {@code HoodieException}
on the first MOR
+ * log write, which is what happened for VORTEX (apache/hudi#19252). Sweeps
the enum (never a
Review Comment:
nit: only the inline-log path calls `getLogBlockType`
(`HoodieInlineLogAppendHandle:321`, Flink `RowDataInlineLogWriteHandle:100`,
`DeltaWriteProfile:131`); `HoodieNativeLogAppendHandle`, the default
write-version-10 path, never does, so a missing case does not surface on "the
first MOR log write". Could we scope this to the inline log write path (write
version below 10)?
##########
hudi-hadoop-common/src/test/java/org/apache/hudi/io/hadoop/TestHoodieHadoopIOFactory.java:
##########
@@ -61,9 +68,50 @@ public void testGetFileFormatUtils() throws IOException {
assertTrue(ioFactory.getFileFormatUtils(HoodieFileFormat.ORC) instanceof
OrcUtils);
assertTrue(ioFactory.getFileFormatUtils(HoodieFileFormat.HFILE)
instanceof HFileUtils);
assertTrue(ioFactory.getFileFormatUtils(HoodieFileFormat.LANCE)
instanceof LanceUtils);
+ assertTrue(ioFactory.getFileFormatUtils(HoodieFileFormat.VORTEX)
instanceof VortexUtils);
assertThrows(
UnsupportedOperationException.class,
() -> ioFactory.getFileFormatUtils(HoodieFileFormat.HOODIE_LOG));
}
}
+
+ /**
+ * {@link HoodieIOFactory#getFileFormatUtils(StoragePath)} maps the file
extension to a format
+ * through its own if-chain before delegating to the {@link
HoodieFileFormat} switch in
+ * {@link HoodieHadoopIOFactory#getFileFormatUtils(HoodieFileFormat)}. A
format added to one
+ * dispatch point but missed in the other only fails at runtime, so this
sweeps every enum value
+ * (never a hardcoded list) and requires both entry points to agree: the
same utils class, or
+ * {@link UnsupportedOperationException} from both. Any other exception type
propagates.
+ */
+ @ParameterizedTest
+ @EnumSource(HoodieFileFormat.class)
+ void testGetFileFormatUtilsEntryPointsAgreeForEveryFormat(HoodieFileFormat
format) throws IOException {
+ try (HoodieStorage storage = newStorage()) {
+ HoodieIOFactory ioFactory = new HoodieHadoopIOFactory(storage);
+ StoragePath path = new StoragePath("file:///a/b" +
format.getFileExtension());
+ Option<Class<?>> byFormat = fileFormatUtilsClass(() ->
ioFactory.getFileFormatUtils(format));
+ Option<Class<?>> byPath = fileFormatUtilsClass(() ->
ioFactory.getFileFormatUtils(path));
+ assertEquals(byFormat, byPath, () -> String.format(
+ "Dispatch asymmetry for %s: getFileFormatUtils(HoodieFileFormat) ->
%s but getFileFormatUtils(%s) -> %s; "
+ + "the extension if-chain in HoodieIOFactory and the format
switch in HoodieHadoopIOFactory must "
+ + "cover the same formats.",
+ format, byFormat, path, byPath));
+ }
+ }
+
+ /**
+ * @return the class of the returned utils, or empty when the entry point
throws
+ * {@link UnsupportedOperationException} for the format.
+ */
+ private static Option<Class<?>>
fileFormatUtilsClass(Supplier<FileFormatUtils> entryPoint) {
+ try {
+ return Option.of(entryPoint.get().getClass());
+ } catch (UnsupportedOperationException e) {
+ return Option.empty();
+ }
+ }
+
+ private static HoodieStorage newStorage() {
Review Comment:
nit: `HoodieTestUtils.getDefaultStorage()` already builds this storage
(sibling `TestHoodieAvroFileReaderFactory:65`), `getFileFormatUtils` never
touches it, and `HoodieHadoopStorage.close()` is a no-op
(`HoodieHadoopStorage.java:313-317`). Could we build the factory once from
`getDefaultStorage()` and drop `newStorage()`, the try-with-resources and
`throws IOException`?
##########
hudi-client/hudi-client-common/src/test/java/org/apache/hudi/utils/TestCommonClientUtils.java:
##########
@@ -110,6 +116,47 @@ private static Stream<Arguments>
provideWriteVersionNativeLogExpectations() {
);
}
+ /**
+ * Every base file format must map to a log block type: a case missing from
the
+ * {@code getLogBlockType} switch only surfaces as a {@code HoodieException}
on the first MOR
+ * log write, which is what happened for VORTEX (apache/hudi#19252). Sweeps
the enum (never a
+ * hardcoded list); HOODIE_LOG is the log format itself and can never be a
base file format.
+ */
+ // TODO: drop the VORTEX exclusion once apache/hudi#19252 adds the case (see
testGetLogBlockTypeForVortex).
+ @ParameterizedTest
+ @EnumSource(value = HoodieFileFormat.class, mode = EnumSource.Mode.EXCLUDE,
names = {"HOODIE_LOG", "VORTEX"})
+ void testGetLogBlockTypeMapsEveryBaseFileFormat(HoodieFileFormat format) {
+ assertNotNull(
+
CommonClientUtils.getLogBlockType(writeConfigWithoutExplicitLogFormat(),
tableConfigWithBaseFormat(format)),
+ () -> "getLogBlockType must return a log block type for base file
format " + format
+ + "; add the missing case to the switch in
CommonClientUtils#getLogBlockType");
+ }
+
+ /**
+ * Same check as {@link #testGetLogBlockTypeMapsEveryBaseFileFormat} for
VORTEX, asserting
+ * the mapping apache/hudi#19252 adds (VORTEX -> AVRO_DATA_BLOCK).
+ */
+ @Disabled("Depends on apache/hudi#19252: on current master getLogBlockType
has no VORTEX case and throws "
Review Comment:
Every other `@Disabled` in the repo cites a JIRA for a broken or
environment-bound test; none waits on an unmerged PR, and #19252 has been
CONFLICTING since 2026-07-21, so nothing forces this back on. Once the sweep's
expected-type switch carries the VORTEX arm this pins nothing extra. Could we
drop it, and use the `TODO(#19252)` form on the exclusion instead of the prose
TODO?
##########
hudi-hadoop-common/src/test/java/org/apache/hudi/io/hadoop/TestHoodieHadoopIOFactory.java:
##########
@@ -61,9 +68,50 @@ public void testGetFileFormatUtils() throws IOException {
assertTrue(ioFactory.getFileFormatUtils(HoodieFileFormat.ORC) instanceof
OrcUtils);
assertTrue(ioFactory.getFileFormatUtils(HoodieFileFormat.HFILE)
instanceof HFileUtils);
assertTrue(ioFactory.getFileFormatUtils(HoodieFileFormat.LANCE)
instanceof LanceUtils);
+ assertTrue(ioFactory.getFileFormatUtils(HoodieFileFormat.VORTEX)
instanceof VortexUtils);
assertThrows(
UnsupportedOperationException.class,
() -> ioFactory.getFileFormatUtils(HoodieFileFormat.HOODIE_LOG));
}
}
+
+ /**
Review Comment:
nit, feel free to ignore: the javadoc plus the failure message run longer
than the test itself, and no sibling test in either class carries javadoc.
Could we trim this (and the `TestCommonClientUtils` one) to one or two lines,
since a failure already names the format and the dispatch point?
--
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]
