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.

Sign up for GitHub

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

Jump to bottom

Use Arrow IPC to encode the column schema #8821

Merged
merged 13 commits into from
Jan 28, 2025
Merged

Use Arrow IPC to encode the column schema #8821

merged 13 commits into from
Jan 28, 2025

Conversation

emilk
Copy link
Member

@emilk emilk commented Jan 27, 2025 •
edited
Loading

@emilk emilk added 🏹 arrow concerning arrow 🚜 refactor Change the code, not the functionality dataplatform Rerun Data Platform integration labels Jan 27, 2025
@github-actions GitHub Actions
Copy link

github-actions bot commented Jan 27, 2025 •
edited
Loading

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

  • I have tested the web viewer
Result Commit Link Manifest
✅ 1ccc32a https://rerun.io/viewer/pr/8821 +nightly +main

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

@emilk emilk added the exclude from changelog PRs with this won't show up in CHANGELOG.md label Jan 27, 2025
@emilk emilk marked this pull request as ready for review January 27, 2025 13:27
jleibs
jleibs approved these changes Jan 27, 2025
View reviewed changes
crates/store/re_protos/proto/rerun/v0/remote_store.proto Outdated Show resolved Hide resolved
crates/store/re_sorbet/src/ipc.rs Show resolved Hide resolved
@emilk
Copy link
Member Author

emilk commented Jan 28, 2025

@rerun-bot full-check

@github-actions GitHub Actions
Copy link

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

zehiko
zehiko reviewed Jan 28, 2025
View reviewed changes
crates/store/re_sorbet/src/ipc.rs
};

/// Encode an arrow schema as IPC bytes.
pub fn ipc_from_schema(schema: &ArrowSchema) -> Result<Vec<u8>, ArrowError> {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: in re_log_types we've mostly been using encode and decode and their variants, I wonder if for consistency we should do the same here? perhaps ipc_encode or ipc_encode_schema, rather than "creating" ipc from something.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good feedback, but I'll do it in a future PR

@emilk emilk merged commit fd5628e into main Jan 28, 2025
66 checks passed
@emilk emilk deleted the emilk/schema-over-ipc branch January 28, 2025 11:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zehiko zehiko zehiko left review comments

@jleibs jleibs jleibs approved these changes

Assignees
No one assigned
Labels
🏹 arrow concerning arrow dataplatform Rerun Data Platform integration exclude from changelog PRs with this won't show up in CHANGELOG.md 🚜 refactor Change the code, not the functionality
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@emilk @jleibs @zehiko