rangareddy commented on code in PR #19510: URL: https://github.com/apache/hudi/pull/19510#discussion_r3790991810
########## hudi-hadoop-mr/src/test/java/org/apache/hudi/hadoop/TestHoodieParquetInputFormatBootstrapSplitSelection.java: ########## @@ -0,0 +1,102 @@ +/* + * 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; + +import org.apache.hudi.common.util.Option; + +import org.apache.hadoop.fs.Path; +import org.apache.hadoop.mapred.FileSplit; +import org.junit.jupiter.api.Test; + +import java.io.IOException; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertSame; + +/** + * Which of a bootstrap split's two files backs the read, per projection. + * + * <p>A bootstrap split carries the skeleton file as its own path - inside the table root - and the external + * source file separately, outside it. Handing Hive a path outside the table root breaks its vectorized + * parquet reader, which derives partition values by looking the split path up in {@code pathToPartitionInfo} + * and fails with {@code cannot find dir = [...] in pathToPartitionInfo: [...]} (HUDI-5526, #15676). Hive 2 + * never vectorized this path, which is why the same query worked there. + * + * <p>This is the first coverage of that selection: nothing in the tree referenced + * {@code BootstrapBaseFileSplit} or the reader built from it. + */ +class TestHoodieParquetInputFormatBootstrapSplitSelection { + + private static final Path SKELETON = new Path("s3://bucket/hudi-table/tbl/event_type=two/skeleton.parquet"); + private static final Path EXTERNAL = new Path("s3://bucket/source-tables/tbl/event_type=two/part-0.parquet"); + + private static BootstrapBaseFileSplit split() throws IOException { + return new BootstrapBaseFileSplit( + new FileSplit(SKELETON, 0, 100, (String[]) null), + new FileSplit(EXTERNAL, 0, 100, (String[]) null)); + } + + /** + * {@code SELECT COUNT(*)} projects nothing, so both "only one file is needed" cases apply at once and the + * order they are tested in decides the answer. It has to be the skeleton: it is inside the table root, and + * bootstrap keeps a one-to-one row correspondence, so the count is the same either way. + */ + @Test + void testCountStarReadsSkeletonSoSplitPathStaysInsideTable() throws IOException { Review Comment: Added. `testNoProjectionReaderReadsSkeletonRowCount` goes through `getRecordReader` with a skeleton of 3 rows and an external file of 7, nothing projected. Without the reorder it fails `expected: <3> but was: <7>`; with it, 3. You were right that the old suite could not have caught HUDI-5526. ########## hudi-hadoop-mr/src/main/java/org/apache/hudi/hadoop/HoodieParquetInputFormat.java: ########## @@ -233,4 +234,34 @@ private RecordReader<NullWritable, ArrayWritable> createBootstrappingRecordReade true); } } + + /** + * The single file backing this read, or empty when both files are needed and have to be stitched. + * + * <p>A bootstrap split carries two paths: the split itself is the skeleton file, which lives inside the + * table root, and {@code getBootstrapFileSplit()} is the external source file, which does not. + * + * <p>The two "only one file is needed" cases both apply when a query projects no columns at all, as + * {@code SELECT COUNT(*)} does, so the order they are tested in decides which file is read. Prefer the + * skeleton: it is inside the table root, and bootstrap keeps a one-to-one row correspondence with the + * external file, so a count over it is identical. Handing Hive a path outside the table root breaks its + * vectorized reader, which derives partition values by looking the split path up in + * {@code pathToPartitionInfo} (HUDI-5526). + * + * @param split the bootstrap split. + * @param anyHoodieColProjected whether the query projects any Hudi meta column. + * @param anyExternalColProjected whether the query projects any column from the external file. + */ + @VisibleForTesting + static Option<FileSplit> resolveSingleFileSplit(BootstrapBaseFileSplit split, + boolean anyHoodieColProjected, + boolean anyExternalColProjected) { + if (!anyExternalColProjected) { + return Option.of(split); + } else if (!anyHoodieColProjected) { + return Option.of(split.getBootstrapFileSplit()); Review Comment: Agreed, and scoped. The body now says this fixes the no-projection shape only, and #19643 tracks the external-only and stitch branches with the `set hive.vectorized.execution.enabled=false` workaround and the three options for fixing them properly. ########## hudi-hadoop-mr/src/main/java/org/apache/hudi/hadoop/HoodieParquetInputFormat.java: ########## @@ -233,4 +234,34 @@ private RecordReader<NullWritable, ArrayWritable> createBootstrappingRecordReade true); } } + + /** + * The single file backing this read, or empty when both files are needed and have to be stitched. + * + * <p>A bootstrap split carries two paths: the split itself is the skeleton file, which lives inside the + * table root, and {@code getBootstrapFileSplit()} is the external source file, which does not. + * + * <p>The two "only one file is needed" cases both apply when a query projects no columns at all, as + * {@code SELECT COUNT(*)} does, so the order they are tested in decides which file is read. Prefer the + * skeleton: it is inside the table root, and bootstrap keeps a one-to-one row correspondence with the + * external file, so a count over it is identical. Handing Hive a path outside the table root breaks its + * vectorized reader, which derives partition values by looking the split path up in + * {@code pathToPartitionInfo} (HUDI-5526). + * + * @param split the bootstrap split. + * @param anyHoodieColProjected whether the query projects any Hudi meta column. + * @param anyExternalColProjected whether the query projects any column from the external file. + */ + @VisibleForTesting + static Option<FileSplit> resolveSingleFileSplit(BootstrapBaseFileSplit split, + boolean anyHoodieColProjected, + boolean anyExternalColProjected) { + if (!anyExternalColProjected) { Review Comment: Corrected. The body now says MOR is reached only for a bootstrap file slice with no log files, and names why: `addVirtualKeysProjection` injects the meta columns via the 3-arg `addProjectionField`, which does not consult `LIST_COLUMNS`, so `hoodieColsProjected` is never empty on that path. ########## hudi-hadoop-mr/src/test/java/org/apache/hudi/hadoop/TestHoodieParquetInputFormatBootstrapSplitSelection.java: ########## @@ -0,0 +1,102 @@ +/* + * 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; + +import org.apache.hudi.common.util.Option; + +import org.apache.hadoop.fs.Path; +import org.apache.hadoop.mapred.FileSplit; +import org.junit.jupiter.api.Test; + +import java.io.IOException; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertSame; + +/** + * Which of a bootstrap split's two files backs the read, per projection. + * + * <p>A bootstrap split carries the skeleton file as its own path - inside the table root - and the external + * source file separately, outside it. Handing Hive a path outside the table root breaks its vectorized + * parquet reader, which derives partition values by looking the split path up in {@code pathToPartitionInfo} + * and fails with {@code cannot find dir = [...] in pathToPartitionInfo: [...]} (HUDI-5526, #15676). Hive 2 + * never vectorized this path, which is why the same query worked there. Review Comment: Verified and fixed in both places. `javap` on `HiveConf$ConfVars` gives `iconst_0` for hive-common 2.3.10 and `iconst_1` for 3.1.3 against the same `hive.vectorized.execution.enabled` constant, so it is the config default that differs, not the reader. Javadoc and body now say that rather than "Hive 2 never vectorized". ########## hudi-hadoop-mr/src/main/java/org/apache/hudi/hadoop/HoodieParquetInputFormat.java: ########## @@ -233,4 +234,34 @@ private RecordReader<NullWritable, ArrayWritable> createBootstrappingRecordReade true); } } + + /** + * The single file backing this read, or empty when both files are needed and have to be stitched. + * + * <p>A bootstrap split carries two paths: the split itself is the skeleton file, which lives inside the + * table root, and {@code getBootstrapFileSplit()} is the external source file, which does not. + * + * <p>The two "only one file is needed" cases both apply when a query projects no columns at all, as + * {@code SELECT COUNT(*)} does, so the order they are tested in decides which file is read. Prefer the + * skeleton: it is inside the table root, and bootstrap keeps a one-to-one row correspondence with the + * external file, so a count over it is identical. Handing Hive a path outside the table root breaks its + * vectorized reader, which derives partition values by looking the split path up in + * {@code pathToPartitionInfo} (HUDI-5526). + * + * @param split the bootstrap split. + * @param anyHoodieColProjected whether the query projects any Hudi meta column. + * @param anyExternalColProjected whether the query projects any column from the external file. Review Comment: Dropped. You are right that Hive takes `INPUT__FILE__NAME` from the outer `HiveInputSplit` via `initIOContext`, so it is the skeleton path before and after. The paragraph is gone from the body. ########## hudi-hadoop-mr/src/test/java/org/apache/hudi/hadoop/TestHoodieParquetInputFormatBootstrapSplitSelection.java: ########## @@ -0,0 +1,102 @@ +/* + * 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; + +import org.apache.hudi.common.util.Option; + +import org.apache.hadoop.fs.Path; +import org.apache.hadoop.mapred.FileSplit; +import org.junit.jupiter.api.Test; + +import java.io.IOException; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertSame; + +/** + * Which of a bootstrap split's two files backs the read, per projection. + * + * <p>A bootstrap split carries the skeleton file as its own path - inside the table root - and the external + * source file separately, outside it. Handing Hive a path outside the table root breaks its vectorized + * parquet reader, which derives partition values by looking the split path up in {@code pathToPartitionInfo} + * and fails with {@code cannot find dir = [...] in pathToPartitionInfo: [...]} (HUDI-5526, #15676). Hive 2 + * never vectorized this path, which is why the same query worked there. + * + * <p>This is the first coverage of that selection: nothing in the tree referenced + * {@code BootstrapBaseFileSplit} or the reader built from it. Review Comment: Applied your wording. The javadoc now says only the no-projection case is new behaviour and that TestBootstrap/TestOrcBootstrap cover the other three end to end but are `@Disabled("HUDI-7353")` since #10551 — confirmed at `TestBootstrap.java:125` and `TestOrcBootstrap.java:110`. ########## hudi-hadoop-mr/src/test/java/org/apache/hudi/hadoop/TestHoodieParquetInputFormatBootstrapSplitSelection.java: ########## @@ -0,0 +1,102 @@ +/* + * 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; + +import org.apache.hudi.common.util.Option; + +import org.apache.hadoop.fs.Path; +import org.apache.hadoop.mapred.FileSplit; +import org.junit.jupiter.api.Test; + +import java.io.IOException; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertSame; + +/** + * Which of a bootstrap split's two files backs the read, per projection. + * + * <p>A bootstrap split carries the skeleton file as its own path - inside the table root - and the external + * source file separately, outside it. Handing Hive a path outside the table root breaks its vectorized + * parquet reader, which derives partition values by looking the split path up in {@code pathToPartitionInfo} + * and fails with {@code cannot find dir = [...] in pathToPartitionInfo: [...]} (HUDI-5526, #15676). Hive 2 + * never vectorized this path, which is why the same query worked there. + * + * <p>This is the first coverage of that selection: nothing in the tree referenced + * {@code BootstrapBaseFileSplit} or the reader built from it. + */ +class TestHoodieParquetInputFormatBootstrapSplitSelection { + + private static final Path SKELETON = new Path("s3://bucket/hudi-table/tbl/event_type=two/skeleton.parquet"); + private static final Path EXTERNAL = new Path("s3://bucket/source-tables/tbl/event_type=two/part-0.parquet"); + + private static BootstrapBaseFileSplit split() throws IOException { + return new BootstrapBaseFileSplit( + new FileSplit(SKELETON, 0, 100, (String[]) null), + new FileSplit(EXTERNAL, 0, 100, (String[]) null)); + } + + /** + * {@code SELECT COUNT(*)} projects nothing, so both "only one file is needed" cases apply at once and the + * order they are tested in decides the answer. It has to be the skeleton: it is inside the table root, and + * bootstrap keeps a one-to-one row correspondence, so the count is the same either way. + */ + @Test + void testCountStarReadsSkeletonSoSplitPathStaysInsideTable() throws IOException { + BootstrapBaseFileSplit split = split(); + + Option<FileSplit> resolved = HoodieParquetInputFormat.resolveSingleFileSplit(split, false, false); + + assertSame(split, resolved.get(), + "a query projecting no columns must read the skeleton, whose path is inside the table root"); + assertEquals(SKELETON, resolved.get().getPath()); + } + + /** Only meta columns projected: the external file is not needed. */ + @Test + void testMetaColumnsOnlyReadsTheSkeleton() throws IOException { Review Comment: Both taken. The tests are now in `TestHoodieParquetInputFormat` — the new class is deleted — with the three non-discriminating cases collapsed into a `@ParameterizedTest`/`@MethodSource` truth table and the count(*) case kept as its own named test. As you said, it is where the reader-level test belongs anyway. ########## hudi-hadoop-mr/src/test/java/org/apache/hudi/hadoop/TestHoodieParquetInputFormatBootstrapSplitSelection.java: ########## @@ -0,0 +1,102 @@ +/* + * 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; + +import org.apache.hudi.common.util.Option; + +import org.apache.hadoop.fs.Path; +import org.apache.hadoop.mapred.FileSplit; +import org.junit.jupiter.api.Test; + +import java.io.IOException; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertSame; + +/** + * Which of a bootstrap split's two files backs the read, per projection. + * + * <p>A bootstrap split carries the skeleton file as its own path - inside the table root - and the external + * source file separately, outside it. Handing Hive a path outside the table root breaks its vectorized + * parquet reader, which derives partition values by looking the split path up in {@code pathToPartitionInfo} + * and fails with {@code cannot find dir = [...] in pathToPartitionInfo: [...]} (HUDI-5526, #15676). Hive 2 + * never vectorized this path, which is why the same query worked there. + * + * <p>This is the first coverage of that selection: nothing in the tree referenced + * {@code BootstrapBaseFileSplit} or the reader built from it. + */ +class TestHoodieParquetInputFormatBootstrapSplitSelection { + + private static final Path SKELETON = new Path("s3://bucket/hudi-table/tbl/event_type=two/skeleton.parquet"); + private static final Path EXTERNAL = new Path("s3://bucket/source-tables/tbl/event_type=two/part-0.parquet"); + + private static BootstrapBaseFileSplit split() throws IOException { + return new BootstrapBaseFileSplit( + new FileSplit(SKELETON, 0, 100, (String[]) null), + new FileSplit(EXTERNAL, 0, 100, (String[]) null)); + } + + /** + * {@code SELECT COUNT(*)} projects nothing, so both "only one file is needed" cases apply at once and the + * order they are tested in decides the answer. It has to be the skeleton: it is inside the table root, and + * bootstrap keeps a one-to-one row correspondence, so the count is the same either way. + */ + @Test + void testCountStarReadsSkeletonSoSplitPathStaysInsideTable() throws IOException { + BootstrapBaseFileSplit split = split(); + + Option<FileSplit> resolved = HoodieParquetInputFormat.resolveSingleFileSplit(split, false, false); + + assertSame(split, resolved.get(), + "a query projecting no columns must read the skeleton, whose path is inside the table root"); + assertEquals(SKELETON, resolved.get().getPath()); Review Comment: Fixed: identity only, and every test asserts `isPresent()` before `get()`, so a regression reports the message rather than `NoSuchElementException`. ########## hudi-hadoop-mr/src/main/java/org/apache/hudi/hadoop/HoodieParquetInputFormat.java: ########## @@ -210,10 +211,10 @@ private RecordReader<NullWritable, ArrayWritable> createBootstrappingRecordReade LOG.info("colNameWithTypes ={}, Num Entries ={}", colNameWithTypes, colNameWithTypes.size()); - if (hoodieColsProjected.isEmpty()) { - return getRecordReaderInternal(eSplit.getBootstrapFileSplit(), job, reporter); - } else if (externalColsProjected.isEmpty()) { - return getRecordReaderInternal(split, job, reporter); + Option<FileSplit> singleSplit = resolveSingleFileSplit(eSplit, !hoodieColsProjected.isEmpty(), Review Comment: Taken, not deferred — it makes the diff smaller and deletes the hazard. Both booleans now come from the names alone. Worth recording that `getReadColumnIDs` carries a comment warning about exactly this pairing ("some code uses this list to correlate with column names ... this call will remove [duplicates] and the other won't"), and `createBootstrappingRecordReader` was that code. -- 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]
