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

Port TransportChunk to arrow-rs #8700

Merged
merged 7 commits into from
Jan 16, 2025
Merged

Port TransportChunk to arrow-rs #8700

merged 7 commits into from
Jan 16, 2025

Conversation

emilk
Copy link
Member

@emilk emilk commented Jan 15, 2025

This makes TransportChunk a wrapper around an arrow RecordBatch.

Future work

  • Remove TransportChunk and replace it with an extension trait on RecordBatch
  • Simplify the dataframe API to always return a full RecordBatch (adding a schema to the rows is basically free now)

@emilk emilk added 🏹 arrow concerning arrow 🚜 refactor Change the code, not the functionality exclude from changelog PRs with this won't show up in CHANGELOG.md labels Jan 15, 2025
Copy link

github-actions bot commented Jan 15, 2025

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

  • I have tested the web viewer
Result Commit Link Manifest
4073a38 https://rerun.io/viewer/pr/8700 +nightly +main

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

@emilk emilk changed the base branch from main to emilk/full-table-format-test January 15, 2025 15:56
Base automatically changed from emilk/full-table-format-test to main January 15, 2025 16:08
@emilk emilk force-pushed the emilk/record-batch branch from dee88f0 to 3bb1a96 Compare January 15, 2025 16:09
@emilk emilk force-pushed the emilk/record-batch branch from 3bb1a96 to 84a06f4 Compare January 15, 2025 16:29
.get(Self::CHUNK_METADATA_KEY_HEAP_SIZE_BYTES)
.and_then(|s| s.parse::<u64>().ok())
}

#[inline]
pub fn schema(&self) -> ArrowSchemaRef {
Copy link
Member Author

Choose a reason for hiding this comment

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

These and some other methods (num_rows, num_columns, etc) are removed because we get them for free with Deref<Target = ArrowRecordBatch>

@emilk
Copy link
Member Author

emilk commented Jan 16, 2025

@rerun-bot full-check

@emilk emilk marked this pull request as ready for review January 16, 2025 07:37
Copy link

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

@emilk
Copy link
Member Author

emilk commented Jan 16, 2025

@rerun-bot full-check

Copy link

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

@emilk emilk mentioned this pull request Jan 14, 2025
19 tasks
@teh-cmc teh-cmc self-requested a review January 16, 2025 08:19
Comment on lines 1264 to 1266
///
/// Only use this if you absolutely need a [`RecordBatch`] as this adds a lot of allocation
/// overhead.
Copy link
Member

Choose a reason for hiding this comment

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

That comment still applies.

@emilk emilk merged commit e99a507 into main Jan 16, 2025
7 of 8 checks passed
@emilk emilk deleted the emilk/record-batch branch January 16, 2025 09:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏹 arrow concerning arrow exclude from changelog PRs with this won't show up in CHANGELOG.md 🚜 refactor Change the code, not the functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants