hudi-agent commented on code in PR #19463: URL: https://github.com/apache/hudi/pull/19463#discussion_r3713845513
########## 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. + */ Review Comment: 🤖 nit: embedding the raw issue number in the method name (`Issue14673`) means the test is opaque without the Javadoc — could you rename it to something like `testOrderFieldsResolvesWhenSingleBlankIdTokenCausedCountMismatch` so the name is self-describing? <sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag quality.</i></sub> ########## hudi-hadoop-mr/src/main/java/org/apache/hudi/hadoop/utils/HoodieRealtimeRecordReaderUtils.java: ########## @@ -273,15 +273,26 @@ 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); Set<String> fieldOrdersSet = new LinkedHashSet<>(Arrays.asList(fieldOrdersWithDups)); String[] fieldOrders = fieldOrdersSet.toArray(new String[0]); 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) { + // The counts are of the de-duplicated lists, which is what was compared, so they are named as such: Review Comment: 🤖 nit: the 4-line comment here re-explains reasoning that the renamed format placeholders (`#distinctFieldNames`, `#distinctFieldPositions`) already make self-evident — could you trim it to something like `// use de-duplicated counts to match what was actually compared`? <sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag quality.</i></sub> -- 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]
