hudi-agent commented on code in PR #19832:
URL: https://github.com/apache/hudi/pull/19832#discussion_r3921092108
##########
hudi-common/src/main/java/org/apache/hudi/common/table/HoodieTableConfig.java:
##########
@@ -680,6 +703,18 @@ public static void create(HoodieStorage storage,
StoragePath metadataFolder, Pro
}
hoodieConfig.setDefaultValue(DROP_PARTITION_COLUMNS);
+ // Record the index configuration the table was created with (HUDI-37).
Only persisted
+ // when explicitly configured, so existing callers see no extra
properties and the
+ // property is never guessed from an engine-specific default.
+ if (!hoodieConfig.contains(INDEX_TYPE)) {
+ if (hoodieConfig.contains(INDEX_CLASS_WRITE_KEY)) {
Review Comment:
🤖 I think this won't fire in the normal table-creation path. `initTable()`
builds the props via `TableBuilder.fromProperties()` → `build()`, and
`fromProperties` only forwards keys in `PERSISTED_CONFIG_LIST` plus the
explicitly-handled table configs — `hoodie.index.type`/`hoodie.index.class`
aren't among them, so they're stripped before `HoodieTableConfig.create()` ever
runs. The new tests pass only because they call `HoodieTableConfig.create()`
directly with the raw props, bypassing the builder. Could you verify against a
real write-path table init? It looks like `INDEX_TYPE` would never be persisted
for normally-created tables.
<sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
##########
hudi-common/src/main/java/org/apache/hudi/common/table/HoodieTableConfig.java:
##########
@@ -680,6 +703,18 @@ public static void create(HoodieStorage storage,
StoragePath metadataFolder, Pro
}
hoodieConfig.setDefaultValue(DROP_PARTITION_COLUMNS);
+ // Record the index configuration the table was created with (HUDI-37).
Only persisted
+ // when explicitly configured, so existing callers see no extra
properties and the
+ // property is never guessed from an engine-specific default.
+ if (!hoodieConfig.contains(INDEX_TYPE)) {
+ if (hoodieConfig.contains(INDEX_CLASS_WRITE_KEY)) {
+ // A custom index class takes precedence over the index type (same
rule as HoodieIndexConfig).
+ hoodieConfig.setValue(INDEX_TYPE,
hoodieConfig.getString(INDEX_CLASS_WRITE_KEY));
Review Comment:
🤖 When `hoodie.index.class` is set, its fully-qualified class name is stored
into a config named `hoodie.table.index.type`, so `getIndexType()` can return
either an IndexType enum string (e.g. `BLOOM`) or an arbitrary class name. Any
future consumer will have to disambiguate the two. Would it be cleaner to store
the class under a separate `hoodie.table.index.class` key (mirroring the
write-time split) so the type field stays a clean enum?
<sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
##########
hudi-common/src/main/java/org/apache/hudi/common/table/HoodieTableConfig.java:
##########
@@ -1393,6 +1428,13 @@ public Option<String> getRelativeIndexDefinitionPath() {
return Option.ofNullable(getString(RELATIVE_INDEX_DEFINITION_PATH));
}
+ /**
Review Comment:
🤖 nit: `getIndexType()` (and `INDEX_TYPE`) is a bit misleading — when
`hoodie.index.class` is set, it returns a fully-qualified class name rather
than a type enum like `BLOOM`. Something like `getCreationTimeIndex()` /
`CREATION_TIME_INDEX` would signal that this is a unified type-or-class value
at creation time, not strictly an enum type.
<sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
##########
hudi-common/src/main/java/org/apache/hudi/common/table/HoodieTableConfig.java:
##########
@@ -1393,6 +1428,13 @@ public Option<String> getRelativeIndexDefinitionPath() {
return Option.ofNullable(getString(RELATIVE_INDEX_DEFINITION_PATH));
}
Review Comment:
🤖 nit: should be `@return` (no trailing `s`) — `@returns` is not a standard
Javadoc tag and won't render in generated docs.
<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]
