hudi-agent commented on code in PR #19305: URL: https://github.com/apache/hudi/pull/19305#discussion_r3712922436
########## 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" Review Comment: 🤖 nit: `testNewTableDefaultKeyFormat` has no assertions — it only prints sample key values and rethrows on failure. Could you add at least a couple of `assertEquals`/`assertTrue` calls to lock in the expected behavior (e.g. aux file exists, cached encoding equals `DEFAULT_NEW_ENCODING_FOR_NEW_TABLE`, or that all sampled keys use the new bare-value format)? Otherwise this reads as a debug/exploratory script rather than a regression guard. <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()) { + 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]; + boolean deducedEncoding = KeyGenUtils.deduceComplexKeyEncodingFromData(metaClient, recordKeyField); + KeyGenUtils.writeComplexKeyEncodingToAuxFile(metaClient.getStorage(), metaClient.getBasePath(), deducedEncoding); + config.setValue(HoodieWriteConfig.COMPLEX_KEYGEN_NEW_ENCODING, String.valueOf(deducedEncoding)); Review Comment: 🤖 Confirmed the split reading the code. The row-writer bulk_insert path builds its own `writeConfig` via `HoodieSparkSqlWriter.getBulkInsertRowConfig(...)`, which is constructed from `hoodieConfig.getProps` (HoodieSparkSqlWriter.scala ~L140/L905), not from the mutated `writeClient.getConfig()` — so the deduced encoding never reaches the keygen built for `DatasetBulkInsertCommitActionExecutor`. Flink is the same: `OptionsResolver.useComplexKeygenNewEncoding` reads `COMPLEX_KEYGEN_NEW_ENCODING` directly from the Flink `Configuration` (OptionsResolver.java L389-392). So on both paths records get written with the default encoding while the aux cache stores the deduced value. Plumbing the deduced value into both configs (or keeping the guard for these two paths) looks necessary. ########## 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()) { Review Comment: 🤖 nit: could you move `DEFAULT_NEW_ENCODING_FOR_NEW_TABLE` up to the constants block at the top of the class (with `COMPLEX_KEY_ENCODING_FILE_NAME`, `NULL_RECORDKEY_PLACEHOLDER`, etc.)? Having a `public static final` field appear between two method bodies makes it easy to miss when scanning the class. <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()) { + 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]; + boolean deducedEncoding = KeyGenUtils.deduceComplexKeyEncodingFromData(metaClient, recordKeyField); + KeyGenUtils.writeComplexKeyEncodingToAuxFile(metaClient.getStorage(), metaClient.getBasePath(), deducedEncoding); Review Comment: 🤖 This caches the deduced value unconditionally, but deduceComplexKeyEncodingFromData returns the fallback default (true) when it finds no .parquet base file, and here that guess gets written to the aux file and pinned exactly like an observed value, so it is never re-deduced. For a MoR table upgraded from 0.14.0 (old field:value encoding) where the first post-upgrade write lands while the latest slice is still log-only, we would cache useNewEncoding=true permanently, even after compaction later produces old-encoding base files, silently duplicating on every upsert. Could we avoid caching (or force a re-deduce next time) when the value came from the default fallback rather than actual data? <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]
