voonhous commented on code in PR #19510:
URL: https://github.com/apache/hudi/pull/19510#discussion_r3774251713
##########
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:
The PR body's closing note -- "`INPUT__FILE__NAME` now reports the skeleton
path rather than the external path" -- describes a behaviour change that does
not happen, and it is worth removing because reviewers will go looking for it.
Hive takes that value from the outer split, not from whichever inner split
Hudi opens: `HiveInputFormat.getRecordReader` calls
`HiveContextAwareRecordReader.initIOContext(hsplit, job, ifClass,
innerReader)`, which does `IOContext.setInputPath(split.getPath())` on the
`HiveInputSplit`. That path is `BootstrapBaseFileSplit.getPath()`, i.e. the
skeleton, both before and after this PR.
Please drop that paragraph from the PR body.
##########
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:
This branch still hands Hive a path outside the table root, so HUDI-5526 is
only partly fixed. Vectorization is selected by a job-level flag, not by the
split: `MapredParquetInputFormat.getRecordReader` branches on
`Utilities.getIsVectorized(job)`, and
`VectorizedParquetRecordReader.initPartitionValues` is gated only on
`rbCtx.getPartitionColumnCount() > 0`. So `SELECT <data_col> FROM
bootstrap_tbl` on a partitioned table throws the identical `cannot find dir`
after this change, and so does the stitch branch, which passes `rightSplit`
(the external split) into `getRecordReaderInternal` at line 232.
You established this yourself on the issue: "This is not actually specific
to COUNT ... a plain projection of data columns should take the same branch and
fail the same way on Hive 3."
Two things to do: scope the PR body's Impact section to the no-projection
shape instead of "Hive 3 `SELECT COUNT(*)` ... stops failing", and file a
follow-up issue for the external-only and stitch branches, so `Closes #15676`
does not bury the part that is still broken. Worth recording the `set
hive.vectorized.execution.enabled=false` workaround there.
##########
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:
Nit, feel free to ignore. Only
`testCountStarReadsSkeletonSoSplitPathStaysInsideTable` discriminates the fix;
these three pass identically before and after the reorder, and the whole input
domain is a four-row truth table. Collapsing them into one `@ParameterizedTest`
over `(anyHoodieCol, anyExternalCol, expected)` and keeping the count(*) case
as its own named test with the HUDI-5526 comment would read better. The module
already uses `@MethodSource` for this --
`TestHoodieRealtimeRecordReader.java:170`.
Same for the standalone class. The "would otherwise be re-run by
`TestGloballyConsistentTimeStampFilteringInputFormat`" justification is
mechanically true but costs nothing: that subclass already re-runs 18 IO-heavy
inherited tests in 0.823s total, so four boolean assertions are free. Given the
standing preference against new test files, folding these into
`TestHoodieParquetInputFormat` is the safer default -- and it is where the
reader-level test from my other comment would live 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:
Nit, feel free to ignore: once the `assertSame` above holds, this adds
nothing, and it leaves the file with three different assertion styles (identity
here, path-only at lines 80 and 90). Pick identity -- it is the stronger claim
for this helper -- and drop this line.
Separately, none of the four tests assert `isPresent()` before calling
`get()`, so a regression surfaces as a bare `NoSuchElementException` rather
than the messages you wrote.
##########
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:
The PR body's "Applies to both COW and MOR bootstrap tables" does not hold
for the MOR case that matters. When a `HoodieRealtimeBootstrapBaseFileSplit`
has delta log paths, `HoodieParquetRealtimeInputFormat.addProjectionToJobConf`
calls `addVirtualKeysProjection` (`HoodieParquetRealtimeInputFormat.java:113`),
which injects `_hoodie_record_key` / `_hoodie_commit_time` /
`_hoodie_partition_path` through the private 3-arg `addProjectionField`
(`HoodieRealtimeInputFormatUtils.java:101-103`) -- the overload that does not
consult `LIST_COLUMNS` and so always succeeds. `hoodieColsProjected` is
therefore never empty on that path and this branch is unreachable; if the table
also has ordering fields configured they get added too, which pushes `COUNT(*)`
into the stitch branch and the crash described above.
The change reaches MOR only for a bootstrap file slice with no log files.
Please narrow that sentence in the PR body to say exactly that.
##########
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:
`hive-exec-2.3.10` ships the identical
`VectorizedParquetRecordReader.initPartitionValues` with the same
`getPartitionValues` -> `getFromPathRecursively` chain:
```
javap -p -c -cp
~/.m2/repository/org/apache/hive/hive-exec/2.3.10/hive-exec-2.3.10.jar \
org.apache.hadoop.hive.ql.io.parquet.vector.VectorizedParquetRecordReader
| grep initPartitionValues
```
The difference is the `hive.vectorized.execution.enabled` default: `false`
in `hive-common-2.3.10`, `true` in `3.1.3` (`javap` on `HiveConf$ConfVars`
shows `iconst_0` vs `iconst_1`). That matters for users -- a Hive 2 deployment
with vectorization turned on hits this bug too, so the guard is config-driven,
not version-gated.
The same sentence is in the PR body; please fix both.
```suggestion
* and fails with {@code cannot find dir = [...] in pathToPartitionInfo:
[...]} (HUDI-5526, #15676). Hive 2.3
* ships the same reader but defaults {@code
hive.vectorized.execution.enabled} to false, where Hive 3
* defaults it to true, which is why the same query worked there.
```
##########
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:
The four tests assert a three-line pure function over two booleans, but the
bug is in how those booleans get derived and in what the reader then opens.
Neither `COUNT(*)` producing `(false, false)` nor "the resulting reader reads
the skeleton" is exercised, so this suite would not have caught HUDI-5526 and
will not catch a regression in it.
A reader-level test is cheap here and needs no Hudi table:
`shouldUseFilegroupReader` ends with `&& !(split instanceof
BootstrapBaseFileSplit)` (`HoodieInputFormatUtils.java:571`), so
`inputFormat.getRecordReader(bootstrapSplit, jobConf, null)` falls straight
through to `createBootstrappingRecordReader` -- no metaclient, no bootstrap
index, no listing. `TestHoodieParquetInputFormat.java:779`
(`testHoodieParquetInputFormatReadTimeType`) already has the whole pattern:
`AvroParquetWriter`, `IOConstants.COLUMNS`, `READ_COLUMN_NAMES_CONF_STR`,
`getRecordReader`.
Please add one test that goes through `getRecordReader`: write a skeleton
file with N rows and an external file with M != N rows, wrap them in a
`BootstrapBaseFileSplit`, set `READ_COLUMN_NAMES_CONF_STR` to empty, and assert
the reader yields N. That fails on master (yields M) and passes here, which is
the assertion this PR actually needs.
##########
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:
Accurate about the class name, misleading about coverage.
`TestBootstrap.checkBootstrapResults` drives three of these four branches end
to end on a METADATA_ONLY bootstrap table: full projection reaches the stitch
branch, "RO Input Format Read - Project only Hoodie Columns" is at
`TestBootstrap.java:613`, "Project only non-hoodie column" at `:633`.
`TestOrcBootstrap` does the same. They simply never run -- both are
`@Disabled("HUDI-7353")` (`TestBootstrap.java:125`), disabled by `565e7c566ed9`
(#10551, 2024-01-29).
Worth stating accurately because it changes what is new here: only the
`(false, false)` case is genuinely uncovered behaviour.
```suggestion
* <p>Only the no-projection case is new behaviour: nothing in the tree
referenced
* {@code BootstrapBaseFileSplit}, but TestBootstrap and TestOrcBootstrap
cover the other three branches
* end to end -- they have just been disabled (HUDI-7353) since #10551.
```
##########
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:
Optional, and arguably a separate PR, but you are refactoring exactly this
decision so it is worth flagging: the `Integer` half of
`projectedColsWithIndex` (lines 194-205) is never read -- both lists are
consumed only via `isEmpty()`, here and nowhere else.
The zip that builds it is a live crash source. It pairs `rawColIds`, which
`getReadColumnIDs` de-duplicates (`HoodieColumnProjectionUtils.java:88-97`),
with `rawColNames`, which `getReadColumnNames` does not, and parses each id
with `Integer.parseInt`. Both hazards reproduce today: ids `",2,5"` -- the
HIVE-22438 shape that `cleanProjectionColumnIds` scrubs on the realtime path
but never on this one -- throws `NumberFormatException`, and a duplicated name
makes `rawColNames[idx]` either misalign or throw
`ArrayIndexOutOfBoundsException`. Hive itself tolerates mismatched lists; it
only logs `Read column counts do not match`.
Deriving the two booleans from names alone deletes lines 193-205 and both
hazards, with no behaviour change:
```java
boolean anyHoodieCol =
Arrays.stream(rawColNames).anyMatch(HoodieRecord.HOODIE_META_COLUMNS::contains);
boolean anyExternalCol = Arrays.stream(rawColNames).anyMatch(n ->
!HoodieRecord.HOODIE_META_COLUMNS.contains(n) &&
!HoodieHiveUtils.VIRTUAL_COLUMN_NAMES.contains(n));
Option<FileSplit> singleSplit = resolveSingleFileSplit(eSplit, anyHoodieCol,
anyExternalCol);
```
Both imports are already present. If you would rather keep this PR minimal,
#19506 is the natural home for it.
--
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]
