hudi-agent commented on code in PR #19305: URL: https://github.com/apache/hudi/pull/19305#discussion_r3659654848
########## 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 Review Comment: 🤖 nit: `testNewTableDefaultKeyFormat` only prints output — there are no `assert*` calls anywhere in the method, so it passes unconditionally and gives no regression signal. Could you convert the `println` observations into actual assertions (e.g. assert the aux file exists, assert `cachedEncoding` has the expected value)? <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()) { Review Comment: 🤖 This looks real — TestCreateTable.scala:973 had to add `auto.deduce=false` with the comment "so the explicitly configured encoding takes effect on this new table", which corroborates that an explicit `new.encoding=false` gets flipped to `true` on an empty table. Worth noting the flip is also sticky: on the empty-timeline path `handleComplexKeygenEncoding` writes `true` into `.aux/complex_key_encoding`, so it persists on subsequent writes even after the user notices. Letting an explicitly-set encoding take precedence over deduction on a new/empty table would avoid both. ########## 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()) { + 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.getTimestamp(), usesNewEncoding); + return usesNewEncoding; + } + } + } + } + + LOG.info("No base files with records found in table {}; defaulting complex key encoding to useNewEncoding={}.", + metaClient.getBasePath(), DEFAULT_NEW_ENCODING_FOR_NEW_TABLE); + return DEFAULT_NEW_ENCODING_FOR_NEW_TABLE; Review Comment: 🤖 Good catch — and I think it extends beyond MoR log-only slices: `deduceComplexKeyEncodingFromData` hardcodes `HoodieFileFormat.PARQUET` and filters on `filePath.endsWith(".parquet")`, so a table with ORC/HFile base files (`hoodie.table.base.file.format`) would never match a base file and would also fall through to the cached `useNewEncoding=true` default. For an existing old-encoding table that silently flips and pins the wrong encoding. Reading `tableConfig.getBaseFileFormat()` rather than assuming parquet, and not caching a guess when the timeline is non-empty but no base file was actually inspected, would both help. ########## 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) { Review Comment: 🤖 nit: `DEFAULT_NEW_ENCODING_FOR_NEW_TABLE` is declared after several method definitions, but constants conventionally live at the top of the class alongside `NULL_RECORDKEY_PLACEHOLDER` and friends. Could you move it up there? <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 on both paths from the code: Flink's `RowDataKeyGen` reads the encoding from `OptionsResolver.useComplexKeygenNewEncoding(conf)` at construction time (RowDataKeyGen.java:160), and the Spark row-writer builds its own `writeConfig` via `getBulkInsertRowConfig(hoodieConfig, ...)` (HoodieSparkSqlWriter.java:905). Both are separate from the `BaseHoodieWriteClient.config` that `handleComplexKeygenEncoding` mutates, so neither observes the deduced value — meaning the aux cache and the actual written keys can disagree. Gating those paths, or threading the deduced encoding into `hoodieConfig`/the Flink `Configuration`, looks necessary here. -- 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]
