vinishjail97 commented on code in PR #951:
URL: https://github.com/apache/incubator-xtable/pull/951#discussion_r4109111953
##########
xtable-core/src/test/java/org/apache/xtable/hudi/TestHudiFileStatsExtractor.java:
##########
@@ -332,6 +332,66 @@ void
columnStatsWithMetadataTablePartialMissingFallsBackToParquetFooters(@TempDi
verify(mockMetaClient, times(1)).getStorage();
}
+ @Test
+ void
columnStatsWithMetadataTableZeroValueCountFallsBackToParquetFooters(@TempDir
Path tempDir) {
+ // Reproduces a real-world case: the metadata table has an entry for the
file (so it isn't
+ // "missing"), but every column's value count is 0 -- e.g. for files
written before
+ // column-stats indexing was enabled for the table. Previously this was
accepted as a
+ // successful enrichment with recordCount=0 (silently wrong for a
non-empty file); it should
+ // instead be treated like "no stats" and fall back to the parquet footer.
+ List<InternalDataFile> inputFiles = generateInputFiles(tempDir, 1);
+ InternalDataFile file = inputFiles.get(0);
+ Pair<String, String> filePair =
+ Pair.of("", new
org.apache.hadoop.fs.Path(file.getPhysicalPath()).getName());
+
+ HoodieTableConfig mockTableConfig = mock(HoodieTableConfig.class);
+
when(mockTableConfig.isMetadataPartitionAvailable(MetadataPartitionType.COLUMN_STATS))
+ .thenReturn(true);
+ when(mockTableConfig.getTableVersion()).thenReturn(HoodieTableVersion.SIX);
+
+ HoodieTableMetaClient mockMetaClient = mock(HoodieTableMetaClient.class);
+ doReturn(storageConf).when(mockMetaClient).getStorageConf();
+ doReturn(new HoodieHadoopStorage(new
StoragePath(tempDir.toUri().getPath()), storageConf))
+ .when(mockMetaClient)
+ .getStorage();
+ when(mockMetaClient.getIndexMetadata()).thenReturn(Option.empty());
+ when(mockMetaClient.getBasePath()).thenReturn(new
StoragePath(tempDir.toUri().getPath()));
+ when(mockMetaClient.getTableConfig()).thenReturn(mockTableConfig);
+
+ HoodieTableMetadata mockMetadataTable = mock(HoodieTableMetadata.class);
+ when(mockMetadataTable.getColumnStats(any(), anyString()))
+ .thenAnswer(
+ invocation -> {
+ String fieldName = invocation.getArgument(1);
+ Map<Pair<String, String>, HoodieMetadataColumnStats> statsMap =
new HashMap<>();
+ statsMap.put(
+ filePair,
+ HoodieMetadataColumnStats.newBuilder()
+ .setFileName(filePair.getRight())
+ .setColumnName(fieldName)
+ .setValueCount(0L)
+ .setNullCount(0L)
+ .setTotalSize(0L)
+ .setIsDeleted(false)
+ .build());
+ return statsMap;
+ });
+
+ HudiFileStatsExtractor extractor = new
HudiFileStatsExtractor(mockMetaClient);
+ List<InternalDataFile> output =
+ extractor
+ .addStatsToFiles(mockMetadataTable, inputFiles.stream(), schema)
+ .collect(Collectors.toList());
+
+ assertEquals(1, output.size());
+ InternalDataFile result = output.get(0);
+ // Must have fallen back to the parquet footer (real row count), not the
degenerate
+ // metadata-table stats (which would previously have produced
recordCount=0).
+ assertEquals(2, result.getRecordCount());
+ assertFalse(result.getColumnStats().isEmpty());
Review Comment:
Would it work to assert the column stats match the footer-derived ones (for
example, a non-zero `numValues`)? `isEmpty()` would still pass if the
zero-count metadata stats leaked through.
##########
xtable-core/src/main/java/org/apache/xtable/hudi/HudiFileStatsExtractor.java:
##########
@@ -201,8 +201,16 @@ private Optional<InternalDataFile>
tryEnrichWithMetadataStats(
fileStats.stream()
.map(pair -> getColumnStatFromHudiStat(pair.getLeft(),
pair.getRight()))
.collect(CustomCollectors.toList(fileStats.size()));
- long recordCount = getMaxFromColumnStats(columnStats).orElse(0L);
- return
Optional.of(file.toBuilder().columnStats(columnStats).recordCount(recordCount).build());
+ Optional<Long> recordCount = getMaxFromColumnStats(columnStats);
+ if (!recordCount.isPresent()) {
Review Comment:
These files now also reach the `log.warn` in `addStatsToFiles` that says
they "had no column stats". Would it be reasonable to reword it to "had missing
or unusable column stats", so the log matches both cases?
--
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]
