Skip to content

ScalarFn as Arrays#7383

Open
connortsui20 wants to merge 5 commits intodevelopfrom
ct/scalar-fn-arrays
Open

ScalarFn as Arrays#7383
connortsui20 wants to merge 5 commits intodevelopfrom
ct/scalar-fn-arrays

Conversation

@connortsui20
Copy link
Copy Markdown
Contributor

@connortsui20 connortsui20 commented Apr 10, 2026

Summary

Continuation of #7361

TODO (tests still failing)

API Changes

TODO

Testing

TODO

@connortsui20 connortsui20 requested a review from gatesn April 10, 2026 14:01
@connortsui20 connortsui20 added the changelog/break A breaking API change label Apr 10, 2026
@codspeed-hq
Copy link
Copy Markdown

codspeed-hq bot commented Apr 10, 2026 •
edited
Loading

Merging this PR will improve performance by 14.91%

⚡ 2 improved benchmarks
✅ 1120 untouched benchmarks
⏩ 1455 skipped benchmarks1

Performance Changes

Mode Benchmark BASE HEAD Efficiency
⚡ Simulation new_bp_prim_test_between[i16, 32768] 133.6 µs 119.8 µs +11.52%
⚡ Simulation new_bp_prim_test_between[i32, 16384] 108.3 µs 94.3 µs +14.91%

Comparing ct/scalar-fn-arrays (24bd039) with develop (8d9052e)

Open in CodSpeed

Footnotes

  1. 1455 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/scalar-fn-arrays branch 2 times, most recently from 08fdcd4 to cadf573 Compare April 10, 2026 16:21
gatesn
gatesn reviewed Apr 10, 2026
View reviewed changes
encodings/zigzag/src/compute/cast.rs Outdated
Copy link
Copy Markdown
Contributor

@gatesn gatesn Apr 10, 2026

Choose a reason for hiding this comment

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

Are we ok to remove this? Or should we figure out how to delegate casting :/

Copy link
Copy Markdown
Contributor

@joseph-isaacs joseph-isaacs Apr 10, 2026

Choose a reason for hiding this comment

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

this!

encodings/zstd/src/zstd_buffers.rs
.ok_or_else(|| vortex_err!("Array does not support serialization"))?;
let metadata = session
.array_serialize(array)?
.ok_or_else(|| vortex_err!("Array does not support ZstdBuffers serialization"))?;
Copy link
Copy Markdown
Contributor

@gatesn gatesn Apr 10, 2026

Choose a reason for hiding this comment

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

This message should actually be Array is not registered for serializations

Copy link
Copy Markdown
Contributor Author

@connortsui20 connortsui20 Apr 10, 2026

Choose a reason for hiding this comment

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

i moved the error directly into array_serialize

vortex-array/src/serde.rs
array: &'a ArrayRef,
) -> VortexResult<Self> {
// Depth-first traversal of the array to ensure it supports serialization.
// FIXME(ngates): this serializes the metadata and throws it away!
Copy link
Copy Markdown
Contributor

@gatesn gatesn Apr 10, 2026

Choose a reason for hiding this comment

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

Note that flat buffer writing is now fallible, so no need to run this check up-front

gatesn and others added 4 commits April 10, 2026 14:14
@gatesn @connortsui20
Use scalar functions as arrays
d4a8e42 
Signed-off-by: Nicholas Gates <nick@nickgates.com>
Signed-off-by: Connor Tsui <connor.tsui20@gmail.com>
@connortsui20
remove metadata methods
16953eb 
Signed-off-by: Connor Tsui <connor.tsui20@gmail.com>
@connortsui20
fix some tests
2cd311b 
Signed-off-by: Connor Tsui <connor.tsui20@gmail.com>
@connortsui20
address comments
705bb98 
Signed-off-by: Connor Tsui <connor.tsui20@gmail.com>
@connortsui20 connortsui20 marked this pull request as ready for review April 10, 2026 18:25
@connortsui20
fix more tests
24bd039 
Signed-off-by: Connor Tsui <connor.tsui20@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gatesn gatesn Awaiting requested review from gatesn

@joseph-isaacs joseph-isaacs Awaiting requested review from joseph-isaacs

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

changelog/break A breaking API change

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@connortsui20 @gatesn @joseph-isaacs