voonhous commented on code in PR #19463: URL: https://github.com/apache/hudi/pull/19463#discussion_r3832454752
########## 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 small points. The javadoc says `",2,0,3,5"` is what HIVE-22438 produces. Hive emits `",,2,0,3,5"`; master's sanitizer strips one comma and leaves this value. And after this change no production path delivers a blank to `orderFields`, so this is defence in depth rather than the #14673 repro its name implies. It also kills no mutant that `testOrderFieldsIgnoresBlankIdTokens` does not, so consider folding it in as a third case. ```suggestion /** * The #14673 numbers: four names, five id tokens, one blank. Hive emits {@code ",,2,0,3,5"} and master's * {@code cleanProjectionColumnIds} strips one comma, leaving the value below. Since this change strips * every blank in the conf, no production path delivers a blank here now, so this is defence in depth. */ ``` ########## 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: The message frames the interior blank as the case leading-comma stripping never reached, which is true, but the transformation has no correct outcome end to end. An interior blank only appears after a Hive prepend following an empty append, i.e. the #19506 asymmetry. For names `"b,c,d"` and ids `"3,,2,0"`: master throws, this branch returns `[d, c, b]`, correct is `[c, b, d]`. That is #19506 and not a regression here, and narrowing to leading blanks would not help (`",,7,2,0"` with `"b,c,g"` mis-orders too). The ask is only that the 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 dedup tests put the duplicate last, the one position where deduped and raw pairing coincide, so they pin the count and not the pairing. Mutants that use the `LinkedHashSet` only for the size check and pair from the raw list or raw array leave the suite green. Moving the duplicate off the tail kills both. Verified on 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`: `"0,0,1"` instead of `"0,1,1"`. -- 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]
