hudi-agent commented on code in PR #19305:
URL: https://github.com/apache/hudi/pull/19305#discussion_r3624773502
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/client/BaseHoodieWriteClient.java:
##########
@@ -1410,7 +1414,10 @@ public void validateTableProperties(HoodieTableConfig
tableConfig, Properties pr
throw new HoodieException("Only simple, non-partitioned or complex key
generator are supported when meta-fields are disabled. Used: " + keyGenClass);
}
}
- if (config.enableComplexKeygenValidation()
+ // Complex keygen single-field validation is handled in
handleComplexKeygenEncoding (called earlier
+ // in initTable). Only fall through to the hard throw if auto-deduce is
disabled and validation is on.
+ if (!config.autoDeduceComplexKeygenEncoding()
Review Comment:
🤖 Since `autoDeduceComplexKeygenEncoding()` defaults to true, this hard
validation is now skipped by default for every engine and write path —
including the row-writer bulk_insert and Flink paths where the deduction
doesn't actually reach key generation (see the note in
`handleComplexKeygenEncoding`). For those paths this turns a previous fail-loud
into a silent write with a possibly-wrong encoding. Would it be safer to keep
the guard for the paths auto-deduction can't fix?
<sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/client/BaseHoodieWriteClient.java:
##########
@@ -1427,6 +1434,33 @@ &&
isComplexKeyGeneratorWithSingleRecordKeyField(tableConfig)) {
}
}
+ private void handleComplexKeygenEncoding(HoodieTableMetaClient metaClient) {
+ HoodieTableConfig tableConfig = metaClient.getTableConfig();
+ if
(!KeyGenUtils.isComplexKeyGeneratorWithSingleRecordKeyField(tableConfig)) {
+ return;
+ }
+ if (config.autoDeduceComplexKeygenEncoding()) {
+ if (!tableConfig.populateMetaFields()) {
+ LOG.warn("Skipping complex key encoding auto-deduction for table {}
because meta fields are "
+ + "disabled (virtual keys); relying on the configured {}.",
metaClient.getBasePath(),
+ HoodieWriteConfig.COMPLEX_KEYGEN_NEW_ENCODING.key());
+ return;
+ }
+ Option<Boolean> cachedEncoding =
KeyGenUtils.readComplexKeyEncodingFromAuxFile(
+ metaClient.getStorage(), metaClient.getBasePath());
+ if (cachedEncoding.isPresent()) {
+ config.setValue(HoodieWriteConfig.COMPLEX_KEYGEN_NEW_ENCODING,
String.valueOf(cachedEncoding.get()));
+ LOG.info("Using cached complex key encoding from aux file: {}",
cachedEncoding.get());
+ } else {
+ String recordKeyField = tableConfig.getRecordKeyFields().get()[0];
+ boolean deducedEncoding =
KeyGenUtils.deduceComplexKeyEncodingFromData(metaClient, recordKeyField);
+ KeyGenUtils.writeComplexKeyEncodingToAuxFile(metaClient.getStorage(),
metaClient.getBasePath(), deducedEncoding);
+ config.setValue(HoodieWriteConfig.COMPLEX_KEYGEN_NEW_ENCODING,
String.valueOf(deducedEncoding));
Review Comment:
🤖 This applies the deduced encoding by mutating the write client's `config`.
That reaches the standard Spark upsert/insert keygen (built lazily from
`client.getConfig().getProps()` at write time), but the row-writer
`bulk_insert` path builds its keygen from a *separate* `writeConfig`
(`getBulkInsertRowConfig`), and Flink's `RowDataKeyGen` reads the encoding
straight from the Flink `Configuration` — neither sees this mutation. On those
paths records get written with the default encoding while the aux cache stores
the deduced value, which can reintroduce the key mismatch/duplicates this PR
targets. These paths aren't exercised by the new tests — @nsivabalan could you
confirm whether the deduced encoding actually reaches them, or whether they
need to stay gated?
<sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/keygen/KeyGenUtils.java:
##########
@@ -302,4 +325,91 @@ public static boolean
encodeSingleKeyFieldNameForComplexKeyGen(TypedProperties p
public static boolean mayUseNewEncodingForComplexKeyGen(HoodieTableConfig
tableConfig) {
return isComplexKeyGeneratorWithSingleRecordKeyField(tableConfig);
}
+
+ public static StoragePath getComplexKeyEncodingFilePath(StoragePath
basePath) {
+ return new StoragePath(basePath, AUXILIARYFOLDER_NAME + "/" +
COMPLEX_KEY_ENCODING_FILE_NAME);
+ }
+
+ public static Option<Boolean>
readComplexKeyEncodingFromAuxFile(HoodieStorage storage, StoragePath basePath) {
+ StoragePath encodingFilePath = getComplexKeyEncodingFilePath(basePath);
+ try {
+ if (storage.exists(encodingFilePath)) {
+ Properties props = new Properties();
+ try (InputStream inputStream = storage.open(encodingFilePath)) {
+ props.load(inputStream);
+ }
+ String value = props.getProperty(COMPLEX_KEYGEN_NEW_ENCODING.key());
+ if (value != null) {
+ return Option.of(Boolean.parseBoolean(value));
+ }
+ }
+ } catch (IOException e) {
+ LOG.warn("Failed to read complex key encoding from aux file: {}",
encodingFilePath, e);
+ }
+ return Option.empty();
+ }
+
+ public static void writeComplexKeyEncodingToAuxFile(HoodieStorage storage,
StoragePath basePath, boolean useNewEncoding) {
+ StoragePath encodingFilePath = getComplexKeyEncodingFilePath(basePath);
+ try {
+ Properties props = new Properties();
+ props.setProperty(COMPLEX_KEYGEN_NEW_ENCODING.key(),
String.valueOf(useNewEncoding));
+ try (OutputStream outputStream = storage.create(encodingFilePath, true))
{
+ props.store(outputStream, "Complex key generator encoding format");
+ }
+ LOG.info("Wrote complex key encoding to aux file: {}", useNewEncoding);
+ } catch (IOException e) {
+ throw new HoodieKeyException("Failed to write complex key encoding file
to " + encodingFilePath, e);
+ }
+ }
+
+ public static final boolean DEFAULT_NEW_ENCODING_FOR_NEW_TABLE = true;
+
+ public static boolean deduceComplexKeyEncodingFromData(HoodieTableMetaClient
metaClient, String recordKeyFieldName) {
+ HoodieTimeline completedTimeline =
metaClient.getActiveTimeline().getCommitsTimeline().filterCompletedInstants();
+ if (completedTimeline.empty()) {
+ LOG.info("No completed commits found in table {}; defaulting complex key
encoding to useNewEncoding={} (new/empty table).",
+ metaClient.getBasePath(), DEFAULT_NEW_ENCODING_FOR_NEW_TABLE);
+ return DEFAULT_NEW_ENCODING_FOR_NEW_TABLE;
+ }
+
+ try {
+ HoodieStorage storage = metaClient.getStorage();
+ FileFormatUtils fileFormatUtils = HoodieIOFactory.getIOFactory(storage)
+ .getFileFormatUtils(HoodieFileFormat.PARQUET);
+
+ List<HoodieInstant> instants =
completedTimeline.getReverseOrderedInstants().collect(Collectors.toList());
Review Comment:
🤖 `getReverseOrderedInstants()` pins the encoding of the *most recent* base
file. If a table already contains mixed encodings (the scenario this PR
describes), the newest file may not represent the bulk of existing records, so
future upserts could still miss the majority. Would deducing from the earliest
base file (the table's original encoding) be safer for preserving the existing
key format?
<sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/keygen/KeyGenUtils.java:
##########
@@ -302,4 +325,91 @@ public static boolean
encodeSingleKeyFieldNameForComplexKeyGen(TypedProperties p
public static boolean mayUseNewEncodingForComplexKeyGen(HoodieTableConfig
tableConfig) {
return isComplexKeyGeneratorWithSingleRecordKeyField(tableConfig);
}
+
+ public static StoragePath getComplexKeyEncodingFilePath(StoragePath
basePath) {
+ return new StoragePath(basePath, AUXILIARYFOLDER_NAME + "/" +
COMPLEX_KEY_ENCODING_FILE_NAME);
+ }
+
+ public static Option<Boolean>
readComplexKeyEncodingFromAuxFile(HoodieStorage storage, StoragePath basePath) {
+ StoragePath encodingFilePath = getComplexKeyEncodingFilePath(basePath);
+ try {
+ if (storage.exists(encodingFilePath)) {
+ Properties props = new Properties();
+ try (InputStream inputStream = storage.open(encodingFilePath)) {
+ props.load(inputStream);
+ }
+ String value = props.getProperty(COMPLEX_KEYGEN_NEW_ENCODING.key());
+ if (value != null) {
+ return Option.of(Boolean.parseBoolean(value));
+ }
+ }
+ } catch (IOException e) {
+ LOG.warn("Failed to read complex key encoding from aux file: {}",
encodingFilePath, e);
+ }
+ return Option.empty();
+ }
+
+ public static void writeComplexKeyEncodingToAuxFile(HoodieStorage storage,
StoragePath basePath, boolean useNewEncoding) {
+ StoragePath encodingFilePath = getComplexKeyEncodingFilePath(basePath);
+ try {
+ Properties props = new Properties();
+ props.setProperty(COMPLEX_KEYGEN_NEW_ENCODING.key(),
String.valueOf(useNewEncoding));
+ try (OutputStream outputStream = storage.create(encodingFilePath, true))
{
+ props.store(outputStream, "Complex key generator encoding format");
+ }
+ LOG.info("Wrote complex key encoding to aux file: {}", useNewEncoding);
+ } catch (IOException e) {
+ throw new HoodieKeyException("Failed to write complex key encoding file
to " + encodingFilePath, e);
+ }
+ }
+
+ public static final boolean DEFAULT_NEW_ENCODING_FOR_NEW_TABLE = true;
+
+ public static boolean deduceComplexKeyEncodingFromData(HoodieTableMetaClient
metaClient, String recordKeyFieldName) {
+ HoodieTimeline completedTimeline =
metaClient.getActiveTimeline().getCommitsTimeline().filterCompletedInstants();
+ if (completedTimeline.empty()) {
Review Comment:
🤖 nit: could you move `DEFAULT_NEW_ENCODING_FOR_NEW_TABLE` up to the
constant block at the top of the class (alongside `NULL_RECORDKEY_PLACEHOLDER`
/ `EMPTY_RECORDKEY_PLACEHOLDER`)? Having a `public static final` field
sandwiched between method bodies makes it easy to miss when a reader is
scanning for class-level constants.
<sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/client/BaseHoodieWriteClient.java:
##########
@@ -1427,6 +1434,33 @@ &&
isComplexKeyGeneratorWithSingleRecordKeyField(tableConfig)) {
}
}
+ private void handleComplexKeygenEncoding(HoodieTableMetaClient metaClient) {
+ HoodieTableConfig tableConfig = metaClient.getTableConfig();
+ if
(!KeyGenUtils.isComplexKeyGeneratorWithSingleRecordKeyField(tableConfig)) {
+ return;
+ }
+ if (config.autoDeduceComplexKeygenEncoding()) {
+ if (!tableConfig.populateMetaFields()) {
+ LOG.warn("Skipping complex key encoding auto-deduction for table {}
because meta fields are "
+ + "disabled (virtual keys); relying on the configured {}.",
metaClient.getBasePath(),
+ HoodieWriteConfig.COMPLEX_KEYGEN_NEW_ENCODING.key());
+ return;
+ }
+ Option<Boolean> cachedEncoding =
KeyGenUtils.readComplexKeyEncodingFromAuxFile(
+ metaClient.getStorage(), metaClient.getBasePath());
+ if (cachedEncoding.isPresent()) {
+ config.setValue(HoodieWriteConfig.COMPLEX_KEYGEN_NEW_ENCODING,
String.valueOf(cachedEncoding.get()));
+ LOG.info("Using cached complex key encoding from aux file: {}",
cachedEncoding.get());
+ } else {
+ String recordKeyField = tableConfig.getRecordKeyFields().get()[0];
+ boolean deducedEncoding =
KeyGenUtils.deduceComplexKeyEncodingFromData(metaClient, recordKeyField);
Review Comment:
🤖 nit: `deducedEncoding` doesn't make it obvious what `true` means here —
could you rename it to `useNewEncoding` to match the parameter name in
`writeComplexKeyEncodingToAuxFile`? That way the three consecutive lines
(assign, write, setValue) all use the same predicate-style name and a reader
can immediately see what the boolean represents.
<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]
