Skip to content

fix(scanner): do not lower a negated InList into a take - #9097

Open
LuciferYang wants to merge 2 commits into
lance-format:mainfrom
LuciferYang:fix/srid-rowid-not-in
Open

LuciferYang wants to merge 2 commits into
lance-format:mainfrom
LuciferYang:fix/srid-rowid-not-in

Conversation

@LuciferYang

@LuciferYang LuciferYang commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

Closes #9080.

TakeOperation::try_from_expr matched Expr::InList without consulting negated, so a filter like _rowid NOT IN (...) was planned as a take of exactly the listed ids, the complement of the predicate. The arm returns no remainder, so the predicate was dropped from the plan rather than merely ignored by the take.

Small lists never reached the lowering, because DataFusion's expression simplifier expands them into != conjunctions first. Only lists large enough to survive unexpanded hit it, which is why this is invisible in small examples.

Skipping the lowering for negated lists lets _rowid and _rowaddr run as regular scan filters, which has the right semantics: the else branch leaves expr_filter_plan untouched and hands the whole predicate to the filtered read.

_rowoffset

_rowoffset shares the arm, and the guard covers it in the sense that matters: a negated list is no longer answered as a take of the listed offsets. It cannot be answered as a scan filter either, though. _rowoffset is reachable only through this lowering, so it is absent from the filterable read schema and any predicate that survives to the planner is rejected. That is the column's existing limit rather than something the guard introduced: on main today, _rowoffset > 2 fails with Column _rowoffset does not exist from Projection::union_column, which has arms for _rowid and _rowaddr but none for _rowoffset. Adding that arm alone is not enough either; the filtered read then rejects the predicate at planner.rs:1089 with valid_fields: [idx, _rowid, _rowaddr], because the derived column is never materialized into the schema the filter is planned against.

Making every _rowoffset predicate work means requesting _rowaddr whenever a filter mentions _rowoffset and computing the derived column before filter refinement. That is a feature rather than part of this correctness fix, so it is filed as #9105. I am happy to do it there, or here if you would rather have both together.

Testing

  • test_filter_to_take_with_stable_row_ids gains a ten-element _rowid NOT IN case. Against the old code it returns idx = [0..9] instead of [10, 11]. It also pins the _rowoffset contract end to end: a negated _rowoffset list must fail rather than return the complement.
  • take_operation_rejects_negated_in_list exercises the lowering directly over all three columns that share the arm, so the coverage does not depend on the simplifier's expansion threshold staying where it is.

@github-actions github-actions Bot added the bug Something isn't working label Sep 9, 2026
lance-gatekeeper[bot]

This comment was marked as outdated.

@lance-gatekeeper lance-gatekeeper Bot added the K-changes Latest Gatekeeper recommendation requests changes. label Sep 9, 2026
… ids

TakeOperation::try_from_expr matched Expr::InList without consulting
negated, so a filter like _rowid NOT IN (...) that survived the
expression simplifier unexpanded (the simplifier only expands small
lists) was planned as a take of exactly the listed ids — the complement
of the predicate. Large NOT-IN lists on _rowid/_rowaddr/_rowoffset
silently returned the wrong rows.

Skip the lowering for negated lists; the filter then runs as a regular
scan filter with correct semantics. The regression test uses a list
large enough to survive unexpanded.
@lance-gatekeeper lance-gatekeeper Bot removed the K-changes Latest Gatekeeper recommendation requests changes. label Sep 10, 2026

@lance-gatekeeper lance-gatekeeper Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Gate recommendation: approve with a non-blocking risk.

The guard removes the silent wrong-result path for all three system columns. _rowid and _rowaddr return the complement; _rowoffset NOT IN now fails explicitly because general _rowoffset scan filtering is not supported. #9105 tracks that bounded feature gap. An explicit error is safer than returning wrong rows, so it does not need to hold this fix.

@lance-gatekeeper lance-gatekeeper Bot added K-approved Latest Gatekeeper recommendation permits acceptance. K-risk Latest Gatekeeper recommendation includes a non-blocking risk. labels Sep 10, 2026

@wjones127 wjones127 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good fix. Thank you!

@lance-gatekeeper lance-gatekeeper Bot removed K-approved Latest Gatekeeper recommendation permits acceptance. K-risk Latest Gatekeeper recommendation includes a non-blocking risk. labels Sep 17, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug: _rowid NOT IN (...) returns exactly the listed rows instead of their complement

3 participants