diff --git a/crates/lance-graph-r2il/src/binding.rs b/crates/lance-graph-r2il/src/binding.rs index 36f76aafc..d45388dd9 100644 --- a/crates/lance-graph-r2il/src/binding.rs +++ b/crates/lance-graph-r2il/src/binding.rs @@ -63,8 +63,9 @@ //! Arguments: `Arg::Field(p)` is the value an earlier effect wrote to `p`, //! else the resident lane the caller bound `p` to ([`FieldLanes`]), whose //! kind is read from the lane itself. `Arg::Const(c)` is pushed as is. A -//! constant compared against a lane must fit that lane's type; the dialect -//! would otherwise saturate it, so the seam refuses it first. +//! constant compared against a lane must fit that lane's type. The seam +//! refuses an out-of-range constant here, at bind time; the dialect also +//! refuses it (`FoldError::ConstOutOfRange`) rather than narrowing it. //! //! The answer is the value each written field holds after the last effect. //! A field that still holds a mask slot or an address is an intermediate @@ -654,8 +655,9 @@ pub(crate) fn lane( Ok(Val::Address(Addr::Lane { idx, kind })) } -/// A lane compare's constant must fit the lane's type: the dialect narrows -/// it with a saturating conversion, which would change the predicate. +/// A lane compare's constant must fit the lane's type: refused here so the +/// binding reports it as an `ExecRefusal` before the dialect runs (the dialect +/// itself refuses it too, with `FoldError::ConstOutOfRange`). pub(crate) fn check_compare(stack: &[Val], effect: usize) -> Result<(), ExecRefusal> { let [.., a, b] = stack else { return Ok(()); diff --git a/crates/lance-graph-r2il/src/lib.rs b/crates/lance-graph-r2il/src/lib.rs index c1e316680..9cc5e4ba5 100644 --- a/crates/lance-graph-r2il/src/lib.rs +++ b/crates/lance-graph-r2il/src/lib.rs @@ -39,7 +39,10 @@ //! Fold band: `VIA`, `SUM`, `MIN`, `MAX`, `GROUP_SUM`, `KEY_RUNS`, `RANGE`, //! `ANY`, `ALL` and `KEEP` are wired. `SCATTER_OR` and `BLEND` are refused as //! [`FoldError::Unimplemented`]. `ANY`/`ALL` push 1 or 0; `KEY_RUNS` pushes -//! `COUNT(DISTINCT key)` over a key-ordered lane. +//! `COUNT(DISTINCT key)` over a key-ordered lane, provided the mask already +//! excludes NULL keys: the lane carries no validity, so a selected NULL row +//! counts its payload ([`procs::count_distinct_sorted`] ANDs the presence +//! plane in). //! //! `MIN`/`MAX` of an empty selection push the fold's identity, `i64::MAX` / //! `i64::MIN`: the same seed mask-risc's grouped `MIN`/`MAX` leave in an @@ -47,7 +50,9 @@ //! //! R2IL band: `Load` (spaces 0 = U32, 1 = I32, 2 = U64, 3 = resident mask //! plane), `IntAdd`, `IntSub`, `IntMult`, `IntSDiv` (scalar/scalar, checked), `IntAnd` (with survivor gating), -//! `IntOr`, `IntXor`, `IntNot`, `IntEqual`, `IntSLess` and `PopCount` are wired. `IntLess` and +//! `IntOr`, `IntXor`, `IntNot`, `IntEqual`, `IntSLess` and `PopCount` are wired. A lane +//! compare's constant that does not fit the lane's type is refused as +//! [`FoldError::ConstOutOfRange`], never narrowed. `IntLess` and //! `IntLessEqual` are refused as [`FoldError::NoUnsignedLaneCompare`] //! (mask-risc has no unsigned lane compare); `IntSub` of two addresses as //! [`FoldError::LaneVersusLane`]; an unknown `Load` space as @@ -252,6 +257,23 @@ pub enum FoldError { /// `numeric_value_out_of_range` for `bigint`; the dialect refuses rather /// than wrap. Overflow(FnIndex), + /// A lane compare's constant does not fit the lane's type: `IntEqual` + /// against a `U32` lane (or a `VIA` join's `U32` key) with a value outside + /// `0..=u32::MAX`, or `IntEqual`/`IntSLess` against an `I32` lane with a + /// value outside `i32::MIN..=i32::MAX`. Narrowing it would compare against + /// a different number (a negative constant would match `u32::MAX`), so + /// the call is refused. Carries the opcode and the constant. + ConstOutOfRange(FnIndex, i64), +} + +/// The constant of a `U32` lane compare, or [`FoldError::ConstOutOfRange`]. +fn const_u32(f: FnIndex, x: i64) -> Result { + u32::try_from(x).map_err(|_| FoldError::ConstOutOfRange(f, x)) +} + +/// The constant of an `I32` lane compare, or [`FoldError::ConstOutOfRange`]. +fn const_i32(f: FnIndex, x: i64) -> Result { + i32::try_from(x).map_err(|_| FoldError::ConstOutOfRange(f, x)) } /// How a body's Boolean calls (`IntAnd`, `IntOr`, `IntNot` on masks) are @@ -902,7 +924,7 @@ impl FoldDialect<'_> { self.emit_pred( Pred::EqU32 { lane: idx, - v: u32::try_from(x).unwrap_or(u32::MAX), + v: const_u32(f, x)?, }, stack, ); @@ -925,7 +947,7 @@ impl FoldDialect<'_> { self.emit_pred( Pred::EqI32 { lane: idx, - v: i32::try_from(x).unwrap_or(i32::MAX), + v: const_i32(f, x)?, }, stack, ); @@ -937,7 +959,7 @@ impl FoldDialect<'_> { Pred::EqU32Via { fk, key, - v: u32::try_from(x).unwrap_or(u32::MAX), + v: const_u32(f, x)?, }, stack, ); @@ -969,7 +991,7 @@ impl FoldDialect<'_> { self.emit_pred( Pred::LtI32 { lane: idx, - t: i32::try_from(t).unwrap_or(i32::MAX), + t: const_i32(f, t)?, }, stack, ); @@ -985,7 +1007,7 @@ impl FoldDialect<'_> { self.emit_pred( Pred::GtI32 { lane: idx, - t: i32::try_from(t).unwrap_or(i32::MAX), + t: const_i32(f, t)?, }, stack, ); @@ -1164,6 +1186,12 @@ impl FoldDialect<'_> { // (`Terminal::CountKeyRunsU32`, O(1) state, no seen-set). An // unordered lane is refused by mask-risc (`LaneNotOrdered`), // never answered. Operands: mask, then the key lane. + // + // Precondition: the mask excludes every row whose key is NULL. + // The lane has no validity of its own, so a NULL row's payload + // (a sentinel, or a value equal to a real key) counts as a key + // if its row is selected. AND the key's presence plane into the + // mask first, as `procs::count_distinct_sorted` does. KEY_RUNS => { let key = stack.pop().ok_or(FoldError::Underflow(f))?; let m = fold_mask(stack.pop().ok_or(FoldError::Underflow(f))?) diff --git a/crates/lance-graph-r2il/src/polyfill.rs b/crates/lance-graph-r2il/src/polyfill.rs index 47e860252..ca2cc1dc1 100644 --- a/crates/lance-graph-r2il/src/polyfill.rs +++ b/crates/lance-graph-r2il/src/polyfill.rs @@ -17,6 +17,11 @@ //! identity (`0`, `i64::MAX`, `i64::MIN`); a three-valued binding is refused //! by [`crate::binding::prepare`], so `NULL` never reaches it. //! +//! `KEY_RUNS` is `COUNT DISTINCT` only over a key-ordered lane under a mask +//! that already excludes NULL keys. The lane has no validity of its own, so a +//! selected NULL row counts its payload as a key; +//! [`crate::procs::count_distinct_sorted`] ANDs the key's presence plane in. +//! //! The positions are the [`crate::binding::EXECUTABLE`] opcodes in that //! order. The opcode, arity and R2IL name of each entry are checked against //! OGAR's `R2ILVocabulary` by test, not at run time. diff --git a/crates/lance-graph-r2il/tests/dialect.rs b/crates/lance-graph-r2il/tests/dialect.rs index 04d2a2ad1..4a2d71348 100644 --- a/crates/lance-graph-r2il/tests/dialect.rs +++ b/crates/lance-graph-r2il/tests/dialect.rs @@ -482,3 +482,198 @@ fn call_owned_reuses_consumed_slots_and_call_does_not() { "plain call must keep one slot per result, got {r:?}" ); } + +/// One lane compare followed by `PopCount`: the operands are pushed in the +/// given order (`lhs` then `rhs`), so `(addr, c)` is `lane OP c` and +/// `(c, addr)` is `c OP lane`. +fn compare_count( + planes: &Planes<'_>, + foreign: &Foreign<'_>, + f: FnIndex, + lhs: Val, + rhs: Val, +) -> Result { + let mut d = FoldDialect::new(planes, foreign); + let mut stack = vec![lhs, rhs]; + call(&mut d, f, [0; 3], &mut stack)?; + call(&mut d, POP_COUNT, [0; 3], &mut stack)?; + match stack.as_slice() { + [Val::Scalar(n)] => Ok(*n), + other => panic!("PopCount left {other:?}"), + } +} + +fn u32_lane(idx: u16) -> Val { + Val::Address(Addr::Lane { + idx, + kind: lance_graph_mask_risc::LaneKind::U32, + }) +} + +/// FAILS IF: a lane compare's constant that fits the lane's type stops +/// behaving as before, including at both ends of each type's range. +#[test] +fn in_range_compare_constants_behave_as_before() { + let codes: Vec = vec![u32::MAX, 5, 5, 0]; + let ints: Vec = vec![i32::MIN, -5, 0, 7, i32::MAX]; + let lanes = [LaneRef::U32(&codes), LaneRef::I32(&ints)]; + let planes = Planes { + n_rows: 4, + masks: &[], + lanes: &lanes[..1], + }; + let iplanes = Planes { + n_rows: 5, + masks: &[], + lanes: &lanes[1..], + }; + let f = Foreign::NONE; + let s = |x: i64| Val::Scalar(x); + let eq_u32 = |x| compare_count(&planes, &f, INT_EQUAL, u32_lane(0), s(x)); + assert_eq!(eq_u32(5), Ok(2)); + assert_eq!(eq_u32(0), Ok(1)); + assert_eq!(eq_u32(i64::from(u32::MAX)), Ok(1)); + // Symmetric: the constant pushed first. + assert_eq!( + compare_count(&planes, &f, INT_EQUAL, s(5), u32_lane(0)), + Ok(2) + ); + let eq_i32 = |x| compare_count(&iplanes, &f, INT_EQUAL, i32_lane(0), s(x)); + assert_eq!(eq_i32(i64::from(i32::MIN)), Ok(1)); + assert_eq!(eq_i32(i64::from(i32::MAX)), Ok(1)); + assert_eq!(eq_i32(-5), Ok(1)); + // `lane < t` and `t < lane`. + let lt = |t| compare_count(&iplanes, &f, INT_S_LESS, i32_lane(0), s(t)); + let gt = |t| compare_count(&iplanes, &f, INT_S_LESS, s(t), i32_lane(0)); + assert_eq!(lt(i64::from(i32::MIN)), Ok(0)); + assert_eq!(lt(i64::from(i32::MAX)), Ok(4)); + assert_eq!(lt(0), Ok(2)); + assert_eq!(gt(i64::from(i32::MIN)), Ok(4)); + assert_eq!(gt(i64::from(i32::MAX)), Ok(0)); + assert_eq!(gt(0), Ok(2)); +} + +/// FAILS IF: a lane compare's out-of-range constant is narrowed instead of +/// refused. Before the refusal the dialect saturated: `-1` against a `U32` +/// lane matched the row holding `u32::MAX` (1 instead of 0), and +/// `lane < i32::MIN - 1` selected every row (3 instead of 0). A large +/// NEGATIVE constant against an `I32` lane saturated to `i32::MAX`, not +/// `i32::MIN`. Disable: put the `try_from(..).unwrap_or(MAX)` narrowing back. +#[test] +fn out_of_range_compare_constants_are_refused() { + let codes: Vec = vec![u32::MAX, 5, 5]; + let ints: Vec = vec![-5, 0, 7, i32::MAX]; + let u = [LaneRef::U32(&codes)]; + let i = [LaneRef::I32(&ints)]; + let planes = Planes { + n_rows: 3, + masks: &[], + lanes: &u, + }; + let iplanes = Planes { + n_rows: 4, + masks: &[], + lanes: &i, + }; + let f = Foreign::NONE; + let s = |x: i64| Val::Scalar(x); + let refused = |op, x| Err(FoldError::ConstOutOfRange(op, x)); + + // U32 lane, IntEqual, both sides of the range and both operand orders. + let above_u32 = i64::from(u32::MAX) + 1; + for x in [-1, i64::MIN, above_u32, i64::MAX] { + assert_eq!( + compare_count(&planes, &f, INT_EQUAL, u32_lane(0), s(x)), + refused(INT_EQUAL, x), + "lane = {x}" + ); + assert_eq!( + compare_count(&planes, &f, INT_EQUAL, s(x), u32_lane(0)), + refused(INT_EQUAL, x), + "{x} = lane" + ); + } + + // I32 lane, IntEqual and IntSLess. `above` would have saturated onto the + // row holding `i32::MAX`; `below` too, because the old narrowing mapped + // every failure to `i32::MAX`. + let above = i64::from(i32::MAX) + 1; + let below = i64::from(i32::MIN) - 1; + for x in [above, below, i64::MIN, i64::MAX] { + for (op, lhs, rhs) in [ + (INT_EQUAL, i32_lane(0), s(x)), + (INT_EQUAL, s(x), i32_lane(0)), + (INT_S_LESS, i32_lane(0), s(x)), + (INT_S_LESS, s(x), i32_lane(0)), + ] { + assert_eq!( + compare_count(&iplanes, &f, op, lhs, rhs), + refused(op, x), + "{op:?} with {x}" + ); + } + } +} + +/// FAILS IF: an `IntEqual` against a `VIA` join's `U32` key narrows an +/// out-of-range constant instead of refusing it, or stops matching an +/// in-range one at the top of the range. +#[test] +fn via_compare_constants_are_range_checked() { + // This table's fk lane names a row of the foreign key lane. + let fk: Vec = vec![0, 1, 1, 2]; + let keys: Vec = vec![u32::MAX, 5, 9]; + let lanes = [LaneRef::U32(&fk)]; + let flanes = [LaneRef::U32(&keys)]; + let planes = Planes { + n_rows: 4, + masks: &[], + lanes: &lanes, + }; + let foreign = Foreign { + planes: &[], + lanes: &flanes, + }; + let via = Val::Address(Addr::Via { fk: 0, key: 0 }); + let count = |x: i64| compare_count(&planes, &foreign, INT_EQUAL, via, Val::Scalar(x)); + assert_eq!(count(5), Ok(2)); + assert_eq!(count(i64::from(u32::MAX)), Ok(1)); + for x in [-1, i64::from(u32::MAX) + 1] { + assert_eq!(count(x), Err(FoldError::ConstOutOfRange(INT_EQUAL, x))); + } +} + +/// FAILS IF: `KEY_RUNS`'s documented precondition stops holding as stated. +/// The key lane carries no validity: a selected row whose key is NULL counts +/// its payload, so the sorted lane `(NULL as 0, 1, 1, 2)` under an +/// all-rows mask gives 3, where SQL `count(DISTINCT key)` gives 2. With the +/// key's presence plane ANDed into the mask it gives 2. +#[test] +fn key_runs_counts_a_selected_null_key_unless_the_mask_excludes_it() { + let key: Vec = vec![0, 1, 1, 2]; + let lanes = [LaneRef::U32(&key)]; + let all = [0b1111u64]; + let valid = [0b1110u64]; // row 0 is NULL + let masks: [&[u64]; 2] = [&all, &valid]; + let planes = Planes { + n_rows: 4, + masks: &masks, + lanes: &lanes, + }; + let foreign = Foreign::NONE; + let runs = |gate: bool| { + let mut d = FoldDialect::new(&planes, &foreign); + let mut stack = vec![Val::Scalar(0)]; + call(&mut d, LOAD, [3, 0, 0], &mut stack).expect("LOAD plane 0"); + if gate { + stack.push(Val::Scalar(1)); + call(&mut d, LOAD, [3, 0, 0], &mut stack).expect("LOAD plane 1"); + call(&mut d, INT_AND, [0; 3], &mut stack).expect("IntAnd"); + } + stack.push(u32_lane(0)); + call(&mut d, KEY_RUNS, [0; 3], &mut stack).expect("KEY_RUNS"); + stack + }; + assert_eq!(runs(false), vec![Val::Scalar(3)], "NULL row counted"); + assert_eq!(runs(true), vec![Val::Scalar(2)], "NULL row masked out"); +}