feat: support Match operators and Phrase queries in prepared FTS - #74
Conversation
There was a problem hiding this comment.
❌ Gate recommendation: request changes.
Phrase preparation needs an explicit supported slop boundary. The pinned Lance executor narrows slop during positional evaluation, so accepting the entire uint32_t domain can silently omit valid phrase matches.
A viable revision would either validate and document a safely supported slop range at the FFI boundary, with boundary tests, or pin an upstream implementation that evaluates the advertised range without signed narrowing.
| let query = FullTextSearchQuery::new_query( | ||
| PhraseQuery::new(query_text) | ||
| .with_column(Some(column.clone())) | ||
| .with_slop(slop) |
There was a problem hiding this comment.
This forwards every uint32_t slop value, but pinned Lance e934cc2c evaluates it with check_positions(slop as i32). Values above i32::MAX therefore become negative and produce false negatives.
Reproducer
In test_prepared_fts_match_phrase_and_legacy_compatibility, after the existing slop-1 assertion, I ran:
let max = unsafe {
lance_dataset_prepare_fts_phrase_query(
dataset,
column.as_ptr(),
query.as_ptr(),
u32::MAX,
LanceFtsCoverageMode::Strict as i32,
)
};
assert!(!max.is_null());
let scores = collect_context_fts_scores(dataset, max, None);
assert!(scores.contains_key(&5));For query "quick brown", row 5 is "quick red brown fox". Preparation succeeded, but the final assertion failed; the same row is returned for slop 1.
Please either reject and document values the pinned executor cannot safely evaluate, with upper-bound regression tests, or use an upstream implementation that keeps the calculation in a non-narrowing domain.
There was a problem hiding this comment.
The boundary is still unsafe: INT32_MAX is accepted, but executing phrase "brown fox" against the existing "quick brown fox" row panics in pinned Lance at wand.rs:3941 (last + slop) because the phrase begins after token position zero. Please validate a range the executor can evaluate without overflow and cover its upper endpoint, or update the pinned executor to use non-overflowing arithmetic.
i think slop use int32_t is ok. and the boundary check shouldn't is here.The executor should perform the calculation without signed overflow. |
There was a problem hiding this comment.
❌ Gate recommendation: request changes.
The author selected the executor-level fix: keep int32_t at the FFI boundary and make phrase-position arithmetic non-overflowing. That direction is viable, but the current pinned executor still panics for an accepted INT32_MAX query, so the recommendation remains changes.
Acceptance requires landing and pinning the executor fix with upper-end execution coverage.
|
hi @jja725 could y have a look about this pr when your free times, very thanks |
jja725
left a comment
There was a problem hiding this comment.
slop = INT32_MAX is accepted, but pinned Lance performs i32 position arithmetic. For "prefix quick brown" queried as "quick brown", scanning panics with: attempt to add with overflow
@jja725 More importantly, I don't think lance-c should duplicate or guess executor-specific arithmetic limits. Lance-c should only validate its FFI contract, such as requiring a non-negative int32_t value. The positional arithmetic overflow should be handled at the calculation site in the Rust Lance executor, using non-overflowing arithmetic. Therefore, I prefer to fix this in Rust Lance rather than introduce an arbitrary slop upper bound in lance-c. |
Prepared fuzzy matching remains disabled with the pinned Lance revision. The
max_fuzzy_distanceparameter is reserved, and non-zero values return an error.