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

Implement support for opt-in eagerly-serialized archetypes #8646

Open
wants to merge 1 commit into
base: cmc/eager_partial_2_tagged_from_to_arrow
Choose a base branch
from

Conversation

teh-cmc
Copy link
Member

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

This introduces a new attr.rust.archetype_eager codegen attribute. When toggled, the associated archetype will now only carry raw Arrow data around, and go through the new eager logging APIs.

The attribute has been set on Points3D:
image

Legacy and eagerly-serialized archetypes can co-exist, making it possible to migrate everything incrementally.

Copy link

github-actions bot commented Jan 10, 2025

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

  • I have tested the web viewer
Result Commit Link Manifest
15b27bd https://rerun.io/viewer/pr/8646 +nightly +main

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

@teh-cmc teh-cmc marked this pull request as ready for review January 10, 2025 16:15
@teh-cmc teh-cmc added 🦀 Rust API Rust logging API do-not-merge Do not merge this PR 🍏 primitives Relating to Rerun primitives codegen/idl exclude from changelog PRs with this won't show up in CHANGELOG.md labels Jan 10, 2025
@emilk emilk self-requested a review January 10, 2025 17:39
Copy link
Member

@emilk emilk left a comment

Choose a reason for hiding this comment

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

BEAUTIFUL ⭐⭐⭐⭐⭐

}
};

// TODO(cmc): This goes away once all archetypes have been made eager.
Copy link
Member

Choose a reason for hiding this comment

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

nit: maybe link to the relevant issue here


let (_, unwrapped) = quote_field_type_from_typ(&field.typ, true);
if unwrapped {
// This was originally a vec/array!
Copy link
Member

Choose a reason for hiding this comment

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

This comment confused me for a bit. What you seem to mean is that this component is defines as being a many-component (as opposed to being a mono-component)?

Copy link
Member

Choose a reason for hiding this comment

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

same below

Comment on lines +329 to +333
let keypoint_ids = arrays_by_descr
.get(&Self::descriptor_keypoint_ids())
.map(|array| {
SerializedComponentBatch::new(array.clone(), Self::descriptor_keypoint_ids())
});
Copy link
Member

Choose a reason for hiding this comment

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

It's so much shorter and beautiful ❤️

///
/// # Fallibility
///
/// There are very few ways in which serialization can fail, all of which are very rare to hit
Copy link
Member

Choose a reason for hiding this comment

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

It would be good to at least mention the few failure cases (e.g. overflow i32)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
codegen/idl do-not-merge Do not merge this PR exclude from changelog PRs with this won't show up in CHANGELOG.md 🍏 primitives Relating to Rerun primitives 🦀 Rust API Rust logging API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants