fix!: null handling when using NOT with scalar indices
#5270
Conversation
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
Scalar indexes currently treat NULL values as FALSE when evaluating filters, which violates Kleene three-valued logic. This causes bugs like `x != 5` incorrectly including rows where x is NULL. This commit adds the foundation for proper null handling: - Extended RowIdMask with null_list field to track NULL rows - Implemented Kleene logic for NOT, AND, OR operations - Updated SearchResult to carry null row information - Modified all index implementations for backward compatibility The infrastructure is complete but indexes don't yet track nulls. Follow-up commits will implement actual null tracking in BTree and Bitmap indexes. Fixes lance-format#4756 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
wjones127
added
the
critical-fix
Added unit tests to verify Kleene three-valued logic operations work correctly: - RowIdMask AND/OR/NOT operations with nulls - also_block and also_allow preserve null_list - Serialization/deserialization of null_list - Bitmap index returns null_list in SearchResult These tests verify the null tracking infrastructure works correctly at the Rust level. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Adds comprehensive tests to verify that bitmap and btree indexes correctly track and return null row IDs in query results. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
The [tool.ruff] section had a duplicate lint key which caused maturin to fail parsing the file. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
After adding null_list to RowIdMask, the serialization now produces 3 elements (block_list, allow_list, null_list) instead of 2. Updated serialize_to_arrow and try_from_arrow to handle 3 elements correctly. This fixes the "all columns in a record batch must have the same length" error when using scalar indexes with null tracking. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
The iter_ids() method was only filtering out block_list entries but not null_list entries. This caused nulls to be included in query results when they should be filtered out according to Kleene three-valued logic. Updated iter_ids() to filter out both block_list and null_list entries, ensuring that null values are never returned in iteration. Added test_iter_ids_with_nulls() to verify the fix. Note: The Python integration test still fails, indicating there may be another code path that needs fixing. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
wjones127
changed the title
NOT with scalar indices
wjones127
changed the title
NOT with scalar indicesNOT with scalar indices
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Previously, NOT operations on inexact index results (AtMost/AtLeast) would keep the same certainty variant while negating the mask. This was incorrect because: - NOT(AtMost(x)) should return AtLeast(!x), not AtMost(!x) (complement of superset is subset) - NOT(AtLeast(x)) should return AtMost(!x), not AtLeast(!x) (complement of subset is superset) This caused incorrect query results when using inexact indices like bloom filters and zonemaps with NOT operators. Changes: - Fixed NOT implementation to flip certainty variants - Removed unused `map` method that was replaced by explicit match - Added unit tests for certainty flipping - Added integration tests for NOT with nulls 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
westonpace
reviewed
Nov 25, 2025
Member
westonpace
left a comment
westonpace
left a comment
There was a problem hiding this comment.
This is great work!
Here is a first pass of questions before looking at the other files.
Comment on lines
+11
to
+14
| * The `SearchResult` returned by scalar indices must now output information about null values. | ||
| Instead of containing a `RowIdTreeMap`, it now contains a `NullableRowIdSet`. Expressions that | ||
| resolve to null values must be included in search results in the null set. This ensures that | ||
| `NOT` can be applied to index search results correctly. |
Member
There was a problem hiding this comment.
It's not clear why this is in the migration guide? Are we considering SearchResult and RowIdTreeMap to be part of our public API?
Or are you trying to explain that filter results may have changed? If it's the latter I think this needs to be reworded to be more clear that this means query results will be different.
Comment on lines
+44
to
+52
| /// Check if a row_id is selected (TRUE) | ||
| pub fn selected(&self, row_id: u64) -> bool { | ||
| self.selected.contains(row_id) && !self.nulls.contains(row_id) | ||
| } | ||
|
|
||
| /// Get the selected rows (TRUE or NULL) | ||
| pub fn selected_rows(&self) -> &RowIdTreeMap { | ||
| &self.selected | ||
| } |
Member
There was a problem hiding this comment.
It's a little unexpected that selected checks for TRUE but selected_rows is both TRUE and NULL. Maybe we could use not_false_rows instead of selected_rows?
Comment on lines
+17
to
+18
| selected: RowIdTreeMap, | ||
| nulls: RowIdTreeMap, |
Member
There was a problem hiding this comment.
Can a row id be in both sets? What does that signify?
| self.selected.clone() - self.nulls.clone() | ||
| } | ||
|
|
||
| pub fn union_all(selections: &[Self]) -> Self { |
Member
There was a problem hiding this comment.
What happens if a row id is null in one set and true in another? I guess that is a repeat of the "can a row id be in both sets" question?
Comment on lines
+182
to
+183
| (Self::AllowList(allow), Self::BlockList(block)) | ||
| | (Self::BlockList(block), Self::AllowList(allow)) => { |
Member
There was a problem hiding this comment.
I'm still a fan of simplifying things so that this case isn't allowed but we can investigate in future issue.
| /// This is often a result of a filter, where `selected` represents the rows that | ||
| /// passed the filter, and `nulls` represents the rows where the filter evaluated | ||
| /// to null. For example, in SQL `NULL > 5` evaluates to null. This is distinct | ||
| /// from being deselected to support proper three-valued logic for NOT. |
Member
There was a problem hiding this comment.
Can we go ahead and state here that NULL | TRUE = TRUE and that FALSE & NULL = FALSE?
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.
BREAKING CHANGE: The
SearchResultstruct returned byScalarIndex::search()now wraps aNullableRowIdSetinstead of aRowIdTreeMap. Scalar indices must now provide the set of row ids where the expression value is null instead of just where it is true. Additionally, theRowIdMaskis now an enum instead of a struct.This PR fixes correctness bugs that show up when (a) running a filter with
NOT, (b) the column you are filtering on contains nulls, and (c) we are using a scalar index (such as btree, or bitmap). Previously, this would give the wrong answer:It should not include
null. The reason it did is that ourRowIdMask(which is output by a scalar index query) was not aware of nulls. So when it processedvalue < 2, it would select just row index0. ThenNOTwould invert that to[1, 2], selecting both[false, null].This PR makes
RowIdMaskaware of nulls. When it processesvalue < 2, it recordsselected: [0]andnulls: [2]. Then, when you invert that and then drop, you getselected: [1], giving the correct final answer of just[5].As part of this, we also refactor RowIdMask to make allow list and deny list mutually exclusive, which simplifies some of the logic.
Fixes #4756