hudi-agent commented on code in PR #19304:
URL: https://github.com/apache/hudi/pull/19304#discussion_r3718394865
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/client/BaseHoodieWriteClient.java:
##########
@@ -1544,6 +1547,36 @@ protected boolean loadActiveTimelineOnTableInit() {
return true;
}
+ private void handleComplexKeygenEncoding(HoodieTableMetaClient metaClient) {
+ HoodieTableConfig tableConfig = metaClient.getTableConfig();
+ if
(!tableConfig.getTableVersion().lesserThan(org.apache.hudi.common.table.HoodieTableVersion.NINE))
{
+ return; // table version 9+ uses fixed encoding; no deduction needed
+ }
+ if
(!KeyGenUtils.isComplexKeyGeneratorWithSingleRecordKeyField(tableConfig)) {
Review Comment:
🤖 nit: could you use the short form `HoodieTableVersion.NINE` here? The
fully-qualified name inside a method body is harder to scan, and
`HoodieTableVersion` is already imported (or easily added) — either way, the
inline FQN breaks the visual rhythm of the method.
<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:
##########
@@ -1544,6 +1547,36 @@ protected boolean loadActiveTimelineOnTableInit() {
return true;
}
+ private void handleComplexKeygenEncoding(HoodieTableMetaClient metaClient) {
+ HoodieTableConfig tableConfig = metaClient.getTableConfig();
+ if
(!tableConfig.getTableVersion().lesserThan(org.apache.hudi.common.table.HoodieTableVersion.NINE))
{
+ return; // table version 9+ uses fixed encoding; no deduction needed
+ }
+ if
(!KeyGenUtils.isComplexKeyGeneratorWithSingleRecordKeyField(tableConfig)) {
+ return;
+ }
+ if (config.autoDeduceComplexKeygenEncoding()) {
+ if (!tableConfig.populateMetaFields()) {
Review Comment:
🤖 For a virtual-key table (populateMetaFields=false) with a single-field
ComplexKeyGenerator on a v8 table, auto-deduce is skipped here (warning only,
no encoding pinned) — but since auto-deduce is still enabled, the validation
guard below is also bypassed via `&&
!config.autoDeduceComplexKeygenEncoding()`. So on upgrade such a table gets
neither self-healing nor the old hard-fail guard, and can silently mix
encodings and create duplicates through the index. Previously that guard would
throw. Should the guard still fire when deduction was skipped for virtual keys?
<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,141 @@
+/*
+ * 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.HoodieTestDataGenerator.recordsToStrings
+import org.apache.hudi.config.HoodieWriteConfig
+import org.apache.hudi.core.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.write.table.version" -> "8", // the encoding fix only applies to
table version 8 and below
+ "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
+ // Do NOT set hoodie.write.complex.keygen.auto.deduce.encoding
+ // Do NOT set hoodie.write.complex.keygen.validation.enable
+ val options = commonOpts ++ Map(
+ DataSourceWriteOptions.RECORDKEY_FIELD.key -> recordKeyField,
+ DataSourceWriteOptions.PARTITIONPATH_FIELD.key -> partitionPathField,
+ DataSourceWriteOptions.KEYGENERATOR_CLASS_NAME.key ->
"org.apache.hudi.keygen.ComplexKeyGenerator"
+ )
+
+ println("========== NEW TABLE DEFAULT WRITE: starting ==========")
+ println(s"recordKeyField=$recordKeyField
partitionPathField=$partitionPathField " +
+ s"keygen=org.apache.hudi.keygen.ComplexKeyGenerator (no
encoding/deduce/validation overrides)")
+
+ try {
+ inputDF.write.format("org.apache.hudi")
+ .options(options)
+ .option(DataSourceWriteOptions.OPERATION.key,
DataSourceWriteOptions.INSERT_OPERATION_OPT_VAL)
+ .mode(SaveMode.Overwrite)
+ .save(basePath)
+
+ println("========== NEW TABLE DEFAULT WRITE: SUCCEEDED ==========")
+
+ val storage = HoodieTestUtils.getStorage(new StoragePath(basePath))
Review Comment:
🤖 nit: `testNewTableDefaultKeyFormat` has no assertions — it only prints
sample keys and swallows the result. If this was an exploratory script to
characterise the default, have you considered converting it to a proper
assertion-based test (or removing it)? As-is, the test will always pass even if
the encoding is wrong.
<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:
##########
@@ -385,4 +408,91 @@ public static boolean
mayUseNewEncodingForComplexKeyGen(HoodieTableConfig tableC
return tableConfig.getTableVersion().lesserThan(HoodieTableVersion.NINE)
&& 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);
+ }
+ }
Review Comment:
🤖 nit: it might be worth moving `DEFAULT_NEW_ENCODING_FOR_NEW_TABLE` up near
the other public constants at the top of the class — having a `public static
final` field sandwiched between two public static methods makes it easy to miss
when someone is looking for the constant declarations.
<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]
