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

Remove use of into_arrow2_buffer in generated code #8731

Merged
merged 3 commits into from
Jan 20, 2025

Conversation

emilk
Copy link
Member

@emilk emilk commented Jan 19, 2025

@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 19, 2025
@emilk
Copy link
Member Author

emilk commented Jan 19, 2025

@rerun-bot full-check

@emilk emilk changed the title Remove warning about unused variable Remove use of into_arrow2_buffer in generated code Jan 19, 2025
Copy link

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

Copy link

github-actions bot commented Jan 19, 2025

Web viewer failed to build.

| Result | Commit | Link | Manifest |
| ------ | ------- | ----- |
| ❌ | | https://rerun.io/viewer/pr/8731 | +nightly +main |

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

@teh-cmc teh-cmc self-requested a review January 20, 2025 08:58
Comment on lines 629 to 639
// Offsets is always non-empty. The last element is the total length of buffer we need.
#[allow(clippy::unwrap_used)]
let capacity = offsets.last().copied().unwrap() as usize;

let mut buffer_builder = arrow::array::builder::BufferBuilder::<u8>::new(capacity);
// NOTE: Flattening to remove the guaranteed layer of nullability: we don't care
// about it while building the backing buffer since it's all offsets driven.
let inner_data: arrow::buffer::Buffer =
#data_src.into_iter().flatten() #quoted_transparent_mapping.collect();
for data in #data_src.iter().flatten() {
buffer_builder.append_slice(data #quoted_member_accessor.as_bytes());
}
let inner_data: arrow::buffer::Buffer = buffer_builder.finish();
Copy link
Member

Choose a reason for hiding this comment

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

I'm surprised we can't just use a collect() here and below, Buffer seems to implement `FromIterator?

Copy link
Member Author

Choose a reason for hiding this comment

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

We can, but I wanted to pre-allocate (reserve) the exact amount of bytes we need.

I'll add a comment explaining this

@emilk emilk closed this Jan 20, 2025
@emilk emilk reopened this Jan 20, 2025
@emilk emilk merged commit 501cb12 into main Jan 20, 2025
28 of 29 checks passed
@emilk emilk deleted the emilk/remove-into_arrow2_buffer branch January 20, 2025 12:00
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