-
Notifications
You must be signed in to change notification settings - Fork 81
feat: Add high level api for timestamp conversion #900
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
feat: Add high level api for timestamp conversion #900
Conversation
ced5735
to
1c16df5
Compare
1c16df5
to
bef29a9
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #900 +/- ##
==========================================
- Coverage 84.68% 84.66% -0.02%
==========================================
Files 92 94 +2
Lines 23015 23581 +566
Branches 23015 23581 +566
==========================================
+ Hits 19491 19966 +475
- Misses 2563 2598 +35
- Partials 961 1017 +56 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
896fa67
to
c4e66d5
Compare
2ffb953
to
6327a89
Compare
aaf4cf7
to
ecc22d7
Compare
kernel/src/history_manager/mod.rs
Outdated
log_segment: LogSegment, | ||
snapshot: Arc<Snapshot>, | ||
commit_to_timestamp_cache: RefCell<HashMap<Url, Timestamp>>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does a cache belong in kernel?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I wondered about that as well. I decided to go with caching because the cache is c*O(size of log segment). And the constant c is rly small.
kernel/src/table.rs
Outdated
/// - `limit`: Optional maximum number of versions to track. When specified, the earliest | ||
/// queryable version will be `snapshot.version() - limit`. This allows trading | ||
/// memory usage for historical reach. | ||
pub fn history_manager_from_snapshot( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
considering we have two constructors here already and we have optional info in each of them, would it make more sense to just have a builder?
let hm = HistoryManagerBuilder::new(engine)
.withSnapshot(snapshot)
.withLimit(1_000)
.build()
aside: I wonder if it's more idiomatic to have engine in new
or build
..?
and should we include end version?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now all functions are static, so we avoid this :)
758166e
to
b8a55ae
Compare
90933a6
to
b32b5ec
Compare
b32b5ec
to
dc718fe
Compare
dc718fe
to
225da5f
Compare
225da5f
to
13f0369
Compare
What changes are proposed in this pull request?
This is a stacked PR. Please look at the LAST commit of each PR for the files changed.
This PR introduces high level apis to perform timestamp to version conversion such as
latest_version_as_of
,first_version_after
, andtimestamp_range_to_versions
.How was this change tested?
The range api is tested on a table generated by Delta Kernel Java. Several timestamp to version conversion queries are performed to verify that range timestamp queries work.