voonhous commented on code in PR #19463:
URL: https://github.com/apache/hudi/pull/19463#discussion_r3832454731
##########
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:
Dead once `conf.get` has a default: `"".split(",")` gives `[""]`, the filter
drops it, the join yields `""`, and `"".equals(columnIds)` skips the write
anyway. Deleting these three lines keeps
`testCleanProjectionColumnIdsWithUnsetKey` and the `clean("")` case green.
Separately, the write-back guard at `:155` is unpinned. A mutant that always
calls `conf.set` leaves the suite green, so the "should be left alone" messages
at `TestHoodieRealtimeInputFormatUtils:72-73` assert nothing.
Delete the early return and reword those two messages, or pin the guard with
a mockito spy (`verify(spied, never()).set(...)`; mockito is already on this
module's test classpath).
##########
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:
This trim and its sibling at `HoodieRealtimeInputFormatUtils:152` are pinned
by nothing. No test input contains whitespace, so deleting both leaves the
suite green. It matters more here: without it `Integer.parseInt(" 0")` at
`:299` throws the bare NFE this PR removes, and `"2"` / `" 2"` count as
distinct set entries.
One assertion each side, both verified on this branch:
```java
assertEquals(Arrays.asList("c", "b"),
HoodieRealtimeRecordReaderUtils.orderFields("b,c", " 2 , 0 ",
Collections.emptyList()));
assertEquals("2,0", clean(" 2 , 0 ")); //
TestHoodieRealtimeInputFormatUtils
```
##########
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 wiring this PR
rests on is unpinned. `addProjectionToJobConf` has no tests at all.
No new fixture needed. `TestHoodieRealtimeRecordReader` already sets
`FILE_GROUP_READER_ENABLED=false` at `:126` and drives
`HoodieParquetRealtimeInputFormat.getRecordReader` at `:730` and `:778`. After
`setHiveColumnNameProps(...)` add:
```java
newJobConf.set(ColumnProjectionUtils.READ_COLUMN_IDS_CONF_STR,
",," + newJobConf.get(ColumnProjectionUtils.READ_COLUMN_IDS_CONF_STR));
```
then assert the rows still read. Two blanks, not one: master strips a single
leading comma at `:137`.
--
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]
