hudi-agent commented on code in PR #19305:
URL: https://github.com/apache/hudi/pull/19305#discussion_r3653924232
##########
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()) {
Review Comment:
🤖 When meta fields are disabled (virtual keys) and auto-deduce is on (the
default), we return here without deducing — and the hard guard in
validateTableProperties is also skipped, since it's now gated on
`!autoDeduceComplexKeygenEncoding()`. So a single-field ComplexKeyGenerator
virtual-key table silently proceeds on the default `new.encoding=false` with no
protection at all, whereas before it hit the guard. Should this path fall
through to the validation guard rather than returning silently? @yihua
<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;
Review Comment:
🤖 nit: `DEFAULT_NEW_ENCODING_FOR_NEW_TABLE` is declared mid-class after
several method definitions, which breaks the usual Java convention of grouping
constants at the top of the class (near `NULL_RECORDKEY_PLACEHOLDER` etc.).
Could you hoist it up with the other constants?
<sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
##########
hudi-spark-datasource/hudi-spark/src/test/scala/org/apache/hudi/functional/TestComplexKeyGenNewTableDefault.scala:
##########
@@ -0,0 +1,139 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.hudi.functional
+
+import org.apache.hudi.DataSourceWriteOptions
+import org.apache.hudi.common.model.HoodieFileFormat
+import org.apache.hudi.common.testutils.{HoodieTestDataGenerator,
HoodieTestUtils}
+import org.apache.hudi.common.testutils.RawTripTestPayload.recordsToStrings
+import org.apache.hudi.config.HoodieWriteConfig
+import org.apache.hudi.io.storage.HoodieIOFactory
+import org.apache.hudi.keygen.KeyGenUtils
+import org.apache.hudi.storage.StoragePath
+import org.apache.hudi.testutils.HoodieSparkClientTestBase
+import org.apache.spark.sql.SaveMode
+import org.junit.jupiter.api.{AfterEach, BeforeEach, Test}
+
+import scala.collection.JavaConverters._
+
+/**
+ * Writes to a BRAND NEW table using ComplexKeyGenerator with a single record
key field
+ * and a single partition path field, using PURE DEFAULTS for the three
keygen-related
+ * write configs (new.encoding, auto.deduce.encoding, validation.enable are
NOT set).
+ *
+ * Goal: empirically determine the default _hoodie_record_key format produced
by this branch
+ * when a fresh table is written directly.
+ */
+class TestComplexKeyGenNewTableDefault extends HoodieSparkClientTestBase {
+
+ var commonOpts: Map[String, String] = Map(
+ "hoodie.insert.shuffle.parallelism" -> "4",
+ "hoodie.upsert.shuffle.parallelism" -> "4",
+ DataSourceWriteOptions.PRECOMBINE_FIELD.key -> "timestamp",
+ HoodieWriteConfig.TBL_NAME.key -> "hoodie_test"
+ )
+
+ @BeforeEach
+ override def setUp(): Unit = {
+ initPath()
+ initSparkContexts()
+ initTestDataGenerator()
+ initHoodieStorage()
+ }
+
+ @AfterEach
+ override def tearDown(): Unit = {
+ cleanupResources()
+ }
+
+ @Test
+ def testNewTableDefaultKeyFormat(): Unit = {
+ val recordKeyField = "_row_key"
+ val partitionPathField = "partition"
+
+ val dataGen = new HoodieTestDataGenerator(0xDEED)
+ val records = recordsToStrings(dataGen.generateInserts("001",
100)).asScala.toList
+ val inputDF =
sparkSession.read.json(sparkSession.sparkContext.parallelize(records, 2))
+
+ // PURE DEFAULTS: only set keygen class + record key + partition path.
+ // Do NOT set hoodie.write.complex.keygen.new.encoding
Review Comment:
🤖 nit: `testNewTableDefaultKeyFormat` contains only `println` calls and no
assertions — it will always pass regardless of what the code actually does.
Could you add at least an `assertEquals`/`assertTrue` on the key format and the
aux-file state so the test can actually catch a regression?
<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()) {
Review Comment:
🤖 Since auto-deduce defaults to on, this branch always overwrites
`COMPLEX_KEYGEN_NEW_ENCODING` with the deduced/default value — even when the
user set it explicitly. On a brand-new table (empty timeline) the deduced value
is hardcoded to `true`, so an explicit `new.encoding=false` is silently flipped
to `true` (this looks like why TestCreateTable had to add `auto.deduce=false`).
Should an explicitly-configured encoding take precedence over deduction on a
new/empty table?
<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());
+ for (HoodieInstant instant : instants) {
+ HoodieCommitMetadata commitMetadata =
TimelineUtils.getCommitMetadata(instant, completedTimeline);
+ for (HoodieWriteStat writeStat : commitMetadata.getWriteStats()) {
+ String filePath = writeStat.getPath();
+ if (filePath == null || filePath.isEmpty() ||
!filePath.endsWith(".parquet")) {
+ continue;
+ }
+ StoragePath baseFilePath = new StoragePath(metaClient.getBasePath(),
filePath);
+ if (!storage.exists(baseFilePath)) {
+ continue;
+ }
+ try (ClosableIterator<HoodieKey> keyIterator =
fileFormatUtils.getHoodieKeyIterator(storage, baseFilePath)) {
+ if (keyIterator.hasNext()) {
+ HoodieKey hoodieKey = keyIterator.next();
+ String hoodieRecordKey = hoodieKey.getRecordKey();
+ String expectedPrefix = recordKeyFieldName +
DEFAULT_COLUMN_VALUE_SEPARATOR;
+ boolean usesNewEncoding =
!hoodieRecordKey.startsWith(expectedPrefix);
+ LOG.info("Deduced complex key encoding from base file {} (commit
{}): useNewEncoding={}",
+ baseFilePath, instant.getTimestamp(), usesNewEncoding);
+ return usesNewEncoding;
+ }
+ }
+ }
+ }
+
+ LOG.info("No base files with records found in table {}; defaulting
complex key encoding to useNewEncoding={}.",
+ metaClient.getBasePath(), DEFAULT_NEW_ENCODING_FOR_NEW_TABLE);
+ return DEFAULT_NEW_ENCODING_FOR_NEW_TABLE;
Review Comment:
🤖 If the timeline is non-empty but no readable `.parquet` base file is found
(e.g. a MoR table whose latest file slices are log-only, or base files
physically absent), this falls through to `useNewEncoding=true` and the caller
caches it permanently. For an existing old-encoding table that would flip the
encoding and risk duplicates. Could we distinguish 'no commits' from 'commits
exist but no base file readable', and avoid caching a guess (or also inspect
log blocks for MoR) in the latter case?
<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]
