hudi-agent commented on code in PR #18901:
URL: https://github.com/apache/hudi/pull/18901#discussion_r3348898647
##########
hudi-common/src/main/java/org/apache/hudi/common/util/LanceUtils.java:
##########
@@ -175,7 +181,42 @@ public List<HoodieColumnRangeMetadata<Comparable>>
readColumnStatsFromMetadata(H
StoragePath filePath,
List<String> columnList,
HoodieIndexVersion indexVersion) {
- throw new UnsupportedOperationException("readColumnStatsFromMetadata is
not yet supported for Lance format");
+ if (columnList == null || columnList.isEmpty()) {
+ return Collections.emptyList();
+ }
+
+ try (HoodieFileReader fileReader =
+ HoodieIOFactory.getIOFactory(storage)
+ .getReaderFactory(HoodieRecord.HoodieRecordType.SPARK)
+ .getFileReader(
+ ConfigUtils.DEFAULT_HUDI_CONFIG_FOR_READER,
+ filePath,
+ HoodieFileFormat.LANCE)) {
+ HoodieSchema fileSchema = fileReader.getSchema();
+ List<Pair<String, HoodieSchemaField>> fieldsToIndex = columnList.stream()
+ .map(columnName -> HoodieSchemaUtils.getNestedField(fileSchema,
columnName))
+ .filter(Option::isPresent)
+ .map(Option::get)
+ .collect(Collectors.toList());
+ if (fieldsToIndex.isEmpty()) {
+ return Collections.emptyList();
+ }
+
+ List<String> projectedColumns = fieldsToIndex.stream()
Review Comment:
🤖 This path iterates every record in the file to compute stats. Since the
Lance writer doesn't populate `writeStat.columnStats`, this full-file re-read
will be triggered for every newly-written Lance base file on every commit (and
for partition-stats aggregation on top of that). For Parquet, the equivalent
path is cheap because it reads block-level statistics from the footer. @yihua
have you considered (a) populating column stats on the writer side in
`HoodieBaseLanceWriter` like Parquet does, or (b) exposing Lance's native
per-fragment column statistics via the Java binding? Either would avoid the
per-commit full-scan. Is the intent to land this read-based path now and
optimize in a follow-up?
<sub><i>- AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
##########
hudi-common/src/test/java/org/apache/hudi/metadata/TestMetadataPartitionType.java:
##########
@@ -111,6 +112,20 @@ public void
testPartitionEnabledByConfigOnly(MetadataPartitionType partitionType
assertTrue(enabledPartitions.contains(partitionType) ||
MetadataPartitionType.ALL_PARTITIONS.equals(partitionType));
}
+ @Test
+ public void testColumnAndPartitionStatsEnabledForLanceTables() {
+ HoodieTableConfig tableConfig = Mockito.mock(HoodieTableConfig.class);
+
Mockito.when(tableConfig.getBaseFileFormat()).thenReturn(HoodieFileFormat.LANCE);
Review Comment:
🤖 nit: the `getBaseFileFormat()` stub looks unused now — neither
`COLUMN_STATS.isMetadataPartitionEnabled()` nor
`PARTITION_STATS.isMetadataPartitionEnabled()` reads the base file format after
the Lance guard was removed. Could you drop it to avoid misleading future
readers into thinking the format still influences the result?
<sub><i>- AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
##########
hudi-common/src/main/java/org/apache/hudi/common/util/LanceUtils.java:
##########
@@ -175,7 +181,42 @@ public List<HoodieColumnRangeMetadata<Comparable>>
readColumnStatsFromMetadata(H
StoragePath filePath,
List<String> columnList,
HoodieIndexVersion indexVersion) {
- throw new UnsupportedOperationException("readColumnStatsFromMetadata is
not yet supported for Lance format");
+ if (columnList == null || columnList.isEmpty()) {
+ return Collections.emptyList();
+ }
+
+ try (HoodieFileReader fileReader =
+ HoodieIOFactory.getIOFactory(storage)
+ .getReaderFactory(HoodieRecord.HoodieRecordType.SPARK)
+ .getFileReader(
+ ConfigUtils.DEFAULT_HUDI_CONFIG_FOR_READER,
+ filePath,
+ HoodieFileFormat.LANCE)) {
+ HoodieSchema fileSchema = fileReader.getSchema();
+ List<Pair<String, HoodieSchemaField>> fieldsToIndex = columnList.stream()
+ .map(columnName -> HoodieSchemaUtils.getNestedField(fileSchema,
columnName))
+ .filter(Option::isPresent)
+ .map(Option::get)
+ .collect(Collectors.toList());
+ if (fieldsToIndex.isEmpty()) {
+ return Collections.emptyList();
+ }
+
+ List<String> projectedColumns = fieldsToIndex.stream()
+ .map(Pair::getKey)
+ .collect(Collectors.toList());
+ HoodieSchema projectedSchema =
HoodieSchemaUtils.projectSchema(fileSchema, projectedColumns);
+ try (ClosableIterator<HoodieRecord> recordIterator =
fileReader.getRecordIterator(projectedSchema)) {
+ Map<String, HoodieColumnRangeMetadata<Comparable>>
columnRangeMetadataMap =
+ HoodieTableMetadataUtil.collectColumnRangeMetadata(
+ recordIterator, fieldsToIndex, filePath.getName(),
projectedSchema, storage.getConf(), indexVersion);
+ return fieldsToIndex.stream()
+ .map(field -> columnRangeMetadataMap.get(field.getKey()))
+ .collect(Collectors.toCollection(ArrayList::new));
+ }
Review Comment:
🤖 nit: `Collectors.toCollection(ArrayList::new)` could just be
`Collectors.toList()` here — both allow nulls and return a mutable list, so the
more specific collector doesn't add anything.
<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]
