Skip to content

ComponentBatch doesn't implement AsComponents anymore #8820

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

Merged
merged 7 commits into from
Jan 28, 2025

Conversation

teh-cmc
Copy link
Member

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

This makes it impossible to pass something implementing ComponentBatch (i.e. some native data that knows how to serialized itself into arrow component data) directly to RecordingStream::log.

This forces the caller to either..:

  • explicitly opt into the low-level / component-level APIs via my_data.serialized(), which expose further custom-tagging facilities, or
  • use the archetype-level APIs instead, which handle tagging automatically.

Similarly, this removes all the legacy support infrastructure that was needed to carry ComponentBatches all the way to the logger:

  • log_component_batches
  • AsComponents::as_component_batches
  • ComponentBatchCow, ComponentBatchCowWithDescriptor, ...
  • etc

I think this is all that's needed in order to..:

At the very least, it makes sure that existing code that used to log ComponentBatches directly breaks at compile-time.

Copy link

github-actions bot commented Jan 27, 2025

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

  • I have tested the web viewer
Result Commit Link Manifest
81bac95 https://rerun.io/viewer/pr/8820 +nightly +main

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

@teh-cmc
Copy link
Member Author

teh-cmc commented Jan 27, 2025

@rerun-bot full-check

@teh-cmc teh-cmc marked this pull request as ready for review January 27, 2025 15:45
Copy link

Started a full build: https://github.com/rerun-io/rerun/actions/runs/12992792501

@teh-cmc
Copy link
Member Author

teh-cmc commented Jan 27, 2025

@rerun-bot full-check

Copy link

Started a full build: https://github.com/rerun-io/rerun/actions/runs/12994028275

Base automatically changed from cmc/python_columnar_cleanup to main January 28, 2025 11:08
@teh-cmc teh-cmc force-pushed the cmc/no_more_dyn_batch branch from 410df5b to 81bac95 Compare January 28, 2025 11:18
@Wumpf Wumpf self-requested a review January 28, 2025 11:59
@teh-cmc teh-cmc removed the do-not-merge Do not merge this PR label Jan 28, 2025
@Wumpf
Copy link
Member

Wumpf commented Jan 28, 2025

Have something very very similar in progress in C++, so looks like we're aligned on how to go about this!

@teh-cmc teh-cmc merged commit 424cf2e into main Jan 28, 2025
33 of 37 checks passed
@teh-cmc teh-cmc deleted the cmc/no_more_dyn_batch branch January 28, 2025 12:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enforce usage of tagged logging APIs: Rust
2 participants