Skip to content

feat(common): query LAYOUT_VERSION_1 timelines in CompletionTimeQueryView by using instant time as completion time - #19338

Open
fhan688 wants to merge 1 commit into
apache:masterfrom
fhan688:query-adaption-for-pre-8-version
Open

feat(common): query LAYOUT_VERSION_1 timelines in CompletionTimeQueryView by using instant time as completion time#19338
fhan688 wants to merge 1 commit into
apache:masterfrom
fhan688:query-adaption-for-pre-8-version

Conversation

@fhan688

@fhan688 fhan688 commented Jul 21, 2026

Copy link
Copy Markdown
Contributor

Describe the issue this Pull Request addresses

The 1.x incremental read stack (IncrementalQueryAnalyzerCompletionTimeQueryView.getInstantTimes) resolves the query range by completion time. For a LAYOUT_VERSION_1 timeline (table versions 5–7, releases 0.12–0.16), CompletionTimeQueryViewV1.getInstantTimes currently throws:

RuntimeException("Incremental query view for timeline version 1 not yet implemented")

As a result, any incremental (and streaming) read against these older tables via the 1.x reader fails outright.

The key observation — as raised in review — is that this is not a "missing completion time" problem: for a LAYOUT_VERSION_1 timeline, an instant's completion time is its instant (request) time, and the semantics are backward compatible. This is already how the V1 view behaves elsewhere (load() maps requestedTime → completionTime, and getCompletionTime returns the begin time for archived instants). This PR applies that same equivalence to the range query, so V1 falls out as the "completion time = instant time" special case of the existing V2 range logic rather than a separate implementation.

Summary and Changelog

Implements CompletionTimeQueryViewV1.getInstantTimes(...) by mirroring the branch structure already used in CompletionTimeQueryViewV2, driven by requested (instant) time instead of a persisted completion time:

  • (_, end] — returns the single latest instant whose requested time is <= end.
  • ['earliest', _) — clears the start bound so it degenerates to consuming from the earliest instant.
  • (_, _) — returns the latest snapshot instant.
  • otherwise — filters instants through a shared InstantRange (honoring the OPEN_CLOSED / CLOSED_CLOSED bounds and nullable boundaries), sorted ascending.

Notes:

  • The candidate instants are taken from the HoodieTimeline passed in by the caller (already filtered per user configs such as skipCompaction / skipClustering), consistent with the V2 code path.
  • The archived timeline is intentionally not consulted for V1 (it is not loaded), which is the only V1-specific boundary; the range/resume semantics are the same logic as V2, not a fork. The earliestInstantTimeFunc parameter is unused for V1 (it only serves V2's archive lazy-load boundary).

Tests: adds TestCompletionTimeQueryViewV1 covering closed/closed first read, open/closed streaming resume (excluding the last issued offset), earliest start, end-only, and no-bounds latest-snapshot cases.

No code was copied verbatim; the implementation follows the shape of CompletionTimeQueryViewV2#getInstantTimes.

Impact

  • Enables incremental and streaming reads on table versions 5–7 (LAYOUT_VERSION_1) through the 1.x reader; these previously threw.
  • No behavior change for table version ≥ 8 (the V2 view is untouched).
  • No public API change: only the previously-throwing package-private getInstantTimes body is implemented.

Risk Level

Medium. This changes read semantics for older tables from "throws" to "returns instants". Mitigated by:

  • New unit tests exercising the range and streaming-resume boundaries (OPEN_CLOSED must exclude the issued offset, CLOSED_CLOSED first read includes the start commit, earliest / end-only / no-bounds).
  • Verified by compiling hudi-common and running TestCompletionTimeQueryViewV1 locally — 5/5 tests pass.

Documentation Update

None. No new config, and no user-facing option changes; this makes an existing documented feature (incremental/streaming query) work on LAYOUT_VERSION_1 tables.

Contributor's checklist

  • Read through contributor's guide
  • Enough context is provided in the sections above
  • Adequate tests were added if applicable

…View by using instant time as completion time
@github-actions github-actions Bot added the size:M PR with lines of changes in (100, 300] label Jul 21, 2026
@hudi-bot

Copy link
Copy Markdown
Collaborator

CI report:

Bot commands @hudi-bot supports the following commands:
  • @hudi-bot run azure re-run the last Azure build

@hudi-agent hudi-agent left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ 🤖 This review was generated by an AI agent and may contain mistakes. Please verify any suggestions before applying.

test. One minor readability nit on parameter mutation in the new method body.

rangeStart = Option.empty();
}

if (rangeStart.isEmpty() && rangeEnd.isEmpty()) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 nit: could you avoid mutating the rangeStart parameter here? When a reader reaches the second if (rangeStart.isEmpty() && rangeEnd.isEmpty()) check they have to mentally track whether the earlier reassignment fired. Something like Option<String> effectiveStart = startFromEarliest ? Option.empty() : rangeStart; introduced upfront, then used in the downstream checks and the builder, would make the control flow self-evident.

⚠️ AI-generated; verify before applying. React 👍/👎 to flag quality.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

Function<String, String> earliestInstantTimeFunc) {
throw new RuntimeException("Incremental query view for timeline version 1 not yet implemented");
final boolean startFromEarliest = START_COMMIT_EARLIEST.equalsIgnoreCase(rangeStart.orElse(null));
HoodieTimeline completedTimeline = timeline.filterCompletedInstants();

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This turns the archived-offset case from a hard failure into a silent skip. Everything below only sees the active timeline, so if a streaming job was down long enough for archival to pass its resume offset, the commits between rangeStart and the first active instant just vanish from the read. The V2 view covers that window by lazily loading the archive; the old RuntimeException here at least made the gap loud. (Same story for the (_, end] branch: V2 falls back to the archived map, this returns empty.)

The view already has isArchived(), so a cheap guard keeps the fail-stop semantics:

if (rangeStart.isPresent() && !startFromEarliest && isArchived(rangeStart.get())) {
  throw new HoodieException("Start instant " + rangeStart.get()
      + " is already archived; incremental read on a LAYOUT_VERSION_1 timeline does not consult the archived timeline");
}

Reading the archived timeline would be even better (analyze() already handles archived instants via getArchivedReadTimeline), but at minimum a loud failure beats silent data loss on resume.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:M PR with lines of changes in (100, 300]

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants