Skip to content

feat: rest of BinaryViewVector{,Mut}#5133

Merged
a10y merged 7 commits intodevelopfrom
aduffy/bin-vector2
Nov 3, 2025
Merged

feat: rest of BinaryViewVector{,Mut}#5133
a10y merged 7 commits intodevelopfrom
aduffy/bin-vector2

Conversation

@a10y
Copy link
Copy Markdown
Contributor

@a10y a10y commented Oct 31, 2025

Tracking Issue: #5028

@a10y a10y force-pushed the aduffy/bin-vector2 branch 3 times, most recently from 1267994 to 3738356 Compare October 31, 2025 14:23
@a10y a10y marked this pull request as ready for review October 31, 2025 14:23
@a10y a10y requested review from connortsui20 and gatesn and removed request for gatesn October 31, 2025 14:23
@a10y a10y added the changelog/feature A new feature label Oct 31, 2025
@a10y a10y force-pushed the aduffy/bin-vector2 branch from 3738356 to e9aee83 Compare October 31, 2025 14:58
@codecov
Copy link
Copy Markdown

codecov bot commented Oct 31, 2025

Codecov Report

❌ Patch coverage is 78.13333% with 82 lines in your changes missing coverage. Please review.
✅ Project coverage is 84.89%. Comparing base (69ef61d) to head (00bebf8).
⚠️ Report is 13 commits behind head on develop.

Files with missing lines Patch % Lines
vortex-vector/src/binaryview/vector_mut.rs 85.63% 27 Missing ⚠️
vortex-vector/src/binaryview/types.rs 20.00% 24 Missing ⚠️
vortex-vector/src/binaryview/vector.rs 79.04% 22 Missing ⚠️
vortex-vector/src/binaryview/view.rs 81.63% 9 Missing ⚠️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Comment thread vortex-vector/src/binaryview/types.rs
Comment thread vortex-vector/src/binaryview/vector.rs Outdated
Comment thread vortex-vector/src/binaryview/vector.rs Outdated
Comment thread vortex-vector/src/binaryview/vector.rs Outdated
Comment thread vortex-vector/src/binaryview/vector.rs Outdated
Comment thread vortex-vector/src/binaryview/vector_mut.rs Outdated
// valid buffer indices in the current array.
let mut buf_index_lookup: Vec<u32> = Vec::with_capacity(other.buffers().len());
let mut new_buffers = Vec::new();
for buffer in other.buffers().iter() {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If you assume the buffers are unique, then don't you just need to offset all the indices by n = self.buffers.len()?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

But after merging the buffer points might be out of order now, so we can't just do a blanket offset.

E.g. if self has buffers [buf1, buf2] and other has buffers [buf3, buf1, buf4, buf2]

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I guess I assumed we just wouldn't de-dupe? As in, it doesn't really matter if other has buf1 or buf2, can just treat them as new buffers.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The existing builders do have some logic to try and dedupe that I figured we'd want to carry over, but if not then that certainly makes things simpler.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I removed the deduping logic in favor of trivial appending

Comment thread vortex-vector/src/binaryview/vector_mut.rs Outdated
Comment thread vortex-vector/src/binaryview/view.rs Outdated
a10y added 5 commits November 3, 2025 15:27
Signed-off-by: Andrew Duffy <andrew@a10y.dev>
Signed-off-by: Andrew Duffy <andrew@a10y.dev>
Signed-off-by: Andrew Duffy <andrew@a10y.dev>
Signed-off-by: Andrew Duffy <andrew@a10y.dev>
Signed-off-by: Andrew Duffy <andrew@a10y.dev>
@a10y a10y force-pushed the aduffy/bin-vector2 branch from ead8e7c to 2c7a641 Compare November 3, 2025 20:27
Comment thread vortex-vector/src/binaryview/vector_mut.rs Outdated
@connortsui20 connortsui20 mentioned this pull request Nov 3, 2025
39 tasks
a10y added 2 commits November 3, 2025 17:41
Signed-off-by: Andrew Duffy <andrew@a10y.dev>
Signed-off-by: Andrew Duffy <andrew@a10y.dev>
@a10y a10y enabled auto-merge (squash) November 3, 2025 22:43
@a10y a10y merged commit dd22351 into develop Nov 3, 2025
39 checks passed
@a10y a10y deleted the aduffy/bin-vector2 branch November 3, 2025 22:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

changelog/feature A new feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants