Skip to content

ScalarFn as Arrays#7383

Closed
connortsui20 wants to merge 3 commits intodevelopfrom
ct/scalar-fn-arrays
Closed

ScalarFn as Arrays#7383
connortsui20 wants to merge 3 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 22.65%

⚡ 3 improved benchmarks
✅ 1144 untouched benchmarks
⏩ 1455 skipped benchmarks1

Performance Changes

Mode Benchmark BASE HEAD Efficiency
⚡ Simulation old_bp_prim_test_between[i32, 32768] 249.6 µs 221.6 µs +12.61%
⚡ Simulation old_bp_prim_test_between[i64, 16384] 219.9 µs 189.7 µs +15.96%
⚡ Simulation old_bp_prim_test_between[i64, 32768] 327.3 µs 266.9 µs +22.65%

Comparing ct/scalar-fn-arrays (c6b2f36) with ct/remove-metadata (26f7426)

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!

Copy link
Copy Markdown
Contributor

@robert3005 robert3005 Apr 13, 2026

Choose a reason for hiding this comment

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

this is just a reduce rule?

encodings/zstd/src/zstd_buffers.rs Outdated
.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 Outdated
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

@connortsui20 connortsui20 marked this pull request as ready for review April 10, 2026 18:25
@connortsui20 connortsui20 marked this pull request as draft April 14, 2026 18:02
@connortsui20 connortsui20 force-pushed the ct/scalar-fn-arrays branch 3 times, most recently from e44abe6 to 9522d62 Compare April 14, 2026 19:03
@connortsui20
remove metadata from DynArray
c897b28 
Signed-off-by: Connor Tsui <connor.tsui20@gmail.com>
@connortsui20
update file sizes
26f7426 
Signed-off-by: Connor Tsui <connor.tsui20@gmail.com>
@connortsui20 connortsui20 changed the base branch from develop to ct/remove-metadata April 14, 2026 19:32
@gatesn @connortsui20
Use scalar functions as arrays
c6b2f36 
Signed-off-by: Nicholas Gates <nick@nickgates.com>
Signed-off-by: Connor Tsui <connor.tsui20@gmail.com>
Base automatically changed from ct/remove-metadata to develop April 14, 2026 20:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@robert3005 robert3005 robert3005 left review comments

@gatesn gatesn Awaiting requested review from gatesn

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

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.

4 participants

@connortsui20 @robert3005 @gatesn @joseph-isaacs