Repository navigation
fix(python): apply offset when combining async hybrid results - #4028
Conversation
AsyncHybridQuery pushes offset() down into the vector and FTS sub-queries, so each of them skipped its own first N rows and the fused result was then sliced from position 0. A paginated async hybrid search therefore repeated rows from the previous page and dropped others entirely. Fetch limit + offset rows per sub-query and slice the requested window out of the combined, reranked table, matching the sync builder.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
![lancedb-gatekeeper[bot]](https://avatars.githubusercontent.com/in/4497274?s=60&v=4){kind=small}
There was a problem hiding this comment.
❌ Gate recommendation: request changes.
Moving the offset after fusion is the right boundary, but the revision still needs one canonical child-query window shared by execution and diagnostics. That window must preserve stable page semantics, honor the effective default limit regardless of builder order, and be the query that explain/analyze actually report.
| # to be taken out of the combined, reranked results instead, so | ||
| # fetch the skipped prefix here too and slice it off afterwards. | ||
| if limit: | ||
| fts_query.limit(limit + offset) |
There was a problem hiding this comment.
Fetching only limit + offset candidates makes the candidate set grow between page requests, so RRF can reorder earlier results and pagination still repeats and drops rows. On this head, source ranks vector [A, B, X, V4] and FTS [C, D, X, F4] produced page1=[A, C], page2=[C, B], while .limit(4) was [X, A, C, B]: C repeats and X is never visited. X enters both lists at rank 3, becomes the top fused result only in the deeper pool, and is then removed by the final offset. Please make the reranker candidate pool invariant across pages (or introduce an explicit fixed candidate/cursor contract) and cover a deep cross-list-overlap case.
Reproducer run against this head
names = ["A", "B", "X", "V4", "C", "D", "F4"]
texts = ["none", "none", "q q", "none", "q q q q", "q q q", "q"]
vectors = [[0, 0], [.1, 0], [.2, 0], [.3, 0], [10, 0], [11, 0], [12, 0]]
data = pa.table({'name': names, 'text': texts, 'vector': pa.array(vectors, type=pa.list_(pa.float32(), 2))})
with tempfile.TemporaryDirectory() as path:
db = await lancedb.connect_async(path)
table = await db.create_table("t", data)
await table.create_index("text", config=FTS(with_position=False))
base = table.query().nearest_to([0, 0]).nearest_to_text("q")
full = (await base.limit(4).to_arrow())["name"].to_pylist()
page1 = (await table.query().nearest_to([0, 0]).nearest_to_text("q").limit(2).to_arrow())["name"].to_pylist()
page2 = (await table.query().nearest_to([0, 0]).nearest_to_text("q").offset(2).limit(2).to_arrow())["name"].to_pylist()
assert not (set(page1) & set(page2)) # fails: {"C"}Observed: full=[X, A, C, B], page1=[A, C], page2=[C, B].
There was a problem hiding this comment.
Understood. I’ll treat the sync-parity candidate-window consequence as an accepted risk and won’t pursue this finding further.
| fts_query.with_row_id() | ||
| vec_query.with_row_id() | ||
|
|
||
| limit = self._inner.get_limit() |
There was a problem hiding this comment.
The effective limit is not available reliably from self._inner.get_limit(): HybridQuery.get_limit() reads only the FTS child, and an FTS-first hybrid without an explicit .limit() leaves that request at None even though hybrid search documents a default of 10. This skips the widening branch and passes limit=None to the combiner. With ten vector-only candidates and ten disjoint FTS candidates, .nearest_to_text("q").nearest_to([0, 0]).offset(2) returned 18 rows on this head instead of 10. Derive the effective hybrid limit independent of construction order (including the default) and add FTS-first coverage.
Reproducer run against this head
names = [f"V{i}" for i in range(10)] + [f"F{i}" for i in range(10)]
texts = ["none"] * 10 + ["q"] * 10
vectors = [[float(i), 0] for i in range(10)] + [[100.0 + i, 0] for i in range(10)]
data = pa.table({'name': names, 'text': texts, 'vector': pa.array(vectors, type=pa.list_(pa.float32(), 2))})
with tempfile.TemporaryDirectory() as path:
db = await lancedb.connect_async(path)
table = await db.create_table("t", data)
await table.create_index("text", config=FTS(with_position=False))
result = await table.query().nearest_to_text("q").nearest_to([0, 0]).offset(2).to_arrow()
assert len(result) == 10 # observed: 18There was a problem hiding this comment.
Fixed in 04d07c2: FTS-first hybrid queries without an explicit limit now fall back to 10, and the regression test verifies a 10-row result.
|
|
||
| limit = self._inner.get_limit() | ||
| offset = req.offset or 0 | ||
| if offset: |
There was a problem hiding this comment.
This rewrite is applied only in to_batches. explain_plan() and analyze_plan() still build the original child queries from self._inner, so they report and execute skip=2, fetch=2 while normal execution now uses skip=0, fetch=4. That makes both diagnostic APIs describe a different query from the one users run. Factor child-query preparation into a shared helper used by execution, explain, and analyze.
Reproducer run against this head
query = table.query().nearest_to([0, 0]).nearest_to_text("q").offset(2).limit(2)
await query.to_arrow()
plan = await query.explain_plan()
limits = [line.strip() for line in plan.splitlines() if "GlobalLimitExec" in line]
assert limits == [
"GlobalLimitExec: skip=0, fetch=4",
"GlobalLimitExec: skip=0, fetch=4",
] # observed two `skip=2, fetch=2` linesanalyze_plan() likewise reported two executed skip=2, fetch=2 operators.
There was a problem hiding this comment.
Fixed in 04d07c2: execution, explain_plan(), and analyze_plan() now share child-query preparation; the verified diagnostic plans report skip=0, fetch=4 for both children.
lancedb-gatekeeper
Bot
added
the
K-changes
winklemad
left a comment
winklemad
left a comment
There was a problem hiding this comment.
Thanks — this is a genuinely useful review, and I've verified all three against the source rather than taking them on trust.
Effective limit (:3888) — confirmed, my bug. HybridQuery::get_limit() at python/src/query.rs:1159 reads self.inner_fts.inner.current_request().limit, i.e. the FTS child only. So an FTS-first hybrid with no explicit .limit() returns None, my if limit: branch is skipped, and limit=None reaches the combiner. Your 18-rows-instead-of-10 result follows directly. I'll derive the effective limit independent of construction order, including the documented default of 10, and add FTS-first coverage.
explain/analyze divergence (:3890) — confirmed, my bug. explain_plan and analyze_plan build children straight from self._inner, so they bypass the rewrite I put in to_batches and report a different query than the one that executes. I'll factor child preparation into one helper used by all three.
Candidate pool (:3896) — you're right, and I want to flag something before fixing it. Widening to limit + offset does change the pool between pages, so RRF can reorder and pagination still repeats. But this is exactly what the merged sync path does today:
# LanceHybridQueryBuilder, sync
sub_query_limit = self._limit + (self._offset or 0)I deliberately mirrored that, on the reasoning that an async fix diverging from merged sync behaviour would be the wrong call to make unilaterally. So the invariant-pool problem isn't specific to this PR — sync has it too, and fixing it properly means changing behaviour on a path that's already released.
That's a scope decision I don't think is mine to make, so: would you prefer I
- keep this PR at parity with sync (fix the two above only) and open a separate issue for pool invariance covering both paths, or
- solve the invariant pool here for async, accepting that async and sync then differ until sync is updated, or
- fix both paths in this PR?
My preference is (1) — the two confirmed bugs above are self-contained and clearly wrong today, whereas pool invariance needs a deliberate contract (fixed candidate pool vs. an explicit cursor) that ought to apply to both paths at once. I'll also scope the PR description down so it doesn't claim more than it delivers.
Happy to go whichever way you'd rather. I'll push the two confirmed fixes in the meantime.
Two follow-ups to the offset fix. The effective limit was read from HybridQuery::get_limit(), which only looks at the FTS sub-query. nearest_to() and nearest_to_text() build their new sibling from a fresh query, and that is the sub-query the default limit is filled in on, so which side carries it depends on the order the hybrid query was built in. An FTS-first hybrid with no explicit limit therefore combined its results with no limit at all and returned the whole union of both candidate lists instead of the documented default of 10. The paging rewrite also lived inline in to_batches, so explain_plan() and analyze_plan() built their sub-queries straight from the hybrid query and reported a skip/fetch window that is never executed. Both now share _create_child_queries() with to_batches, so the plans describe the query that runs.
There was a problem hiding this comment.
The current revision fixes both actionable defects: FTS-first hybrids now retain the default limit of 10, and execution, explain_plan(), and analyze_plan() share the same child-query preparation.
The author has chosen sync parity. The accepted risk is that deeper hybrid pages can repeat or omit rows when the enlarged candidate pool changes reranking. No further change is requested for this pull request.
lancedb-gatekeeper
Bot
added
K-approved
|
Thanks @wjones127! |
Fixes #4027
Summary
AsyncHybridQuery(table.query().nearest_to(...).nearest_to_text(...)) paginates incorrectly when.offset()is used: the second page repeats rows from the first page and silently drops others.offset()on a hybrid query pushes the offset down into both sub-queries (HybridQuery::offsetinpython/src/query.rsforwards toinner_vecandinner_fts), so each sub-query independently skips its own firstoffsetrows before the results are fused.AsyncHybridQuery.to_batchesthen called_combine_hybrid_results(..., limit=self._inner.get_limit())without anoffset, so the reranked table was sliced starting at position 0 and the sub-query limits were never raised to cover the skipped prefix.On the 4-row fixture in
test_hybrid_query.py, with_rowidordering[3, 0, 2, 1]:.limit(2)[0, 3][0, 3].offset(2).limit(2)[3, 1][2, 1]Row
3was returned on both pages and row2was never returned at all.This is the async counterpart of #3769 (
Fixes #3765), which fixed the same bug in the synchronousLanceHybridQueryBuilder. #3765 explicitly deferred the async path; this PR closes that gap and reuses theoffsetparameter that #3769 already added to_combine_hybrid_results. The synchronous path is unaffected — it was fixed in #3769.Changes
python/python/lancedb/query.py,AsyncHybridQuery.to_batches:limit + offsetrows and its own offset is reset to 0, so the fused result contains the full prefix the window is sliced out of.offset=instead of always starting at 0.Both halves are needed: raising the sub-query limits without the final slice still returns page 1, and slicing without raising the limits still misses rows.
nodejshas no equivalent hybrid combine path, so there is no SDK parity gap here.Test plan
test_async_hybrid_query_offsetinpython/python/tests/test_hybrid_query.py, mirroring the synctest_hybrid_query_offset. It asserts the offset window is a suffix of the un-offset result and that page 1 + page 2 together cover every row exactly once (a row-count-only assertion would pass even with duplicates).pytest python/tests/test_hybrid_query.py— 16 passedpytest python/tests/test_rerankers.py— 9 passed, 11 skippedpytest python/tests/test_query.py— 86 passedpytest --doctest-modules python/lancedb/query.py— 13 passedruff format --check/ruff check— cleanScope, after review
@lancedb-gatekeeper raised three points. Two were mine and are fixed in
04d07c2; the third is deliberately left alone and I'd like a maintainer's call on it.Fixed — effective limit was read from the FTS child only.
HybridQuery::get_limit()(python/src/query.rs:1159) returnsself.inner_fts.inner.current_request().limit, so an FTS-first hybrid with no explicit.limit()yieldedNone, skipped the widening branch and passedlimit=Noneto the combiner — returning the union of both candidate lists instead of the documented default of 10. The limit is now derived from both children with aDEFAULT_HYBRID_LIMIT = 10fallback, so construction order no longer matters.Fixed —
explain_plan()/analyze_plan()described a different query than the one that ran. Both built their children straight fromself._inner, bypassing the limit/offset rewrite into_batches, and reportedskip=2, fetch=2while execution usedskip=0, fetch=4. Child preparation now lives in one_create_child_queries()helper used by all three.Not fixed here — RRF candidate-pool invariance. Widening each sub-query to
limit + offsetdoes change the candidate pool between page requests, so the fused ranking can shift and pagination can still repeat rows. That's a real problem, but it is exactly what the merged sync path does today:Making the pool invariant means choosing a contract — a fixed candidate pool, or an explicit cursor — and that ought to apply to sync and async together rather than letting the two paths diverge. I've asked in the review thread which way you'd prefer, and I'm happy to do it here or in a follow-up covering both paths.
So, to be precise about what this PR delivers: it makes
.offset()take effect on the async hybrid path and makes the diagnostics honest. It does not make hybrid pagination stable across pages under reranking — that needs the contract decision above.