voonhous commented on code in PR #19463:
URL: https://github.com/apache/hudi/pull/19463#discussion_r3889365536
##########
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:
Addressed in f1eada0d0b74. `TestHiveTableSchemaEvolution:163` now sets the
ids to `",,6,7"`, so the cow arm drives `cleanProjectionColumnIds` ahead of
`SchemaEvolutionContext` on the shape HIVE-22438 produces, and
`setColumnNameList` gets its first coverage.
I could not run that suite locally -- it fails here on a pre-existing
`NoClassDefFoundError` for `VariantShreddingSchemaInferrer$VariantSample`,
identically on the unmodified file -- so this arm is on CI.
The bootstrap arm is still uncovered, since no test in the repo constructs a
`BootstrapBaseFileSplit`. Left alone as out of scope here.
##########
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:
Applied in f1eada0d0b74, and renamed to
`testSetColumnTypeListWithBlankReadColumnId` since `",0"` is one blank plus one
real id rather than blanks only.
Two related nits went in with it: `TWO_FIELDS` now ends in a record with a
renamed nested field, so `setColumnTypeList` has something to write back and
the `assertEquals` on the padded-ids test checks the id-to-field pairing
instead of restating what `setUp` wrote. All three cases in the class now fail
against the pre-change code; previously two did.
--
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]
