voonhous commented on code in PR #19463:
URL: https://github.com/apache/hudi/pull/19463#discussion_r3889078587
##########
hudi-hadoop-mr/src/main/java/org/apache/hudi/hadoop/HoodieParquetInputFormat.java:
##########
@@ -119,6 +119,10 @@ private static boolean checkIfHudiTable(final InputSplit
split, final JobConf jo
public RecordReader<NullWritable, ArrayWritable> getRecordReader(final
InputSplit split, final JobConf job,
final
Reporter reporter) throws IOException {
HoodieRealtimeInputFormatUtils.addProjectionField(job,
job.get(hive_metastoreConstants.META_TABLE_PARTITION_COLUMNS, "").split("/"));
+ // The bootstrap and schema-evolution paths below parse the read-column
ids with Integer#parseInt, so the
+ // blank ids HIVE-22438 leaves in the conf have to be dropped here too;
neither path goes through a
+ // realtime input format. The call is idempotent, so the realtime formats'
own call becomes a no-op.
+ HoodieRealtimeInputFormatUtils.cleanProjectionColumnIds(job);
Review Comment:
**major:** Nothing discriminates this line. No test in the repo constructs a
`BootstrapBaseFileSplit` (`grep -rn BootstrapBaseFileSplit
hudi-hadoop-mr/src/test/` is empty), and `TestSchemaEvolutionContext` sets
`hudi.hive.schema.evolution=false`, so `doEvolutionForParquetFormat` never runs
and `setColumnNameList` is reached by zero tests.
`TestHiveTableSchemaEvolution:163` already drives exactly this path (cow
arm, `new HoodieParquetInputFormat()`, `hoodie.schema.on.read.enable=true`, ids
`"6,7"`). Could we prefix `,,` to the ids there, so this call and both changed
`SchemaEvolutionContext` methods are covered in one line?
##########
hudi-hadoop-mr/src/test/java/org/apache/hudi/hadoop/TestSchemaEvolutionContext.java:
##########
@@ -0,0 +1,89 @@
+/*
+ * 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.schema.internal.Types;
+import org.apache.hudi.exception.HoodieException;
+
+import org.apache.hadoop.fs.Path;
+import org.apache.hadoop.hive.serde.serdeConstants;
+import org.apache.hadoop.hive.serde2.ColumnProjectionUtils;
+import org.apache.hadoop.mapred.FileSplit;
+import org.apache.hadoop.mapred.JobConf;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+
+import java.io.IOException;
+import java.util.Arrays;
+import java.util.List;
+
+import static org.junit.jupiter.api.Assertions.assertDoesNotThrow;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+/**
+ * Covers how {@link SchemaEvolutionContext#setColumnTypeList} reads {@code
hive.io.file.readcolumn.ids}.
+ * Every id there is parsed with {@code Integer#parseInt}, so the blank
entries HIVE-22438 leaves behind and
+ * the unset key both used to surface as a bare {@code NumberFormatException}
or an NPE rather than as the
+ * size mismatch the method already reports.
+ */
+public class TestSchemaEvolutionContext {
+
+ private static final List<Types.Field> TWO_FIELDS = Arrays.asList(
+ Types.Field.get(0, "col1", Types.StringType.get()),
+ Types.Field.get(1, "col2", Types.StringType.get()));
+
+ private JobConf job;
+ private SchemaEvolutionContext context;
+
+ @BeforeEach
+ public void setUp() throws IOException {
+ job = new JobConf();
+ // Keeps the constructor off the table: it is the projection-id parsing
below that is under test.
+ job.setBoolean("hudi.hive.schema.evolution", false);
+ job.set(serdeConstants.LIST_COLUMN_TYPES, "string,string");
+ context = new SchemaEvolutionContext(new FileSplit(new
Path("file:///tmp/unused"), 0, 0, (String[]) null), job);
+ }
+
+ @Test
+ public void testSetColumnTypeListWithUnsetReadColumnIds() {
+ job.unset(ColumnProjectionUtils.READ_COLUMN_IDS_CONF_STR);
+ HoodieException thrown = assertThrows(HoodieException.class, () ->
context.setColumnTypeList(job, TWO_FIELDS));
+ assertTrue(thrown.getMessage().contains("is not equal to projection
columns"),
+ () -> "Expected the size mismatch rather than an NPE, got: " +
thrown.getMessage());
+ }
+
+ @Test
+ public void testSetColumnTypeListWithOnlyBlankReadColumnIds() {
+ job.set(ColumnProjectionUtils.READ_COLUMN_IDS_CONF_STR, ",,");
Review Comment:
**minor:** Not blocking. This case passes unchanged on master, so it pins
nothing: `",,".split(",")` returns a zero-length array in Java, so the old code
also reached the size check and threw the same `HoodieException`. Reverting
`src/main` to the merge base leaves this test green while the other two go red.
A leading blank only reaches `parseInt` when the raw token count matches the
field count, which needs one real id:
```suggestion
job.set(ColumnProjectionUtils.READ_COLUMN_IDS_CONF_STR, ",0");
```
With that the pre-change code fails with `NumberFormatException: For input
string: ""`, which is what the message on line 79 already claims. Could we
switch to it?
##########
hudi-hadoop-mr/src/main/java/org/apache/hudi/hadoop/HoodieParquetInputFormat.java:
##########
@@ -119,6 +119,10 @@ private static boolean checkIfHudiTable(final InputSplit
split, final JobConf jo
public RecordReader<NullWritable, ArrayWritable> getRecordReader(final
InputSplit split, final JobConf job,
final
Reporter reporter) throws IOException {
HoodieRealtimeInputFormatUtils.addProjectionField(job,
job.get(hive_metastoreConstants.META_TABLE_PARTITION_COLUMNS, "").split("/"));
Review Comment:
**minor:** Not blocking. This is an unsynchronized get-then-set of the same
two conf keys that the clean on line 125 now takes `synchronized (job)` for. On
the legacy realtime path those writes run under `synchronized (jobConf)` in
`addProjectionToJobConf`, and then re-enter this line unlocked via
`super.getRecordReader`.
Pre-existing rather than introduced here, but the new javadoc claims parity
with `addProjectionToJobConf`. Would it be worth wrapping lines 121-125 in a
single `synchronized (job)` so the write that produces the ids is guarded the
same way?
--
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]
