rangareddy commented on code in PR #19463:
URL: https://github.com/apache/hudi/pull/19463#discussion_r3817413334
##########
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:
Moved. `cleanProjectionColumnIds` now drops every blank entry and writes the
joined value back, replacing the `substring(1)`. Confirmed both other consumers
locally before moving it: `SchemaEvolutionContext:259` maps `parseInt` over the
split with no filter, and `getReadColumnIDs:84-89` relies on hadoop
`StringUtils.split`, which only drops trailing empties, so a leading blank
reaches `parseInt` on the bootstrap path. `orderFields` keeps its own filter as
defence in depth for callers that build the csv without going through the conf.
Also fixed the NPE you pointed at: `conf.get` there had no default, so an
unset key threw before anything else ran. Same shape as `addProjectionField`
four lines above.
Added `testCleanProjectionColumnIds*` in
`TestHoodieRealtimeInputFormatUtils`, which had one assertion-free test. One
deviation from your list: you asked to pin `",,2,0" -> ",2,0"`, which was the
pre-fix behaviour. Since the filter now lives in this method that case pins
`",,2,0" -> "2,0"`, and I added `"3,,2,0" -> "3,2,0"` for the interior blank
that leading-comma stripping could never reach. Both fail on master, the second
with the NPE.
##########
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:
Applied your suggestion as written. You are right on both counts: the trim
was only in the predicate, so `" 0"` survived untrimmed into `parseInt` and
counted as a `LinkedHashSet` entry distinct from `"0"`, and the `isEmpty()`
ternary was dead once the filter drops `[""]`.
##########
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:
Correct, and the Verification block did say so. Added
`testOrderFieldsBlankIdTokenNoLongerReachesIntegerParse` with `("a,b,c",
",2,0")` as suggested. Verified it discriminates: against master `orderFields`
it fails with `NumberFormatException: For input string: ""`, which is the
failure this PR is named for and which no other test here reached.
Dropped the stale sentence from both javadocs. The production comment at
`HoodieRealtimeRecordReaderUtils` now describes the filter as defence in depth,
since the primary fix moved to `cleanProjectionColumnIds`.
##########
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:
Added as `testOrderFieldsDeduplicatesRepeatedIds`, verbatim. Passes on
master, so it characterises the id-side de-duplication rather than pinning a
change.
##########
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:
Applied. It now asserts `#distinctFieldNames: 3, #distinctFieldPositions:
2`, so it fails on master on the message rather than passing on the bare
`assertThrows`.
##########
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:
Understood, and agreed the mis-pairing is #19506 rather than this PR. I will
record on that issue that the count check no longer catches the divergence, so
it is not assumed to still be loud. Worth noting the worked example gets
slightly stronger with this revision: `cleanProjectionColumnIds` now cleans
`",,7,2,0"` to `"7,2,0"` rather than `",7,2,0"`, so the blank never reaches
`orderFields` at all and the count check is bypassed one step earlier.
--
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]
