fix(fts): include must-not clauses in query planning - #8443
Conversation
|
The current CI failures are unrelated to this PR. This is being fixed in #8440. |
There was a problem hiding this comment.
❌ Gate recommendation: request changes.
Partial-column resolution must preserve every leaf's query semantics and handle accepted query shapes without panicking. A viable revision would clone missing Match leaves per inferred column while retaining all options, and either reject partially specified MultiMatch queries with a normal input error or flatten them safely.
| Self::Boolean(query) => { | ||
| query.must.iter().any(|q| q.is_missing_column()) | ||
| || query.should.iter().any(|q| q.is_missing_column()) | ||
| || query.must_not.iter().any(|q| q.is_missing_column()) |
There was a problem hiding this comment.
Filling a missing must_not Match across multiple indexed columns currently rebuilds it with MultiMatchQuery::try_new(terms, columns), which silently discards boost, fuzziness, max_expansions, operator, prefix_length, and document_granularity. This can change both matches and scores; clone the original Match for each inferred column and change only column.
Reproducer run on this head
let implicit = MatchQuery::new("exclude terms".into())
.with_boost(3.0)
.with_operator(Operator::And);
let query = FtsQuery::Boolean(BooleanQuery::new([
(Occur::Must, FtsQuery::Match(
MatchQuery::new("include".into()).with_column(Some("a".into())))),
(Occur::MustNot, FtsQuery::Match(implicit)),
]));
let FtsQuery::Boolean(filled) = fill_fts_query_column(
&query, &["a".into(), "b".into()], false).unwrap() else { unreachable!() };
let FtsQuery::MultiMatch(expanded) = &filled.must_not[0] else { unreachable!() };
assert_eq!(expanded.match_queries[0].boost, 3.0);
assert_eq!(expanded.match_queries[0].operator, Operator::And);The executed assertion observed boost as 1.0 instead of 3.0; the reconstructed child also uses the default Or operator.
There was a problem hiding this comment.
Fixed on the current head: implicit Match expansion now clones the original MatchQuery and changes only column, preserving the query options. I reran the original reproducer; boost and operator are retained. Resolving this thread.
| ) -> Result<FullTextSearchQuery> { | ||
| let mut resolved = query.clone(); | ||
| if resolved.columns().is_empty() { | ||
| if resolved.query.is_missing_column() { |
There was a problem hiding this comment.
This broader gate can now panic for a partially specified MultiMatchQuery. With more than one indexed field, filling its missing Match child returns another MultiMatch, but the parent fill arm assumes every child remains Match and enters unreachable!("Expected MatchQuery"). These queries are constructible through the public JSON parser, so this should either return a descriptive input error or flatten the expansion without panicking.
Reproducer run on this head
let partial = MultiMatchQuery { match_queries: vec![
MatchQuery::new("include".into()).with_column(Some("a".into())),
MatchQuery::new("include".into()),
] };
let query = FtsQuery::Boolean(BooleanQuery::new([
(Occur::Must, FtsQuery::MultiMatch(partial)),
(Occur::MustNot, FtsQuery::Match(
MatchQuery::new("exclude".into()).with_column(Some("a".into())))),
]));
let outcome = std::panic::catch_unwind(||
fill_fts_query_column(&query, &["a".into(), "b".into()], false));
assert!(outcome.is_ok());The executed test panicked at query.rs:929 with internal error: entered unreachable code: Expected MatchQuery.
There was a problem hiding this comment.
Fixed on the current head: partial MultiMatchQuery expansion now flattens child Match and MultiMatch results instead of assuming every child remains a Match. I reran the original catch_unwind reproducer; it completes without a panic. Resolving this thread.
75a3ca4 to
cd596cc
Compare
There was a problem hiding this comment.
✅ Gate recommendation: approve.
The two earlier findings are fixed: implicit field expansion now preserves each Match query’s options, and partially specified MultiMatch queries expand without panicking. The Boolean must_not path now applies the intended exclusion across all indexed fields and fragment coverage.
Summary
Tests
cargo fmt --all -- --checkcargo test -p lance-index scalar::inverted::query::tests --libcargo test -p lance --features slow_tests --test integration_tests test_boolean_must_not_uses_all_index_fragment_coveragecargo clippy --all --tests --benches -- -D warnings(blocked by pre-existingmaintest compilation errors inrust/lance/src/dataset/transaction.rs: test constructors still usemerged_generations, whileOperation::Updateand the protobuf now exposecompacted_sstables)