wombatu-kun commented on code in PR #19859:
URL: https://github.com/apache/hudi/pull/19859#discussion_r4012844279
##########
hudi-hadoop-common/src/test/java/org/apache/hudi/metadata/TestHoodieTableMetadataUtil.java:
##########
@@ -605,6 +605,48 @@ public void testGetColumnsToIndex() {
Lazy.eagerly(Option.of(schema)), true, V1).keySet()));
}
+ /**
+ * The schema-absent branch of {@code
getColumnsToIndexWithoutRequiredMetaFields}, which
+ * {@link #testGetColumnsToIndex()} never reaches because every case there
supplies a schema.
+ *
+ * <p>Two outcomes, and the difference matters: with no explicit column list
the inner call returns an
+ * empty map, so the caller is left with just the always-indexed meta
columns rather than failing. With
+ * an explicit list it
+ * throws instead, because the names cannot be resolved to field schemas
without a schema to resolve
+ * them against, and silently indexing nothing would look like the config
had been honoured.
+ */
+ @Test
+ public void testGetColumnsToIndexWhenTableSchemaIsAbsent() {
+ HoodieTableConfig tableConfig = metaClient.getTableConfig();
+
+ HoodieMetadataConfig noColumnList = HoodieMetadataConfig.newBuilder()
+ .enable(true).withMetadataIndexColumnStats(true)
+ .build();
+ assertListEquality(new
ArrayList<>(Arrays.asList(HoodieTableMetadataUtil.META_COLS_TO_ALWAYS_INDEX)),
+ new ArrayList<>(HoodieTableMetadataUtil.getColumnsToIndex(tableConfig,
noColumnList,
+ Lazy.eagerly(Option.empty()), false, V1).keySet()));
+
+ HoodieMetadataConfig withColumnList = HoodieMetadataConfig.newBuilder()
+ .enable(true).withMetadataIndexColumnStats(true)
+ .withColumnStatsIndexForColumns("col_1,col_2")
+ .build();
+ Throwable thrown = assertThrows(IllegalArgumentException.class,
+ () -> HoodieTableMetadataUtil.getColumnsToIndex(tableConfig,
withColumnList,
+ Lazy.eagerly(Option.empty()), false, V1),
+ "an explicit column list cannot be resolved without a table schema");
+ assertTrue(String.valueOf(thrown.getMessage()).contains("Table schema not
found"),
+ () -> "the failure should name the missing schema, but was: " +
thrown.getMessage());
+
+ // Table initialisation is the exception: the configured names are
recorded without schemas, so col
+ // stats can be enabled before the first commit has produced one. The meta
columns are added by the
+ // caller either way.
+ List<String> expectedWhileInitialising = new
ArrayList<>(Arrays.asList(HoodieTableMetadataUtil.META_COLS_TO_ALWAYS_INDEX));
+ expectedWhileInitialising.addAll(Arrays.asList("col_1", "col_2"));
+ assertListEquality(expectedWhileInitialising,
+ new ArrayList<>(HoodieTableMetadataUtil.getColumnsToIndex(tableConfig,
withColumnList,
+ Lazy.eagerly(Option.empty()), true, V1).keySet()));
Review Comment:
`Lazy.eagerly(Option.empty())` cannot show that the initializing branch
never resolves the schema, and `ColumnStatsIndexer` reaches that branch with a
lazy `tryResolveSchemaForTable` whenever an explicit column list is set,
including on tables that already have data. A `Lazy.lazily` supplier that fails
the test when invoked would pin that, and would still catch the `checkArgument`
moving above the initializing return.
--
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]
