Repository navigation
r2il: refuse out-of-range lane-compare constants in FoldDialect - #1980
Merged
Merged
Conversation
The raw FoldDialect narrowed a lane compare's constant with `try_from(..).unwrap_or(MAX)` at five sites (IntEqual on U32, I32 and a VIA key; IntSLess in both operand orders). A constant outside the lane's type therefore compared against a different number: -1 against a U32 lane matched the row holding u32::MAX, and every out-of-range I32 constant, including a large negative one, became i32::MAX. The dialect now refuses such a call as the new FoldError::ConstOutOfRange(opcode, value); no existing variant names a constant that does not fit a lane. The binding seam already refused these first, so its results do not change. KEY_RUNS takes no validity: a selected NULL row counts its payload as a key. The precondition (key-ordered lane, NULL keys masked out) is now documented on the arm, the crate doc and the polyfill lens, and pinned by a test (3 unmasked, 2 with the presence plane ANDed in, as procs::count_distinct_sorted does). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XAqKMMWgnUU9g7Ldyn6sbk
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars.
 |
Codex Review Summary: ✅ CompletedThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
…-range constants The seam's compare check stays; it reports the refusal as an ExecRefusal at bind time. The dialect no longer saturates (FoldError::ConstOutOfRange), so the 'would otherwise saturate' wording was stale. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XAqKMMWgnUU9g7Ldyn6sbk
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
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.
Problem
The raw
FoldDialect(crates/lance-graph-r2il/src/lib.rs) clamped any lane-compare constant that did not fit the lane type, instead of refusing it. The clamp wasi32::try_from(x).unwrap_or(i32::MAX)or theu32equivalent. As a result:Change
The five compare sites now refuse through new
const_u32/const_i32helpers:Each refusal is the new
FoldError::ConstOutOfRange(FnIndex, i64). No existing variant fits:Overflowmeans scalar arithmetic left i64, andWrongOperandKindis about operand shape.KEY_RUNS counted a selected NULL key. Its precondition (ordered keys, NULLs masked) is now documented on the KEY_RUNS arm, in the crate doc and on the polyfill lens. The behaviour does not change, because the validity channel already exists:
procs::count_distinct_sortedANDs the key's presence plane into the mask.Non-test diff: about 40 lines (
lib.rs,polyfill.rs).binding.rsandbinding_native.rsare untouched, so there is no overlap with #1979.Callers
None needed changes:
i32/u32: r2il procs, relalglower.rs, the nal crate, math-polyfill, and r2il-mask-abi-probe.FoldErrormatch is exhaustive. egglog-r2il-probe and holo-h14d-probe use wildcard arms, which I verified by reading them.No published result changes.
Tests
New tests in
tests/dialect.rs:in_range_compare_constants_behave_as_before: type-range boundaries, both operand orders.out_of_range_compare_constants_are_refused:-1,i64::MIN,u32::MAX+1,i32::MAX+1,i32::MIN-1,i64::MAX.via_compare_constants_are_range_checked.key_runs_counts_a_selected_null_key_unless_the_mask_excludes_it: 3 without the presence mask, 2 with it.Disable run. After the commit, I restored the clamp at all five sites (the patch applied 2+1+2 times as asserted).
out_of_range_compare_constants_are_refusedandvia_compare_constants_are_range_checkedwent red; the other 11 tests indialect.rsstayed green. I then restored the code withgit checkout.Validation
All runs used
CARGO_PROFILE_DEV_DEBUG=0 CARGO_PROFILE_TEST_DEBUG=0 CARGO_INCREMENTAL=0.cargo teston r2il: 88 passed, 0 failed (re-run independently).cargo testpasses on math-polyfill-probe, lance-graph-relalg, lance-graph-nal and r2il-mask-abi-probe.cargo clippy --all-targets -D warningsis clean on r2il.cargo fmtis applied.Open
binding.rs:66and:606still say the dialect "would otherwise saturate". That is now stale. It is left alone here becausebinding.rsis in dt64/r2il: stamp MethodRef with its issuing store and binding digest; make Prepared private #1979, and will be corrected after that merges.LOAD/VIAlane and plane indices still clamp withu16::try_from(..).unwrap_or(u16::MAX). This is a separate site, not changed here.Generated by Claude Code