Skip to content

Conversation

grtlr
Copy link
Contributor

@grtlr grtlr commented Jul 4, 2025

Related

Important

This PR does not remove all occurrences of indicators from our code base, but I want to keep PRs small. Because of this I'm not closing #8129 yet.

What

This PR does several things, it:

  • stops indicator from being logged in all SDKs
  • drops indicators (and columns with rerun:is_indicator) during migration
  • adds a note to the multiple_scalars snippet
  • adds a rerun rrd compare --ignore-chunks-without-components option for snippet comparison.
    • Please review, or better test carefully!

TODO

  • carefully think through sandbox implications
    • For now, we don't handle any logic changes for indicators, this will be done in a follow up PR.
  • dataplatform companion PR
  • full-check (unrelated error via @Wumpf)

@grtlr grtlr added include in changelog 🔩 data model Sorbet 🪵 Log & send APIs Affects the user-facing API for all languages labels Jul 4, 2025
Copy link

github-actions bot commented Jul 4, 2025

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

  • I have tested the web viewer
Result Commit Link Manifest
50c96f1 https://rerun.io/viewer/pr/10521 +nightly +main

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

@Wumpf Wumpf self-requested a review July 4, 2025 15:40
Copy link
Member

@Wumpf Wumpf left a comment

Choose a reason for hiding this comment

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

oh-my-god-its-happening

awesome, and it's just the beginning 😄

Comment on lines 125 to +126
batch = maybe_apply::<v0_0_2__to__v0_1_0::Migration>(&batch_version, batch);
batch = maybe_apply::<v0_1_0__to__v0_1_1::Migration>(&batch_version, batch);
Copy link
Member

Choose a reason for hiding this comment

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

so is that an intra 0.24alpha port then? doesn't seem to hurt, indeed very nice to have these smaller, but I figure but maybe worth pointing out somewhere (unless I missunderstand)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, we have substantial amount of data out there that still uses indicators but is on 0.1.0. Without the bump we could not migrate it with our current logic.

use re_log_types::example_components::{MyLabel, MyPoints};

use super::*;

struct DisplayDescrs(Chunk);
Copy link
Member

Choose a reason for hiding this comment

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

nice, cool construct for snapshot testing. Wasn't familiar with that pattern

@grtlr grtlr force-pushed the grtlr/indicators-be-gone-pt2 branch 3 times, most recently from 0b68491 to 1500c7d Compare July 7, 2025 14:02
@grtlr
Copy link
Contributor Author

grtlr commented Jul 7, 2025

@rerun-bot full-check

Copy link

github-actions bot commented Jul 7, 2025

grtlr added a commit that referenced this pull request Jul 7, 2025
### Related

* Required for #10521.

### What

This PR prevents static chunks _without components_ to enter the chunk
store.

> A static chunk without any components would be dangling from the
start.
> Our dangling logic retrieves relevant chunks by component descriptors,
> so it would never be able to find it. So we never add that chunk in
the
> first place.

Before this PR, the following code snippet would cause a `debug_assert!`
to be hit.

```rs
let chunk = Chunk::builder(entity_path.clone())
    .with_component_batches(RowId::new(), TimePoint::STATIC, [])
    .build()?;
let chunk = Arc::new(chunk);

let events = store.insert_chunk(&chunk)?;
```

Also, that `chunk` would never get free'd up by our logic for finding
dangling rows.

#### More considerations

Another option would be to emit both `Addition` and `Deletion` events in
the same `insert`. Finally a last question would be do we want the
chunk's entity path to show up in
`ChunkStore::static_chunk_ids_per_entity`.
@grtlr grtlr force-pushed the grtlr/indicators-be-gone-pt2 branch from 9ae497a to b222ccb Compare July 7, 2025 14:51
@grtlr
Copy link
Contributor Author

grtlr commented Jul 7, 2025

@rerun-bot full-check

Copy link

github-actions bot commented Jul 7, 2025

@grtlr grtlr merged commit a6f8cb2 into main Jul 7, 2025
89 of 94 checks passed
@grtlr grtlr deleted the grtlr/indicators-be-gone-pt2 branch July 7, 2025 16:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🔩 data model Sorbet include in changelog 🪵 Log & send APIs Affects the user-facing API for all languages
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants