rangareddy commented on code in PR #19463:
URL: https://github.com/apache/hudi/pull/19463#discussion_r3710051287
##########
hudi-hadoop-mr/src/main/java/org/apache/hudi/hadoop/utils/HoodieRealtimeRecordReaderUtils.java:
##########
@@ -279,9 +279,13 @@ public static List<String> orderFields(String
fieldNameCsv, String fieldOrderCsv
List<String> fieldNames = fieldNameCsv.isEmpty() ? new ArrayList<>() :
Arrays.stream(fieldNameCsv.split(",")).collect(Collectors.toList());
Set<String> fieldNamesSet = new LinkedHashSet<>(fieldNames);
Review Comment:
Fixed in the description. It is `ColumnProjectionUtils.getReadColumnIDs`
that de-duplicates the **ids** while nothing de-duplicates the names, which is
also what the comment above this line says — I had it exactly the wrong way
round. The description now states that and flags that the earlier revision was
inverted, so anyone reading the thread is not left with the wrong version.
##########
hudi-hadoop-mr/src/test/java/org/apache/hudi/hadoop/utils/TestHoodieRealtimeRecordReaderUtils.java:
##########
@@ -0,0 +1,93 @@
+/*
+ * 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.hadoop.utils;
+
+import org.apache.hudi.exception.HoodieException;
+
+import org.junit.jupiter.api.Test;
+
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.List;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+/**
+ * Tests {@link HoodieRealtimeRecordReaderUtils#orderFields}, which maps Hive's
+ * {@code hive.io.file.readcolumn.names} and {@code
hive.io.file.readcolumn.ids} onto an ordered
+ * projection list.
+ */
+class TestHoodieRealtimeRecordReaderUtils {
Review Comment:
Both applied. The class is `public` with `test`-prefixed method names now,
matching `TestHoodieRealtimeInputFormatUtils` and the rest of the module, and
`Arrays.asList("partition_path")` is `Collections.singletonList(...)`.
##########
hudi-hadoop-mr/src/test/java/org/apache/hudi/hadoop/realtime/TestHoodieRealtimeRecordReader.java:
##########
@@ -310,6 +312,65 @@ private File getLogTempFile(long startTime, long endTime,
String diskType) {
.orElseGet(() -> new File(""));
}
+ /**
+ * HUDI-1286: a MOR _rt query fails with "Error ordering fields for storage
read" when Hive's
+ * read-column names and ids have different lengths. CombineHiveInputFormat
shares one JobConf across
+ * splits, so the two projection lists can accumulate independently and
diverge; this reproduces that
+ * shape directly on the reader and pins the diagnostics the failure has to
carry.
+ */
+ @Test
+ public void testReaderFailsClearlyWhenHiveProjectionListsDiverge() throws
Exception {
+ HoodieSchema schema =
HoodieSchemaUtils.addMetadataFields(SchemaTestUtil.getEvolvedSchema());
+ HoodieTestUtils.init(storageConf, basePath.toString(),
HoodieTableType.MERGE_ON_READ);
+ String instantTime = "100";
+ final int numRecords = 10;
+ File partitionDir = InputFormatTestUtil.prepareParquetTable(basePath,
schema, 1, numRecords, instantTime,
+ HoodieTableType.MERGE_ON_READ);
+ HoodieCommitMetadata commitMetadata =
CommitUtils.buildMetadata(Collections.emptyList(), Collections.emptyMap(),
+ Option.empty(), WriteOperationType.UPSERT, schema.toString(),
HoodieTimeline.DELTA_COMMIT_ACTION);
+ FileCreateUtilsLegacy.createDeltaCommit(COMMIT_METADATA_SER_DE,
basePath.toString(), instantTime, commitMetadata);
+ FileInputFormat.setInputPaths(baseJobConf, partitionDir.getPath());
+
+ String newCommitTime = "101";
+ HoodieLogFormat.Writer writer =
+ InputFormatTestUtil.writeDataBlockToLogFile(partitionDir, storage,
schema, "fileid0", instantTime,
+ newCommitTime, numRecords, numRecords, 0);
+ writer.close();
+ FileCreateUtilsLegacy.createDeltaCommit(COMMIT_METADATA_SER_DE,
basePath.toString(), newCommitTime, commitMetadata);
+
+ HoodieRealtimeFileSplit split = new HoodieRealtimeFileSplit(
+ new FileSplit(new Path(partitionDir + "/fileid0_1-0-1_" + instantTime
+ ".parquet"), 0, 1, baseJobConf),
+ basePath.toUri().toString(),
Collections.singletonList(writer.getLogFile()), newCommitTime, false,
+ Option.empty());
+ RecordReader<NullWritable, ArrayWritable> reader = new
MapredParquetInputFormat().getRecordReader(
+ new FileSplit(split.getPath(), 0, fs.getLength(split.getPath()),
(String[]) null), baseJobConf, null);
+
+ JobConf jobConf = new JobConf(baseJobConf);
+ List<HoodieSchemaField> fields = schema.getFields();
+ setHiveColumnNameProps(fields, jobConf, true);
+ // One more position than there are names, the shape reported on HUDI-1286.
+ String positions =
jobConf.get(ColumnProjectionUtils.READ_COLUMN_IDS_CONF_STR);
+ jobConf.set(ColumnProjectionUtils.READ_COLUMN_IDS_CONF_STR, positions +
"," + fields.size());
+
+ HoodieException thrown = assertThrows(HoodieException.class,
+ () -> new HoodieRealtimeRecordReader(split, jobConf, reader));
+ // The failure is wrapped twice on the way out ("Exception when
constructing record reader" then
+ // "Could not create HoodieRealtimeRecordReader on path ..."), so the
actionable detail sits a couple of
+ // causes down. That nesting is why the original report only shows it
under "Caused by".
+ assertNotNull(thrown.getCause(), "The construction failure should keep its
cause");
Review Comment:
Both resolved by deleting the test, so neither the redundant `assertNotNull`
nor the unnecessary log block survives.
You were right on the log block regardless:
`AbstractRealtimeRecordReader.init()` throws while resolving the projection,
before any log file is read, so writing one was pure setup cost.
--
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]
