rangareddy commented on code in PR #19463:
URL: https://github.com/apache/hudi/pull/19463#discussion_r3710050149
##########
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:
You are right on all three points, and I checked each before changing
anything.
- The reported frame is
`HiveInputFormat.getRecordReader(HiveInputFormat.java:376)` and `:379` — the
base class, so `CombineHiveInputFormat` was not on that stack.
- `HoodieRealtimeInputFormatUtils#addProjectionField` sets
`READ_COLUMN_NAMES_CONF_STR` and `READ_COLUMN_IDS_CONF_STR` in the same `if`
body (lines 76-77), so Hudi cannot make them diverge that way.
- HIVE-22438 is already documented in this repo, at
`HoodieRealtimeInputFormatUtils.java:134`, for exactly this query.
The claim is gone from the code and from the commit message — I rewrote the
commit rather than leaving it in history, since as you say it would outlive the
PR. The message now says the mechanism is **unconfirmed**, names HIVE-22438 /
HUDI-313 as what the repo documents for this query, and states explicitly that
an earlier draft asserted CombineHiveInputFormat and why that was wrong.
Thank you for this one in particular — it was the difference between a
plausible story and the actual evidence.
##########
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:
Confirmed and fixed. Probed all three of your inputs on master first:
```
("a,b,c", ",2,0") -> NumberFormatException: For input string: ""
("a,b,c,ts", ",2,0,3,5") -> #fieldNames: 4, #fieldPositions: 5, ...
("a,b,c", ",,2,0") -> NumberFormatException: For input string: ""
```
The middle line reproduces #14673 exactly, which I take as good evidence
that HIVE-22438 is the real mechanism.
Blank tokens are now filtered where the ids are split, with the HIVE-22438
reference in the comment.
One thing that came out better than your comment predicted: the `(a,b,c,ts |
,2,0,3,5)` case does not just reach the diagnostic, it **resolves**. Dropping
the blank leaves four real ids against four names, so the counts only ever
disagreed because the blank was counted — this is a functional fix for that
shape, not only a message improvement. I found that when my first version of
the test asserted an exception and failed.
Both of your inputs are now cases in `TestHoodieRealtimeRecordReaderUtils`:
`testOrderFieldsIgnoresBlankIdTokens` (including the `,,2,0` form, since
`cleanProjectionColumnIds` strips only one comma) and
`testOrderFieldsResolvesTheShapeReportedInTheIssue`. Both fail on master.
##########
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:
Verified and filed: **#19506**, referenced from the PR description.
Your reading holds — `appendReadColumns` prepends ids while
`appendReadColumnNames` appends names, so duplicate positions can differ
between the two lists, the counts still match, and the mapping is silently
wrong. I included your worked example and the `getReadColumnIDs` citation, and
credited you.
I have deliberately not touched the pairing in this PR, and the issue says
so explicitly, so the message improvement here is not later mistaken for a fix
for that. Agreed it is not a regression from this change.
##########
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:
You are right, and I have deleted the test.
The names come from `schema.getFields()` and Avro forbids duplicate field
names, so `fieldNames.size() == fieldNamesSet.size()` always holds for that
input and the count assertion cannot discriminate. Only the projection-list
assertion could, and `TestHoodieRealtimeRecordReaderUtils` covers that without
building a MOR table, a parquet base file and a log block.
I chose deletion over making it discriminate because your other two comments
apply to the same test — it was a third copy of the `testUnMergedReader`
fixture, and the log block it wrote was not needed to reach `orderFields`.
Removing it resolves all three and keeps the PR proportionate to what it
changes, which is an error message.
Happy to add it back as a discriminating test routed through a shared
`prepareMorSplitAndBaseReader` helper if you would rather have end-to-end
coverage of the cause chain; I did not want to land the helper refactor as a
side effect of a diagnostics fix.
##########
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:
Corrected, and thank you — I had this backwards in the PR description too.
`30d497a19844` ([HUDI-5308], #7355) did remove the `.filter(fn ->
!partitioningFields.contains(fn))` and its comment, so the parameter was
implemented and then deliberately dropped. My "intent that was never
implemented" line was wrong and would have misled whoever picks up HUDI-1286;
the description now says what actually happened and names the commit.
The test is now your version, as a HUDI-5308 guard with the partition column
inside the name list:
```java
@Test
public void testOrderFieldsNoLongerFiltersPartitionFields() {
assertThrows(HoodieException.class, () ->
HoodieRealtimeRecordReaderUtils.orderFields(
"rider,driver,partition_path", "0,1",
Collections.singletonList("partition_path")));
}
```
`java.util.List` is gone from the 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);
Review Comment:
Resolved by deleting the test — see my reply on the assertions. I agreed
with that comment, so there is no third copy of the fixture to extract a helper
from.
For the record I think the helper is worth doing on its own:
`testReaderInternal` and `testUnMergedReader` still open-code the same setup
twice. I would rather not land that refactor inside a diagnostics PR, but say
the word and I will raise it separately.
##########
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:
Right — that block asserted only counts, which master already printed for an
input with no duplicates, so it added nothing.
Split into two separately named tests as you suggested, so which one guards
the change is obvious:
- `testOrderFieldsMismatchReportsDistinctCountsWhenNamesRepeat` — the
duplicate-name case, asserting `#distinctFieldNames: 3` /
`#distinctFieldPositions: 4`.
- `testOrderFieldsMismatchReportsBothProjectionLists` — your
`("rider,driver", "0,1,2")` input, now asserting `read column names:
[rider,driver]` and `read column ids: [0,1,2]`.
Both fail on master. In total four of the eight tests fail against master's
`orderFields`:
```
testOrderFieldsMismatchReportsBothProjectionLists Error ...
#fieldNames: 2, #fieldPositions: 3
testOrderFieldsMismatchReportsDistinctCountsWhenNamesRepeat Error ...
#fieldNames: 4, #fieldPositions: 4
testOrderFieldsIgnoresBlankIdTokens » Hoodie Error
... #fieldNames: 2, #fieldPositions: 3
testOrderFieldsResolvesTheShapeReportedInTheIssue » Hoodie Error
... #fieldNames: 4, #fieldPositions: 5
```
The second line is the equal-numbers pathology and the fourth is the #14673
shape. The remaining four characterise existing behaviour, which I have said so
in the commit message rather than implying they guard this change.
##########
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:
Applied — `#distinctFieldNames` / `#distinctFieldPositions`. You are right
that a de-duplicated count printed beside a raw list reads as a contradiction,
and naming them removes the need to explain it:
```
Error ordering fields for storage read. #distinctFieldNames: 3,
#distinctFieldPositions: 4,
read column names: [rider,driver,fare,fare], read column ids: [0,1,2,3]
```
The three-line comment is down to the two points that are still not obvious
from the code. Substring assertions updated in both tests.
--
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]
