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

Forbid compaction of tagged+untagged data for a single component #8782

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

teh-cmc
Copy link
Member

@teh-cmc teh-cmc commented Jan 22, 2025

This implements the second solution described in #8760 (comment):

I knew I was forgetting yet another subtle complication: maintaining an untagged index is not enough, you can still end up in a situation where a single Chunk has both untagged and tagged data for a component, and no index is gonna save you there.
This is what's happening here (see attached screenshot). if i had to guess, this is because a runtime blueprint write ends up compacted in a pre-existing, tagged blueprint chunk, and now the resulting chunk is both tagged and untagged for that component.

Once we're done with all the API updates on the SDK side, it shouldn't ever be possible for a user to end up in that situation when working in new recording, so that end is covered.
That leaves A) runtime blueprint writes and B) user writes to a pre-existing, legacy recording. Obviously the correct fix for blueprint writes is to port all of them to tagged APIs, but A) that will not happen for 0.22 and B) that doesn't take care of the other problem.

I see two possible avenues here:

  • Modify the compaction logic so that when tagged data is compacted with untagged data of the same component, we merge them together and keep the tags going forward. This should be well-specified with the current data model, and helps with propagating tags going forward.
  • Modify the compaction logic so that tagged and untagged data of the same component is never compacted together.

I can confirm this fixes the "changing view origin crashes in debug" issue.

@teh-cmc teh-cmc added 🪳 bug Something isn't working 🏹 arrow concerning arrow ⛃ re_datastore affects the datastore itself exclude from changelog PRs with this won't show up in CHANGELOG.md 🔩 data model labels Jan 22, 2025
Copy link

github-actions bot commented Jan 22, 2025

Web viewer built successfully. If applicable, you should also test it:

  • I have tested the web viewer
Result Commit Link Manifest
6a40798 https://rerun.io/viewer/pr/8782 +nightly +main

Note: This comment is updated whenever you push a commit.

@teh-cmc
Copy link
Member Author

teh-cmc commented Jan 22, 2025

Before:
image

After:
image
image

@teh-cmc
Copy link
Member Author

teh-cmc commented Jan 22, 2025

@rerun-bot full-check

Copy link

Copy link
Member

@abey79 abey79 left a comment

Choose a reason for hiding this comment

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

Makse sense to me 👍🏻

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏹 arrow concerning arrow 🪳 bug Something isn't working 🔩 data model exclude from changelog PRs with this won't show up in CHANGELOG.md ⛃ re_datastore affects the datastore itself
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants