Skip to content

Views behind null rows are unvalidated, so a dense kernel over VarBinViewArray can panic #9090

Description

@connortsui20

Drafted by Claude Code on Connor's behalf, and not yet edited by him.

VarBinViewArray::validate_views only validates the views of valid rows, which is intentional. So a legal array can hold a view behind a null row naming a buffer that does not exist.

ScalarFnVTable::is_strict consequence 2 reads as an unconditional licence to ignore that: "Values behind null slots are irrelevant, so kernels may compute g densely over all lanes (including garbage) and apply validity afterwards." That holds for a flat fixed-width payload, where a null row is unused bytes and reading it can't fault. It doesn't hold for a VarBinViewArray row, where reading the value means following a buffer index and offset into a data buffer.

A kernel that resolves every row densely panics on a legal array:

index out of bounds: the len is 1 but the index is 9

Repro:

let views = buffer[
    BinaryView::make_view(b"a longer string here", 0, 0),
    // Null row: buffer 9 does not exist and the offset is past the end of the data.
    BinaryView::new_ref(64, *b"junk", 9, 4096),
];
let array = VarBinViewArray::try_new(
    views,
    Arc::from([ByteBuffer::copy_from(b"a longer string here")]),
    DType::Utf8(Nullability::Nullable),
    Validity::from_iter([true, false]),
)?;

try_new succeeds, which is correct. Any kernel that then resolves row 1's bytes panics.

Note that no kernel on develop is broken by this, since byte_length reads view.len() out of the view rather than resolving the row. So this is a foot-gun plus a missing test rather than a live bug.

Two steps:

  1. Narrow consequence 2. Dense evaluation is sound when reading a row can't fault, which excludes offset-following elements.
  2. Add a regression test pinning byte_length over the array above to [Some(20), None], guarding against a later "optimization" into resolving rows.

Both exist on claude/strict-scalar-fn-abstraction-ah88x3 as a DENSE_SAFE const on the element type, but neither the doc fix nor the test needs any of that.

Activity

  1. connortsui20 commented on Jul 30, 2026

    @connortsui20
    MemberAuthor

    (llm generated, will touch up soon)

  2. self-assigned this
    on Jul 30, 2026
  3. robert3005 commented on Jul 30, 2026

    @robert3005
    Contributor

    I think we need to be able to push down some notion of only compute these rows and this should be a row demand, if the function is non panicing you can push it down without any row demand

  4. myrrc commented on Aug 11, 2026

    @myrrc
    Contributor

    Fixed by #9033 #9122

  5. connortsui20 commented on Aug 11, 2026

    @connortsui20
    MemberAuthor

    Those were docs changes to the idea of strictness, but we still dont have a good definition of "definedness" vs null values.

  6. connortsui20 commented on Aug 19, 2026

    @connortsui20
    MemberAuthor

    I believe this is closed by #9188

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

documentationImprovements or additions to documentation

Type

No type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions