hudi-agent commented on code in PR #19205:
URL: https://github.com/apache/hudi/pull/19205#discussion_r3735334794
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/io/HoodieWriteMergeHandle.java:
##########
@@ -413,7 +418,16 @@ protected void writeToFile(HoodieKey key, HoodieRecord<T>
record, HoodieSchema s
if (shouldPreserveRecordMetadata) {
// NOTE: `FILENAME_METADATA_FIELD` has to be rewritten to correctly
point to the
// file holding this record even in cases when overall metadata is
preserved
- HoodieRecord populatedRecord = record.updateMetaField(schema,
HoodieRecord.FILENAME_META_FIELD_ORD, newFilePath.getName());
+ //
+ // The rewrite is gated on the mode: hoodie.meta.fields.mode is the
single authority on which
+ // meta columns hold values, and this path would otherwise populate
_hoodie_file_name on a
+ // COMMIT_TIME_ONLY / NONE table. The value written when the mode opts
out is an explicit null
+ // rather than a skipped update, because the record being preserved here
came from the previous
+ // base file — under a narrowed mode it can still carry a file name
written while the table was
+ // on ALL, and leaving that in place would carry a stale value forward.
+ String fileNameToWrite =
+ metaFieldsMode.isFileNamePopulated() ? newFilePath.getName() : null;
+ HoodieRecord populatedRecord = record.updateMetaField(schema,
HoodieRecord.FILENAME_META_FIELD_ORD, fileNameToWrite);
Review Comment:
🤖 When the mode doesn't populate `_hoodie_file_name` (COMMIT_TIME_ONLY /
NONE), `fileNameToWrite` is null and `record.updateMetaField(...,
FILENAME_META_FIELD_ORD, null)` NPEs for a `HoodieSparkRecord`: it routes to
`HoodieInternalRow.update(4, null)`, which hits the `else` branch and calls
`value.getClass()` on the null (neither `instanceof UTF8String` nor `instanceof
String`). The default `FileGroupReaderBasedMergeHandle` copy-forward branch
calls this with `shouldPreserveRecordMetadata=true` for every unmodified old
record, so a Spark CoW `COMMIT_TIME_ONLY` table would fail on the first upsert
that copies a record forward. The Avro path (`data.put(ordinal, null)`)
tolerates null, and the E2E tests only use `SaveMode.Overwrite`, so this never
gets exercised. Could we null the column via a null-safe path (e.g. `setNullAt`
on the overlay) instead of `updateMetaField(null)`? @nsivabalan
<sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/io/BaseCreateHandle.java:
##########
@@ -164,8 +167,17 @@ record = record.prependMetaFields(schema,
writeSchemaWithMetaFields, new Metadat
}
protected HoodieRecord<T> updateFileName(HoodieRecord<T> record,
HoodieSchema schema, HoodieSchema targetSchema, String fileName, Properties
prop) {
- MetadataValues metadataValues = new MetadataValues().setFileName(fileName);
- return record.prependMetaFields(schema, targetSchema, metadataValues,
prop);
+ // hoodie.meta.fields.mode decides whether _hoodie_file_name carries a
value. On the
+ // preserve-metadata path the record comes from an existing file, so
leaving the column alone is
+ // not enough — MetadataValues skips null entries, and a record written
while the table was on
+ // ALL would keep the file name it already had. Overwrite it with an
explicit null instead.
+ if (metaFieldsMode.isFileNamePopulated()) {
+ MetadataValues metadataValues = new
MetadataValues().setFileName(fileName);
+ return record.prependMetaFields(schema, targetSchema, metadataValues,
prop);
+ }
+ HoodieRecord<T> withMetaFields =
+ record.prependMetaFields(schema, targetSchema, new MetadataValues(),
prop);
+ return withMetaFields.updateMetaField(targetSchema,
HoodieRecord.FILENAME_META_FIELD_ORD, null);
Review Comment:
🤖 Same null-to-`updateMetaField` issue as in `HoodieWriteMergeHandle`:
`withMetaFields.updateMetaField(targetSchema, FILENAME_META_FIELD_ORD, null)`
NPEs for a `HoodieSparkRecord` (via `HoodieInternalRow.update(4, null)` →
`value.getClass()` on the null). This fires on the preserve-metadata
create/clustering path (`writeRecordToFile` when `preserveMetadata` is true)
under COMMIT_TIME_ONLY / NONE. Could we set the column to null via a null-safe
path (`setNullAt`) rather than passing null through `updateMetaField`?
<sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
##########
hudi-client/hudi-client-common/src/test/java/org/apache/hudi/table/upgrade/TestNineToTenUpgradeHandler.java:
##########
@@ -0,0 +1,87 @@
+/*
+ * 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.table.upgrade;
+
+import org.apache.hudi.common.engine.HoodieEngineContext;
+import org.apache.hudi.common.model.MetaFieldsMode;
+import org.apache.hudi.common.table.HoodieTableConfig;
+import org.apache.hudi.common.table.HoodieTableMetaClient;
+import org.apache.hudi.config.HoodieWriteConfig;
+import org.apache.hudi.table.HoodieTable;
+
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.CsvSource;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+import static org.mockito.Mockito.RETURNS_DEEP_STUBS;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+
+/**
+ * Version 9 tables predate {@code hoodie.meta.fields.mode}, so the upgrade
records the value
+ * derived from the deprecated {@code hoodie.populate.meta.fields} boolean.
This makes an upgraded
+ * table describe its meta-field layout the same way a freshly created version
10 table does,
+ * instead of relying on the legacy fallback at every read.
+ */
+class TestNineToTenUpgradeHandler {
+
+ private static SupportsUpgradeDowngrade helperFor(MetaFieldsMode
resolvedMode) {
+ HoodieTable table = mock(HoodieTable.class, RETURNS_DEEP_STUBS);
+ HoodieTableMetaClient metaClient = mock(HoodieTableMetaClient.class,
RETURNS_DEEP_STUBS);
+ HoodieTableConfig tableConfig = mock(HoodieTableConfig.class);
+ when(tableConfig.getMetaFieldsMode()).thenReturn(resolvedMode);
+ when(metaClient.getTableConfig()).thenReturn(tableConfig);
+ when(table.getMetaClient()).thenReturn(metaClient);
+
+ SupportsUpgradeDowngrade helper = mock(SupportsUpgradeDowngrade.class);
+
when(helper.getTable(org.mockito.ArgumentMatchers.any(HoodieWriteConfig.class),
+
org.mockito.ArgumentMatchers.any(HoodieEngineContext.class))).thenReturn(table);
+ return helper;
+ }
+
+ @ParameterizedTest
+ @CsvSource({"ALL", "NONE"})
+ void upgradeRecordsTheModeDerivedFromTheLegacyBoolean(String modeName) {
+ MetaFieldsMode expected = MetaFieldsMode.valueOf(modeName);
+ UpgradeDowngrade.TableConfigChangeSet changeSet = new
NineToTenUpgradeHandler().upgrade(
+ mock(HoodieWriteConfig.class), mock(HoodieEngineContext.class), "001",
helperFor(expected));
+
+ assertTrue(changeSet.propertiesToDelete().isEmpty());
Review Comment:
🤖 nit: could you add static imports for `org.mockito.ArgumentMatchers.any`
here (as TestTenToNineDowngradeHandler already does) rather than inlining the
fully-qualified names? It reads a bit noisily.
<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]
