voonhous commented on code in PR #19463:
URL: https://github.com/apache/hudi/pull/19463#discussion_r3765214550
##########
hudi-hadoop-mr/src/main/java/org/apache/hudi/hadoop/utils/HoodieRealtimeRecordReaderUtils.java:
##########
@@ -273,15 +273,24 @@ public static List<String> orderFields(String
fieldNameCsv, String fieldOrderCsv
// /org/apache/hadoop/hive/serde2/ColumnProjectionUtils.java#L188}
// Field Names -> {@link
https://github.com/apache/hive/blob/f37c5de6c32b9395d1b34fa3c02ed06d1bfbf6eb/serde/src/java
// /org/apache/hadoop/hive/serde2/ColumnProjectionUtils.java#L229}
- String[] fieldOrdersWithDups = fieldOrderCsv.isEmpty() ? new String[0] :
fieldOrderCsv.split(",");
+ // Blank tokens are dropped rather than carried into the loop below. For
SELECT COUNT(*) on Hive before
+ // 3.0.0 the read-column ids arrive empty and Hive combines them into e.g.
",2,0,3" (HIVE-22438, see
+ // HoodieRealtimeInputFormatUtils#cleanProjectionColumnIds, which only
strips one leading comma). A blank
Review Comment:
The blank token stays in the JobConf, so `orderFields` is not the first
thing that parses it. On the legacy MOR path,
`HoodieParquetRealtimeInputFormat.java:89-90` is
```java
return new HoodieRealtimeRecordReader(realtimeSplit, jobConf,
super.getRecordReader(split, jobConf, reporter));
```
Java evaluates the third argument first, so
`HoodieParquetInputFormat.getRecordReader` runs before this method. That
reaches `SchemaEvolutionContext.doEvolutionForParquetFormat()` and then
`SchemaEvolutionContext.java:259-260`:
```java
List<Integer> tmpColIdList =
Arrays.stream(job.get(ColumnProjectionUtils.READ_COLUMN_IDS_CONF_STR).split(","))
.map(Integer::parseInt).collect(Collectors.toList());
```
No blank filter, so the same input still dies with a bare
`NumberFormatException` before your message is reached. Schema evolution forces
that path: `shouldUseFilegroupReader` requires `!SCHEMA_EVOLUTION_ENABLE`
(`HoodieInputFormatUtils.java:568-571`).
Same crash in `HoodieColumnProjectionUtils.getReadColumnIDs` (`:83-91`):
hadoop's `StringUtils.split` drops only trailing empties, so a leading blank
reaches `Integer.parseInt("")`. Called at `HoodieParquetInputFormat.java:193`
on the bootstrap path, which never calls the sanitizer at all.
Please move the filter into
`HoodieRealtimeInputFormatUtils#cleanProjectionColumnIds` (`:136-142`),
dropping every blank token and writing the joined value back to the conf in
place of the current `substring(1)`. That fixes all three consumers at once,
and it is the only way to reach an interior blank, which `substring(1)` can
never touch -- Hive prepending a real id after an empty prepend gives `"3" +
"," + ",2,0"` = `3,,2,0`. Keep this filter as defence in depth.
While you are in that method: `cleanProjectionColumnIds` has had no test
since `3251d62bd3c7` (2019). `grep -rn cleanProjectionColumnIds
hudi-hadoop-mr/src/test/` returns nothing, and
`TestHoodieRealtimeInputFormatUtils` holds a single assertion-free test. Please
add `testCleanProjectionColumnIds` pinning `",2,0" -> "2,0"`, `",,2,0" ->
",2,0"` (which pins the incompleteness this comment documents), and the
unset-key case, which NPEs today because `conf.get(READ_COLUMN_IDS_CONF_STR)`
has no default -- the same bug `bd59a866ea8c` fixed four lines above it in
`addProjectionField`.
##########
hudi-hadoop-mr/src/main/java/org/apache/hudi/hadoop/utils/HoodieRealtimeRecordReaderUtils.java:
##########
@@ -273,15 +273,24 @@ public static List<String> orderFields(String
fieldNameCsv, String fieldOrderCsv
// /org/apache/hadoop/hive/serde2/ColumnProjectionUtils.java#L188}
// Field Names -> {@link
https://github.com/apache/hive/blob/f37c5de6c32b9395d1b34fa3c02ed06d1bfbf6eb/serde/src/java
// /org/apache/hadoop/hive/serde2/ColumnProjectionUtils.java#L229}
- String[] fieldOrdersWithDups = fieldOrderCsv.isEmpty() ? new String[0] :
fieldOrderCsv.split(",");
+ // Blank tokens are dropped rather than carried into the loop below. For
SELECT COUNT(*) on Hive before
+ // 3.0.0 the read-column ids arrive empty and Hive combines them into e.g.
",2,0,3" (HIVE-22438, see
+ // HoodieRealtimeInputFormatUtils#cleanProjectionColumnIds, which only
strips one leading comma). A blank
+ // token used to reach Integer.parseInt and fail with a bare
NumberFormatException carrying none of the
+ // projection lists.
+ String[] fieldOrdersWithDups = fieldOrderCsv.isEmpty() ? new String[0]
+ : Arrays.stream(fieldOrderCsv.split(",")).filter(id ->
!id.trim().isEmpty()).toArray(String[]::new);
Review Comment:
Two small things on this expression, both fixed by the same edit.
The filter tests blankness with `trim()` but the surviving token is parsed
untrimmed at line 298, so a whitespace-padded id now *downgrades* the
diagnostic. `orderFields("a,b", ",2, 0")` throws the count-mismatch
`HoodieException` on master (3 tokens vs 2 names); with this patch the blank is
dropped, the counts line up, and `Integer.parseInt(" 0")` throws a bare
`NumberFormatException` -- the exact failure mode the PR removes. `"2"` and `"
2"` are also distinct `LinkedHashSet` entries, inflating `fieldOrders.length`.
This is also why `isBlank()` from the earlier bot nit would not fix it.
The `fieldOrderCsv.isEmpty()` ternary is dead now: `"".split(",")` returns
`[""]`, which the filter already drops to an empty array.
```suggestion
String[] fieldOrdersWithDups = Arrays.stream(fieldOrderCsv.split(","))
.map(String::trim).filter(id ->
!id.isEmpty()).toArray(String[]::new);
```
Known producers only emit bare integers, so this is defensive, but the
half-trim is worth not leaving behind.
##########
hudi-hadoop-mr/src/test/java/org/apache/hudi/hadoop/utils/TestHoodieRealtimeRecordReaderUtils.java:
##########
@@ -0,0 +1,124 @@
+/*
+ * 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 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.
+ */
+public class TestHoodieRealtimeRecordReaderUtils {
+
+ @Test
+ public void testOrderFieldsSortsNamesByTheirHivePosition() {
+ assertEquals(Arrays.asList("rider", "driver", "fare"),
+ HoodieRealtimeRecordReaderUtils.orderFields("driver,fare,rider",
"1,2,0", Collections.emptyList()));
+ }
+
+ @Test
+ public void testOrderFieldsReturnsEmptyForEmptyInput() {
+ 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
+ public void testOrderFieldsDeduplicatesRepeatedNames() {
+ assertEquals(Arrays.asList("rider", "driver"),
+ HoodieRealtimeRecordReaderUtils.orderFields("rider,driver,rider",
"0,1", Collections.emptyList()));
Review Comment:
nit, feel free to ignore: only the name side of the de-duplication is
pinned. The `LinkedHashSet` on the ids exists because Hive tolerates duplicate
ids -- the comment at `HoodieRealtimeRecordReaderUtils.java:269-270` says Hive
"handles duplicate fields orders correctly" -- and nothing here covers it. One
method does:
```java
@Test
public void testOrderFieldsDeduplicatesRepeatedIds() {
assertEquals(Arrays.asList("rider", "driver"),
HoodieRealtimeRecordReaderUtils.orderFields("rider,driver", "0,1,1",
Collections.emptyList()));
}
```
##########
hudi-hadoop-mr/src/test/java/org/apache/hudi/hadoop/utils/TestHoodieRealtimeRecordReaderUtils.java:
##########
@@ -0,0 +1,124 @@
+/*
+ * 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 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.
+ */
+public class TestHoodieRealtimeRecordReaderUtils {
+
+ @Test
+ public void testOrderFieldsSortsNamesByTheirHivePosition() {
+ assertEquals(Arrays.asList("rider", "driver", "fare"),
+ HoodieRealtimeRecordReaderUtils.orderFields("driver,fare,rider",
"1,2,0", Collections.emptyList()));
+ }
+
+ @Test
+ public void testOrderFieldsReturnsEmptyForEmptyInput() {
+ 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
+ public void testOrderFieldsDeduplicatesRepeatedNames() {
+ 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 prints two equal numbers for a real mismatch, which is
unusable when diagnosing
+ * something like HUDI-1286. This is the case that fails without the
production change.
+ */
+ @Test
+ public void testOrderFieldsMismatchReportsDistinctCountsWhenNamesRepeat() {
+ HoodieException thrown = assertThrows(HoodieException.class, () ->
+ HoodieRealtimeRecordReaderUtils.orderFields("rider,driver,fare,fare",
"0,1,2,3", Collections.emptyList()));
+ assertTrue(thrown.getMessage().contains("#distinctFieldNames: 3"),
+ () -> "Expected the de-duplicated name count, got: " +
thrown.getMessage());
+ assertTrue(thrown.getMessage().contains("#distinctFieldPositions: 4"),
+ () -> "Expected the position count, got: " + thrown.getMessage());
+ }
+
+ /** A mismatch with no duplicates on either side still has to carry both
projection lists. */
+ @Test
+ public void testOrderFieldsMismatchReportsBothProjectionLists() {
+ HoodieException thrown = assertThrows(HoodieException.class, () ->
+ HoodieRealtimeRecordReaderUtils.orderFields("rider,driver", "0,1,2",
Collections.emptyList()));
+ assertTrue(thrown.getMessage().contains("read column names:
[rider,driver]")
+ && thrown.getMessage().contains("read column ids: [0,1,2]"),
+ () -> "Expected both projection lists, got: " + thrown.getMessage());
+ }
+
+ /**
+ * HIVE-22438: for {@code SELECT COUNT(*)} on Hive before 3.0.0 the
read-column ids arrive empty and Hive
+ * combines them into e.g. {@code ",2,0,3"}. {@code
cleanProjectionColumnIds} strips only one leading
+ * comma, so a blank token can still reach here. It used to fail on {@code
Integer.parseInt} with a bare
+ * {@code NumberFormatException} carrying neither list.
+ */
+ @Test
+ public void testOrderFieldsIgnoresBlankIdTokens() {
+ assertEquals(Arrays.asList("c", "b"),
+ HoodieRealtimeRecordReaderUtils.orderFields("b,c", ",2,0",
Collections.emptyList()),
+ "a leading blank id token should be ignored rather than parsed");
+ assertEquals(Arrays.asList("c", "b"),
+ HoodieRealtimeRecordReaderUtils.orderFields("b,c", ",,2,0",
Collections.emptyList()),
+ "cleanProjectionColumnIds strips only one comma, so more than one
blank token can arrive");
+ }
Review Comment:
No test here reaches `Integer.parseInt` with a blank token, which is the
bare-`NumberFormatException`-to-`HoodieException` conversion the PR is named
for.
`("b,c", ",2,0")` is two names against three raw tokens, so master throws
the *count-mismatch* `HoodieException`, not an NFE. The blank only reaches
`parseInt` when it makes the counts line up, which needs one more name:
`("a,b,c", ",2,0")` is 3 names vs 3 raw tokens on master. That was the exact
input in the round-1 comment; the reply said both inputs had been added, but a
name was dropped from this one, which turns it into a different shape.
Your own Verification block records the same thing:
`testOrderFieldsIgnoresBlankIdTokens ยป Hoodie Error ... #fieldNames: 2,
#fieldPositions: 3`. So the last sentence of this javadoc describes a failure
no input in this file produces.
```suggestion
/**
* HIVE-22438: for {@code SELECT COUNT(*)} on Hive before 3.0.0 the
read-column ids arrive empty and Hive
* combines them into e.g. {@code ",2,0,3"}. {@code
cleanProjectionColumnIds} strips only one leading
* comma, so a blank token can still reach here.
*/
@Test
public void testOrderFieldsIgnoresBlankIdTokens() {
assertEquals(Arrays.asList("c", "b"),
HoodieRealtimeRecordReaderUtils.orderFields("b,c", ",2,0",
Collections.emptyList()),
"a leading blank id token should be ignored rather than parsed");
assertEquals(Arrays.asList("c", "b"),
HoodieRealtimeRecordReaderUtils.orderFields("b,c", ",,2,0",
Collections.emptyList()),
"cleanProjectionColumnIds strips only one comma, so more than one
blank token can arrive");
}
/**
* A blank token only reaches {@code Integer.parseInt} when it makes the
two counts line up, which needs one
* more name than the cases above. That is the shape that failed with a
bare {@code NumberFormatException}
* carrying neither projection list, and the only case here that exercises
the parse guard.
*/
@Test
public void testOrderFieldsBlankIdTokenNoLongerReachesIntegerParse() {
HoodieException thrown = assertThrows(HoodieException.class, () ->
HoodieRealtimeRecordReaderUtils.orderFields("a,b,c", ",2,0",
Collections.emptyList()));
assertTrue(thrown.getMessage().contains("read column ids: [,2,0]"),
() -> "Expected the raw id list, got: " + thrown.getMessage());
}
```
Please also drop the matching "used to reach Integer.parseInt and fail with
a bare NumberFormatException" sentence from the production comment at
`HoodieRealtimeRecordReaderUtils.java:278-280`, or keep it and point it at the
case added above.
##########
hudi-hadoop-mr/src/test/java/org/apache/hudi/hadoop/utils/TestHoodieRealtimeRecordReaderUtils.java:
##########
@@ -0,0 +1,124 @@
+/*
+ * 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 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.
+ */
+public class TestHoodieRealtimeRecordReaderUtils {
+
+ @Test
+ public void testOrderFieldsSortsNamesByTheirHivePosition() {
+ assertEquals(Arrays.asList("rider", "driver", "fare"),
+ HoodieRealtimeRecordReaderUtils.orderFields("driver,fare,rider",
"1,2,0", Collections.emptyList()));
+ }
+
+ @Test
+ public void testOrderFieldsReturnsEmptyForEmptyInput() {
+ 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
+ public void testOrderFieldsDeduplicatesRepeatedNames() {
+ 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 prints two equal numbers for a real mismatch, which is
unusable when diagnosing
+ * something like HUDI-1286. This is the case that fails without the
production change.
+ */
+ @Test
+ public void testOrderFieldsMismatchReportsDistinctCountsWhenNamesRepeat() {
+ HoodieException thrown = assertThrows(HoodieException.class, () ->
+ HoodieRealtimeRecordReaderUtils.orderFields("rider,driver,fare,fare",
"0,1,2,3", Collections.emptyList()));
+ assertTrue(thrown.getMessage().contains("#distinctFieldNames: 3"),
+ () -> "Expected the de-duplicated name count, got: " +
thrown.getMessage());
+ assertTrue(thrown.getMessage().contains("#distinctFieldPositions: 4"),
+ () -> "Expected the position count, got: " + thrown.getMessage());
+ }
+
+ /** A mismatch with no duplicates on either side still has to carry both
projection lists. */
+ @Test
+ public void testOrderFieldsMismatchReportsBothProjectionLists() {
+ HoodieException thrown = assertThrows(HoodieException.class, () ->
+ HoodieRealtimeRecordReaderUtils.orderFields("rider,driver", "0,1,2",
Collections.emptyList()));
+ assertTrue(thrown.getMessage().contains("read column names:
[rider,driver]")
+ && thrown.getMessage().contains("read column ids: [0,1,2]"),
+ () -> "Expected both projection lists, got: " + thrown.getMessage());
+ }
+
+ /**
+ * HIVE-22438: for {@code SELECT COUNT(*)} on Hive before 3.0.0 the
read-column ids arrive empty and Hive
+ * combines them into e.g. {@code ",2,0,3"}. {@code
cleanProjectionColumnIds} strips only one leading
+ * comma, so a blank token can still reach here. It used to fail on {@code
Integer.parseInt} with a bare
+ * {@code NumberFormatException} carrying neither list.
+ */
+ @Test
+ public void testOrderFieldsIgnoresBlankIdTokens() {
+ assertEquals(Arrays.asList("c", "b"),
+ HoodieRealtimeRecordReaderUtils.orderFields("b,c", ",2,0",
Collections.emptyList()),
+ "a leading blank id token should be ignored rather than parsed");
+ assertEquals(Arrays.asList("c", "b"),
+ HoodieRealtimeRecordReaderUtils.orderFields("b,c", ",,2,0",
Collections.emptyList()),
+ "cleanProjectionColumnIds strips only one comma, so more than one
blank token can arrive");
+ }
+
+ /**
+ * The shape reported in #14673 - four names against five id tokens, one of
them blank - is what the
+ * HIVE-22438 combining produces. Dropping the blank leaves four real ids
against four names, so it
+ * resolves rather than failing at all: the counts only ever disagreed
because the blank was counted.
+ */
+ @Test
+ public void testOrderFieldsResolvesBlankIdTokenCountMismatchFromIssue14673()
{
+ assertEquals(Arrays.asList("b", "a", "c", "ts"),
+ HoodieRealtimeRecordReaderUtils.orderFields("a,b,c,ts", ",2,0,3,5",
Collections.emptyList()),
+ "the blank id token was the whole mismatch; without it the projection
is well formed");
+ }
+
+ /**
+ * HUDI-5308 (#7355) removed the filter that dropped partitioning fields
from the name list before the
+ * comparison, so a partition column in that list now counts towards it.
Pins that removal.
+ */
+ @Test
+ public void testOrderFieldsNoLongerFiltersPartitionFields() {
+ assertThrows(HoodieException.class, () ->
HoodieRealtimeRecordReaderUtils.orderFields(
+ "rider,driver,partition_path", "0,1",
Collections.singletonList("partition_path")));
+ }
Review Comment:
nit: `partitioningFields` is dead in the method body -- `30d497a19844`
removed its only use -- so this passes identically with
`Collections.emptyList()`, and the bare `assertThrows` does not assert *why* it
threw. Asserting the counts makes it discriminate the reason as well as the
removal:
```suggestion
@Test
public void testOrderFieldsNoLongerFiltersPartitionFields() {
HoodieException thrown = assertThrows(HoodieException.class, () ->
HoodieRealtimeRecordReaderUtils.orderFields(
"rider,driver,partition_path", "0,1",
Collections.singletonList("partition_path")));
assertTrue(thrown.getMessage().contains("#distinctFieldNames: 3,
#distinctFieldPositions: 2"),
() -> "the partition column should still count towards the
comparison, got: " + thrown.getMessage());
}
```
--
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]
