Skip to content

Conversation

@connortsui20
Copy link
Contributor

@connortsui20 connortsui20 commented Nov 25, 2025

Adds take implementations and tests for:

  • BitBuffer
  • Mask
  • BoolVector
  • PrimitiveVector

For BitBuffer, I don't think it makes sense to implement take with nullable indices since I would argue that there isn't a clear behavior to have (because it might not be obvious that true means not null and false means null). But for Mask it feels a bit different, we are using it for validity in all of our vectors, so I think it makes a bit more sense (and it will make the implementation of take for the rest of the vectors easier).

We represent indices in 3 different ways here (a generic indices slice of unsigned integers, generic unsigned pvector, and primitive vector). I feel quite strongly that we should not allow signed indices, as that opens the door to too many bugs. See this Slack thread.

This is also based on top of #5537, so that needs to merge first.

@connortsui20 connortsui20 added the feature Release label indicating a new feature or request label Nov 25, 2025
@connortsui20 connortsui20 marked this pull request as draft November 25, 2025 23:33
@codecov
Copy link

codecov bot commented Nov 25, 2025

Codecov Report

❌ Patch coverage is 93.12977% with 18 lines in your changes missing coverage. Please review.
✅ Project coverage is 85.46%. Comparing base (cd80330) to head (9583f20).

Files with missing lines Patch % Lines
vortex-compute/src/take/mask.rs 69.04% 13 Missing ⚠️
vortex-compute/src/take/vector/primitive.rs 55.55% 4 Missing ⚠️
vortex-compute/src/take/vector/tests.rs 99.32% 1 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.

@connortsui20 connortsui20 changed the title Feature: add take for primitive vector Feature: add take for bitbuffer, mask, primitive vector Nov 26, 2025
@connortsui20 connortsui20 changed the base branch from ct/take-mask to ct/take-slice November 26, 2025 15:19
@codspeed-hq
Copy link

codspeed-hq bot commented Nov 26, 2025

CodSpeed Performance Report

Merging #5540 will not alter performance

Comparing ct/take-pvector (9583f20) with develop (cd80330)

Summary

✅ 1478 untouched
⏩ 235 skipped1

Footnotes

  1. 235 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports.

@connortsui20 connortsui20 force-pushed the ct/take-pvector branch 2 times, most recently from f3d3dde to eea184b Compare November 26, 2025 15:33
@connortsui20 connortsui20 changed the title Feature: add take for bitbuffer, mask, primitive vector Feature: add take for primitive and bool vectors Nov 26, 2025
@connortsui20 connortsui20 force-pushed the ct/take-slice branch 4 times, most recently from bd6549c to dfa2f74 Compare November 26, 2025 16:52
@connortsui20 connortsui20 force-pushed the ct/take-pvector branch 2 times, most recently from 12379a7 to ae63951 Compare November 26, 2025 16:54
@connortsui20 connortsui20 marked this pull request as ready for review November 26, 2025 17:22
@connortsui20 connortsui20 requested review from a10y and gatesn November 26, 2025 17:22
@connortsui20 connortsui20 changed the base branch from ct/take-slice to develop November 26, 2025 17:24
@connortsui20 connortsui20 changed the base branch from develop to ct/avx512-filter November 26, 2025 17:48
@connortsui20 connortsui20 changed the base branch from ct/avx512-filter to develop November 26, 2025 17:48
Signed-off-by: Connor Tsui <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feature Release label indicating a new feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants