This is an automated email from the ASF dual-hosted git repository.
voonhous pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/hudi.git
The following commit(s) were added to refs/heads/master by this push:
new 55c7a308094c fix(timeline): do not NPE on archived instants without a
completion time (#19452)
55c7a308094c is described below
commit 55c7a308094c2330189584d9df4cf03f85edd56a
Author: Ranga Reddy <[email protected]>
AuthorDate: Mon Aug 3 18:45:29 2026 +0530
fix(timeline): do not NPE on archived instants without a completion time
(#19452)
* fix(timeline): do not NPE on archived instants without a completion time
Upgrading a table written by 0.x fails while polling the archived timeline:
java.lang.NullPointerException: Cannot invoke "Object.toString()" because
the
return value of "org.apache.avro.generic.GenericRecord.get(String)" is
null
at CompletionTimeQueryViewV2.readCompletionTime
completionTime is declared ["null","string"] with a null default in
HoodieLSMTimelineInstant, and instants archived before that field existed
carry
no value for it. setCompletionTime already handles the null case by falling
back
to the instant time, with a comment saying so, but readCompletionTime called
toString() on the raw field before reaching it.
Read the field as an Object and let the existing fallback apply. Adds unit
tests
for both the missing and present cases; readCompletionTime is widened to
package-private with @VisibleForTesting, matching the annotation already
used in
this class.
The same unguarded toString() on this field also appears in
ArchivedTimelineV2#readCommit and MetadataConversionUtils, where the right
behaviour for a null value is less obvious. Left alone here and called out
in the
PR instead.
Closes #17095
* fix(timeline): use StringUtils.objToString and tidy the regression test
Review feedback.
- readCompletionTime now uses StringUtils.objToString, the existing
null-safe
toString that HoodieAvroUtils.getNullableValAsString is built on, instead
of a
local variable and a ternary.
- Test: added the missing class javadoc, renamed to match the convention in
this
area (testReadCompletionTime /
testReadCompletionTimeWithoutCompletionTime),
dropped the instantTime and action fields that readCompletionTime never
reads
and which implied a coupling that is not there, and made the assertion
messages consistent across both cases.
On moving the test onto the real archiving harness in hudi-client-common:
tried
it, and it does not reproduce this bug. Details in the review thread.
* test(timeline): cover the null completion time on the real archived read
path
Adds testReadCompletionTimeWithoutCompletionTime to
TestCompletionTimeQueryView.
It archives an instant carrying no completion time through
LSMTimelineWriter and
reads it back through the archived timeline, so the fallback in
readCompletionTime
is exercised on the path that actually broke. Reverting the fix makes it
fail with
the HUDI-9655 NPE.
The test asserts LSMTimelineWriter's exception handler collected nothing.
That
handler is optional and the write loop swallows per-instant failures, so
without
the assertion a failed archive write would leave the test passing against an
empty archive.
With real-path coverage the mocked TestCompletionTimeQueryViewV2 is
redundant, so
it goes, and readCompletionTime returns to private.
* fix(timeline): null-safe the other two reads of the archived
completionTime
Review question: the same raw
record.get(COMPLETION_TIME_ARCHIVED_META_FIELD)
.toString() also lives in ArchivedTimelineV2#readCommit and
MetadataConversionUtils#createMetaWrapper. Checked, and both do NPE on the
same
record shape - createMetaWrapper demonstrably, at line 174, on a record
with the
field left unset. Both read the same LSM records as the query view, so the
trigger
is identical: a table archived before completionTime existed.
Add ArchivedTimelineV2#completionTimeOrInstantTime so the two sites cannot
drift,
and route both through it. Falling back to the instant time is the behaviour
CompletionTimeQueryViewV2#setCompletionTime already documents for these
records, so
this follows existing precedent rather than inventing a rule. Both sites
build a
COMPLETED HoodieInstant, and leaving the completion time null there would
only move
the failure to whatever compares it.
CompletionTimeQueryViewV2#readCompletionTime is left as is: it hands a
possibly-null
value to setCompletionTime, which owns the fallback, so it needs nothing
further.
Note the two sites already null-check the neighbouring nullable fields,
metadata and
plan, so completionTime was the odd one out rather than a deliberate choice.
---------
Co-authored-by: voon <[email protected]>
---
.../timeline/TestCompletionTimeQueryView.java | 53 ++++++++++
.../table/timeline/MetadataConversionUtils.java | 2 +-
.../timeline/versioning/v2/ArchivedTimelineV2.java | 21 +++-
.../versioning/v2/CompletionTimeQueryViewV2.java | 6 +-
.../TestArchivedInstantCompletionTime.java | 116 +++++++++++++++++++++
5 files changed, 194 insertions(+), 4 deletions(-)
diff --git
a/hudi-client/hudi-client-common/src/test/java/org/apache/hudi/client/timeline/TestCompletionTimeQueryView.java
b/hudi-client/hudi-client-common/src/test/java/org/apache/hudi/client/timeline/TestCompletionTimeQueryView.java
index f007854a0d4e..93656debb7b3 100644
---
a/hudi-client/hudi-client-common/src/test/java/org/apache/hudi/client/timeline/TestCompletionTimeQueryView.java
+++
b/hudi-client/hudi-client-common/src/test/java/org/apache/hudi/client/timeline/TestCompletionTimeQueryView.java
@@ -126,6 +126,59 @@ public class TestCompletionTimeQueryView {
}
}
+ /**
+ * The {@code completionTime} field of {@code HoodieLSMTimelineInstant} is
declared
+ * {@code ["null","string"]} with a null default, and instants archived
before the field existed carry
+ * no value for it. Reading such an instant must fall back to the instant
time rather than throwing.
+ *
+ * <p>See HUDI-9655: upgrading a table written by 0.x produced
+ * {@code NullPointerException: Cannot invoke "Object.toString()" because
the return value of
+ * "org.apache.avro.generic.GenericRecord.get(String)" is null} while
loading the archived timeline.
+ */
+ @Test
+ void testReadCompletionTimeWithoutCompletionTime() throws Exception {
+ String tableName = "testTable";
+ String tablePath = tempFile.getAbsolutePath() + StoragePath.SEPARATOR +
tableName;
+ HoodieTableMetaClient metaClient = HoodieTestUtils.init(
+ HoodieTestUtils.getDefaultStorageConf(), tablePath,
HoodieTableType.COPY_ON_WRITE, tableName);
+ HoodieWriteConfig writeConfig =
HoodieWriteConfig.newBuilder().withPath(tablePath)
+
.withIndexConfig(HoodieIndexConfig.newBuilder().withIndexType(HoodieIndex.IndexType.INMEMORY).build())
+ .withMarkersType("DIRECT")
+ .build();
+ HoodieTestTable testTable = HoodieTestTable.of(metaClient);
+
+ // instant 1 only ever exists on the LSM timeline, as an instant archived
by an older writer would.
+ String archivedInstantTime = String.format("%08d", 1);
+ HoodieCommitMetadata archivedMetadata = testTable.createCommitMetadata(
+ archivedInstantTime, WriteOperationType.INSERT, Arrays.asList("par1",
"par2"), 10, false);
+ // instants 2..4 stay active, so that the query for instant 1 falls
through to the archive.
+ for (int i = 2; i < 5; i++) {
+ String instantTime = String.format("%08d", i);
+ HoodieCommitMetadata metadata = testTable.createCommitMetadata(
+ instantTime, WriteOperationType.INSERT, Arrays.asList("par1",
"par2"), 10, false);
+ testTable.addCommit(instantTime, Option.of(String.format("%08d", i +
1000)), Option.of(metadata));
+ }
+
+ // archive instant 1 with no completion time at all
+ ActiveAction activeAction = new DummyActiveAction(
+ INSTANT_GENERATOR.createNewInstant(HoodieInstant.State.COMPLETED,
"commit", archivedInstantTime, null),
+ convertMetadataToByteArray(archivedMetadata));
+ List<Exception> archiveFailures = new ArrayList<>();
+ // LSMTimelineWriter#write swallows per-instant failures, so surface them
rather than
+ // silently archiving nothing and leaving the assertion below to pass
vacuously.
+ LSMTimelineWriter.getInstance(writeConfig, getMockHoodieTable(metaClient))
+ .write(Collections.singletonList(activeAction), Option.empty(),
Option.of(archiveFailures::add));
+ assertTrue(archiveFailures.isEmpty(),
+ "Archiving an instant without a completion time should not fail: " +
archiveFailures);
+
+ metaClient.reloadActiveTimeline();
+ try (CompletionTimeQueryView view =
+
metaClient.getTableFormat().getTimelineFactory().createCompletionTimeQueryView(metaClient))
{
+ assertThat("An archived instant without a completion time should fall
back to its instant time",
+ view.getCompletionTime(archivedInstantTime).orElse(""),
is(archivedInstantTime));
+ }
+ }
+
@Test
void testReadStartTime() throws Exception {
String tableName = "testTable";
diff --git
a/hudi-common/src/main/java/org/apache/hudi/common/table/timeline/MetadataConversionUtils.java
b/hudi-common/src/main/java/org/apache/hudi/common/table/timeline/MetadataConversionUtils.java
index 8ea9ccc03fb9..b7f4019dfacd 100644
---
a/hudi-common/src/main/java/org/apache/hudi/common/table/timeline/MetadataConversionUtils.java
+++
b/hudi-common/src/main/java/org/apache/hudi/common/table/timeline/MetadataConversionUtils.java
@@ -171,7 +171,7 @@ public class MetadataConversionUtils {
Option<byte[]> planBytes = planBuffer != null ?
Option.of(planBuffer.array()) : Option.empty();
String instantTime =
lsmTimelineRecord.get(ArchivedTimelineV2.INSTANT_TIME_ARCHIVED_META_FIELD).toString();
- String completionTime =
lsmTimelineRecord.get(ArchivedTimelineV2.COMPLETION_TIME_ARCHIVED_META_FIELD).toString();
+ String completionTime =
ArchivedTimelineV2.completionTimeOrInstantTime(lsmTimelineRecord, instantTime);
HoodieArchivedMetaEntry archivedMetaWrapper = new
HoodieArchivedMetaEntry();
archivedMetaWrapper.setCommitTime(instantTime);
diff --git
a/hudi-common/src/main/java/org/apache/hudi/common/table/timeline/versioning/v2/ArchivedTimelineV2.java
b/hudi-common/src/main/java/org/apache/hudi/common/table/timeline/versioning/v2/ArchivedTimelineV2.java
index f88168ea985c..2f8cdfe7d4e9 100644
---
a/hudi-common/src/main/java/org/apache/hudi/common/table/timeline/versioning/v2/ArchivedTimelineV2.java
+++
b/hudi-common/src/main/java/org/apache/hudi/common/table/timeline/versioning/v2/ArchivedTimelineV2.java
@@ -27,6 +27,7 @@ import org.apache.hudi.common.table.timeline.HoodieTimeline;
import org.apache.hudi.common.table.timeline.InstantComparison;
import org.apache.hudi.common.util.CollectionUtils;
import org.apache.hudi.common.util.Option;
+import org.apache.hudi.common.util.StringUtils;
import org.apache.avro.generic.GenericRecord;
import org.slf4j.Logger;
@@ -217,9 +218,27 @@ public class ArchivedTimelineV2 extends BaseTimelineV2
implements HoodieArchived
}
}
+ /**
+ * The completion time of an archived instant, falling back to its instant
time when the record carries
+ * none.
+ *
+ * <p>{@code completionTime} is declared {@code ["null","string"]} with a
null default in
+ * {@code HoodieLSMTimelineInstant} and has no value for instants archived
before the field existed, so it
+ * must not be dereferenced. Defaulting to the instant time is the fallback
+ * {@code CompletionTimeQueryViewV2#setCompletionTime} already documents for
the same records.
+ *
+ * @param record an LSM timeline record.
+ * @param instantTime the instant time to fall back to.
+ * @return the completion time, never null as long as {@code instantTime} is
not.
+ */
+ public static String completionTimeOrInstantTime(GenericRecord record,
String instantTime) {
+ String completionTime =
StringUtils.objToString(record.get(COMPLETION_TIME_ARCHIVED_META_FIELD));
+ return completionTime != null ? completionTime : instantTime;
+ }
+
private HoodieInstant readCommit(String instantTime, GenericRecord record,
Option<BiConsumer<String, GenericRecord>> instantDetailsConsumer) {
final String action = record.get(ACTION_ARCHIVED_META_FIELD).toString();
- final String completionTime =
record.get(COMPLETION_TIME_ARCHIVED_META_FIELD).toString();
+ final String completionTime = completionTimeOrInstantTime(record,
instantTime);
instantDetailsConsumer.ifPresent(consumer -> consumer.accept(instantTime,
record));
return instantGenerator.createNewInstant(HoodieInstant.State.COMPLETED,
action, instantTime, completionTime);
}
diff --git
a/hudi-common/src/main/java/org/apache/hudi/common/table/timeline/versioning/v2/CompletionTimeQueryViewV2.java
b/hudi-common/src/main/java/org/apache/hudi/common/table/timeline/versioning/v2/CompletionTimeQueryViewV2.java
index 516b4ad4cedd..be8dbd0462b1 100644
---
a/hudi-common/src/main/java/org/apache/hudi/common/table/timeline/versioning/v2/CompletionTimeQueryViewV2.java
+++
b/hudi-common/src/main/java/org/apache/hudi/common/table/timeline/versioning/v2/CompletionTimeQueryViewV2.java
@@ -27,6 +27,7 @@ import
org.apache.hudi.common.table.timeline.HoodieInstantTimeGenerator;
import org.apache.hudi.common.table.timeline.HoodieTimeline;
import org.apache.hudi.common.table.timeline.InstantComparison;
import org.apache.hudi.common.util.Option;
+import org.apache.hudi.common.util.StringUtils;
import org.apache.hudi.common.util.VisibleForTesting;
import lombok.Getter;
@@ -303,8 +304,9 @@ public class CompletionTimeQueryViewV2 implements
CompletionTimeQueryView, Seria
}
private void readCompletionTime(String instantTime, GenericRecord record) {
- final String completionTime =
record.get(COMPLETION_TIME_ARCHIVED_META_FIELD).toString();
- setCompletionTime(instantTime, completionTime);
+ // The field is nullable in HoodieLSMTimelineInstant and is absent for
instants archived before it
+ // existed, so leave the fallback to setCompletionTime rather than
dereferencing here.
+ setCompletionTime(instantTime,
StringUtils.objToString(record.get(COMPLETION_TIME_ARCHIVED_META_FIELD)));
}
private void setCompletionTime(String beginInstantTime, String
completionTime) {
diff --git
a/hudi-common/src/test/java/org/apache/hudi/common/table/timeline/TestArchivedInstantCompletionTime.java
b/hudi-common/src/test/java/org/apache/hudi/common/table/timeline/TestArchivedInstantCompletionTime.java
new file mode 100644
index 000000000000..cb6356aa807f
--- /dev/null
+++
b/hudi-common/src/test/java/org/apache/hudi/common/table/timeline/TestArchivedInstantCompletionTime.java
@@ -0,0 +1,116 @@
+/*
+ * 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.common.table.timeline;
+
+import org.apache.hudi.avro.model.HoodieArchivedMetaEntry;
+import org.apache.hudi.avro.model.HoodieLSMTimelineInstant;
+import org.apache.hudi.common.model.HoodieCommitMetadata;
+import org.apache.hudi.common.table.HoodieTableConfig;
+import org.apache.hudi.common.table.HoodieTableMetaClient;
+import org.apache.hudi.common.table.HoodieTableVersion;
+import org.apache.hudi.common.table.timeline.versioning.v2.ArchivedTimelineV2;
+import org.apache.hudi.common.table.timeline.versioning.v2.InstantGeneratorV2;
+
+import org.apache.avro.generic.GenericData;
+import org.apache.avro.generic.GenericRecord;
+import org.junit.jupiter.api.Test;
+
+import java.io.IOException;
+import java.io.InputStream;
+import java.nio.ByteBuffer;
+import java.util.function.BooleanSupplier;
+
+import static
org.apache.hudi.common.table.timeline.versioning.v2.ArchivedTimelineV2.ACTION_ARCHIVED_META_FIELD;
+import static
org.apache.hudi.common.table.timeline.versioning.v2.ArchivedTimelineV2.COMPLETION_TIME_ARCHIVED_META_FIELD;
+import static
org.apache.hudi.common.table.timeline.versioning.v2.ArchivedTimelineV2.INSTANT_TIME_ARCHIVED_META_FIELD;
+import static
org.apache.hudi.common.table.timeline.versioning.v2.ArchivedTimelineV2.METADATA_ARCHIVED_META_FIELD;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.ArgumentMatchers.eq;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+
+/**
+ * The {@code completionTime} field of {@code HoodieLSMTimelineInstant} is
declared
+ * {@code ["null","string"]} with a null default, and carries no value for
instants archived before the
+ * field existed. Every read of it therefore has to be null-safe;
dereferencing it produced
+ * {@code NullPointerException: Cannot invoke "Object.toString()" because the
return value of
+ * "GenericRecord.get(String)" is null} on a table upgraded from 0.x
(HUDI-9655).
+ *
+ * <p>This covers the two readers that build a completed {@link HoodieInstant}
from such a record, both of
+ * which fall back to the instant time.
+ */
+class TestArchivedInstantCompletionTime {
+
+ private static final String INSTANT_TIME = "00000001";
+
+ @Test
+ void completionTimeFallsBackToTheInstantTimeWhenAbsent() {
+ GenericRecord record = new
GenericData.Record(HoodieLSMTimelineInstant.getClassSchema());
+ // completionTime deliberately left unset, as it is for an instant
archived before the field existed
+
+ assertEquals(INSTANT_TIME,
ArchivedTimelineV2.completionTimeOrInstantTime(record, INSTANT_TIME),
+ "An archived instant without a completion time should fall back to its
instant time");
+ }
+
+ @Test
+ void completionTimeIsUsedWhenPresent() {
+ GenericRecord record = new
GenericData.Record(HoodieLSMTimelineInstant.getClassSchema());
+ record.put(COMPLETION_TIME_ARCHIVED_META_FIELD, "00001001");
+
+ assertEquals("00001001",
ArchivedTimelineV2.completionTimeOrInstantTime(record, INSTANT_TIME),
+ "A present completion time should be used as-is");
+ }
+
+ /**
+ * The same field read on the way to a {@code HoodieArchivedMetaEntry},
which is the path a CLI or
+ * metadata-conversion caller takes rather than the query view.
+ */
+ @Test
+ void createMetaWrapperFallsBackToTheInstantTimeWhenCompletionTimeIsAbsent()
throws IOException {
+ GenericRecord record = new
GenericData.Record(HoodieLSMTimelineInstant.getClassSchema());
+ record.put(INSTANT_TIME_ARCHIVED_META_FIELD, INSTANT_TIME);
+ record.put(ACTION_ARCHIVED_META_FIELD, HoodieTimeline.COMMIT_ACTION);
+ record.put(METADATA_ARCHIVED_META_FIELD, ByteBuffer.wrap(new byte[0]));
+ // completionTime deliberately left unset
+
+ HoodieArchivedMetaEntry entry =
+
MetadataConversionUtils.createMetaWrapper(mockMetaClientReturningEmptyCommitMetadata(),
record);
+
+ assertEquals(INSTANT_TIME, entry.getStateTransitionTime(),
+ "The archived entry should carry the instant time when the record has
no completion time");
+ assertEquals(INSTANT_TIME, entry.getCommitTime());
+ }
+
+ private static HoodieTableMetaClient
mockMetaClientReturningEmptyCommitMetadata() throws IOException {
+ HoodieTableMetaClient metaClient = mock(HoodieTableMetaClient.class);
+ HoodieTableConfig tableConfig = mock(HoodieTableConfig.class);
+ when(metaClient.getTableConfig()).thenReturn(tableConfig);
+ when(tableConfig.getTableVersion()).thenReturn(HoodieTableVersion.EIGHT);
+ when(metaClient.getInstantGenerator()).thenReturn(new
InstantGeneratorV2());
+
+ CommitMetadataSerDe serDe = mock(CommitMetadataSerDe.class);
+ when(serDe.<HoodieCommitMetadata>deserialize(any(HoodieInstant.class),
any(InputStream.class),
+ any(BooleanSupplier.class), eq(HoodieCommitMetadata.class)))
+ .thenReturn(new HoodieCommitMetadata());
+ when(metaClient.getCommitMetadataSerDe()).thenReturn(serDe);
+ return metaClient;
+ }
+}
