improvement(knowledge): rank inside the permitted set for organization search - #7996
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
|
There was a problem hiding this comment.
All reported issues were addressed across 5 files
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
|
@cubic-dev-ai review this PR |
@waleedlatif1 I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
All reported issues were addressed across 5 files
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
|
@cubic-dev-ai review this PR |
@waleedlatif1 I have started the AI code review. It will take a few minutes to complete. |
Summary
Organization search indexes grant most documents to one mailbox, channel, or file owner, so a typical member can read about 0.1% of the index. Both retrieval legs searched the whole index and checked access afterwards. The vector walk then post-filtered nearly every neighbour away, and common keyword terms ranked matches the member could never see.
retrieveKnowledgeSearchcallsresolvePermittedDocumentsbefore both legs for live user scopes (a user scope with an access provider and no explicitdocumentIds). It returnsbounded(documents plus their connector) orunbounded. The set is resolved with the same candidate predicate both legs apply, so restricting a leg to it can only narrow.boundedranks the permitted chunks exactly and skips the HNSW walk.unboundedruns the walk only, with no second tractability probe.boundedreads the permitted chunks directly throughembedding_keyword_search_document_idx, using anOFFSET 0fenced subquery aliased as the table. Base, enabled, tsquery and tag checks are applied outside it. The existing visibility CTE still re-applies the candidate predicate.doc_acl_gin_idx(deleted_at IS NULL AND acl && tokens) inside anOFFSET 0fence and applies the base check to the rows the index returns, so the planner cannot swap to a whole-base heap scan.pub/ws/org) are shared by every tenant; a global count would saturate for nearly every caller.saturatedsentinel row reportsunbounded.IN (subquery)is now a correlatedEXISTS, served by the observation PK(document_id, member_id). Semantics are identical: every compared column isNOT NULL, and the clause sits inside the connectorEXISTS, sodocument.connector_idis non-null there. PostgreSQL no longer hashes every observation once per statement.unboundedand the vector leg is marked timed out; the keyword leg is not failed.permitted_documentsand new metadatapermittedDocuments/permittedDocumentCount.Evidence
Server-side execution time on a restored copy of a large organization search index, measured with the same members and queries on main and on this branch:
EXISTS;VECTOR_PROBE_DOCUMENT_LIMIT(100k documents).Type of Change
Testing
VECTOR_PROBE_DOCUMENT_LIMIT + 1reachable documents in a sibling base must leave the target basebounded. The unit tests cover bounded exact ranking skips the walk; empty sets; no re-probe when unbounded; keyword confinement; excluded-source refill; saturation; budget exhaustion; resolved once before both legs. Each was confirmed to fail against a broken planner.type-check,lint,check:api-validationandcheck:auditsare clean.organization-personal-tokenstest that also fails on staging.Checklist