Skip to content

Feature: ListViewArray#4708

Merged
connortsui20 merged 2 commits intodevelopfrom
ct/list-view-array
Sep 22, 2025
Merged

Feature: ListViewArray#4708
connortsui20 merged 2 commits intodevelopfrom
ct/list-view-array

Conversation

@connortsui20
Copy link
Contributor

@connortsui20 connortsui20 commented Sep 19, 2025

Tracking Issue: #4699

Adds a new ListViewArray encoding.

The compute functions and the ListViewBuilder will come in a future PR.

@connortsui20 connortsui20 mentioned this pull request Sep 18, 2025
Closed
7 tasks
@connortsui20 connortsui20 added the changelog/feature A new feature label Sep 19, 2025
@codecov
Copy link

codecov bot commented Sep 19, 2025 •
edited
Loading

Codecov Report

❌ Patch coverage is 88.86414% with 100 lines in your changes missing coverage. Please review.
✅ Project coverage is 89.01%. Comparing base (8be7e5f) to head (1148cca).
⚠️ Report is 3 commits behind head on develop.

Files with missing lines Patch % Lines
vortex-array/src/arrays/listview/vtable/serde.rs 0.00% 40 Missing ⚠️
vortex-array/src/arrays/listview/mod.rs 79.47% 39 Missing ⚠️
vortex-array/src/arrays/listview/vtable/visitor.rs 0.00% 8 Missing ⚠️
vortex-array/src/arrays/listview/vtable/mod.rs 0.00% 6 Missing ⚠️
vortex-array/src/arrays/listview/tests/validate.rs 98.21% 3 Missing ⚠️
...x-array/src/arrays/fixed_size_list/vtable/serde.rs 33.33% 2 Missing ⚠️
...rtex-array/src/arrays/listview/vtable/canonical.rs 0.00% 2 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 force-pushed the ct/list-view-array branch 4 times, most recently from efffb3c to a50064c Compare September 19, 2025 20:10
@codspeed-hq
Copy link

codspeed-hq bot commented Sep 19, 2025 •
edited
Loading

CodSpeed Performance Report

Merging #4708 will not alter performance

Comparing ct/list-view-array (1148cca) with develop (450dbd2)

Summary

✅ 1441 untouched

connortsui20
connortsui20 commented Sep 19, 2025
View reviewed changes
connortsui20
connortsui20 commented Sep 19, 2025
View reviewed changes
@connortsui20 connortsui20 marked this pull request as ready for review September 19, 2025 20:55
robert3005
robert3005 reviewed Sep 20, 2025
View reviewed changes
Copy link
Contributor

@robert3005 robert3005 left a comment

Choose a reason for hiding this comment

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

Haven't looked at the tests yet, left some minor style comments

@connortsui20
Copy link
Contributor Author

connortsui20 commented Sep 20, 2025

I just realized my optimization is wrong. Even if the offsets are sorted, it could be "sparse". So I think I'm just going to refactor that function entirely. In fact, I'm probably going to remove it from this PR and do a followup PR

@gatesn
Copy link
Contributor

gatesn commented Sep 21, 2025

Just confirming this is binary compatible with Arrow's ListView

joseph-isaacs
joseph-isaacs reviewed Sep 22, 2025
View reviewed changes
@connortsui20 connortsui20 marked this pull request as draft September 22, 2025 13:19
@connortsui20
move OffsetPType to the root of vortex-array
9c58698 
Since we need `OffsetPType` for `ListView` (not just `List` anymore).

Signed-off-by: Connor Tsui <[email protected]>
@connortsui20
Copy link
Contributor Author

connortsui20 commented Sep 22, 2025

Just confirming this is binary compatible with Arrow's ListView

I'm pretty sure it is? At least when everything is in canonical form

@connortsui20 connortsui20 force-pushed the ct/list-view-array branch 2 times, most recently from 8f91cf6 to 594a5ac Compare September 22, 2025 14:15
connortsui20
connortsui20 commented Sep 22, 2025
View reviewed changes
@connortsui20 connortsui20 marked this pull request as ready for review September 22, 2025 15:22
@robert3005
Copy link
Contributor

robert3005 commented Sep 22, 2025

Yes, this is compatible with Arrow ListViewArray when offsets and lengths are u32/i32 or u64/i64

@connortsui20 connortsui20 marked this pull request as draft September 22, 2025 15:54
@connortsui20 connortsui20 force-pushed the ct/list-view-array branch 2 times, most recently from cf87fd0 to e34a7b4 Compare September 22, 2025 16:24
@connortsui20 connortsui20 marked this pull request as ready for review September 22, 2025 16:26
robert3005
robert3005 approved these changes Sep 22, 2025
View reviewed changes
Copy link
Contributor

@robert3005 robert3005 left a comment

Choose a reason for hiding this comment

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

Only a style comment

@connortsui20 connortsui20 enabled auto-merge (squash) September 22, 2025 22:37
@connortsui20 connortsui20 merged commit b9e29a1 into develop Sep 22, 2025
37 checks passed
@connortsui20 connortsui20 deleted the ct/list-view-array branch September 22, 2025 22:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@joseph-isaacs joseph-isaacs joseph-isaacs left review comments

@robert3005 robert3005 robert3005 approved these changes

Assignees

No one assigned

Labels

changelog/feature A new feature

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@connortsui20 @gatesn @robert3005 @joseph-isaacs