rangareddy commented on code in PR #19463: URL: https://github.com/apache/hudi/pull/19463#discussion_r3888333574
########## 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, () -> Review Comment: Done, renamed to testOrderFieldsMismatchAfterBlankIdFilteringReportsRawIdList so the HoodieException is visible in the name. ########## 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: Fixed by the first option: HoodieParquetInputFormat#getRecordReader now calls cleanProjectionColumnIds(job) right after addProjectionField, so the bootstrap and parquet schema-evolution readers are covered on COW. The javadoc, commit message and PR body now say that rather than claiming the realtime formats covered it, and TestHiveTableSchemaEvolution passes (5/5). ########## 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: Done, dropped. You are right that setSchemas overwrites both keys from the requested schema before the reader is built and createRequestedSchema reads names only, so it is no longer listed as a consumer in the javadoc or the PR body. ########## 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: Fixed rather than deferred: both methods now go through a parseReadColumnIds helper that reads the key with an empty default and drops blank tokens, so an unset key reports the size mismatch the method already had a message for instead of an NPE. setColumnNameList is added to the list above, and TestSchemaEvolutionContext covers the unset, blank-only and blank-plus-padded cases. The fullColNamelist.get(id) IOOBE needs an id larger than the column list rather than a blank one, so I have left that out of this PR. ########## 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: Done, the body is wrapped in synchronized (conf) and the call stays where f41539a9cb5f put it. Added a javadoc paragraph on why, since the write now fires on any blank or padded token rather than only a leading comma. ########## 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: Done, the early return is deleted. That also pins the write-back guard for free: testCleanProjectionColumnIdsWithUnsetKey now reaches the guard and fails on a mutant that always calls conf.set, so no spy is needed. The two "left alone" messages are reworded to claim only what they assert. ########## 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: Done, both assertions added verbatim: orderFields("b,c", " 2 , 0 ", ...) in TestHoodieRealtimeRecordReaderUtils and clean(" 2 , 0 ") in TestHoodieRealtimeInputFormatUtils. Both fail against the pre-change code. ########## 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: Done. testIncrementalWithReplace is now a @ParameterizedTest over a blankProjectionIds flag that prepends ",," to the read-column ids after setHiveColumnNameProps, and asserts all 100 rows still read. It fails against the pre-change code with NumberFormatException: For input string: "". ########## 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: Done, folded in as a third case on testOrderFieldsIgnoresBlankIdTokens and the separate test is gone. The surviving javadoc uses your wording: Hive emits ",,2,0,3,5", master's sanitiser strips one comma, and no production path delivers a blank here now. ########## 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: Done, applied the suggestion verbatim so the message no longer reads as a fix for the interior-blank pairing. -- 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]
