refactor: support formatting of bound expressions - #9228
Merged
Conversation
Merging this PR will improve performance by 14.47%
Performance Changes
Tip Curious why this is faster? Comment Comparing 
Footnotes
|
joseph-isaacs
force-pushed
the
ji/bound-expression-display
branch
3 times, most recently
from
August 6, 2026 12:40
3e24e3c to
8a859c2
Compare
joseph-isaacs
added
changelog/break
joseph-isaacs
changed the title
Signed-off-by: Joe Isaacs <joe.isaacs@live.co.uk>
joseph-isaacs
force-pushed
the
ji/bound-expression-display
branch
from
August 6, 2026 13:10
8a859c2 to
14313eb
Compare
joseph-isaacs
marked this pull request as ready for review
August 6, 2026 13:11
robert3005
approved these changes
Aug 6, 2026
connortsui20
pushed a commit
that referenced
this pull request
Aug 6, 2026
Brings `develop` through #9228. Three files conflicted, all under `scalar_fn/fns/binary/numeric/`, because #9210 optimized the very kernels this branch replaced with the row framework. `primitive.rs` keeps this branch's version. #9210 arrived independently at the same design, and its four `mul_failure` bodies are identical to this branch's width for width: the evidence is the discarded high half of the widened product, so the lane never compares and LLVM cannot fold the check into `llvm.umul.with.overflow`. What `develop` keeps and the row path does not need is `CHECKED_VALUE_LOOP`, `DIV_CHECKS_IN_VALUE_LOOP`, `div_checked` and `CheckedPrimitiveOp::checked`, which exist to choose between its split and early-exit lane kernels; a row kernel produces the value and the evidence in one pass and has no choice to make. `develop`'s named intermediates in the signed 64-bit body are adopted. `checked.rs` takes `develop`'s `checked_lanes`, including its rewritten docs and the relaxation from `#[inline(always)]` to `#[inline]`. `checked_apply_lanes` and the `Failure` trait beside it are dropped: the split value/evidence pass they served is now `CheckedSink`, and decimal, their only other neighbour, uses `checked_lanes` alone. `tests.rs` takes both sides. #9210's `test_multiply_overflow_boundaries` drives `execute_numeric` end to end rather than the kernels directly, so its 20 cases cross-check the row path against `develop`'s intent at every overflow boundary of every width formula. They pass unmodified. `NUMERIC_ROWFN_PLAN.md` is corrected for what this does to its benchmark table, which it had already predicted: the unsigned multiply rows compared the row framework against a defect `develop` has now fixed, so they are not a `RowFn` win and the re-measurement they asked for is now like-for-like. Verification: 3355 vortex-array, 179 vortex-tensor and 241 vortex-geo tests, 73 doctests, clippy over those three plus vortex-compute with `--all-targets --all-features`, nightly fmt, `git diff --check`, and builds of vortex-file, vortex-datafusion, vortex-layout, vortex-scan, vortex-compute and vortex-btrblocks. `lance-bench` and `vortex-nvcomp` do not build in this environment: `protoc` and CUDA are absent, on `develop` as well. Signed-off-by: "Connor Tsui" <connor@spiraldb.com> Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014mwiAahcxc5xBfTrhDK11L
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
ExprDisplayinterface shared byExpressionandBoundExpressionScalarFnVTable::fmt_sql_displayand migrate built-in formatters to itWhy
Bound-expression consumers need compact SQL-style formatting without calling
unbind(). Keeping the existingfmt_sql(&Expression, ...)method avoids a source-breaking signature change for external scalar-function vtables.Break
Update the fmt function on scalar fns.