Skip to content

Remove len parameter from ScalarFnArray::try_new#8378

Merged
connortsui20 merged 2 commits into
developfrom
ct/remove-len-scalar-fn-array
Jun 12, 2026
Merged

Remove len parameter from ScalarFnArray::try_new#8378
connortsui20 merged 2 commits into
developfrom
ct/remove-len-scalar-fn-array

Conversation

@connortsui20

Copy link
Copy Markdown
Contributor

Summary

This didn't need to take length because it already gets it from the array children.

API Changes

Removes len parameter.

Testing

N/A

@connortsui20 connortsui20 requested review from a team, AdamGS and robert3005 June 11, 2026 21:41
@connortsui20 connortsui20 added the changelog/break A breaking API change label Jun 11, 2026
@connortsui20 connortsui20 changed the title Remove len parameter from ScalarFnArray::try_new Remove len parameter from ScalarFnArray::try_new Jun 11, 2026
@connortsui20 connortsui20 enabled auto-merge (squash) June 11, 2026 21:41
@codspeed-hq

codspeed-hq Bot commented Jun 11, 2026

Copy link
Copy Markdown

Merging this PR will improve performance by 36.9%

⚠️ Unknown Walltime execution environment detected

Using the Walltime instrument on standard Hosted Runners will lead to inconsistent data.

For the most accurate results, we recommend using CodSpeed Macro Runners: bare-metal machines fine-tuned for performance measurement consistency.

⚡ 3 improved benchmarks
✅ 1523 untouched benchmarks
⏩ 10 skipped benchmarks1

Performance Changes

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)

Open in CodSpeed

Footnotes

  1. 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.

@connortsui20 connortsui20 marked this pull request as draft June 11, 2026 21:51
auto-merge was automatically disabled June 11, 2026 21:51

Pull request was converted to draft

Signed-off-by: Connor Tsui <connor.tsui20@gmail.com>
@connortsui20 connortsui20 force-pushed the ct/remove-len-scalar-fn-array branch from 9409852 to 49ece36 Compare June 11, 2026 21:57
@connortsui20 connortsui20 marked this pull request as ready for review June 11, 2026 21:58
@connortsui20

Copy link
Copy Markdown
Contributor Author

@claude fix the merge conflicts

@github-actions

github-actions Bot commented Jun 11, 2026

Copy link
Copy Markdown
Contributor

Claude finished @connortsui20's task in 3m 54s —— View job


Merge conflict analysis

  • Fetch latest develop and identify conflicts
  • Determine the correct resolution
  • Resolve conflicts and pushblocked by environment restrictions (see below)

What's conflicting

develop has advanced since this branch was created. Its current HEAD is "Remove all TurboQuant stuff" (#8377), which deletes the entire vortex-turboquant crate and all TurboQuant/tensor code. This PR modifies many of those now-deleted files, producing modify/delete conflicts.

Of the 25 files in this PR, 15 no longer exist on develop and should simply be dropped:

Deleted on develop (drop these)
vortex-turboquant/src/scalar_fns/decode.rs, encode.rs
vortex-turboquant/src/vector/normalize.rs
vortex-tensor/src/encodings/turboquant/compress.rs
vortex-tensor/src/encodings/turboquant/tests/{compute,nullable,structural}.rs
vortex-tensor/src/scalar_fns/{cosine_similarity,inner_product,l2_denorm,l2_norm}.rs
vortex-tensor/src/scalar_fns/sorf_transform/{mod,tests}.rs
vortex-tensor/src/{utils,vector_search}.rs

The remaining 10 files carry the actual API change (removing len from ScalarFnArray::try_new) and still exist on develop, so they should be kept:

  • vortex-array/src/arrays/scalar_fn/{array,plugin,rules}.rs
  • vortex-array/src/arrays/scalar_fn/vtable/operations.rs
  • vortex-array/src/arrays/chunked/compute/rules.rs
  • vortex-array/src/arrays/dict/compute/rules.rs
  • vortex-array/src/arrays/fixed_size_list/tests/nested.rs
  • vortex-array/src/expression.rs
  • encodings/datetime-parts/src/compute/rules.rs
  • encodings/runend/src/rules.rs

Why I couldn't push the fix

This review environment denies the git operations required to build the merge commit:

  • The checkout is a shallow clone, so there's no reachable merge base between this branch and origin/develop.
  • git fetch --deepen / --unshallow (network) and git merge are both blocked / require approval that isn't grantable here — even with the sandbox disabled.

So I can't create or push the merge commit from this job. To enable it, add git fetch and git merge to --allowedTools (and allow network), or resolve locally:

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 push

Note

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 try_new signature) lives entirely in the 10 vortex-array/encodings files. You may even want to retitle/trim the PR after the merge, since over half its diff disappears.
· branch ct/remove-len-scalar-fn-array

…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
@connortsui20 connortsui20 merged commit c6a0b7b into develop Jun 12, 2026
68 of 70 checks passed
@connortsui20 connortsui20 deleted the ct/remove-len-scalar-fn-array branch June 12, 2026 01:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

changelog/break A breaking API change

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants