[Kernel] Add support for ICT-based time travel (part 1) #4581
Conversation
dhruvarya-db
requested review from
ryan-johnson-databricks,
vkorukanti and
prakharjain09
dhruvarya-db
changed the title
|
@scovich I have copied over your comments from https://github.com/delta-io/delta/pull/4483/files#diff-07b47ed7d50294a001f579299ebd0e7d2328a991babcf6ba23af3c9cacdf8cdcR182 to this PR. This PR just focuses on the binary search part. I will add the exponential search incrementally to make the reviews simpler. |
dhruvarya-db
removed request for
vkorukanti,
ryan-johnson-databricks and
prakharjain09
| if (ictEnablementCommit.getTimestamp() <= timestamp) { | ||
| // The target commit is in the ICT range. | ||
| long latestSnapshotTimestamp = latestSnapshot.getTimestamp(engine); | ||
| if (latestSnapshotTimestamp <= timestamp) { |
There was a problem hiding this comment.
https://github.com//pull/4483/files#r2101187752
There was a problem hiding this comment.
Added a comment that explains this: https://github.com/delta-io/delta/pull/4581/files#diff-07b47ed7d50294a001f579299ebd0e7d2328a991babcf6ba23af3c9cacdf8cdcR91
| searchResult = | ||
| lastCommitBeforeOrAtTimestamp(commits, timestamp) | ||
| .orElse( | ||
| commits.get(0)); // This is only returned if canReturnEarliestCommit (see below) |
There was a problem hiding this comment.
https://github.com//pull/4483/files#r2101190894
There was a problem hiding this comment.
This is due to the formatter. I can't find a way to undo this.
| * @return An optional which contains a tuple containing the index and the value of the greatest | ||
| * lower bound when found, or an empty optional if not found. | ||
| */ | ||
| public static Optional<Tuple2<Long, Long>> greatestLowerBound( |
There was a problem hiding this comment.
https://github.com//pull/4483/files#r2101200771
There was a problem hiding this comment.
@scovich I have updated the implementation so that it is more explicit about when None is returned. I have also added explicit test cases for greatestLowerBound
dhruvarya-db
force-pushed
the
ict-time-travel-1
branch
from
a347fa1 to
c12d9a6
Compare
| + "based lookup. This can happen when the commit log is corrupted or when " | ||
| + "there is a parallel operation like metadata cleanup that is deleting " | ||
| + "commits. Please retry the query.", |
There was a problem hiding this comment.
Maybe skip the corruption mention? Metadata cleanup is by far the more likely scenario, because Delta clients don't honor the configured data retention window when resolving time travel.
There was a problem hiding this comment.
Makes sense, updated.
| if (enablementTimestampOpt.isPresent() && enablementVersionOpt.isPresent()) { | ||
| return new Commit(enablementVersionOpt.get(), enablementTimestampOpt.get()); | ||
| } else if (!enablementTimestampOpt.isPresent() && !enablementVersionOpt.isPresent()) { | ||
| // This means that ICT has been enabled for the entire history. |
There was a problem hiding this comment.
can you help me to understand why this is the case? If my table doesn't support ICT --> then won't these fields be empty?
There was a problem hiding this comment.
In this case we have already established that ICT is enabled on the table (L209). When ICT is enabled, this fields being empty indicates that ICT has been enabled for all of available history.
| * Get the version of the checkpoint, checksum or delta file. Returns an empty optional if the | ||
| * file is not a checkpoint, checksum or delta file. | ||
| */ | ||
| public static Optional<Long> getFileVersionOpt(Path path) { |
There was a problem hiding this comment.
why this change? I don't see any usages of getFileVersionOpt --> maybe I'm just missing it?
There was a problem hiding this comment.
My bad, this is not need for part 1. Removed.
dhruvarya-db
force-pushed
the
ict-time-travel-1
branch
from
e45ebb5 to
3c5fb63
Compare
Which Delta project/connector is this regarding?
Description
Adds initial support for performing ICT-based time travel. More specifically, this PR:
How was this patch tested?
Added test cases to DeltaHistoryManagerSuite
Does this PR introduce any user-facing changes?
No
Resolves: #4294