hudi-agent commented on code in PR #19304:
URL: https://github.com/apache/hudi/pull/19304#discussion_r3705883116
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/client/BaseHoodieWriteClient.java:
##########
@@ -1544,6 +1548,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))
{
Review Comment:
🤖 nit: `org.apache.hudi.common.table.HoodieTableVersion` is used unqualified
elsewhere in this file (e.g. line 1598) — could you add an import and drop the
fully-qualified reference here for consistency?
<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,140 @@
+/*
+ * 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
Review Comment:
🤖 nit: `testNewTableDefaultKeyFormat` has no assertions — it only prints to
stdout and will always pass (green) regardless of what the actual key format
is. Could you either add a concrete `assertEquals`/`assertTrue` on the key
format, or if this is truly exploratory, remove it before merging?
<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);
+ }
+ }
+
+ 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);
Review Comment:
🤖 This deduction hardcodes parquet (both
`getFileFormatUtils(HoodieFileFormat.PARQUET)` here and
`filePath.endsWith(".parquet")` below). For a table whose base format is
ORC/HFILE (`hoodie.table.base.file.format`), every base file is skipped, so we
fall through to `DEFAULT_NEW_ENCODING_FOR_NEW_TABLE=true` and cache it
permanently — silently pinning bare-value encoding even if the table was
written with `field:value`, which would break upsert matching. Could this
instead derive from `tableConfig.getBaseFileFormat()` / its extension?
<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);
+ }
+ }
+
+ public static final boolean DEFAULT_NEW_ENCODING_FOR_NEW_TABLE = true;
Review Comment:
🤖 nit: `DEFAULT_NEW_ENCODING_FOR_NEW_TABLE` is a class-level constant but
it's declared between two static methods rather than with the other constants
at the top of the class — could you move it up next to
`COMPLEX_KEY_ENCODING_FILE_NAME`?
<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]
