voonhous commented on code in PR #19463:
URL: https://github.com/apache/hudi/pull/19463#discussion_r3704374645
##########
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);
if (fieldNamesSet.size() != fieldOrders.length) {
+ // Report the de-duplicated counts, since those are what were compared:
quoting the raw name count
+ // can produce a message whose two numbers are equal even though the
mismatch is real. The inputs are
+ // included because the projection lists come from Hive and are the only
way to diagnose the mismatch.
throw new HoodieException(String
Review Comment:
The blank-token shape that HIVE-22438 produces still bypasses this message
entirely, which undercuts the goal of the PR. With a leading empty id token the
two counts can line up, and `Integer.parseInt` on the next statement then
throws a bare `NumberFormatException` with no projection lists attached:
```
orderFields("a,b,c", ",2,0") -> NumberFormatException: For input
string: ""
orderFields("a,b,c,ts", ",2,0,3,5") -> Error ordering fields for storage
read. #fieldNames: 4, #fieldPositions: 5, ...
```
The second line reproduces the numbers reported in #14673 exactly. And
`cleanProjectionColumnIds` only strips a single leading comma
(`columnIds.substring(1)`), so `",,2,0"` still lands in the first case.
Please filter blank tokens where the ids are split (line 276,
`fieldOrderCsv.split(",")`) so this shape produces the new diagnostic message
instead of a bare NFE, and add both inputs above as cases in
`TestHoodieRealtimeRecordReaderUtils`.
##########
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.
Review Comment:
The javadoc states a root cause that #14673 does not support, and this
wording also lands in the commit message, so it will outlive the PR.
The reported query is `select count(*) from testdb.table1_rt` on hive-exec
2.3.3, and the reported frame is
`HiveInputFormat.getRecordReader(HiveInputFormat.java:376)` -- the base class.
`CombineHiveInputFormat` overrides `getRecordReader`, so it was not on that
stack. Hudi also cannot make the two lists diverge that way:
`HoodieRealtimeInputFormatUtils.addProjectionField` sets `READ_COLUMN_NAMES`
and `READ_COLUMN_IDS` inside the same `if`, always together.
The mechanism this repo already documents for exactly this query is
HIVE-22438, in `HoodieRealtimeInputFormatUtils.java:130-135`: for `SELECT
COUNT(*)` the read-column ids are an empty string, which Hive combines into
e.g. `",2,0,3"`. That is also how HUDI-313 (`3251d62bd3c7`, "Fix select count
star error when querying a realtime table") addressed it before -- by
sanitising the list.
Please replace the CombineHiveInputFormat explanation with a reference to
HIVE-22438 / HUDI-313, and say plainly that the mechanism behind #14673 is
unconfirmed rather than asserting one.
##########
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);
if (fieldNamesSet.size() != fieldOrders.length) {
+ // Report the de-duplicated counts, since those are what were compared:
quoting the raw name count
+ // can produce a message whose two numbers are equal even though the
mismatch is real. The inputs are
+ // included because the projection lists come from Hive and are the only
way to diagnose the mismatch.
throw new HoodieException(String
- .format("Error ordering fields for storage read. #fieldNames: %d,
#fieldPositions: %d",
- fieldNames.size(), fieldOrders.length));
+ .format("Error ordering fields for storage read. #fieldNames: %d,
#fieldPositions: %d, "
+ + "read column names: [%s], read column ids: [%s]",
+ fieldNamesSet.size(), fieldOrders.length, fieldNameCsv,
fieldOrderCsv));
}
TreeMap<Integer, String> orderedFieldMap = new TreeMap<>();
String[] fieldNamesArray = fieldNamesSet.toArray(new String[0]);
Review Comment:
Names and ids are de-duplicated independently above, then paired
positionally here and on the line below. That is only sound if duplicates fall
at the same offsets on both sides, and Hive guarantees they do not:
`ColumnProjectionUtils.appendReadColumns` **prepends** ids (`newConfStr = id +
"," + old`) while `appendReadColumnNames` **appends** names. Two accumulation
rounds on one JobConf:
```
READ_COLUMN_NAMES = _hoodie_commit_time,rider,driver,fare
READ_COLUMN_IDS = 2,3,0,1
orderFields -> [driver, fare, _hoodie_commit_time, rider]
```
No exception, wrong mapping. Hive's own `getReadColumnIDs` annotates the
hazard: "some code uses this list to correlate with column names, and yet these
lists may contain duplicates, which this call will remove and the other won't."
To be clear, this is not a regression from this PR, and the wrong order
looks masked today because `projectionFields` only feeds
`generateProjectionSchema` and the downstream consumers resolve by name. But it
is a live trap in the very method whose diagnostics you are fixing. Please file
a follow-up JIRA and reference it here, so the message improvement is not later
mistaken for a fix.
##########
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");
+ StringBuilder chain = new StringBuilder();
+ for (Throwable t = thrown; t != null; t = t.getCause()) {
+ chain.append(t.getMessage()).append(" | ");
+ }
+ String message = chain.toString();
+ assertTrue(message.contains("Error ordering fields for storage read"), ()
-> "Got: " + message);
+ assertTrue(message.contains("#fieldNames: " + fields.size())
+ && message.contains("#fieldPositions: " + (fields.size() + 1)),
+ () -> "The failure must report the counts that were compared. Got: " +
message);
Review Comment:
These two assertions cannot fail if the production change is reverted, so
this test does not guard what its failure message says it guards.
The names come from `schema.getFields()`, and Avro forbids duplicate field
names in a record, so `fieldNames.size() == fieldNamesSet.size()` always holds
for this input. Both before and after the change, the message reads
`#fieldNames: N, #fieldPositions: N+1`. Only the `read column names:` / `read
column ids:` assertion below actually discriminates -- and
`TestHoodieRealtimeRecordReaderUtils#orderFieldsMismatchReportsTheComparedCountsAndTheInputs`
already covers that, without building a MOR table, a parquet base file and a
log block.
Either drop this test, or make it discriminate by also appending a duplicate
name to `READ_COLUMN_NAMES_CONF_STR` so the raw and de-duplicated counts
genuinely differ. As written, the message on line 369 claims a check the
assertion cannot perform.
##########
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);
Review Comment:
This fixture is a near-verbatim copy of `testUnMergedReader` further down
the same file (lines 377-412): same `HoodieTestUtils.init`, same
`prepareParquetTable`, same `buildMetadata` / `createDeltaCommit`, same
`writeDataBlockToLogFile`, same split construction. The only real differences
are `numRecords` (1000 vs 10) and a dropped assertion. `testReaderInternal`
(lines 198-267) open-codes the same thing a third time.
If this test survives the point raised on the assertions below, please
extract a private helper -- something like
`prepareMorSplitAndBaseReader(HoodieSchema schema, int numRecords)` returning
the split plus the base reader -- and route both `testUnMergedReader` and this
test through it, rather than landing a third copy.
##########
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 {
+
+ @Test
+ void orderFieldsSortsNamesByTheirHivePosition() {
+ assertEquals(Arrays.asList("rider", "driver", "fare"),
+ HoodieRealtimeRecordReaderUtils.orderFields("driver,fare,rider",
"1,2,0", Collections.emptyList()));
+ }
+
+ @Test
+ void orderFieldsReturnsEmptyForEmptyInput() {
+ assertEquals(Collections.emptyList(),
+ HoodieRealtimeRecordReaderUtils.orderFields("", "",
Collections.emptyList()));
+ }
+
+ /**
+ * Hive can repeat a name in the read-column list while keeping ids unique,
which the method
+ * deliberately tolerates by de-duplicating both sides before pairing them.
+ */
+ @Test
+ void orderFieldsDeduplicatesRepeatedNames() {
+ assertEquals(Arrays.asList("rider", "driver"),
+ HoodieRealtimeRecordReaderUtils.orderFields("rider,driver,rider",
"0,1", Collections.emptyList()));
+ }
+
+ /**
+ * The counts compared are the de-duplicated ones, so the failure has to
report those. Reporting the
+ * raw name count instead can produce a message whose two numbers are equal
even though the mismatch
+ * is real, which is unusable when diagnosing something like HUDI-1286.
+ */
+ @Test
+ void orderFieldsMismatchReportsTheComparedCountsAndTheInputs() {
+ HoodieException withDuplicates = assertThrows(HoodieException.class, () ->
+ HoodieRealtimeRecordReaderUtils.orderFields("rider,driver,fare,fare",
"0,1,2,3", Collections.emptyList()));
+ assertTrue(withDuplicates.getMessage().contains("#fieldNames: 3"),
+ () -> "Expected the de-duplicated name count, got: " +
withDuplicates.getMessage());
+ assertTrue(withDuplicates.getMessage().contains("#fieldPositions: 4"),
+ () -> "Expected the position count, got: " +
withDuplicates.getMessage());
+ assertTrue(withDuplicates.getMessage().contains("rider,driver,fare,fare")
+ && withDuplicates.getMessage().contains("0,1,2,3"),
+ () -> "Expected the offending inputs to be included, got: " +
withDuplicates.getMessage());
+
+ HoodieException fewerNames = assertThrows(HoodieException.class, () ->
+ HoodieRealtimeRecordReaderUtils.orderFields("rider,driver", "0,1,2",
Collections.emptyList()));
+ assertTrue(fewerNames.getMessage().contains("#fieldNames: 2")
+ && fewerNames.getMessage().contains("#fieldPositions: 3"),
+ () -> "Got: " + fewerNames.getMessage());
Review Comment:
This second block passes unchanged on master, so it adds no coverage of the
change in this PR: `("rider,driver", "0,1,2")` has no duplicate names, so the
old message already read `#fieldNames: 2, #fieldPositions: 3`. The first block
is the only assertion in the whole PR that fails without the production change.
Point this block at what is actually new instead:
```suggestion
HoodieException fewerNames = assertThrows(HoodieException.class, () ->
HoodieRealtimeRecordReaderUtils.orderFields("rider,driver", "0,1,2",
Collections.emptyList()));
assertTrue(fewerNames.getMessage().contains("read column names:
[rider,driver]")
&& fewerNames.getMessage().contains("read column ids: [0,1,2]"),
() -> "Got: " + fewerNames.getMessage());
```
Splitting the two scenarios into separately named `@Test` methods would also
make it obvious which one covers the regression.
##########
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);
if (fieldNamesSet.size() != fieldOrders.length) {
+ // Report the de-duplicated counts, since those are what were compared:
quoting the raw name count
+ // can produce a message whose two numbers are equal even though the
mismatch is real. The inputs are
+ // included because the projection lists come from Hive and are the only
way to diagnose the mismatch.
throw new HoodieException(String
- .format("Error ordering fields for storage read. #fieldNames: %d,
#fieldPositions: %d",
- fieldNames.size(), fieldOrders.length));
+ .format("Error ordering fields for storage read. #fieldNames: %d,
#fieldPositions: %d, "
+ + "read column names: [%s], read column ids: [%s]",
Review Comment:
nit, feel free to ignore: `#fieldNames: 3` printed next to `read column
names: [rider,driver,fare,fare]` reads as a contradiction, because the count is
de-duplicated but the list beside it is raw. Naming the counts for what they
are removes the need to explain it at all:
```suggestion
.format("Error ordering fields for storage read.
#distinctFieldNames: %d, #distinctFieldPositions: %d, "
+ "read column names: [%s], read column ids: [%s]",
```
With that, the three-line comment above collapses to roughly one line. Note
this needs the matching substring assertions updated in both new tests.
##########
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:
nit, feel free to ignore: the class is package-private with sentence-style
method names, where this module is predominantly `public class` with
`test`-prefixed methods (129 of 141 test methods under
`hudi-hadoop-mr/src/test` start with `test`; the nearest neighbour,
`TestHoodieRealtimeInputFormatUtils`, is public). JUnit 5 does not require
public, so the visibility is fine to leave as is -- the method naming is the
part worth matching to its siblings.
Also `Arrays.asList("partition_path")` on line 90 can be
`Collections.singletonList(...)`, which this file already imports.
##########
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:
nit, feel free to ignore: this assertion is redundant. If the cause were
null, the loop below would collect only the outer message and the `Error
ordering fields for storage read` assertion on line 366 would already fail,
with a clearer diff than this one gives.
Separately, if this test survives: the log block written on lines 334-339 is
not needed to reach `orderFields`. `AbstractRealtimeRecordReader.init()` throws
while resolving the projection, before any log file is read. Dropping it would
cut most of the setup cost.
##########
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 {
+
+ @Test
+ void orderFieldsSortsNamesByTheirHivePosition() {
+ assertEquals(Arrays.asList("rider", "driver", "fare"),
+ HoodieRealtimeRecordReaderUtils.orderFields("driver,fare,rider",
"1,2,0", Collections.emptyList()));
+ }
+
+ @Test
+ void orderFieldsReturnsEmptyForEmptyInput() {
+ assertEquals(Collections.emptyList(),
+ HoodieRealtimeRecordReaderUtils.orderFields("", "",
Collections.emptyList()));
+ }
+
+ /**
+ * Hive can repeat a name in the read-column list while keeping ids unique,
which the method
+ * deliberately tolerates by de-duplicating both sides before pairing them.
+ */
+ @Test
+ void orderFieldsDeduplicatesRepeatedNames() {
+ assertEquals(Arrays.asList("rider", "driver"),
+ HoodieRealtimeRecordReaderUtils.orderFields("rider,driver,rider",
"0,1", Collections.emptyList()));
+ }
+
+ /**
+ * The counts compared are the de-duplicated ones, so the failure has to
report those. Reporting the
+ * raw name count instead can produce a message whose two numbers are equal
even though the mismatch
+ * is real, which is unusable when diagnosing something like HUDI-1286.
+ */
+ @Test
+ void orderFieldsMismatchReportsTheComparedCountsAndTheInputs() {
+ HoodieException withDuplicates = assertThrows(HoodieException.class, () ->
+ HoodieRealtimeRecordReaderUtils.orderFields("rider,driver,fare,fare",
"0,1,2,3", Collections.emptyList()));
+ assertTrue(withDuplicates.getMessage().contains("#fieldNames: 3"),
+ () -> "Expected the de-duplicated name count, got: " +
withDuplicates.getMessage());
+ assertTrue(withDuplicates.getMessage().contains("#fieldPositions: 4"),
+ () -> "Expected the position count, got: " +
withDuplicates.getMessage());
+ assertTrue(withDuplicates.getMessage().contains("rider,driver,fare,fare")
+ && withDuplicates.getMessage().contains("0,1,2,3"),
+ () -> "Expected the offending inputs to be included, got: " +
withDuplicates.getMessage());
+
+ HoodieException fewerNames = assertThrows(HoodieException.class, () ->
+ HoodieRealtimeRecordReaderUtils.orderFields("rider,driver", "0,1,2",
Collections.emptyList()));
+ assertTrue(fewerNames.getMessage().contains("#fieldNames: 2")
+ && fewerNames.getMessage().contains("#fieldPositions: 3"),
+ () -> "Got: " + fewerNames.getMessage());
+ }
+
+ @Test
+ void orderFieldsIgnoresPartitioningFieldsArgument() {
+ List<String> ordered =
+ HoodieRealtimeRecordReaderUtils.orderFields("rider,driver", "0,1",
Arrays.asList("partition_path"));
+ assertEquals(Arrays.asList("rider", "driver"), ordered);
+ }
Review Comment:
This test passes identically with and without the behaviour it appears to
pin, so it cannot fail.
The parameter is not "the remains of an intent that was never implemented"
as the PR description says -- it was implemented, then deliberately removed by
`30d497a19844` ([HUDI-5308] Hive3 query returns null when the where clause has
a partition field, #7355), which deleted:
```
- .filter(fn ->
!partitioningFields.contains(fn)).collect(Collectors.toList());
- // Hive does not provide ids for partitioning fields, so check for
lengths excluding that.
```
Because `partition_path` is not in the name list here, that removed filter
would have been a no-op on this input anyway.
Make it a real HUDI-5308 regression guard by putting the partition column
inside the name list:
```suggestion
@Test
void orderFieldsNoLongerFiltersPartitionFields() {
// HUDI-5308 (#7355) removed the partition-field filter, so a partition
column in the name list
// now counts towards the comparison instead of being dropped before it.
assertThrows(HoodieException.class, () ->
HoodieRealtimeRecordReaderUtils.orderFields(
"rider,driver,partition_path", "0,1",
Collections.singletonList("partition_path")));
}
```
That leaves `java.util.List` unused, so drop that import too. Worth
correcting the sentence in the PR description as well, since it will mislead
whoever picks up HUDI-1286.
##########
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:
nit, for the PR description rather than the code: it says the method
de-duplicates both lists "because Hive tolerates duplicate names but not
duplicate ids". That is the wrong way round.
The comment just above this line says Hive "does not handle duplicate field
names correctly but handles duplicate fields orders correctly", and it is
Hive's `ColumnProjectionUtils.getReadColumnIDs` that de-duplicates ("it may
contain duplicates, remove duplicates") while nothing de-duplicates the names.
Worth fixing the sentence so the rationale is not inverted for the next reader.
--
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]
