Remove len parameter from ScalarFnArray::try_new#8378
Conversation
ScalarFnArray::try_newlen parameter from ScalarFnArray::try_new
Merging this PR will improve performance by 36.9%
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ⚡ | Simulation | chunked_bool_canonical_into[(1000, 10)] |
35.5 µs | 20.5 µs | +73.18% |
| ⚡ | Simulation | chunked_varbinview_canonical_into[(1000, 10)] |
198.1 µs | 161.5 µs | +22.63% |
| ⚡ | Simulation | chunked_varbinview_into_canonical[(1000, 10)] |
213.5 µs | 176.8 µs | +20.81% |
Tip
Curious why this is faster? Comment @codspeedbot explain why this is faster on this PR, or directly use the CodSpeed MCP with your agent.
Comparing ct/remove-len-scalar-fn-array (ea3ad14) with develop (8475910)
Footnotes
-
10 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. ↩
Pull request was converted to draft
Signed-off-by: Connor Tsui <connor.tsui20@gmail.com>
9409852 to
49ece36
Compare
|
@claude fix the merge conflicts |
|
Claude finished @connortsui20's task in 3m 54s —— View job Merge conflict analysis
What's conflicting
Of the 25 files in this PR, 15 no longer exist on
The remaining 10 files carry the actual API change (removing
Why I couldn't push the fixThis review environment denies the git operations required to build the merge commit:
So I can't create or push the merge commit from this job. To enable it, add Recommended resolution (≈1 min locally)git switch ct/remove-len-scalar-fn-array
git fetch origin develop
git merge origin/develop
# All conflicts are modify/delete on removed TurboQuant/tensor files — accept the deletions:
git rm vortex-turboquant/src/scalar_fns/decode.rs \
vortex-turboquant/src/scalar_fns/encode.rs \
vortex-turboquant/src/vector/normalize.rs \
vortex-tensor/src/encodings/turboquant/compress.rs \
vortex-tensor/src/encodings/turboquant/tests/compute.rs \
vortex-tensor/src/encodings/turboquant/tests/nullable.rs \
vortex-tensor/src/encodings/turboquant/tests/structural.rs \
vortex-tensor/src/scalar_fns/cosine_similarity.rs \
vortex-tensor/src/scalar_fns/inner_product.rs \
vortex-tensor/src/scalar_fns/l2_denorm.rs \
vortex-tensor/src/scalar_fns/l2_norm.rs \
vortex-tensor/src/scalar_fns/sorf_transform/mod.rs \
vortex-tensor/src/scalar_fns/sorf_transform/tests.rs \
vortex-tensor/src/utils.rs \
vortex-tensor/src/vector_search.rs
# The 10 vortex-array / encodings files carrying the `len` removal should merge cleanly.
git commit # completes the merge
git pushNote Given that #8377 removed the whole TurboQuant subsystem, those 15 files in this PR were only call-site updates for code that no longer exists — dropping them loses nothing. The meaningful change (the |
…ar-fn-array Signed-off-by: Claude <noreply@anthropic.com> # Conflicts: # vortex-tensor/src/encodings/turboquant/compress.rs # vortex-tensor/src/encodings/turboquant/tests/compute.rs # vortex-tensor/src/encodings/turboquant/tests/nullable.rs # vortex-tensor/src/encodings/turboquant/tests/structural.rs # vortex-tensor/src/scalar_fns/inner_product.rs # vortex-tensor/src/scalar_fns/sorf_transform/mod.rs # vortex-tensor/src/scalar_fns/sorf_transform/tests.rs # vortex-turboquant/src/scalar_fns/decode.rs # vortex-turboquant/src/scalar_fns/encode.rs # vortex-turboquant/src/vector/normalize.rs
|
What is the scalar fn doesn't have children?
|
|
There’s still a method that lets you provide len. I think it’s nicer since the other cases are very few compared to cases with non empty children |
|
No cause now if the children is empty this becomes a bail or panic! For no reason |
|
This is the same thing as constructing chunkedarray from children and dtype or just children, having empty children panics. |
ScalarFnArray::try_new lost its len parameter in #8378 and now infers the length from the first child, which matches the old behavior of passing a.len(). Signed-off-by: "Nemo Yu" <zhenghong@spiraldb.com>
ScalarFnArray::try_new lost its len parameter in #8378 and now infers the length from the first child, which matches the old behavior of passing a.len(). DCO Remediation Commit for Nemo Yu <zyu379@wisc.edu> I, Nemo Yu <zyu379@wisc.edu>, hereby add my Signed-off-by to this commit: 88cb5a2 I, Nemo Yu <zyu379@wisc.edu>, hereby add my Signed-off-by to this commit: 8789430 I, Nemo Yu <zyu379@wisc.edu>, hereby add my Signed-off-by to this commit: 549f5c4 I, Nemo Yu <zyu379@wisc.edu>, hereby add my Signed-off-by to this commit: ec95875 I, Nemo Yu <zyu379@wisc.edu>, hereby add my Signed-off-by to this commit: 874c171 Signed-off-by: Nemo Yu <zyu379@wisc.edu>
Summary
This didn't need to take length because it already gets it from the array children.
API Changes
Removes
lenparameter.Testing
N/A