feat(scanner): ignore selected scalar indices - #9034
Conversation
github-actions
Bot
added
A-python
majin1102
changed the title
Codex Review SummaryThis 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. |
![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.
Reviewed commit: 9503ceeb27
ℹ️ 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".
![lance-gatekeeper[bot]](https://avatars.githubusercontent.com/in/4433141?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
lance-gatekeeper
Bot
added
the
K-changes
lance-gatekeeper
Bot
removed
the
K-changes
There was a problem hiding this comment.
✅ Gate recommendation: approve.
The Java/JNI naming issue is fixed in 89c43eca: the public option and its full binding path now consistently use ignoredScalarIndices. The selective exclusion remains at the scalar-parser construction boundary, preserving the original filter and all unaffected index paths.
lance-gatekeeper
Bot
added
the
K-approved
LuciferYang
left a comment
LuciferYang
left a comment
There was a problem hiding this comment.
The placement is good. Only one production site reads ScalarIndexInfo (the expression-filter branch of create_plan; the other three scalar_index_info calls in filtered_read.rs are inside #[cfg(test)] mod tests), so there is no path left unwired, and scalar_index_info_ignoring rebuilds from load_indices() on every call without writing any session cache, so a filtered result cannot leak to another caller.
Four comments inline, two MEDIUM and two LOW, none blocking. The one I would like settled before merge is the fast_search interaction: putting a column's only scalar index in the ignore list removes the index query, which also removes the only_indexed_fragments restriction, so the same fast_search query comes back with a different row set. The result is a superset rather than wrong rows, and use_scalar_index(false) already behaves this way, but the docs enumerate the other cases the option does not control and omit this one.
The second MEDIUM is observability: an unmatched name is a no-op that nothing reports, and the cross-process use case this option exists for is exactly where names drift. One debug log covers it.
Two things I checked that are fine: the Java and Python plumbing has no silent drop (env.get_strings_opt matches Optional<List<String>>, both native declarations agree with the Rust parameter positions, the Builder copy constructor carries the field, and the pyo3 signatures line up with to_scanner's positional call), and the ignore list correctly stays a planning-time input rather than something that has to travel with a serialized plan, since index selection is already baked into the physical plan.
| /// # Ok(()) | ||
| /// # } | ||
| /// ``` | ||
| pub fn with_ignored_scalar_indices<I, S>(&mut self, index_names: I) -> &mut Self |
There was a problem hiding this comment.
[MEDIUM · behavior compatibility] fast_search only sets only_indexed_fragments when filter_plan.has_index_query() holds (scanner.rs:3543). Once the only scalar index on a column is in the ignore list, create_filter_plan produces no index query, that branch is skipped, and unindexed fragments get scanned too, so the same fast_search query returns a different row set with the ignore list than without it. It is a superset, since the filter still refines, so no wrong rows.
The docs here enumerate what the option does not control (vector index segments, explicit full-text search, and being ignored under use_scalar_index=false) but not fast_search. use_scalar_index(false) behaves the same way, so the interaction is not new; a selective switch just makes it easier to hit.
A sentence in the docs is enough. If you do change the behavior, do not keep only_indexed_fragments unconditionally: existing fast_search queries whose filter has no usable index would start dropping unindexed fragments.
| // so the optimizer treats coverage as unknown. | ||
| let mut fragment_bitmaps: HashMap<(String, String), Option<RoaringBitmap>> = HashMap::new(); | ||
| for index in indices.iter().filter(|idx| { | ||
| if ignored_index_names.contains(&idx.name) { |
There was a problem hiding this comment.
[MEDIUM · robustness] The filter is ignored_index_names.contains(&idx.name), so a misspelled name or a renamed index makes the option do nothing, and nothing reports the miss: explain_plan still prints @a_zonemap(ZoneMap), exactly as it would without the ignore list, and no log says which names matched nothing.
The use case this option exists for passes names across processes (the driver uses the ZoneMap, the executor skips it), which is exactly where names drift. The only symptom is the executor loading and probing the index for nothing while still returning the right rows, so nobody finds out.
A difference after load_indices() and one debug log for the names that matched nothing is enough, or have explain_plan(verbose) list the ignored names.
| .build(); | ||
| try (AsyncScanner scanner = AsyncScanner.create(dataset, options, allocator); | ||
| ArrowReader reader = scanner.scanBatchesAsync().get(10, TimeUnit.SECONDS)) { | ||
| assertEquals(20, countRows(reader)); |
There was a problem hiding this comment.
[LOW · tests] The test creates id_btree_index, puts it in the ignore list, and then asserts only that the row count is 20, which it is either way, so the test passes whether or not the ignore list is wired up. The blocking sibling (ScannerTest.java:1294) distinguishes with getIndexComparisons() == 0, and the Rust test asserts the plan no longer contains the index; both are effective.
AsyncScanner has no getStats, so the blocking assertion cannot be copied, and re-asserting getIgnoredScalarIndices() would add nothing because ScannerTest already does it. Exercising the async path for real needs a stats accessor on AsyncScanner; without one, this test only covers what it shares with the blocking path through build_scanner_with_options.
| * @param indexNames logical scalar index names to ignore. | ||
| * @return Builder instance for method chaining. | ||
| */ | ||
| public Builder ignoredScalarIndices(List<String> indexNames) { |
There was a problem hiding this comment.
[LOW · docs] The early return at index.rs:3449 sits ahead of the FTS branch in the same closure, and ScalarIndexInfo does carry inverted indices (inverted.rs:456's new_query_parser registers the FtsQueryParser that handles contains_tokens). So putting an inverted index name in the ignore list also turns off contains_tokens pushdown, falling back to the flat UDF.
All three doc copies say only that the option does not control explicit full-text search. That is true, since explicit FTS never goes through ScalarIndexInfo, but it reads as though inverted indices are unaffected. One sentence saying inverted index names are honored too would cover it.
2 participants
Summary
with_ignored_scalar_indices), Java (ignoredScalarIndices), and Python (ignored_scalar_indices)Motivation
External query planners can consume a scalar index during task planning and pass the resulting row selection to Lance workers. For example, lance-spark can query a ZoneMap on the driver, produce fragment-local physical slices, and then avoid querying that same ZoneMap again on executors without disabling useful BTree or other scalar indices.
This fills the gap between the existing all-or-nothing
use_scalar_indexoption and retaining every available scalar index.Semantics
use_scalar_index = falsecontinues to disable all scalar-index planningValidation
cargo fmt --all --checkcargo check -p lance --testscd java && ./mvnw spotless:checkcd java && ./mvnw -Dtest=ScannerTest,AsyncScannerTest test(54 Java tests and 22 JNI tests)cd python && cargo check --releaseThe PR is intentionally opened as a draft so the API shape and naming can be discussed before lance-spark adopts it.