Skip to content
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

Closed
teh-cmc opened this issue Jan 16, 2024 · 0 comments · Fixed by #5534
Closed

TimeInt::BEGINNING vs. TimeInt::MIN vs. Option<TimeInt> #4832

teh-cmc opened this issue Jan 16, 2024 · 0 comments · Fixed by #5534
Assignees
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
Copy link
Member

teh-cmc commented Jan 16, 2024

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 is i64::MIN / 2 ⚠️), if there's no timeline associated, is sometimes considered to refer to timeless data.
  • TimeInt::BEGINNING (which is i64::MIN / 2 ⚠️), if there is a timeline associated, is just a very negative timestamp, I think..?
  • TimeInt::MIN (which is i64::MIN), if there's no timeline associated, is sometimes considered to refer to timeless data.
  • TimeInt::MIN (which is i64::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 and TimeInt 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

  1. timeless data to always be refered to using a time: Option<TimeInt> = None value, which is always unambiguous, irrelevant of the surrounding context; or
  2. timeless data to always be refered to using a constant TimeInt value but A) it has to be the same everywhere and B) we must prevent code from instantiating that special TimeInt value in a timeful context.
  1. is probably more realistic in practice and would also simplify a lot of the code e.g. in the cache.

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.

@teh-cmc 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 teh-cmc self-assigned this Mar 14, 2024
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
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant