You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@hudi.apache.org by "rangareddy (via GitHub)" <gi...@apache.org> on 2026/07/31 16:21:07 UTC
[PR] fix(timeline): do not NPE on archived instants without a completion time [hudi]
rangareddy opened a new pull request, #19452:
URL: https://github.com/apache/hudi/pull/19452
### Describe the issue this Pull Request addresses
Closes #17095 (HUDI-9655).
Migrating a table written by 0.x to 1.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 org.apache.hudi.common.table.timeline.versioning.v2.CompletionTimeQueryViewV2.readCompletionTime
```
`completionTime` is declared `["null","string"]` with a null default in `HoodieLSMTimelineInstant.avsc`, so a
null is legal, and instants archived before that field existed carry no value for it.
`setCompletionTime` already handles exactly this, falling back to the instant time:
```java
private void setCompletionTime(String beginInstantTime, String completionTime) {
if (completionTime == null) {
// the meta-server instant does not have completion time
completionTime = beginInstantTime;
}
```
but `readCompletionTime` called `.toString()` on the raw field before reaching it.
### Summary and Changelog
- `CompletionTimeQueryViewV2#readCompletionTime` reads the field as an `Object` and passes null through, so
the existing documented fallback applies instead of throwing. Widened to package-private with
`@VisibleForTesting`, the annotation this class already uses for `getInstantTimes`.
- New `TestCompletionTimeQueryViewV2` covering both cases: an archived record with no completion time falls
back to the instant time, and one with a completion time keeps it. The first reproduces the reported NPE
before the change.
The same unguarded `.toString()` on this field appears in two more places, left alone here because the
correct behaviour for a null value is less obvious than it is in this method and deserves its own decision:
- `ArchivedTimelineV2#readCommit`, which would pass the null into
`instantGenerator.createNewInstant(COMPLETED, action, instantTime, completionTime)`, producing a completed
instant with no completion time. Worth noting #16876 reports exactly that symptom for metaserver instants.
- `MetadataConversionUtils`, which would pass it to `HoodieArchivedMetaEntry#setStateTransitionTime`.
Happy to follow up on either once the intended semantics are settled.
### Impact
Fixes a hard failure on the 0.x to 1.x upgrade path when the archived timeline contains instants without a
completion time. Behaviour when the field is present is unchanged; when it is absent the instant time is used,
which is what `setCompletionTime` already specified. No API, config or format change.
### Risk Level
low — one null guard that defers to a fallback already present in the adjacent method, covered by new unit
tests, with the surrounding timeline tests in `hudi-common` re-run.
### Documentation Update
none — no new config and no user-facing behaviour change beyond not throwing.
### Contributor's checklist
- [x] Read through [contributor's guide](https://hudi.apache.org/contribute/how-to-contribute)
- [x] Enough context is provided in the sections above
- [x] Adequate tests were added if applicable
- [x] CI passes on my PR
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(timeline): do not NPE on archived instants without a completion time [hudi]
Posted by "hudi-bot (via GitHub)" <gi...@apache.org>.
hudi-bot commented on PR #19452:
URL: https://github.com/apache/hudi/pull/19452#issuecomment-5164489934
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "e277fb301f3dc51e203fbe5e11efcce80526f552",
"status" : "DELETED",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=15929",
"triggerID" : "e277fb301f3dc51e203fbe5e11efcce80526f552",
"triggerType" : "PUSH"
}, {
"hash" : "e4777ed33e92b52fcdb90732636e1e90f4f94033",
"status" : "SUCCESS",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=15956",
"triggerID" : "e4777ed33e92b52fcdb90732636e1e90f4f94033",
"triggerType" : "PUSH"
}, {
"hash" : "93cf5b1cbddd1526076cb23f28fa8b10e228c206",
"status" : "PENDING",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=15970",
"triggerID" : "93cf5b1cbddd1526076cb23f28fa8b10e228c206",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* e4777ed33e92b52fcdb90732636e1e90f4f94033 Azure: [SUCCESS](https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=15956)
* 93cf5b1cbddd1526076cb23f28fa8b10e228c206 Azure: [PENDING](https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=15970)
<details>
<summary>Bot commands</summary>
@hudi-bot supports the following commands:
- `@hudi-bot run azure` re-run the last Azure build
</details>
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(timeline): do not NPE on archived instants without a completion time [hudi]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #19452:
URL: https://github.com/apache/hudi/pull/19452#issuecomment-5145229712
## [Codecov](https://app.codecov.io/gh/apache/hudi/pull/19452?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `50.00000%` with `1 line` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 75.85%. Comparing base ([`466929b`](https://app.codecov.io/gh/apache/hudi/commit/466929b150d4cd62d966d22663da2b68344cd892?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`e277fb3`](https://app.codecov.io/gh/apache/hudi/commit/e277fb301f3dc51e203fbe5e11efcce80526f552?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 1 commits behind head on master.
| [Files with missing lines](https://app.codecov.io/gh/apache/hudi/pull/19452?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...eline/versioning/v2/CompletionTimeQueryViewV2.java](https://app.codecov.io/gh/apache/hudi/pull/19452?src=pr&el=tree&filepath=hudi-common%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fhudi%2Fcommon%2Ftable%2Ftimeline%2Fversioning%2Fv2%2FCompletionTimeQueryViewV2.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-aHVkaS1jb21tb24vc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2h1ZGkvY29tbW9uL3RhYmxlL3RpbWVsaW5lL3ZlcnNpb25pbmcvdjIvQ29tcGxldGlvblRpbWVRdWVyeVZpZXdWMi5qYXZh) | 50.00% | [0 Missing and 1 partial :warning: ](https://app.codecov.io/gh/apache/hudi/pull/19452?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #19452 +/- ##
============================================
- Coverage 75.86% 75.85% -0.01%
Complexity 32891 32891
============================================
Files 2576 2576
Lines 143558 143558
Branches 17607 17608 +1
============================================
- Hits 108905 108902 -3
- Misses 26632 26633 +1
- Partials 8021 8023 +2
```
| [Components](https://app.codecov.io/gh/apache/hudi/pull/19452/components?src=pr&el=components&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [hudi-common](https://app.codecov.io/gh/apache/hudi/pull/19452/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `82.26% <50.00%> (-0.01%)` | :arrow_down: |
| [hudi-client](https://app.codecov.io/gh/apache/hudi/pull/19452/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `81.83% <ø> (-0.01%)` | :arrow_down: |
| [hudi-flink](https://app.codecov.io/gh/apache/hudi/pull/19452/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `83.97% <ø> (ø)` | |
| [hudi-spark-datasource](https://app.codecov.io/gh/apache/hudi/pull/19452/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `68.92% <ø> (ø)` | |
| [hudi-utilities](https://app.codecov.io/gh/apache/hudi/pull/19452/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `73.65% <ø> (ø)` | |
| [hudi-cli](https://app.codecov.io/gh/apache/hudi/pull/19452/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `15.32% <ø> (ø)` | |
| [hudi-hadoop](https://app.codecov.io/gh/apache/hudi/pull/19452/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `63.49% <ø> (ø)` | |
| [hudi-sync](https://app.codecov.io/gh/apache/hudi/pull/19452/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `70.92% <ø> (ø)` | |
| [hudi-io](https://app.codecov.io/gh/apache/hudi/pull/19452/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `79.60% <ø> (ø)` | |
| [hudi-timeline-service](https://app.codecov.io/gh/apache/hudi/pull/19452/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `83.44% <ø> (ø)` | |
| [hudi-cloud](https://app.codecov.io/gh/apache/hudi/pull/19452/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `64.00% <ø> (ø)` | |
| [hudi-kafka-connect](https://app.codecov.io/gh/apache/hudi/pull/19452/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `53.96% <ø> (ø)` | |
| [Flag](https://app.codecov.io/gh/apache/hudi/pull/19452/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [hadoop-mr-java-client](https://app.codecov.io/gh/apache/hudi/pull/19452/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `43.41% <50.00%> (-0.03%)` | :arrow_down: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Files with missing lines](https://app.codecov.io/gh/apache/hudi/pull/19452?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [...eline/versioning/v2/CompletionTimeQueryViewV2.java](https://app.codecov.io/gh/apache/hudi/pull/19452?src=pr&el=tree&filepath=hudi-common%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fhudi%2Fcommon%2Ftable%2Ftimeline%2Fversioning%2Fv2%2FCompletionTimeQueryViewV2.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-aHVkaS1jb21tb24vc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2h1ZGkvY29tbW9uL3RhYmxlL3RpbWVsaW5lL3ZlcnNpb25pbmcvdjIvQ29tcGxldGlvblRpbWVRdWVyeVZpZXdWMi5qYXZh) | `88.50% <50.00%> (-1.15%)` | :arrow_down: |
... and [1 file with indirect coverage changes](https://app.codecov.io/gh/apache/hudi/pull/19452/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
</details>
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(timeline): do not NPE on archived instants without a completion time [hudi]
Posted by "hudi-bot (via GitHub)" <gi...@apache.org>.
hudi-bot commented on PR #19452:
URL: https://github.com/apache/hudi/pull/19452#issuecomment-5165139840
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "e277fb301f3dc51e203fbe5e11efcce80526f552",
"status" : "DELETED",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=15929",
"triggerID" : "e277fb301f3dc51e203fbe5e11efcce80526f552",
"triggerType" : "PUSH"
}, {
"hash" : "e4777ed33e92b52fcdb90732636e1e90f4f94033",
"status" : "SUCCESS",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=15956",
"triggerID" : "e4777ed33e92b52fcdb90732636e1e90f4f94033",
"triggerType" : "PUSH"
}, {
"hash" : "93cf5b1cbddd1526076cb23f28fa8b10e228c206",
"status" : "PENDING",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=15970",
"triggerID" : "93cf5b1cbddd1526076cb23f28fa8b10e228c206",
"triggerType" : "PUSH"
}, {
"hash" : "827b6ba9e3b10da55cf6c509a0a0493cc3c81cc9",
"status" : "UNKNOWN",
"url" : "TBD",
"triggerID" : "827b6ba9e3b10da55cf6c509a0a0493cc3c81cc9",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* e4777ed33e92b52fcdb90732636e1e90f4f94033 Azure: [SUCCESS](https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=15956)
* 93cf5b1cbddd1526076cb23f28fa8b10e228c206 Azure: [PENDING](https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=15970)
* 827b6ba9e3b10da55cf6c509a0a0493cc3c81cc9 UNKNOWN
<details>
<summary>Bot commands</summary>
@hudi-bot supports the following commands:
- `@hudi-bot run azure` re-run the last Azure build
</details>
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(timeline): do not NPE on archived instants without a completion time [hudi]
Posted by "hudi-agent (via GitHub)" <gi...@apache.org>.
hudi-agent commented on code in PR #19452:
URL: https://github.com/apache/hudi/pull/19452#discussion_r3702898326
##########
hudi-common/src/main/java/org/apache/hudi/common/table/timeline/versioning/v2/CompletionTimeQueryViewV2.java:
##########
@@ -303,8 +304,9 @@ private void load() {
}
private void readCompletionTime(String instantTime, GenericRecord record) {
Review Comment:
🤖 Nice fix. Since the root cause is the nullable `completionTime` field, I noticed the same raw `record.get(COMPLETION_TIME_ARCHIVED_META_FIELD).toString()` also lives in `ArchivedTimelineV2.readCommit` and `MetadataConversionUtils.createMetaWrapper`, both reading the same LSM field. Would an old archived instant loaded through those paths NPE the same way? Might be worth applying the same null-safe treatment there (or noting it's intentionally out of scope).
<sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag quality.</i></sub>
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(timeline): do not NPE on archived instants without a completion time [hudi]
Posted by "hudi-bot (via GitHub)" <gi...@apache.org>.
hudi-bot commented on PR #19452:
URL: https://github.com/apache/hudi/pull/19452#issuecomment-5163506922
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "e277fb301f3dc51e203fbe5e11efcce80526f552",
"status" : "SUCCESS",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=15929",
"triggerID" : "e277fb301f3dc51e203fbe5e11efcce80526f552",
"triggerType" : "PUSH"
}, {
"hash" : "e4777ed33e92b52fcdb90732636e1e90f4f94033",
"status" : "PENDING",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=15956",
"triggerID" : "e4777ed33e92b52fcdb90732636e1e90f4f94033",
"triggerType" : "PUSH"
}, {
"hash" : "93cf5b1cbddd1526076cb23f28fa8b10e228c206",
"status" : "UNKNOWN",
"url" : "TBD",
"triggerID" : "93cf5b1cbddd1526076cb23f28fa8b10e228c206",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* e277fb301f3dc51e203fbe5e11efcce80526f552 Azure: [SUCCESS](https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=15929)
* e4777ed33e92b52fcdb90732636e1e90f4f94033 Azure: [PENDING](https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=15956)
* 93cf5b1cbddd1526076cb23f28fa8b10e228c206 UNKNOWN
<details>
<summary>Bot commands</summary>
@hudi-bot supports the following commands:
- `@hudi-bot run azure` re-run the last Azure build
</details>
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(timeline): do not NPE on archived instants without a completion time [hudi]
Posted by "hudi-bot (via GitHub)" <gi...@apache.org>.
hudi-bot commented on PR #19452:
URL: https://github.com/apache/hudi/pull/19452#issuecomment-5163546426
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "e277fb301f3dc51e203fbe5e11efcce80526f552",
"status" : "SUCCESS",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=15929",
"triggerID" : "e277fb301f3dc51e203fbe5e11efcce80526f552",
"triggerType" : "PUSH"
}, {
"hash" : "e4777ed33e92b52fcdb90732636e1e90f4f94033",
"status" : "PENDING",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=15956",
"triggerID" : "e4777ed33e92b52fcdb90732636e1e90f4f94033",
"triggerType" : "PUSH"
}, {
"hash" : "93cf5b1cbddd1526076cb23f28fa8b10e228c206",
"status" : "PENDING",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=15970",
"triggerID" : "93cf5b1cbddd1526076cb23f28fa8b10e228c206",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* e277fb301f3dc51e203fbe5e11efcce80526f552 Azure: [SUCCESS](https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=15929)
* e4777ed33e92b52fcdb90732636e1e90f4f94033 Azure: [PENDING](https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=15956)
* 93cf5b1cbddd1526076cb23f28fa8b10e228c206 Azure: [PENDING](https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=15970)
<details>
<summary>Bot commands</summary>
@hudi-bot supports the following commands:
- `@hudi-bot run azure` re-run the last Azure build
</details>
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(timeline): do not NPE on archived instants without a completion time [hudi]
Posted by "hudi-bot (via GitHub)" <gi...@apache.org>.
hudi-bot commented on PR #19452:
URL: https://github.com/apache/hudi/pull/19452#issuecomment-5165188761
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "e277fb301f3dc51e203fbe5e11efcce80526f552",
"status" : "DELETED",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=15929",
"triggerID" : "e277fb301f3dc51e203fbe5e11efcce80526f552",
"triggerType" : "PUSH"
}, {
"hash" : "e4777ed33e92b52fcdb90732636e1e90f4f94033",
"status" : "SUCCESS",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=15956",
"triggerID" : "e4777ed33e92b52fcdb90732636e1e90f4f94033",
"triggerType" : "PUSH"
}, {
"hash" : "93cf5b1cbddd1526076cb23f28fa8b10e228c206",
"status" : "PENDING",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=15970",
"triggerID" : "93cf5b1cbddd1526076cb23f28fa8b10e228c206",
"triggerType" : "PUSH"
}, {
"hash" : "827b6ba9e3b10da55cf6c509a0a0493cc3c81cc9",
"status" : "PENDING",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=15984",
"triggerID" : "827b6ba9e3b10da55cf6c509a0a0493cc3c81cc9",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* e4777ed33e92b52fcdb90732636e1e90f4f94033 Azure: [SUCCESS](https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=15956)
* 93cf5b1cbddd1526076cb23f28fa8b10e228c206 Azure: [PENDING](https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=15970)
* 827b6ba9e3b10da55cf6c509a0a0493cc3c81cc9 Azure: [PENDING](https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=15984)
<details>
<summary>Bot commands</summary>
@hudi-bot supports the following commands:
- `@hudi-bot run azure` re-run the last Azure build
</details>
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(timeline): do not NPE on archived instants without a completion time [hudi]
Posted by "voonhous (via GitHub)" <gi...@apache.org>.
voonhous commented on code in PR #19452:
URL: https://github.com/apache/hudi/pull/19452#discussion_r3694742346
##########
hudi-common/src/main/java/org/apache/hudi/common/table/timeline/versioning/v2/CompletionTimeQueryViewV2.java:
##########
@@ -302,9 +302,12 @@ private void load() {
.forEach(instant -> setCompletionTime(instant.requestedTime(), instant.getCompletionTime()));
}
- private void readCompletionTime(String instantTime, GenericRecord record) {
- final String completionTime = record.get(COMPLETION_TIME_ARCHIVED_META_FIELD).toString();
- setCompletionTime(instantTime, completionTime);
+ @VisibleForTesting
Review Comment:
If the regression test moves to `TestCompletionTimeQueryView` in `hudi-client-common` (see my comment on the test class), this method can stay `private` and the `@VisibleForTesting` widening is not needed.
##########
hudi-common/src/main/java/org/apache/hudi/common/table/timeline/versioning/v2/CompletionTimeQueryViewV2.java:
##########
@@ -302,9 +302,12 @@ private void load() {
.forEach(instant -> setCompletionTime(instant.requestedTime(), instant.getCompletionTime()));
}
- private void readCompletionTime(String instantTime, GenericRecord record) {
- final String completionTime = record.get(COMPLETION_TIME_ARCHIVED_META_FIELD).toString();
- setCompletionTime(instantTime, completionTime);
+ @VisibleForTesting
+ void readCompletionTime(String instantTime, GenericRecord record) {
+ // 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.
+ final Object completionTime = record.get(COMPLETION_TIME_ARCHIVED_META_FIELD);
+ setCompletionTime(instantTime, completionTime != null ? completionTime.toString() : null);
Review Comment:
`StringUtils.objToString` (hudi-io, `org.apache.hudi.common.util`) is exactly this null-safe `toString`, and it is what `HoodieAvroUtils.getNullableValAsString` is built on. Using it drops the local variable and the ternary:
```suggestion
setCompletionTime(instantTime, StringUtils.objToString(record.get(COMPLETION_TIME_ARCHIVED_META_FIELD)));
```
(needs `import org.apache.hudi.common.util.StringUtils;`)
If `ArchivedTimelineV2#readCommit` and `MetadataConversionUtils#createMetaWrapper` get the same treatment, using one helper in all three places keeps them uniform.
The comment above is good, by the way -- it explains why the null is legal rather than restating the code.
##########
hudi-common/src/test/java/org/apache/hudi/common/table/timeline/versioning/v2/TestCompletionTimeQueryViewV2.java:
##########
@@ -0,0 +1,94 @@
+/*
+ * 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.versioning.v2;
+
+import org.apache.hudi.avro.model.HoodieLSMTimelineInstant;
+import org.apache.hudi.common.table.HoodieTableMetaClient;
+import org.apache.hudi.common.table.timeline.HoodieActiveTimeline;
+import org.apache.hudi.common.table.timeline.HoodieTimeline;
+import org.apache.hudi.common.util.Option;
+
+import org.apache.avro.generic.GenericData;
+import org.apache.avro.generic.GenericRecord;
+import org.junit.jupiter.api.Test;
+
+import java.util.stream.Stream;
+
+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.junit.jupiter.api.Assertions.assertEquals;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+
+class TestCompletionTimeQueryViewV2 {
+
+ /**
+ * 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, as
+ * {@code setCompletionTime} already documents, 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} from this path.
+ */
+ @Test
+ void readCompletionTimeFallsBackWhenTheArchivedRecordHasNoCompletionTime() {
+ try (CompletionTimeQueryViewV2 view = new CompletionTimeQueryViewV2(mockMetaClientWithEmptyTimeline())) {
+ GenericRecord record = new GenericData.Record(HoodieLSMTimelineInstant.getClassSchema());
+ record.put(INSTANT_TIME_ARCHIVED_META_FIELD, "00000001");
+ record.put(ACTION_ARCHIVED_META_FIELD, "commit");
Review Comment:
These two fields are not read by `readCompletionTime`. Harmless as realism, but they suggest a coupling that is not there -- the only field under test is `completionTime`. Same in the second test.
##########
hudi-common/src/test/java/org/apache/hudi/common/table/timeline/versioning/v2/TestCompletionTimeQueryViewV2.java:
##########
@@ -0,0 +1,94 @@
+/*
+ * 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.versioning.v2;
+
+import org.apache.hudi.avro.model.HoodieLSMTimelineInstant;
+import org.apache.hudi.common.table.HoodieTableMetaClient;
+import org.apache.hudi.common.table.timeline.HoodieActiveTimeline;
+import org.apache.hudi.common.table.timeline.HoodieTimeline;
+import org.apache.hudi.common.util.Option;
+
+import org.apache.avro.generic.GenericData;
+import org.apache.avro.generic.GenericRecord;
+import org.junit.jupiter.api.Test;
+
+import java.util.stream.Stream;
+
+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.junit.jupiter.api.Assertions.assertEquals;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+
+class TestCompletionTimeQueryViewV2 {
+
+ /**
+ * 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, as
+ * {@code setCompletionTime} already documents, 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} from this path.
+ */
+ @Test
+ void readCompletionTimeFallsBackWhenTheArchivedRecordHasNoCompletionTime() {
+ try (CompletionTimeQueryViewV2 view = new CompletionTimeQueryViewV2(mockMetaClientWithEmptyTimeline())) {
+ GenericRecord record = new GenericData.Record(HoodieLSMTimelineInstant.getClassSchema());
+ record.put(INSTANT_TIME_ARCHIVED_META_FIELD, "00000001");
+ record.put(ACTION_ARCHIVED_META_FIELD, "commit");
+ // completionTime deliberately left unset
+
+ view.readCompletionTime("00000001", record);
+
+ assertEquals(Option.of("00000001"), view.getCompletionTime("00000001"),
+ "An archived instant without a completion time should fall back to its instant time");
+ }
+ }
+
+ @Test
+ void readCompletionTimeUsesTheArchivedCompletionTimeWhenPresent() {
+ try (CompletionTimeQueryViewV2 view = new CompletionTimeQueryViewV2(mockMetaClientWithEmptyTimeline())) {
+ GenericRecord record = new GenericData.Record(HoodieLSMTimelineInstant.getClassSchema());
+ record.put(INSTANT_TIME_ARCHIVED_META_FIELD, "00000001");
+ record.put(ACTION_ARCHIVED_META_FIELD, "commit");
+ record.put(COMPLETION_TIME_ARCHIVED_META_FIELD, "00001001");
+
+ view.readCompletionTime("00000001", record);
+
+ assertEquals(Option.of("00001001"), view.getCompletionTime("00000001"));
+ }
+ }
+
+ private static HoodieTableMetaClient mockMetaClientWithEmptyTimeline() {
Review Comment:
All five stubs exist purely to get the constructor to return -- none of them are used by the method under test. That makes the tests brittle in an unhelpful way: any future `metaClient` call added to the `CompletionTimeQueryViewV2` constructor breaks them with an NPE that has nothing to do with `readCompletionTime`. Another argument for putting the test where a real meta client is cheap to build.
##########
hudi-common/src/test/java/org/apache/hudi/common/table/timeline/versioning/v2/TestCompletionTimeQueryViewV2.java:
##########
@@ -0,0 +1,94 @@
+/*
+ * 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.versioning.v2;
+
+import org.apache.hudi.avro.model.HoodieLSMTimelineInstant;
+import org.apache.hudi.common.table.HoodieTableMetaClient;
+import org.apache.hudi.common.table.timeline.HoodieActiveTimeline;
+import org.apache.hudi.common.table.timeline.HoodieTimeline;
+import org.apache.hudi.common.util.Option;
+
+import org.apache.avro.generic.GenericData;
+import org.apache.avro.generic.GenericRecord;
+import org.junit.jupiter.api.Test;
+
+import java.util.stream.Stream;
+
+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.junit.jupiter.api.Assertions.assertEquals;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+
+class TestCompletionTimeQueryViewV2 {
Review Comment:
Two things on the placement of this class.
**Consider putting the regression test on the real read path instead.** There is already a harness for it: `hudi-client/hudi-client-common/src/test/java/org/apache/hudi/client/timeline/TestCompletionTimeQueryView.java` builds a real table and archives through `LSMTimelineWriter`. Its `prepareTimeline` constructs each `DummyActiveAction` from `INSTANT_GENERATOR.createNewInstant(COMPLETED, "commit", instantTime, completionTime)`, and that instant flows straight into `MetadataConversionUtils#createLSMTimelineInstant` -> `setCompletionTime(...)`. Archiving one entry with a null completion time there should reproduce the reported NPE through `ArchivedTimelineLoaderV2`, and it would cover the archived-timeline read as well as this method. Mocking is a reasonable fallback in `hudi-common` (`HoodieTestUtils.init` needs a storage impl that lives outside the module, and there is precedent in `TestCheckpointUtils` and `TestIncrementalQueryAnalyzer`), but here it costs the coverage that matte
rs most.
**Missing class-level javadoc.** The sibling class has `/** Test cases for {@link CompletionTimeQueryView}. */`; worth matching.
##########
hudi-common/src/test/java/org/apache/hudi/common/table/timeline/versioning/v2/TestCompletionTimeQueryViewV2.java:
##########
@@ -0,0 +1,94 @@
+/*
+ * 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.versioning.v2;
+
+import org.apache.hudi.avro.model.HoodieLSMTimelineInstant;
+import org.apache.hudi.common.table.HoodieTableMetaClient;
+import org.apache.hudi.common.table.timeline.HoodieActiveTimeline;
+import org.apache.hudi.common.table.timeline.HoodieTimeline;
+import org.apache.hudi.common.util.Option;
+
+import org.apache.avro.generic.GenericData;
+import org.apache.avro.generic.GenericRecord;
+import org.junit.jupiter.api.Test;
+
+import java.util.stream.Stream;
+
+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.junit.jupiter.api.Assertions.assertEquals;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+
+class TestCompletionTimeQueryViewV2 {
+
+ /**
+ * 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, as
+ * {@code setCompletionTime} already documents, 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} from this path.
+ */
+ @Test
+ void readCompletionTimeFallsBackWhenTheArchivedRecordHasNoCompletionTime() {
Review Comment:
Nit: the method naming deviates from the convention in this area. The sibling class uses `testReadCompletionTime` / `testReadCompletionTimeWithCornerCase`; something like `testReadCompletionTimeWithoutCompletionTime` would fit better and is a lot shorter.
##########
hudi-common/src/test/java/org/apache/hudi/common/table/timeline/versioning/v2/TestCompletionTimeQueryViewV2.java:
##########
@@ -0,0 +1,94 @@
+/*
+ * 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.versioning.v2;
+
+import org.apache.hudi.avro.model.HoodieLSMTimelineInstant;
+import org.apache.hudi.common.table.HoodieTableMetaClient;
+import org.apache.hudi.common.table.timeline.HoodieActiveTimeline;
+import org.apache.hudi.common.table.timeline.HoodieTimeline;
+import org.apache.hudi.common.util.Option;
+
+import org.apache.avro.generic.GenericData;
+import org.apache.avro.generic.GenericRecord;
+import org.junit.jupiter.api.Test;
+
+import java.util.stream.Stream;
+
+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.junit.jupiter.api.Assertions.assertEquals;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+
+class TestCompletionTimeQueryViewV2 {
+
+ /**
+ * 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, as
+ * {@code setCompletionTime} already documents, 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} from this path.
+ */
+ @Test
+ void readCompletionTimeFallsBackWhenTheArchivedRecordHasNoCompletionTime() {
+ try (CompletionTimeQueryViewV2 view = new CompletionTimeQueryViewV2(mockMetaClientWithEmptyTimeline())) {
+ GenericRecord record = new GenericData.Record(HoodieLSMTimelineInstant.getClassSchema());
+ record.put(INSTANT_TIME_ARCHIVED_META_FIELD, "00000001");
+ record.put(ACTION_ARCHIVED_META_FIELD, "commit");
+ // completionTime deliberately left unset
+
+ view.readCompletionTime("00000001", record);
+
+ assertEquals(Option.of("00000001"), view.getCompletionTime("00000001"),
+ "An archived instant without a completion time should fall back to its instant time");
+ }
+ }
+
+ @Test
+ void readCompletionTimeUsesTheArchivedCompletionTimeWhenPresent() {
+ try (CompletionTimeQueryViewV2 view = new CompletionTimeQueryViewV2(mockMetaClientWithEmptyTimeline())) {
+ GenericRecord record = new GenericData.Record(HoodieLSMTimelineInstant.getClassSchema());
+ record.put(INSTANT_TIME_ARCHIVED_META_FIELD, "00000001");
+ record.put(ACTION_ARCHIVED_META_FIELD, "commit");
+ record.put(COMPLETION_TIME_ARCHIVED_META_FIELD, "00001001");
+
+ view.readCompletionTime("00000001", record);
+
+ assertEquals(Option.of("00001001"), view.getCompletionTime("00000001"));
Review Comment:
Nit: the first test carries an assertion message and this one does not. Worth making the two consistent.
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(timeline): do not NPE on archived instants without a completion time [hudi]
Posted by "rangareddy (via GitHub)" <gi...@apache.org>.
rangareddy commented on code in PR #19452:
URL: https://github.com/apache/hudi/pull/19452#discussion_r3703007006
##########
hudi-common/src/main/java/org/apache/hudi/common/table/timeline/versioning/v2/CompletionTimeQueryViewV2.java:
##########
@@ -303,8 +304,9 @@ private void load() {
}
private void readCompletionTime(String instantTime, GenericRecord record) {
Review Comment:
Checked both, and you are right — fixed in `827b6ba` rather than left out of scope.
`MetadataConversionUtils.createMetaWrapper` demonstrably NPEs on the same record shape. A record with `completionTime` left unset:
```
java.lang.NullPointerException
at org.apache.hudi.common.table.timeline.MetadataConversionUtils.createMetaWrapper(MetadataConversionUtils.java:174)
```
`ArchivedTimelineV2.readCommit` is the identical one-line pattern on the same field, and both read the same LSM records as the query view, so the trigger is the same: a table archived before `completionTime` existed. Worth noting both methods already null-check the neighbouring nullable fields, `metadata` and `plan` — `completionTime` was the odd one out rather than a deliberate choice.
Both now go through one helper so they cannot drift:
```java
public static String completionTimeOrInstantTime(GenericRecord record, String instantTime) {
String completionTime = StringUtils.objToString(record.get(COMPLETION_TIME_ARCHIVED_META_FIELD));
return completionTime != null ? completionTime : instantTime;
}
```
On the choice of fallback: both sites build a **completed** `HoodieInstant`, so this is not the same question as the query view, which is what I had flagged as unsettled earlier. Defaulting to the instant time is the behaviour `CompletionTimeQueryViewV2#setCompletionTime` already documents for exactly these records ("the meta-server instant does not have completion time"), so it follows existing precedent rather than inventing a rule. Leaving the value null instead would only move the failure to whatever compares completion times later.
`CompletionTimeQueryViewV2#readCompletionTime` is left alone — it hands a possibly-null value to `setCompletionTime`, which owns the fallback.
New `TestArchivedInstantCompletionTime` covers the helper both ways plus `createMetaWrapper` end to end, asserting the archived entry carries the instant time. With the null-safety removed, the two absent-completion-time tests fail with `NullPointerException` and the present-value one stays green. 28 tests green across `TestCompletionTimeQueryView`, `TestArchivedTimelineV2` and `TestMetadataConversionUtils`; checkstyle and rat clean.
##########
hudi-common/src/test/java/org/apache/hudi/common/table/timeline/versioning/v2/TestCompletionTimeQueryViewV2.java:
##########
@@ -0,0 +1,94 @@
+/*
+ * 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.versioning.v2;
+
+import org.apache.hudi.avro.model.HoodieLSMTimelineInstant;
+import org.apache.hudi.common.table.HoodieTableMetaClient;
+import org.apache.hudi.common.table.timeline.HoodieActiveTimeline;
+import org.apache.hudi.common.table.timeline.HoodieTimeline;
+import org.apache.hudi.common.util.Option;
+
+import org.apache.avro.generic.GenericData;
+import org.apache.avro.generic.GenericRecord;
+import org.junit.jupiter.api.Test;
+
+import java.util.stream.Stream;
+
+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.junit.jupiter.api.Assertions.assertEquals;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+
+class TestCompletionTimeQueryViewV2 {
Review Comment:
Correction to my earlier reply here: @voonhous pushed `93cf5b1` to the branch showing the real-path test does work, and I was wrong to conclude it could not.
I verified it rather than take it on trust, since I had claimed the opposite. With the fix reverted their test fails with the reported NPE, through the real load path:
```
java.lang.NullPointerException
at CompletionTimeQueryViewV2.readCompletionTime(CompletionTimeQueryViewV2.java:309)
at CompletionTimeQueryViewV2.loadCompletionTimeIncrementally(CompletionTimeQueryViewV2.java:286)
at CompletionTimeQueryViewV2.getCompletionTime(CompletionTimeQueryViewV2.java:178)
```
With the fix in place it passes. So it is genuinely non-vacuous, which is exactly what my attempt failed to achieve.
What I got wrong: I drove the archive through `HoodieTestTable#addCommit`/`prepareTimeline`, and the null does not survive that route — `ActiveAction#getCompletionTime` reads it back off the completed instant, so `createLSMTimelineInstant` receives a value and the archived record is well-formed. I concluded from that the null could not reach an LSM record without hand-writing an archive file. The step I missed is that you can construct the `ActiveAction` yourself with a null completion time and hand it straight to `LSMTimelineWriter#write`, which skips the part that was repopulating it. That is the right entry point and it is a better test than the mock, so the mocked `TestCompletionTimeQueryViewV2` going away is the right call too.
Their `assertTrue(archiveFailures.isEmpty(), ...)` is a good catch as well — `LSMTimelineWriter#write` swallows per-instant failures, so without it a failed archive write would have left the test passing against an empty archive, i.e. vacuous for a different reason.
I have rebased my remaining commit on top of theirs rather than over it, so `93cf5b1` is intact.
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(timeline): do not NPE on archived instants without a completion time [hudi]
Posted by "voonhous (via GitHub)" <gi...@apache.org>.
voonhous merged PR #19452:
URL: https://github.com/apache/hudi/pull/19452
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(timeline): do not NPE on archived instants without a completion time [hudi]
Posted by "hudi-bot (via GitHub)" <gi...@apache.org>.
hudi-bot commented on PR #19452:
URL: https://github.com/apache/hudi/pull/19452#issuecomment-5145144349
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "e277fb301f3dc51e203fbe5e11efcce80526f552",
"status" : "UNKNOWN",
"url" : "TBD",
"triggerID" : "e277fb301f3dc51e203fbe5e11efcce80526f552",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* e277fb301f3dc51e203fbe5e11efcce80526f552 UNKNOWN
<details>
<summary>Bot commands</summary>
@hudi-bot supports the following commands:
- `@hudi-bot run azure` re-run the last Azure build
</details>
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(timeline): do not NPE on archived instants without a completion time [hudi]
Posted by "hudi-bot (via GitHub)" <gi...@apache.org>.
hudi-bot commented on PR #19452:
URL: https://github.com/apache/hudi/pull/19452#issuecomment-5145196720
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "e277fb301f3dc51e203fbe5e11efcce80526f552",
"status" : "PENDING",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=15929",
"triggerID" : "e277fb301f3dc51e203fbe5e11efcce80526f552",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* e277fb301f3dc51e203fbe5e11efcce80526f552 Azure: [PENDING](https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=15929)
<details>
<summary>Bot commands</summary>
@hudi-bot supports the following commands:
- `@hudi-bot run azure` re-run the last Azure build
</details>
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(timeline): do not NPE on archived instants without a completion time [hudi]
Posted by "hudi-bot (via GitHub)" <gi...@apache.org>.
hudi-bot commented on PR #19452:
URL: https://github.com/apache/hudi/pull/19452#issuecomment-5162642552
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "e277fb301f3dc51e203fbe5e11efcce80526f552",
"status" : "SUCCESS",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=15929",
"triggerID" : "e277fb301f3dc51e203fbe5e11efcce80526f552",
"triggerType" : "PUSH"
}, {
"hash" : "e4777ed33e92b52fcdb90732636e1e90f4f94033",
"status" : "PENDING",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=15956",
"triggerID" : "e4777ed33e92b52fcdb90732636e1e90f4f94033",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* e277fb301f3dc51e203fbe5e11efcce80526f552 Azure: [SUCCESS](https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=15929)
* e4777ed33e92b52fcdb90732636e1e90f4f94033 Azure: [PENDING](https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=15956)
<details>
<summary>Bot commands</summary>
@hudi-bot supports the following commands:
- `@hudi-bot run azure` re-run the last Azure build
</details>
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(timeline): do not NPE on archived instants without a completion time [hudi]
Posted by "hudi-bot (via GitHub)" <gi...@apache.org>.
hudi-bot commented on PR #19452:
URL: https://github.com/apache/hudi/pull/19452#issuecomment-5147990477
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "e277fb301f3dc51e203fbe5e11efcce80526f552",
"status" : "SUCCESS",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=15929",
"triggerID" : "e277fb301f3dc51e203fbe5e11efcce80526f552",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* e277fb301f3dc51e203fbe5e11efcce80526f552 Azure: [SUCCESS](https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=15929)
<details>
<summary>Bot commands</summary>
@hudi-bot supports the following commands:
- `@hudi-bot run azure` re-run the last Azure build
</details>
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(timeline): do not NPE on archived instants without a completion time [hudi]
Posted by "hudi-bot (via GitHub)" <gi...@apache.org>.
hudi-bot commented on PR #19452:
URL: https://github.com/apache/hudi/pull/19452#issuecomment-5162615052
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "e277fb301f3dc51e203fbe5e11efcce80526f552",
"status" : "SUCCESS",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=15929",
"triggerID" : "e277fb301f3dc51e203fbe5e11efcce80526f552",
"triggerType" : "PUSH"
}, {
"hash" : "e4777ed33e92b52fcdb90732636e1e90f4f94033",
"status" : "UNKNOWN",
"url" : "TBD",
"triggerID" : "e4777ed33e92b52fcdb90732636e1e90f4f94033",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* e277fb301f3dc51e203fbe5e11efcce80526f552 Azure: [SUCCESS](https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=15929)
* e4777ed33e92b52fcdb90732636e1e90f4f94033 UNKNOWN
<details>
<summary>Bot commands</summary>
@hudi-bot supports the following commands:
- `@hudi-bot run azure` re-run the last Azure build
</details>
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(timeline): do not NPE on archived instants without a completion time [hudi]
Posted by "rangareddy (via GitHub)" <gi...@apache.org>.
rangareddy commented on code in PR #19452:
URL: https://github.com/apache/hudi/pull/19452#discussion_r3701472236
##########
hudi-common/src/main/java/org/apache/hudi/common/table/timeline/versioning/v2/CompletionTimeQueryViewV2.java:
##########
@@ -302,9 +302,12 @@ private void load() {
.forEach(instant -> setCompletionTime(instant.requestedTime(), instant.getCompletionTime()));
}
- private void readCompletionTime(String instantTime, GenericRecord record) {
- final String completionTime = record.get(COMPLETION_TIME_ARCHIVED_META_FIELD).toString();
- setCompletionTime(instantTime, completionTime);
+ @VisibleForTesting
+ void readCompletionTime(String instantTime, GenericRecord record) {
+ // 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.
+ final Object completionTime = record.get(COMPLETION_TIME_ARCHIVED_META_FIELD);
+ setCompletionTime(instantTime, completionTime != null ? completionTime.toString() : null);
Review Comment:
Applied your suggestion in e4777ed — `StringUtils.objToString` reads better and drops both the local and the ternary. Thanks also for pointing at `HoodieAvroUtils.getNullableValAsString`; using the same helper in `ArchivedTimelineV2#readCommit` and `MetadataConversionUtils` is the right shape for a follow-up, once the semantics there are settled (a null in `readCommit` would produce a *completed* instant with no completion time, which is a different question from this one).
##########
hudi-common/src/test/java/org/apache/hudi/common/table/timeline/versioning/v2/TestCompletionTimeQueryViewV2.java:
##########
@@ -0,0 +1,94 @@
+/*
+ * 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.versioning.v2;
+
+import org.apache.hudi.avro.model.HoodieLSMTimelineInstant;
+import org.apache.hudi.common.table.HoodieTableMetaClient;
+import org.apache.hudi.common.table.timeline.HoodieActiveTimeline;
+import org.apache.hudi.common.table.timeline.HoodieTimeline;
+import org.apache.hudi.common.util.Option;
+
+import org.apache.avro.generic.GenericData;
+import org.apache.avro.generic.GenericRecord;
+import org.junit.jupiter.api.Test;
+
+import java.util.stream.Stream;
+
+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.junit.jupiter.api.Assertions.assertEquals;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+
+class TestCompletionTimeQueryViewV2 {
Review Comment:
I tried the harness route and it does not reproduce the bug, so I have kept the unit test. Reporting what I found, since it is the interesting part.
Added a `testReadCompletionTimeWithoutCompletionTime` to `TestCompletionTimeQueryView`, archiving instant `00000003` with a null completion time through `prepareTimeline` (`Option.of` -> `Option.ofNullable` so the null gets through `HoodieTestTable#addCommit`). Then I removed the production fix and re-ran it: **it passed**. No NPE, and `getCompletionTime("00000003")` returned the instant time either way, so the test never exercised `readCompletionTime` and would have been a vacuous regression test.
The null does not survive into the archived record. `ActiveAction#getCompletionTime` reads it back off the `HoodieInstant` (`getCompleted().getCompletionTime()`), and by the time `MetadataConversionUtils#createLSMTimelineInstant` calls `setCompletionTime(activeAction.getCompletionTime())` it is no longer null, so the archived `HoodieLSMTimelineInstant` carries a value. Producing a genuinely null `completionTime` in an LSM record needs either an instant that predates the field or a hand-written archive file, neither of which that harness builds.
So the mocked test stays, with your other points applied. If you would still prefer real-path coverage I am happy to look at writing an LSM archive file directly, but I did not want to land a test that passes with the fix reverted. Also added the missing class javadoc.
##########
hudi-common/src/main/java/org/apache/hudi/common/table/timeline/versioning/v2/CompletionTimeQueryViewV2.java:
##########
@@ -302,9 +302,12 @@ private void load() {
.forEach(instant -> setCompletionTime(instant.requestedTime(), instant.getCompletionTime()));
}
- private void readCompletionTime(String instantTime, GenericRecord record) {
- final String completionTime = record.get(COMPLETION_TIME_ARCHIVED_META_FIELD).toString();
- setCompletionTime(instantTime, completionTime);
+ @VisibleForTesting
Review Comment:
Kept `@VisibleForTesting` for now, since the real-path test did not work out — see my reply on the test class. If the harness route becomes viable this widening should go with it.
##########
hudi-common/src/test/java/org/apache/hudi/common/table/timeline/versioning/v2/TestCompletionTimeQueryViewV2.java:
##########
@@ -0,0 +1,94 @@
+/*
+ * 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.versioning.v2;
+
+import org.apache.hudi.avro.model.HoodieLSMTimelineInstant;
+import org.apache.hudi.common.table.HoodieTableMetaClient;
+import org.apache.hudi.common.table.timeline.HoodieActiveTimeline;
+import org.apache.hudi.common.table.timeline.HoodieTimeline;
+import org.apache.hudi.common.util.Option;
+
+import org.apache.avro.generic.GenericData;
+import org.apache.avro.generic.GenericRecord;
+import org.junit.jupiter.api.Test;
+
+import java.util.stream.Stream;
+
+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.junit.jupiter.api.Assertions.assertEquals;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+
+class TestCompletionTimeQueryViewV2 {
+
+ /**
+ * 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, as
+ * {@code setCompletionTime} already documents, 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} from this path.
+ */
+ @Test
+ void readCompletionTimeFallsBackWhenTheArchivedRecordHasNoCompletionTime() {
Review Comment:
Renamed to `testReadCompletionTimeWithoutCompletionTime`, and the sibling case to `testReadCompletionTime`, matching the convention.
##########
hudi-common/src/test/java/org/apache/hudi/common/table/timeline/versioning/v2/TestCompletionTimeQueryViewV2.java:
##########
@@ -0,0 +1,94 @@
+/*
+ * 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.versioning.v2;
+
+import org.apache.hudi.avro.model.HoodieLSMTimelineInstant;
+import org.apache.hudi.common.table.HoodieTableMetaClient;
+import org.apache.hudi.common.table.timeline.HoodieActiveTimeline;
+import org.apache.hudi.common.table.timeline.HoodieTimeline;
+import org.apache.hudi.common.util.Option;
+
+import org.apache.avro.generic.GenericData;
+import org.apache.avro.generic.GenericRecord;
+import org.junit.jupiter.api.Test;
+
+import java.util.stream.Stream;
+
+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.junit.jupiter.api.Assertions.assertEquals;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+
+class TestCompletionTimeQueryViewV2 {
+
+ /**
+ * 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, as
+ * {@code setCompletionTime} already documents, 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} from this path.
+ */
+ @Test
+ void readCompletionTimeFallsBackWhenTheArchivedRecordHasNoCompletionTime() {
+ try (CompletionTimeQueryViewV2 view = new CompletionTimeQueryViewV2(mockMetaClientWithEmptyTimeline())) {
+ GenericRecord record = new GenericData.Record(HoodieLSMTimelineInstant.getClassSchema());
+ record.put(INSTANT_TIME_ARCHIVED_META_FIELD, "00000001");
+ record.put(ACTION_ARCHIVED_META_FIELD, "commit");
Review Comment:
Removed from both tests — you are right that they implied a coupling that does not exist. The record now carries only `completionTime`, with a comment saying that is the only field the method reads. The now-unused `INSTANT_TIME_ARCHIVED_META_FIELD` and `ACTION_ARCHIVED_META_FIELD` imports went with them.
##########
hudi-common/src/test/java/org/apache/hudi/common/table/timeline/versioning/v2/TestCompletionTimeQueryViewV2.java:
##########
@@ -0,0 +1,94 @@
+/*
+ * 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.versioning.v2;
+
+import org.apache.hudi.avro.model.HoodieLSMTimelineInstant;
+import org.apache.hudi.common.table.HoodieTableMetaClient;
+import org.apache.hudi.common.table.timeline.HoodieActiveTimeline;
+import org.apache.hudi.common.table.timeline.HoodieTimeline;
+import org.apache.hudi.common.util.Option;
+
+import org.apache.avro.generic.GenericData;
+import org.apache.avro.generic.GenericRecord;
+import org.junit.jupiter.api.Test;
+
+import java.util.stream.Stream;
+
+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.junit.jupiter.api.Assertions.assertEquals;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+
+class TestCompletionTimeQueryViewV2 {
+
+ /**
+ * 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, as
+ * {@code setCompletionTime} already documents, 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} from this path.
+ */
+ @Test
+ void readCompletionTimeFallsBackWhenTheArchivedRecordHasNoCompletionTime() {
+ try (CompletionTimeQueryViewV2 view = new CompletionTimeQueryViewV2(mockMetaClientWithEmptyTimeline())) {
+ GenericRecord record = new GenericData.Record(HoodieLSMTimelineInstant.getClassSchema());
+ record.put(INSTANT_TIME_ARCHIVED_META_FIELD, "00000001");
+ record.put(ACTION_ARCHIVED_META_FIELD, "commit");
+ // completionTime deliberately left unset
+
+ view.readCompletionTime("00000001", record);
+
+ assertEquals(Option.of("00000001"), view.getCompletionTime("00000001"),
+ "An archived instant without a completion time should fall back to its instant time");
+ }
+ }
+
+ @Test
+ void readCompletionTimeUsesTheArchivedCompletionTimeWhenPresent() {
+ try (CompletionTimeQueryViewV2 view = new CompletionTimeQueryViewV2(mockMetaClientWithEmptyTimeline())) {
+ GenericRecord record = new GenericData.Record(HoodieLSMTimelineInstant.getClassSchema());
+ record.put(INSTANT_TIME_ARCHIVED_META_FIELD, "00000001");
+ record.put(ACTION_ARCHIVED_META_FIELD, "commit");
+ record.put(COMPLETION_TIME_ARCHIVED_META_FIELD, "00001001");
+
+ view.readCompletionTime("00000001", record);
+
+ assertEquals(Option.of("00001001"), view.getCompletionTime("00000001"));
Review Comment:
Made consistent — both cases now carry an assertion message.
##########
hudi-common/src/test/java/org/apache/hudi/common/table/timeline/versioning/v2/TestCompletionTimeQueryViewV2.java:
##########
@@ -0,0 +1,94 @@
+/*
+ * 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.versioning.v2;
+
+import org.apache.hudi.avro.model.HoodieLSMTimelineInstant;
+import org.apache.hudi.common.table.HoodieTableMetaClient;
+import org.apache.hudi.common.table.timeline.HoodieActiveTimeline;
+import org.apache.hudi.common.table.timeline.HoodieTimeline;
+import org.apache.hudi.common.util.Option;
+
+import org.apache.avro.generic.GenericData;
+import org.apache.avro.generic.GenericRecord;
+import org.junit.jupiter.api.Test;
+
+import java.util.stream.Stream;
+
+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.junit.jupiter.api.Assertions.assertEquals;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+
+class TestCompletionTimeQueryViewV2 {
+
+ /**
+ * 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, as
+ * {@code setCompletionTime} already documents, 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} from this path.
+ */
+ @Test
+ void readCompletionTimeFallsBackWhenTheArchivedRecordHasNoCompletionTime() {
+ try (CompletionTimeQueryViewV2 view = new CompletionTimeQueryViewV2(mockMetaClientWithEmptyTimeline())) {
+ GenericRecord record = new GenericData.Record(HoodieLSMTimelineInstant.getClassSchema());
+ record.put(INSTANT_TIME_ARCHIVED_META_FIELD, "00000001");
+ record.put(ACTION_ARCHIVED_META_FIELD, "commit");
+ // completionTime deliberately left unset
+
+ view.readCompletionTime("00000001", record);
+
+ assertEquals(Option.of("00000001"), view.getCompletionTime("00000001"),
+ "An archived instant without a completion time should fall back to its instant time");
+ }
+ }
+
+ @Test
+ void readCompletionTimeUsesTheArchivedCompletionTimeWhenPresent() {
+ try (CompletionTimeQueryViewV2 view = new CompletionTimeQueryViewV2(mockMetaClientWithEmptyTimeline())) {
+ GenericRecord record = new GenericData.Record(HoodieLSMTimelineInstant.getClassSchema());
+ record.put(INSTANT_TIME_ARCHIVED_META_FIELD, "00000001");
+ record.put(ACTION_ARCHIVED_META_FIELD, "commit");
+ record.put(COMPLETION_TIME_ARCHIVED_META_FIELD, "00001001");
+
+ view.readCompletionTime("00000001", record);
+
+ assertEquals(Option.of("00001001"), view.getCompletionTime("00000001"));
+ }
+ }
+
+ private static HoodieTableMetaClient mockMetaClientWithEmptyTimeline() {
Review Comment:
Agreed on the brittleness, and it was the main reason I tried the harness route. It did not reproduce the bug (details in the thread above), so the stubs stay for now. They are down to the constructor's three `metaClient` calls, and your point stands that any new call added there breaks these tests for an unrelated reason — worth revisiting if a real-meta-client route opens up.
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(timeline): do not NPE on archived instants without a completion time [hudi]
Posted by "voonhous (via GitHub)" <gi...@apache.org>.
voonhous commented on code in PR #19452:
URL: https://github.com/apache/hudi/pull/19452#discussion_r3701951807
##########
hudi-common/src/test/java/org/apache/hudi/common/table/timeline/versioning/v2/TestCompletionTimeQueryViewV2.java:
##########
@@ -0,0 +1,94 @@
+/*
+ * 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.versioning.v2;
+
+import org.apache.hudi.avro.model.HoodieLSMTimelineInstant;
+import org.apache.hudi.common.table.HoodieTableMetaClient;
+import org.apache.hudi.common.table.timeline.HoodieActiveTimeline;
+import org.apache.hudi.common.table.timeline.HoodieTimeline;
+import org.apache.hudi.common.util.Option;
+
+import org.apache.avro.generic.GenericData;
+import org.apache.avro.generic.GenericRecord;
+import org.junit.jupiter.api.Test;
+
+import java.util.stream.Stream;
+
+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.junit.jupiter.api.Assertions.assertEquals;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+
+class TestCompletionTimeQueryViewV2 {
Review Comment:
I had a go at this myself, and the harness route does reproduce it.
Reverting the fix makes the test below fail (frame line numbers are off my local tree, so ignore those):
```
java.lang.NullPointerException
at CompletionTimeQueryViewV2.readCompletionTime(CompletionTimeQueryViewV2.java:306)
at ArchivedTimelineLoaderV2.lambda$loadAllInstantsFromFilesInRange$1(ArchivedTimelineLoaderV2.java:129)
at java.base/java.util.stream.ReferencePipeline$3$1.accept(ReferencePipeline.java:195)
...
at CompletionTimeQueryViewV2.loadCompletionTimeIncrementally(CompletionTimeQueryViewV2.java:285)
at CompletionTimeQueryViewV2.getCompletionTime(CompletionTimeQueryViewV2.java:177)
at TestCompletionTimeQueryView.testReadCompletionTimeWithoutCompletionTime(TestCompletionTimeQueryView.java:178)
```
With the fix it passes, and the other five tests in the class are unaffected.
The null does survive into the archived record. `HoodieInstant` is `@AllArgsConstructor @Getter` with `AccessLevel.NONE` on only `requestedTime` and `comparator`, so `getCompletionTime()` is a plain generated getter with no defaulting. `DummyActiveAction` goes through the `ActiveAction` constructor rather than `fromInstants`, so the `Comparator.comparing(HoodieInstant::getCompletionTime)` sort never runs on it. `MetadataConversionUtils#createLSMTimelineInstant` then hands it unguarded to `setCompletionTime`, and the avsc is `["null","string"]` with a null default, so it round-trips fine.
On the read side, `loadCompletionTimeIncrementally` goes through `loadAllInstantsFromFilesInRange`, which unlike `loadInstants` applies no per-record filter, so the record does reach `readCompletionTime`.
My guess at what went wrong the first time round: `LSMTimelineWriter#write` catches per-instant exceptions, and `prepareTimeline` passes `Option.empty()` as the handler, so a failed archive write leaves you asserting against an empty archive and everything looks green. That is what the `archiveFailures` assertion below is guarding against.
I kept it self-contained instead of widening `prepareTimeline`'s `Option.of`, so the existing tests are untouched:
```java
@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));
}
}
```
No new imports needed, and `getMockHoodieTable` is already in the class.
With this in, `TestCompletionTimeQueryViewV2` and its mock stubs can go, and `readCompletionTime` can go back to `private`. That closes the other two threads as well.
One aside while I was setting this up: the build needs a JDK that lombok 1.18.36 can instrument. On JDK 25 `hudi-io` dies with `TypeTag :: UNKNOWN`. JDK 11 matching `<java.version>` is fine.
##########
hudi-common/src/main/java/org/apache/hudi/common/table/timeline/versioning/v2/CompletionTimeQueryViewV2.java:
##########
@@ -302,9 +302,12 @@ private void load() {
.forEach(instant -> setCompletionTime(instant.requestedTime(), instant.getCompletionTime()));
}
- private void readCompletionTime(String instantTime, GenericRecord record) {
- final String completionTime = record.get(COMPLETION_TIME_ARCHIVED_META_FIELD).toString();
- setCompletionTime(instantTime, completionTime);
+ @VisibleForTesting
Review Comment:
The harness route does work after all -- see https://github.com/apache/hudi/pull/19452#discussion_r3701951807. With the regression test on the real read path this can go back to `private` and the `@VisibleForTesting` widening is not needed.
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(timeline): do not NPE on archived instants without a completion time [hudi]
Posted by "voonhous (via GitHub)" <gi...@apache.org>.
voonhous commented on code in PR #19452:
URL: https://github.com/apache/hudi/pull/19452#discussion_r3701952656
##########
hudi-common/src/test/java/org/apache/hudi/common/table/timeline/versioning/v2/TestCompletionTimeQueryViewV2.java:
##########
@@ -0,0 +1,94 @@
+/*
+ * 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.versioning.v2;
+
+import org.apache.hudi.avro.model.HoodieLSMTimelineInstant;
+import org.apache.hudi.common.table.HoodieTableMetaClient;
+import org.apache.hudi.common.table.timeline.HoodieActiveTimeline;
+import org.apache.hudi.common.table.timeline.HoodieTimeline;
+import org.apache.hudi.common.util.Option;
+
+import org.apache.avro.generic.GenericData;
+import org.apache.avro.generic.GenericRecord;
+import org.junit.jupiter.api.Test;
+
+import java.util.stream.Stream;
+
+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.junit.jupiter.api.Assertions.assertEquals;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+
+class TestCompletionTimeQueryViewV2 {
+
+ /**
+ * 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, as
+ * {@code setCompletionTime} already documents, 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} from this path.
+ */
+ @Test
+ void readCompletionTimeFallsBackWhenTheArchivedRecordHasNoCompletionTime() {
+ try (CompletionTimeQueryViewV2 view = new CompletionTimeQueryViewV2(mockMetaClientWithEmptyTimeline())) {
+ GenericRecord record = new GenericData.Record(HoodieLSMTimelineInstant.getClassSchema());
+ record.put(INSTANT_TIME_ARCHIVED_META_FIELD, "00000001");
+ record.put(ACTION_ARCHIVED_META_FIELD, "commit");
+ // completionTime deliberately left unset
+
+ view.readCompletionTime("00000001", record);
+
+ assertEquals(Option.of("00000001"), view.getCompletionTime("00000001"),
+ "An archived instant without a completion time should fall back to its instant time");
+ }
+ }
+
+ @Test
+ void readCompletionTimeUsesTheArchivedCompletionTimeWhenPresent() {
+ try (CompletionTimeQueryViewV2 view = new CompletionTimeQueryViewV2(mockMetaClientWithEmptyTimeline())) {
+ GenericRecord record = new GenericData.Record(HoodieLSMTimelineInstant.getClassSchema());
+ record.put(INSTANT_TIME_ARCHIVED_META_FIELD, "00000001");
+ record.put(ACTION_ARCHIVED_META_FIELD, "commit");
+ record.put(COMPLETION_TIME_ARCHIVED_META_FIELD, "00001001");
+
+ view.readCompletionTime("00000001", record);
+
+ assertEquals(Option.of("00001001"), view.getCompletionTime("00000001"));
+ }
+ }
+
+ private static HoodieTableMetaClient mockMetaClientWithEmptyTimeline() {
Review Comment:
Same thread of reasoning as https://github.com/apache/hudi/pull/19452#discussion_r3701951807 -- the real-path test does reproduce the NPE, so `TestCompletionTimeQueryViewV2` and these stubs can go entirely rather than the brittleness being left to a follow-up.
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org