-
Notifications
You must be signed in to change notification settings - Fork 331
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
TimeInt::BEGINNING
vs. TimeInt::MIN
vs. Option<TimeInt>
#4832
Labels
😤 annoying
Something in the UI / SDK is annoying to use
🪳 bug
Something isn't working
🔩 data model
📖 documentation
Improvements or additions to documentation
⛃ re_datastore
affects the datastore itself
🔍 re_query
affects re_query itself
Comments
teh-cmc
added
🪳 bug
Something isn't working
📖 documentation
Improvements or additions to documentation
⛃ re_datastore
affects the datastore itself
😤 annoying
Something in the UI / SDK is annoying to use
🔍 re_query
affects re_query itself
🔩 data model
labels
Jan 16, 2024
teh-cmc
added a commit
that referenced
this issue
Jan 16, 2024
teh-cmc
added a commit
that referenced
this issue
Jan 17, 2024
teh-cmc
added a commit
that referenced
this issue
Jan 18, 2024
teh-cmc
added a commit
that referenced
this issue
Jan 23, 2024
teh-cmc
added a commit
that referenced
this issue
Jan 23, 2024
Simply add a timeless path for the range cache, and actually only iterate over the range the user asked for (we were still blindly iterating over everything until now). Also some very minimal clean up related to #4832, but we have a long way to go... - #4832 --- - Fixes #4821 --- Part of the primary caching series of PR (index search, joins, deserialization): - #4592 - #4593 - #4659 - #4680 - #4681 - #4698 - #4711 - #4712 - #4721 - #4726 - #4773 - #4784 - #4785 - #4793 - #4800 - #4851 - #4852 - #4853 - #4856
teh-cmc
added a commit
that referenced
this issue
Apr 5, 2024
_Commits make no sense, review the final changelog directly._ _All the interesting bits happen in `re_log_types/time_point` & `re_sdk` -- everything else is just change propagation._ - `TimeInt` now ranges from `i64:MIN + 1` to `i64::MAX`. - `TimeInt::STATIC`, which takes the place of the now illegal `TimeInt(i64::MIN)`, is now _the only way_ of identifying static data. - It is impossible to create `TimeInt::STATIC` inadvertently -- users of the SDK cannot set the clock to that value. - Similarly, it is impossible to create a `TimeRange`, a `TimePoint`, a `LatestAtQuery` or a `RangeQuery` that includes `TimeInt::STATIC`. If static data exists, that's what will be returned, unconditionally -- there's no such thing as querying for it explicitely. - `TimePoint::timeless` is gone -- we already have `TimePoint::default` that we use all over the place, we don't need two ways of doing the same thing. There still exists a logical mapping between an empty `TimePoint` and static data, as that is how one represents static data on the wire -- terminology wise: "a timeless timepoint results in static data". Similar to the "ensure `RowId`s are unique" refactor from back when, this seemingly tiny change on the surface will vastly simplify downstream code that finally has some invariants to rely on. - Fixes #4832 - Related to #5264 --- Part of a PR series that removes the concept of timeless data in favor of the much simpler concept of static data: - #5534 - #5535 - #5536 - #5537 - #5540
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Labels
😤 annoying
Something in the UI / SDK is annoying to use
🪳 bug
Something isn't working
🔩 data model
📖 documentation
Improvements or additions to documentation
⛃ re_datastore
affects the datastore itself
🔍 re_query
affects re_query itself
We currently have many different systems that treat these values differently depending on the surrounding context, and the situation is becoming quite hard to reason about.
Here's a few I've faced on my way to implementing the primary cache:
TimeInt::BEGINNING
(which isi64::MIN / 2
TimeInt::BEGINNING
(which isi64::MIN / 2
TimeInt::MIN
(which isi64::MIN
), if there's no timeline associated, is sometimes considered to refer to timeless data.TimeInt::MIN
(which isi64::MIN
), if there is a timeline associated, is just a very negative timestamp, I think..? Might be interpreted as timeless in some contexts, it's hard to tell.Option<TimeInt>
always unequivocally refers to timeless data.In addition, a lot of our systems will work interchangeably with arbitrary
i64
andTimeInt
values which makes things even harder to reason about.Also, special
TimeInt
values are sometimes interpreted differently when reading vs. writing data...There are even more subtleties the deeper you dig, but you get the idea.
Ideally I would want either
time: Option<TimeInt> = None
value, which is always unambiguous, irrelevant of the surrounding context; orTimeInt
value but A) it has to be the same everywhere and B) we must prevent code from instantiating that specialTimeInt
value in a timeful context.In the meantime, I've tried to at least make everything use the same constant, but this resulted in a bunch of failed tests all over the place and I don't have time to look into this now.
The text was updated successfully, but these errors were encountered: