wombatu-kun commented on code in PR #19252:
URL: https://github.com/apache/hudi/pull/19252#discussion_r3566193847
##########
hudi-common/src/main/java/org/apache/hudi/core/io/storage/HoodieFileReaderFactory.java:
##########
@@ -99,6 +99,8 @@ public HoodieFileReader getFileReader(HoodieConfig
hoodieConfig, StoragePathInfo
return newOrcFileReader(pathInfo.getPath());
case LANCE:
return newLanceFileReader(hoodieConfig, pathInfo.getPath());
+ case VORTEX:
+ return newVortexFileReader(hoodieConfig, pathInfo.getPath());
Review Comment:
The same "handles LANCE, misses VORTEX" pattern is still present in at least
three more dispatch sites, and one of them means this fix cannot be reached end
to end.
`HoodieFileGroupReaderBasedFileFormat#supportBatch` branches on PARQUET /
ORC / LANCE and throws `HoodieNotSupportedException` in the else. Spark calls
`supportBatch` at planning time for every scan, so a Spark read of a Vortex
table throws before it ever reaches this factory (the only escape is the early
return when the schema has VECTOR columns). VORTEX should take the Lance branch
and return false until columnar batch reading lands. `buildBaseFileReader` in
that same class does have a VORTEX branch, which is what makes this look like
an oversight rather than a deliberate gap.
`HoodieFileGroupReaderBasedFileFormat#isSplitable` excludes only Lance from
splitting. `SparkVortexReaderBase#read` derives the URI from the partitioned
file and never reads `file.start` / `file.length`, exactly like
`SparkLanceReaderBase`, so once `supportBatch` is fixed any Vortex base file
above `maxSplitBytes` gets split and each split re-reads the whole file,
duplicating rows.
`SparkFileFormatInternalRowReaderContext`'s `isLanceBaseFile` guard, which
skips the binary-vector rewrite for Arrow-native base files, also does not
cover VORTEX.
These survived because there is no end-to-end Vortex table test:
`TestHoodieFileGroupReaderOnSpark` parameterizes PARQUET / ORC / LANCE only,
and `TestVortexReaderWriterRoundTrip` stays at the file level. The
dispatch-exhaustiveness tests in #19253 cover the two factories and
`CommonClientUtils`, so they would not catch these either. Adding VORTEX to
that parameterization would surface all of them.
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/util/CommonClientUtils.java:
##########
@@ -120,6 +120,7 @@ public static HoodieLogBlock.HoodieLogBlockType
getLogBlockType(HoodieWriteConfi
HoodieFileFormat baseFileFormat = getBaseFileFormat(writeConfig,
tableConfig);
switch (getBaseFileFormat(writeConfig, tableConfig)) {
case LANCE:
+ case VORTEX:
Review Comment:
The write side has the same gap.
`HoodieInternalRowFileWriterFactory#getInternalRowFileWriter` dispatches on the
PARQUET and LANCE extensions only and throws `UnsupportedOperationException`
otherwise, so `bulk_insert` and row-writer clustering on a Vortex table fail
even though `HoodieSparkVortexWriter` already implements
`HoodieInternalRowFileWriter` and the row writer is enabled by default. It
needs a VORTEX branch mirroring the Lance one.
`MetadataPartitionType` is the other one: COLUMN_STATS and PARTITION_STATS
both gate on `getBaseFileFormat() != LANCE`, and
`HoodieTableMetadataUtil#readColumnRangeMetadataFrom` only reads column ranges
from `.parquet` files. Vortex base files therefore contribute empty ranges, and
per the comment on that LANCE guard the index then silently prunes everything
on read. VORTEX needs the same exclusion until Vortex emits column-range
metadata.
##########
hudi-client/hudi-client-common/src/test/java/org/apache/hudi/utils/TestCommonClientUtils.java:
##########
@@ -116,14 +118,36 @@ void testShouldWriteNativeLogs(HoodieTableVersion
writeVersion, HoodieFileFormat
}
private static Stream<Arguments> provideWriteVersionNativeLogExpectations() {
- // Native log format is the default for write version >= TEN, except for
Lance base files.
+ // Native log format is the default for write version >= TEN, except for
Lance and Vortex base files.
return Stream.of(
Arguments.of(HoodieTableVersion.SIX, HoodieFileFormat.PARQUET, false),
Arguments.of(HoodieTableVersion.EIGHT, HoodieFileFormat.PARQUET,
false),
Arguments.of(HoodieTableVersion.NINE, HoodieFileFormat.PARQUET, false),
Arguments.of(HoodieTableVersion.TEN, HoodieFileFormat.PARQUET, true),
Arguments.of(HoodieTableVersion.TEN, HoodieFileFormat.ORC, true),
- Arguments.of(HoodieTableVersion.TEN, HoodieFileFormat.LANCE, false)
+ Arguments.of(HoodieTableVersion.TEN, HoodieFileFormat.LANCE, false),
+ Arguments.of(HoodieTableVersion.TEN, HoodieFileFormat.VORTEX, false)
+ );
+ }
+
+ @ParameterizedTest(name = "Base file format {0} should use log block type
{1}")
+ @MethodSource("provideLogBlockTypeExpectations")
+ void testGetLogBlockType(HoodieFileFormat baseFileFormat,
HoodieLogBlock.HoodieLogBlockType expected) {
+ HoodieWriteConfig writeConfig = mock(HoodieWriteConfig.class);
+ HoodieTableConfig tableConfig = mock(HoodieTableConfig.class);
+ when(writeConfig.getLogDataBlockFormat()).thenReturn(Option.empty());
+ when(tableConfig.getBaseFileFormat()).thenReturn(baseFileFormat);
+
+ assertEquals(expected, CommonClientUtils.getLogBlockType(writeConfig,
tableConfig));
+ }
+
+ private static Stream<Arguments> provideLogBlockTypeExpectations() {
+ return Stream.of(
+ Arguments.of(HoodieFileFormat.PARQUET,
HoodieLogBlock.HoodieLogBlockType.AVRO_DATA_BLOCK),
+ Arguments.of(HoodieFileFormat.ORC,
HoodieLogBlock.HoodieLogBlockType.AVRO_DATA_BLOCK),
+ Arguments.of(HoodieFileFormat.LANCE,
HoodieLogBlock.HoodieLogBlockType.AVRO_DATA_BLOCK),
+ Arguments.of(HoodieFileFormat.VORTEX,
HoodieLogBlock.HoodieLogBlockType.AVRO_DATA_BLOCK),
+ Arguments.of(HoodieFileFormat.HFILE,
HoodieLogBlock.HoodieLogBlockType.HFILE_DATA_BLOCK)
Review Comment:
The `default:` throw in `getLogBlockType` is the branch this PR exists to
fix, and it stays untested: with VORTEX added, `HOODIE_LOG` is the only value
that still reaches it. A row asserting `HoodieException` for `HOODIE_LOG` would
pin that contract, and the `getLogDataBlockFormat().isPresent()` early return
is likewise uncovered.
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/util/CommonClientUtils.java:
##########
@@ -120,6 +120,7 @@ public static HoodieLogBlock.HoodieLogBlockType
getLogBlockType(HoodieWriteConfi
HoodieFileFormat baseFileFormat = getBaseFileFormat(writeConfig,
tableConfig);
switch (getBaseFileFormat(writeConfig, tableConfig)) {
case LANCE:
+ case VORTEX:
Review Comment:
`getLogBlockType` already assigns `baseFileFormat` and then calls
`getBaseFileFormat(writeConfig, tableConfig)` a second time in the `switch`
head, so the lookup runs twice per call. Pointing the `switch` at the existing
local matches what this PR does in `shouldWriteNativeLogs` just below.
--
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]
