hudi-agent commented on code in PR #19304:
URL: https://github.com/apache/hudi/pull/19304#discussion_r3712938444
##########
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);
+
+ 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.requestedTime(), usesNewEncoding);
+ return usesNewEncoding;
+ }
+ }
+ }
+ }
+
+ LOG.info("No base files with records found in table {}; defaulting
complex key encoding to useNewEncoding={}.",
Review Comment:
🤖 When the timeline is non-empty but no readable `.parquet` base file is
found, this falls through to the new-encoding default (`true`) — same as a
brand-new table. A MoR table with a log-indexing index (bucket/HBase) writes
inserts straight to log files with no base parquet (see
`BaseSparkDeltaCommitActionExecutor.handleInsert` → `canIndexLogFiles()`), so
an existing old-encoding (`field:value`) table would be mis-deduced as new
encoding, pinned, and cached permanently → the exact duplicate-record failure
this PR aims to fix. Could we fall back to the validation/error path (not the
new-encoding default) when detection can't read any base file for a non-empty
table? @nsivabalan does this direction of default look safe to you for MoR?
<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))
{
Review Comment:
🤖 nit: the log message says "to aux file: {}" but `{}` is bound to the
`useNewEncoding` boolean, so it prints something like "Wrote complex key
encoding to aux file: true" — could you swap the binding to `encodingFilePath`
(or include both) so the path is visible in logs?
<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: this constant is declared between two method bodies, which is easy to
miss. Have you considered moving it to the top of the class with the other
`public static final` constants (`COMPLEX_KEY_ENCODING_FILE_NAME`,
`HUDI_DEFAULT_PARTITION_PATH`, etc.)?
<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)
Review Comment:
🤖 nit: `testNewTableDefaultKeyFormat` has no assertions — it only prints
sample keys and reports whether the aux file exists. Could you either add a
concrete `assertEquals`/`assertTrue` to lock in the expected default behaviour,
or drop this as a standalone test and move the setup into a helper used by the
other test classes?
<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:
🤖 Agreeing with this — the two parquet assumptions line up into a silent
failure for ORC/HFILE base-format tables:
`getFileFormatUtils(HoodieFileFormat.PARQUET)` on line 462 plus the
`.endsWith(".parquet")` filter on line 469 mean every base file is skipped, so
the loop falls through to the `return DEFAULT_NEW_ENCODING_FOR_NEW_TABLE` on
line 492 rather than the empty-table branch on 456. And since the deduced value
is cached/pinned, a `field:value` table with a non-parquet base format gets
permanently pinned to bare-value encoding, breaking upsert matching with no
error surfaced. Deriving both the `FileFormatUtils` and the extension from
`metaClient.getTableConfig().getBaseFileFormat()` would close the gap.
--
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]
