vinishjail97 commented on code in PR #19869:
URL: https://github.com/apache/hudi/pull/19869#discussion_r3965696244
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/metadata/index/record/BaseRecordIndexer.java:
##########
@@ -478,11 +478,48 @@ private HoodieData<HoodieRecord>
getRecordIndexAdditionalUpserts(
} else if (operationType == WriteOperationType.DELETE_PARTITION) {
// all records from the target partition(s) to be deleted from RLI
return getRecordIndexReplacedRecords((HoodieReplaceCommitMetadata)
commitMetadata, fsView);
+ } else if (commitMetadata instanceof HoodieReplaceCommitMetadata &&
operationType != WriteOperationType.CLUSTER) {
+ // a replace commit that is neither a table service nor an overwrite,
e.g. files written outside Hudi being
+ // registered in the table. The replaced file groups are dropped without
their records being rewritten under
+ // the same key, so the records of the replaced base files are deleted
from RLI unless this commit wrote them again.
+ return
getRecordIndexReplacedFileGroupRecords((HoodieReplaceCommitMetadata)
commitMetadata, fsView)
+ .mapToPair(r -> Pair.of(r.getKey(), r))
+ .leftOuterJoin(updatesFromWriteStatuses.mapToPair(r ->
Pair.of(r.getKey(), r)))
+ .values()
+ .filter(p -> !p.getRight().isPresent())
+ .map(Pair::getLeft);
} else {
return engineContext.emptyHoodieData();
}
}
+ /**
+ * Reads the record keys of the latest base file of every file group
replaced by the given commit and
+ * returns a delete record for each of them.
+ */
+ private HoodieData<HoodieRecord>
getRecordIndexReplacedFileGroupRecords(HoodieReplaceCommitMetadata
replaceCommitMetadata, Lazy<HoodieTableFileSystemView> fsView) {
Review Comment:
Fixed in b10d8167. For a table without a record key
(`HoodieTableConfig.hasRecordKey()` is false) the initialize path now keys
every row positionally through the same `FileFormatUtils.readRowKeys(storage,
file, basePath)` call the update path uses, so bootstrap and incremental
updates produce identical keys by construction. It uses
`getWritesFileIdEncoding()` instead of the hardcoded 0, and fails if the slice
has log files or no base file.
`testIndexesAreBuiltFromRegisteredFilesWhenEnabledLater` registers a file with
both indexes off, enables RLI and SI, registers a second file and asserts both
indexes cover both files, then drops the first file and asserts both indexes
forget it.
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/metadata/SecondaryIndexRecordGenerationUtils.java:
##########
@@ -287,6 +341,12 @@ private static <T> ClosableIterator<Pair<String, String>>
createSecondaryIndexRe
boolean allowInflightInstants) throws IOException {
String secondaryKeyField = indexDefinition.getSourceFieldsKey();
HoodieSchema requestedSchema =
getRequestedSchemaForSecondaryIndex(metaClient, tableSchema, secondaryKeyField);
+ // Files written outside Hudi may carry neither the record key meta field
nor record key fields. Their rows are
+ // keyed by the file path relative to the table base path and the row
position, the same key the record index uses.
+ boolean hasRecordKeyMetaField =
tableSchema.getField(RECORD_KEY_METADATA_FIELD).isPresent();
+ boolean hasRecordKeyFields =
metaClient.getTableConfig().getRecordKeyFields().map(fields -> fields.length >
0).orElse(false);
+ Option<String> relativeFilePath = fileSlice.getBaseFile()
Review Comment:
Fixed in b10d8167: the initialize path uses `baseFile.getStoragePath()`. The
functional test is now parameterized over the four shapes
`TestExternalPathHandling` uses (partitioned and unpartitioned, flat and
`bucket-0/` prefix), with the same `fileId = prefix/fileName` and marker path
construction. I kept the coverage in the new class so the RLI and SI assertions
stay together; see my reply on the test placement thread.
##########
hudi-client/hudi-client-common/src/test/java/org/apache/hudi/metadata/index/secondary/TestSecondaryIndexer.java:
##########
@@ -171,6 +173,34 @@ void testBuildUpdateForCompactReturnsEmpty() {
commitMetadata)).isEmpty());
}
+ @Test
+ void testBuildUpdateForReplaceCommitFromExternalWriterIsNotRejected() {
+ // files written outside Hudi are registered through replace commits with
an unknown operation type
+ HoodieEngineContext engineContext = mock(HoodieEngineContext.class);
+ HoodieWriteConfig writeConfig = mock(HoodieWriteConfig.class);
+ HoodieTableMetaClient metaClient = mock(HoodieTableMetaClient.class);
+ HoodieTableConfig tableConfig = mock(HoodieTableConfig.class);
+ HoodieIndexMetadata indexMetadata = mock(HoodieIndexMetadata.class);
+ HoodieIndexDefinition indexDefinition = mock(HoodieIndexDefinition.class);
+
+
when(metaClient.getIndexMetadata()).thenReturn(org.apache.hudi.common.util.Option.of(indexMetadata));
+ when(metaClient.getTableConfig()).thenReturn(tableConfig);
+
when(tableConfig.getMetadataPartitions()).thenReturn(Collections.emptySet());
+
when(indexMetadata.getIndexDefinitions()).thenReturn(Collections.singletonMap("secondary_index_idx",
indexDefinition));
+ when(indexDefinition.getIndexName()).thenReturn("secondary_index_idx");
+
+ HoodieReplaceCommitMetadata commitMetadata = new
HoodieReplaceCommitMetadata();
+ commitMetadata.setOperationType(WriteOperationType.UNKNOWN);
+ commitMetadata.addReplaceFileId("p1", "file_1.parquet");
+
+ SecondaryIndexer indexer = new SecondaryIndexer(engineContext,
writeConfig, metaClient);
+ assertTrue(indexer.buildUpdate(IndexUpdateContext.of(
Review Comment:
Rewritten in b10d8167. The test now sets the operation type to null
explicitly, stubs a `secondary_index_idx` partition, uses a replace commit
without write stats, and asserts one `IndexPartitionAndRecords` that carries
the delete record returned by a mocked
`convertWriteStatsToSecondaryIndexRecords`. It fails when either the null guard
or the early-return change is reverted. One note: a fresh
`HoodieCommitMetadata` initializes `operationType` to `UNKNOWN`, so "never set"
is `UNKNOWN`; null only comes from an explicit `setOperationType(null)` or a
JSON null, which is why the guard accepts both.
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/metadata/index/secondary/SecondaryIndexer.java:
##########
@@ -115,7 +115,7 @@ public List<IndexPartitionAndRecords>
buildUpdate(IndexUpdateContext context) {
// If write operation type based on commit metadata is COMPACT or CLUSTER
then no need to update,
// because these operations do not change the secondary key - record key
mapping.
WriteOperationType operationType =
context.commitMetadata().getOperationType();
- if (operationType.isInsertOverwriteOrDeletePartition()) {
+ if (operationType != null &&
operationType.isInsertOverwriteOrDeletePartition()) {
Review Comment:
Done in b10d8167:
`WriteOperationType.DELETE_PARTITION.equals(context.commitMetadata().getOperationType())`.
I left the setter alone; rejecting null there touches every writer and belongs
in its own change.
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/metadata/SecondaryIndexRecordGenerationUtils.java:
##########
@@ -107,7 +114,10 @@ public static <T> HoodieData<HoodieRecord>
convertWriteStatsToSecondaryIndexReco
HoodieSchema tableSchema;
try {
- tableSchema = tryResolveSchemaForTable(dataMetaClient).get();
+ // a table without any completed commit, e.g. one that registers files
written outside Hudi for the first time,
+ // only has the schema of the current commit.
+ tableSchema = tryResolveSchemaForTable(dataMetaClient)
+ .orElseGet(() ->
HoodieSchema.parse(commitMetadata.getMetadata(HoodieCommitMetadata.SCHEMA_KEY)));
Review Comment:
Took the suggestion in b10d8167.
`fallsBackToTheCommitSchemaWhenTheTableHasNoCompletedCommit` covers the
fallback and asserts the error names `SCHEMA_KEY` when it is missing.
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/metadata/index/record/BaseRecordIndexer.java:
##########
@@ -478,11 +478,48 @@ private HoodieData<HoodieRecord>
getRecordIndexAdditionalUpserts(
} else if (operationType == WriteOperationType.DELETE_PARTITION) {
// all records from the target partition(s) to be deleted from RLI
return getRecordIndexReplacedRecords((HoodieReplaceCommitMetadata)
commitMetadata, fsView);
+ } else if (commitMetadata instanceof HoodieReplaceCommitMetadata &&
operationType != WriteOperationType.CLUSTER) {
Review Comment:
Done in b10d8167 through `WriteOperationType.isUnknown(op)` (`null ||
UNKNOWN`). The same predicate gates the RLI branch, the SI replaced-file-group
generation, and the SI early return.
##########
hudi-spark-datasource/hudi-spark/src/test/java/org/apache/hudi/client/functional/TestExternalFileRecordAndSecondaryIndex.java:
##########
@@ -0,0 +1,219 @@
+/*
+ * 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.client.functional;
+
+import org.apache.hudi.client.WriteStatus;
+import org.apache.hudi.common.config.HoodieMetadataConfig;
+import org.apache.hudi.common.model.HoodieDeltaWriteStat;
+import org.apache.hudi.common.model.HoodieRecordGlobalLocation;
+import org.apache.hudi.common.model.WriteOperationType;
+import org.apache.hudi.common.schema.HoodieSchema;
+import org.apache.hudi.common.schema.HoodieSchemaField;
+import org.apache.hudi.common.schema.HoodieSchemaType;
+import org.apache.hudi.common.table.HoodieTableConfig;
+import org.apache.hudi.common.table.HoodieTableMetaClient;
+import org.apache.hudi.common.table.timeline.HoodieInstant;
+import org.apache.hudi.common.table.timeline.HoodieTimeline;
+import org.apache.hudi.common.util.ExternalFilePathUtil;
+import org.apache.hudi.common.util.Option;
+import org.apache.hudi.common.util.collection.Pair;
+import org.apache.hudi.config.HoodieIndexConfig;
+import org.apache.hudi.config.HoodieWriteConfig;
+import org.apache.hudi.data.HoodieJavaRDD;
+import org.apache.hudi.metadata.HoodieBackedTableMetadata;
+import org.apache.hudi.metadata.HoodieTableMetadataUtil;
+import org.apache.hudi.testutils.HoodieClientTestBase;
+
+import org.apache.avro.generic.GenericData;
+import org.apache.avro.generic.GenericRecord;
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.fs.Path;
+import org.apache.parquet.avro.AvroParquetWriter;
+import org.apache.parquet.hadoop.ParquetWriter;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.ValueSource;
+
+import java.io.IOException;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Properties;
+import java.util.Set;
+import java.util.stream.Collectors;
+
+import static
org.apache.hudi.common.testutils.HoodieTestUtils.INSTANT_GENERATOR;
+import static org.apache.hudi.index.HoodieIndex.IndexType.INMEMORY;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+/**
+ * Asserts that the record index and the secondary index are maintained for
parquet files written outside Hudi
+ * and registered in the table through replace commits, the way tables
converted from other formats are.
+ * Such files carry no record key, so every row is keyed by the file path
relative to the table and the row position.
+ */
+public class TestExternalFileRecordAndSecondaryIndex extends
HoodieClientTestBase {
Review Comment:
I parameterized the new test over the same four shapes and borrowed the
`fileId = prefix/fileName` and marker path construction, but kept it in
`TestExternalFileRecordAndSecondaryIndex`: the RLI and SI assertions, the
bootstrap flow and the drop-only commit share the helpers there, and
`TestExternalPathHandling` would otherwise need index setup on top of its
column stats flow. Happy to move it if you prefer a single class.
--
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]
