hudi-agent commented on code in PR #19305:
URL: https://github.com/apache/hudi/pull/19305#discussion_r3718226301
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/client/BaseHoodieWriteClient.java:
##########
@@ -1427,6 +1434,43 @@ &&
isComplexKeyGeneratorWithSingleRecordKeyField(tableConfig)) {
}
}
+ private void handleComplexKeygenEncoding(HoodieTableMetaClient metaClient) {
Review Comment:
🤖 nit: `handle` is pretty vague here — could you rename this to something
like `resolveAndApplyComplexKeygenEncoding` or
`applyDeducedComplexKeygenEncoding`? The existing naming guidelines flag
`handle*` as hiding intent.
<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,109 @@ 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: it might be worth moving `DEFAULT_NEW_ENCODING_FOR_NEW_TABLE` up to
the top of the class with the other constants (`NULL_RECORDKEY_PLACEHOLDER`,
etc.) — declaring it between two method bodies makes it easy to miss when
someone's scanning for the defaults.
<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
+ // 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)")
Review Comment:
🤖 nit: `testNewTableDefaultKeyFormat` doesn't assert anything — it's
entirely `println` + rethrow. Have you considered replacing (or supplementing)
the prints with actual `assertEquals`/`assertTrue` checks on the key format and
the aux-file state? Otherwise this test only confirms the write doesn't throw,
not that the defaults produce the expected encoding.
<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,43 @@ &&
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];
+ Option<Boolean> deducedEncoding =
KeyGenUtils.deduceComplexKeyEncodingFromData(metaClient, recordKeyField);
+ if (deducedEncoding.isPresent()) {
+
KeyGenUtils.writeComplexKeyEncodingToAuxFile(metaClient.getStorage(),
metaClient.getBasePath(), deducedEncoding.get());
Review Comment:
🤖 `writeComplexKeyEncodingToAuxFile` throws `HoodieKeyException` on any
IOException, and it runs *before* `config.setValue(...)` on the next line. So a
transient failure persisting this small cache file (e.g. an S3 hiccup writing
`.aux/complex_key_encoding`) aborts the whole write, even though the encoding
was already correctly deduced from data — and since `setValue` runs after the
throwing call, the correct deduced value is discarded too. Could we apply the
deduced encoding to `config` first and make the cache write best-effort
(log-and-continue, like `readComplexKeyEncodingFromAuxFile` already does) so
caching stays a pure optimization that never fails an otherwise-correct write?
<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]
