voonhous commented on code in PR #19463:
URL: https://github.com/apache/hudi/pull/19463#discussion_r3832454704
##########
hudi-hadoop-mr/src/main/java/org/apache/hudi/hadoop/utils/HoodieRealtimeInputFormatUtils.java:
##########
@@ -128,16 +128,33 @@ public static boolean canAddProjectionToJobConf(final
RealtimeSplit realtimeSpli
}
/**
- * Hive will append read columns' ids to old columns' ids during
getRecordReader. In some cases, e.g. SELECT COUNT(*),
- * the read columns' id is an empty string and Hive will combine it with
Hoodie required projection ids and becomes
- * e.g. ",2,0,3" and will cause an error. Actually this method is a
temporary solution because the real bug is from
- * Hive. Hive has fixed this bug after 3.0.0, but the version before that
would still face this problem. (HIVE-22438)
+ * Drops blank entries from the read-column id list held in {@code conf}.
+ *
+ * <p>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"} (HIVE-22438). Every consumer parses those
ids with
+ * {@code Integer#parseInt}, so a blank entry fails with a bare {@code
NumberFormatException} that carries
+ * none of the projection lists: {@code
SchemaEvolutionContext#setColumnTypeList},
+ * {@code HoodieColumnProjectionUtils#getReadColumnIDs} and
+ * {@code HoodieRealtimeRecordReaderUtils#orderFields} all do this. Cleaning
the conf once here covers all
+ * of them, including the bootstrap path that never reaches {@code
orderFields}.
Review Comment:
"Cleaning the conf once here covers all of them, including the bootstrap
path" does not hold for COW, and the same claim is in the commit message, so it
will outlive the PR.
The sanitizer has three call sites, none of them in
`HoodieParquetInputFormat`:
```
$ grep -rn "cleanProjectionColumnIds" --include="*.java"
hudi-hadoop-mr/src/main/java/
.../HoodieFileGroupReaderBasedRecordReader.java:114
.../utils/HoodieRealtimeInputFormatUtils.java:146 (definition)
.../realtime/HoodieHFileRealtimeInputFormat.java:81
.../realtime/HoodieParquetRealtimeInputFormat.java:129
```
Both of the other two consumers are reached from
`HoodieParquetInputFormat.getRecordReader`:
- `:154` `if (split instanceof BootstrapBaseFileSplit)` ->
`createBootstrappingRecordReader` -> `:193 getReadColumnIDs` -> `parseInt`
- `:159` `new SchemaEvolutionContext(...).doEvolutionForParquetFormat()` ->
`SchemaEvolutionContext:259 parseInt`
`shouldUseFilegroupReader` (`HoodieInputFormatUtils:568-572`) returns false
for `BootstrapBaseFileSplit` and when schema evolution is on, so both are
forced onto the legacy path. For MOR `_rt` they run after
`HoodieParquetRealtimeInputFormat:129` and are covered. For COW they are not:
`TestHiveTableSchemaEvolution:233` uses `new HoodieParquetInputFormat()`
directly for the `cow` arm, and nothing sanitizes that conf.
Pick one. Either add the call so the claim becomes true, which is safe
because the method is idempotent after this change and the realtime subclass
calling it again at `:129` is a no-op:
```java
// HoodieParquetInputFormat.getRecordReader, after line 121
HoodieRealtimeInputFormatUtils.cleanProjectionColumnIds(job);
```
Or drop "including the bootstrap path that never reaches orderFields" from
this javadoc, from the commit message, and from the table in the PR body, and
say that COW bootstrap and COW schema-on-read remain unprotected.
##########
hudi-hadoop-mr/src/main/java/org/apache/hudi/hadoop/utils/HoodieRealtimeInputFormatUtils.java:
##########
@@ -128,16 +128,33 @@ public static boolean canAddProjectionToJobConf(final
RealtimeSplit realtimeSpli
}
/**
- * Hive will append read columns' ids to old columns' ids during
getRecordReader. In some cases, e.g. SELECT COUNT(*),
- * the read columns' id is an empty string and Hive will combine it with
Hoodie required projection ids and becomes
- * e.g. ",2,0,3" and will cause an error. Actually this method is a
temporary solution because the real bug is from
- * Hive. Hive has fixed this bug after 3.0.0, but the version before that
would still face this problem. (HIVE-22438)
+ * Drops blank entries from the read-column id list held in {@code conf}.
+ *
+ * <p>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"} (HIVE-22438). Every consumer parses those
ids with
+ * {@code Integer#parseInt}, so a blank entry fails with a bare {@code
NumberFormatException} that carries
+ * none of the projection lists: {@code
SchemaEvolutionContext#setColumnTypeList},
+ * {@code HoodieColumnProjectionUtils#getReadColumnIDs} and
+ * {@code HoodieRealtimeRecordReaderUtils#orderFields} all do this. Cleaning
the conf once here covers all
Review Comment:
The third call site does not count towards "all of them" either.
`HoodieFileGroupReaderBasedRecordReader:114` cleans `jobConfCopy`, but
`HiveHoodieReaderContext.setSchemas` (`:113-115`, called from `:175`) then
overwrites both `READ_COLUMN_NAMES` and `READ_COLUMN_IDS` from the requested
schema before the Hive reader is built, and nothing in between reads the ids:
`createRequestedSchema` (`:317-323`) uses names only.
So of the three sites, one is inert for ids and one covers the realtime
formats only. Please stop citing the file-group-reader path as coverage in this
javadoc and in the PR body. Optional, a one-line `// no-op for ids today:
setSchemas overwrites them` at that call site so the next reader does not
assume it is load-bearing.
##########
hudi-hadoop-mr/src/main/java/org/apache/hudi/hadoop/utils/HoodieRealtimeInputFormatUtils.java:
##########
@@ -128,16 +128,33 @@ public static boolean canAddProjectionToJobConf(final
RealtimeSplit realtimeSpli
}
/**
- * Hive will append read columns' ids to old columns' ids during
getRecordReader. In some cases, e.g. SELECT COUNT(*),
- * the read columns' id is an empty string and Hive will combine it with
Hoodie required projection ids and becomes
- * e.g. ",2,0,3" and will cause an error. Actually this method is a
temporary solution because the real bug is from
- * Hive. Hive has fixed this bug after 3.0.0, but the version before that
would still face this problem. (HIVE-22438)
+ * Drops blank entries from the read-column id list held in {@code conf}.
+ *
+ * <p>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"} (HIVE-22438). Every consumer parses those
ids with
+ * {@code Integer#parseInt}, so a blank entry fails with a bare {@code
NumberFormatException} that carries
+ * none of the projection lists: {@code
SchemaEvolutionContext#setColumnTypeList},
+ * {@code HoodieColumnProjectionUtils#getReadColumnIDs} and
+ * {@code HoodieRealtimeRecordReaderUtils#orderFields} all do this. Cleaning
the conf once here covers all
+ * of them, including the bootstrap path that never reaches {@code
orderFields}.
+ *
+ * <p>This is a workaround: the underlying bug is in Hive, fixed after
3.0.0, but earlier versions still
+ * hit it. Stripping a single leading comma is not enough. Hive prepends ids
while appending names, so repeated
+ * empty appends give {@code ",,2,0"}, and an id prepended after an empty
one gives {@code "3,,2,0"} where
+ * the blank is interior and no amount of leading-comma stripping reaches it.
*/
public static void cleanProjectionColumnIds(Configuration conf) {
- String columnIds =
conf.get(ColumnProjectionUtils.READ_COLUMN_IDS_CONF_STR);
- if (!columnIds.isEmpty() && columnIds.charAt(0) == ',') {
- conf.set(ColumnProjectionUtils.READ_COLUMN_IDS_CONF_STR,
columnIds.substring(1));
- LOG.debug("The projection Ids: {{}} start with ','. First comma is
removed", columnIds);
+ String columnIds =
conf.get(ColumnProjectionUtils.READ_COLUMN_IDS_CONF_STR, "");
+ if (columnIds.isEmpty()) {
+ return;
+ }
Review Comment:
This early return is dead once `conf.get` has a default. `"".split(",")`
gives `[""]`, the filter drops it, `collect` yields `""`, and
`"".equals(columnIds)` is true, so no write happens on either route. Both
`testCleanProjectionColumnIdsWithUnsetKey` and the `clean("")` assertion still
pass with these three lines deleted.
Related: the write-back guard at `:155` is pinned by nothing. A mutant that
deletes the guard and always calls `conf.set` leaves the whole suite green, so
`"a clean list should be left alone"` and `"an empty list should be left
alone"` (`TestHoodieRealtimeInputFormatUtils:72-73`) do not assert what their
messages claim.
Pick one: delete the early return and reword those two messages to drop the
"left alone" claim, or pin the guard for real. mockito is already on this
module's test classpath (`TestHoodieRealtimeFileSplit`,
`TestHiveHoodieReaderContext`):
```java
Configuration base = new Configuration();
base.set(ColumnProjectionUtils.READ_COLUMN_IDS_CONF_STR, "2,0");
Configuration spied = Mockito.spy(base);
HoodieRealtimeInputFormatUtils.cleanProjectionColumnIds(spied);
Mockito.verify(spied,
Mockito.never()).set(eq(ColumnProjectionUtils.READ_COLUMN_IDS_CONF_STR), any());
```
##########
hudi-hadoop-mr/src/test/java/org/apache/hudi/hadoop/utils/TestHoodieRealtimeRecordReaderUtils.java:
##########
@@ -0,0 +1,148 @@
+/*
+ * 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} now strips every blank id
+ * from the conf, so this is the defence-in-depth path for callers that
build the csv themselves.
+ */
+ @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()),
+ "more than one blank token can arrive when Hive appends empty ids
repeatedly");
+ }
+
+ /**
+ * 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());
+ }
+
+ /**
+ * Hive tolerates duplicate ids as well as duplicate names, which is why
both sides are de-duplicated
+ * before pairing. Only the name side was pinned.
+ */
+ @Test
+ public void testOrderFieldsDeduplicatesRepeatedIds() {
+ assertEquals(Arrays.asList("rider", "driver"),
+ HoodieRealtimeRecordReaderUtils.orderFields("rider,driver", "0,1,1",
Collections.emptyList()));
+ }
+
+ /**
+ * 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.
+ */
Review Comment:
Two points on this test, neither blocking.
The javadoc says `",2,0,3,5"` "is what the HIVE-22438 combining produces".
It is not. Hive emits `",,2,0,3,5"` for those numbers; master's
`cleanProjectionColumnIds` strips one leading comma and leaves the value this
test uses, which is what `orderFields` actually saw. Worth stating, because
after this change no production path delivers a blank to `orderFields` at all,
so this case is defence in depth rather than the #14673 repro its name implies.
Second, it kills no mutant that `testOrderFieldsIgnoresBlankIdTokens` does
not also kill. Same shape, counts reconcile after filtering, successful
projection; only the arity differs.
```suggestion
/**
* The #14673 numbers, four names against five id tokens with one blank.
Hive emits {@code ",,2,0,3,5"};
* master's {@code cleanProjectionColumnIds} strips one leading comma and
leaves the value below, which is
* what {@code orderFields} saw. Since this change strips every blank in
the conf, no production path
* delivers a blank here any more, so this is defence in depth rather than
the reported repro.
*/
```
Optional: fold the assertion into `testOrderFieldsIgnoresBlankIdTokens` as a
third case instead of carrying a separate `@Test`.
##########
hudi-hadoop-mr/src/test/java/org/apache/hudi/hadoop/utils/TestHoodieRealtimeInputFormatUtils.java:
##########
@@ -45,4 +50,38 @@ public void testAddProjectionField() {
hadoopConf.set(hive_metastoreConstants.META_TABLE_PARTITION_COLUMNS, "");
HoodieRealtimeInputFormatUtils.addProjectionField(hadoopConf,
hadoopConf.get(hive_metastoreConstants.META_TABLE_PARTITION_COLUMNS,
"").split("/"));
}
+
+ private String clean(String columnIds) {
+ hadoopConf.set(ColumnProjectionUtils.READ_COLUMN_IDS_CONF_STR, columnIds);
+ HoodieRealtimeInputFormatUtils.cleanProjectionColumnIds(hadoopConf);
+ return hadoopConf.get(ColumnProjectionUtils.READ_COLUMN_IDS_CONF_STR);
+ }
+
+ /**
+ * 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"}. Every consumer of this conf
value parses the ids with
+ * {@code Integer#parseInt}, so any blank entry left behind fails with a
bare {@code NumberFormatException}.
+ */
+ @Test
+ public void testCleanProjectionColumnIdsDropsBlankEntries() {
+ assertEquals("2,0", clean(",2,0"), "a leading blank id should be dropped");
+ assertEquals("2,0", clean(",,2,0"),
+ "Hive appending empty ids repeatedly yields more than one leading
blank");
+ assertEquals("3,2,0", clean("3,,2,0"),
+ "an id prepended after an empty one leaves the blank interior, where
leading-comma stripping never reached");
Review Comment:
This message frames the interior blank as the case leading-comma stripping
could never reach, which is true, but the transformation it pins has no correct
outcome end to end.
An interior blank can only appear after a Hive prepend that follows an empty
append, which is exactly the ids-prepend/names-append asymmetry in #19506.
Driving hive-serde 2.3.10 directly:
```
append [2,0]/[b,c] ids "2,0" names "b,c"
append []/[] ids ",2,0" names "b,c"
append [3]/[d] ids "3,,2,0" names "b,c,d"
```
So for names `"b,c,d"` and ids `"3,,2,0"`: master throws the count mismatch,
this branch returns `[d, c, b]`, and the correct pairing is `[c, b, d]`.
To be clear this is #19506 and not a regression you introduced, and
narrowing the filter would not fix it -- leading blanks are not universally
safe either, `",,7,2,0"` with `"b,c,g"` mis-orders the same way. The ask is
only that the assertion message stops reading as a fix:
```suggestion
assertEquals("3,2,0", clean("3,,2,0"),
"an interior blank, which leading-comma stripping never reached; the
resulting pairing is still "
+ "unsound per #19506, this only stops the bare
NumberFormatException");
```
##########
hudi-hadoop-mr/src/test/java/org/apache/hudi/hadoop/utils/TestHoodieRealtimeRecordReaderUtils.java:
##########
@@ -0,0 +1,148 @@
+/*
+ * 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:
Both de-duplication tests put the duplicate last, which is the one position
where deduped and raw pairing coincide, so they pin the count rather than the
pairing. Mutants that keep the `LinkedHashSet` only for the size check and then
pair from the raw list (`fieldNamesArray = fieldNames.toArray(...)`) or the raw
array (`parseInt(fieldOrdersWithDups[ox])`) leave the whole suite green.
Moving the duplicate off the tail kills both. Verified against this branch:
```suggestion
assertEquals(Arrays.asList("rider", "driver"),
HoodieRealtimeRecordReaderUtils.orderFields("rider,rider,driver",
"0,1", Collections.emptyList()));
```
Same on the id side at `:121-122`: use `"0,0,1"` instead of `"0,1,1"`.
##########
hudi-hadoop-mr/src/main/java/org/apache/hudi/hadoop/utils/HoodieRealtimeInputFormatUtils.java:
##########
@@ -128,16 +128,33 @@ public static boolean canAddProjectionToJobConf(final
RealtimeSplit realtimeSpli
}
/**
- * Hive will append read columns' ids to old columns' ids during
getRecordReader. In some cases, e.g. SELECT COUNT(*),
- * the read columns' id is an empty string and Hive will combine it with
Hoodie required projection ids and becomes
- * e.g. ",2,0,3" and will cause an error. Actually this method is a
temporary solution because the real bug is from
- * Hive. Hive has fixed this bug after 3.0.0, but the version before that
would still face this problem. (HIVE-22438)
+ * Drops blank entries from the read-column id list held in {@code conf}.
+ *
+ * <p>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"} (HIVE-22438). Every consumer parses those
ids with
+ * {@code Integer#parseInt}, so a blank entry fails with a bare {@code
NumberFormatException} that carries
+ * none of the projection lists: {@code
SchemaEvolutionContext#setColumnTypeList},
Review Comment:
`SchemaEvolutionContext#setColumnTypeList` is named here as a consumer this
change protects, but it still carries the exact bug you fixed one method below.
`job.get(ColumnProjectionUtils.READ_COLUMN_IDS_CONF_STR)` has no default at
`SchemaEvolutionContext:259`, `:263` and `:392`, which is the same
missing-default NPE that `dd31c9b5d8dc` ([MINOR] NPE fix while adding
projection field, #10313) fixed in `addProjectionField`.
`setColumnNameList` (`SchemaEvolutionContext:388-404`) is also missing from
this list. It runs first (`:250`, ahead of `:251 setColumnTypeList`) and has
the same two hazards plus an `IndexOutOfBounds` on `fullColNamelist.get(id)`.
Add the `, ""` default at those three lines, or file a follow-up and
reference it here. Either way please add `setColumnNameList` to the list above,
so this javadoc is not read as an exhaustive audit of the consumers.
##########
hudi-hadoop-mr/src/test/java/org/apache/hudi/hadoop/utils/TestHoodieRealtimeInputFormatUtils.java:
##########
@@ -45,4 +50,38 @@ public void testAddProjectionField() {
hadoopConf.set(hive_metastoreConstants.META_TABLE_PARTITION_COLUMNS, "");
HoodieRealtimeInputFormatUtils.addProjectionField(hadoopConf,
hadoopConf.get(hive_metastoreConstants.META_TABLE_PARTITION_COLUMNS,
"").split("/"));
}
+
+ private String clean(String columnIds) {
+ hadoopConf.set(ColumnProjectionUtils.READ_COLUMN_IDS_CONF_STR, columnIds);
+ HoodieRealtimeInputFormatUtils.cleanProjectionColumnIds(hadoopConf);
+ return hadoopConf.get(ColumnProjectionUtils.READ_COLUMN_IDS_CONF_STR);
+ }
+
+ /**
+ * 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"}. Every consumer of this conf
value parses the ids with
+ * {@code Integer#parseInt}, so any blank entry left behind fails with a
bare {@code NumberFormatException}.
+ */
+ @Test
+ public void testCleanProjectionColumnIdsDropsBlankEntries() {
Review Comment:
Nothing exercises the two changed methods together, so the claim this PR
rests on -- that cleaning the conf here covers `orderFields` -- has no coverage
at any call site. `addProjectionToJobConf` has no tests at all (`git grep -n
addProjectionToJobConf -- '*/src/test/*'` is empty), and both new test classes
drive the statics directly.
This does not need the fixture that was dropped in round 1.
`TestHoodieRealtimeRecordReader` already sets `FILE_GROUP_READER_ENABLED=false`
in `setUp` (`:126`) and already drives
`HoodieParquetRealtimeInputFormat.getRecordReader` end to end at `:730` and
`:778`. Two lines in one of those, right after `setHiveColumnNameProps(fields,
newJobConf, false)`:
```java
newJobConf.set(ColumnProjectionUtils.READ_COLUMN_IDS_CONF_STR,
",," + newJobConf.get(ColumnProjectionUtils.READ_COLUMN_IDS_CONF_STR));
```
then assert the rows still read. Use two blanks, not one: master strips a
single leading comma at `HoodieRealtimeInputFormatUtils:137`, so `","` passes
on master and would not discriminate.
##########
hudi-hadoop-mr/src/main/java/org/apache/hudi/hadoop/utils/HoodieRealtimeRecordReaderUtils.java:
##########
@@ -273,15 +273,25 @@ 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(",");
+ // Defence in depth.
HoodieRealtimeInputFormatUtils#cleanProjectionColumnIds now drops blank ids
from the
+ // JobConf before any reader runs, which is what HIVE-22438 produces for
SELECT COUNT(*) on Hive before
+ // 3.0.0, so blanks should no longer arrive here. Callers that assemble
the csv without going through that
+ // conf still can, and a blank token would otherwise reach
Integer.parseInt below and fail with a bare
+ // NumberFormatException carrying neither projection list. Trim before
filtering so a padded id parses and
+ // de-duplicates as the same entry rather than as a distinct one.
+ String[] fieldOrdersWithDups = Arrays.stream(fieldOrderCsv.split(","))
+ .map(String::trim).filter(id -> !id.isEmpty()).toArray(String[]::new);
Review Comment:
The `.map(String::trim)` added here in the last round is pinned by nothing,
and neither is its sibling at `HoodieRealtimeInputFormatUtils:152`. No input in
either test class contains whitespace, so deleting both trims leaves the full
suite green and the next refactor can drop them silently.
That matters more on this side than in the conf method: without the trim,
`Integer.parseInt(" 0")` at `:299` throws the bare `NumberFormatException` this
PR exists to convert into a diagnosable message, and `"2"` and `" 2"` count as
distinct `LinkedHashSet` entries.
One assertion on each side closes it. Both verified against this branch:
```java
// TestHoodieRealtimeRecordReaderUtils#testOrderFieldsIgnoresBlankIdTokens
assertEquals(Arrays.asList("c", "b"),
HoodieRealtimeRecordReaderUtils.orderFields("b,c", " 2 , 0 ",
Collections.emptyList()),
"a padded id should parse rather than reach Integer.parseInt untrimmed");
//
TestHoodieRealtimeInputFormatUtils#testCleanProjectionColumnIdsDropsBlankEntries
assertEquals("2,0", clean(" 2 , 0 "), "padded ids should be trimmed, not
left for parseInt");
```
##########
hudi-hadoop-mr/src/main/java/org/apache/hudi/hadoop/utils/HoodieRealtimeInputFormatUtils.java:
##########
@@ -128,16 +128,33 @@ public static boolean canAddProjectionToJobConf(final
RealtimeSplit realtimeSpli
}
/**
- * Hive will append read columns' ids to old columns' ids during
getRecordReader. In some cases, e.g. SELECT COUNT(*),
- * the read columns' id is an empty string and Hive will combine it with
Hoodie required projection ids and becomes
- * e.g. ",2,0,3" and will cause an error. Actually this method is a
temporary solution because the real bug is from
- * Hive. Hive has fixed this bug after 3.0.0, but the version before that
would still face this problem. (HIVE-22438)
+ * Drops blank entries from the read-column id list held in {@code conf}.
+ *
+ * <p>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"} (HIVE-22438). Every consumer parses those
ids with
+ * {@code Integer#parseInt}, so a blank entry fails with a bare {@code
NumberFormatException} that carries
+ * none of the projection lists: {@code
SchemaEvolutionContext#setColumnTypeList},
+ * {@code HoodieColumnProjectionUtils#getReadColumnIDs} and
+ * {@code HoodieRealtimeRecordReaderUtils#orderFields} all do this. Cleaning
the conf once here covers all
+ * of them, including the bootstrap path that never reaches {@code
orderFields}.
+ *
+ * <p>This is a workaround: the underlying bug is in Hive, fixed after
3.0.0, but earlier versions still
+ * hit it. Stripping a single leading comma is not enough. Hive prepends ids
while appending names, so repeated
+ * empty appends give {@code ",,2,0"}, and an id prepended after an empty
one gives {@code "3,,2,0"} where
+ * the blank is interior and no amount of leading-comma stripping reaches it.
*/
public static void cleanProjectionColumnIds(Configuration conf) {
Review Comment:
Judgement call, I have not reproduced it: the body of this method is an
unsynchronized read-modify-write on a JobConf that the caller deliberately
locks, and this change widens the window.
History, all three ancestors of master:
- `ee0fd06de73e` (2019-10-30, "synchronized lock on conf object instead of
class") put `synchronized (conf)` inside this method.
- `3a05edab01f7` (2019-11-03, "Fixing RT queries for HiveOnSpark that causes
race conditions") removed that inner lock and moved the call inside the
caller's `synchronized (jobConf)` latch.
- `f41539a9cb5f` (2021-11-07, [HUDI-313] #3630) moved the call back out of
the latch so it runs on every `getRecordReader`, and did not restore the inner
lock.
`Configuration.get` and `Configuration.set` are not synchronized. Before
this change the write fired only when the value started with a comma; now it
fires for any blank or padded token, a strict superset. A thread outside the
lock can therefore clobber a concurrent `addProjectionField` write from inside
it, producing the names/ids divergence this PR improves the message for.
Wrap the body in `synchronized (conf) { ... }`, restoring `ee0fd06de73e`. Do
not move the call back inside the latch, that would revert `f41539a9cb5f`.
--
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]
