-
Notifications
You must be signed in to change notification settings - Fork 29
limiting the exact limit search window to 5000 records #358
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -488,6 +488,50 @@ def get_image_embedding_dino_exact_match( | |||||||||||||||||||
|
|
||||||||||||||||||||
| return sql_query, params | ||||||||||||||||||||
|
|
||||||||||||||||||||
| def get_filtered_document_count_query( | ||||||||||||||||||||
| self, | ||||||||||||||||||||
| kb_id: str, | ||||||||||||||||||||
| filter1: Optional[str] = None, | ||||||||||||||||||||
| document_date_start: Optional[Any] = None, | ||||||||||||||||||||
| document_date_end: Optional[Any] = None, | ||||||||||||||||||||
| filter2: Optional[str] = None, | ||||||||||||||||||||
| filter3: Optional[str] = None, | ||||||||||||||||||||
| filter4: Optional[str] = None, | ||||||||||||||||||||
| filter5: Optional[str] = None, | ||||||||||||||||||||
| filter6: Optional[str] = None, | ||||||||||||||||||||
| created_at_start: Optional[Any] = None, | ||||||||||||||||||||
| created_at_end: Optional[Any] = None, | ||||||||||||||||||||
| ) -> Tuple[str, Dict[str, Any]]: | ||||||||||||||||||||
| """ | ||||||||||||||||||||
| Cheap, index-only count of documents in `kb_id` matching the same | ||||||||||||||||||||
| filters as `get_image_embedding_dino_exact_match`. Used as a | ||||||||||||||||||||
| pre-flight check against a candidate-count cap before running that | ||||||||||||||||||||
| much more expensive brute-force query. | ||||||||||||||||||||
| """ | ||||||||||||||||||||
| filter_columns_clause, params = self.build_filter_columns_clause( | ||||||||||||||||||||
| filter1, | ||||||||||||||||||||
| filter2, | ||||||||||||||||||||
| filter3, | ||||||||||||||||||||
| filter4, | ||||||||||||||||||||
| filter5, | ||||||||||||||||||||
| filter6, | ||||||||||||||||||||
| document_date_start, | ||||||||||||||||||||
| document_date_end, | ||||||||||||||||||||
| table_alias='d', | ||||||||||||||||||||
| created_at_start=created_at_start, | ||||||||||||||||||||
| created_at_end=created_at_end, | ||||||||||||||||||||
| ) | ||||||||||||||||||||
| params['kb_id'] = str(kb_id) | ||||||||||||||||||||
|
|
||||||||||||||||||||
| sql_query = f""" | ||||||||||||||||||||
| SELECT COUNT(*) AS candidate_count | ||||||||||||||||||||
| FROM {KnowledgeBaseDocuments.__tablename__} d | ||||||||||||||||||||
| WHERE d.knowledge_base_id = :kb_id | ||||||||||||||||||||
| {filter_columns_clause} | ||||||||||||||||||||
|
Comment on lines
+527
to
+530
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win Count embedding rows that exact matching can score. This query counts Join Proposed fix- SELECT COUNT(*) AS candidate_count
- FROM {KnowledgeBaseDocuments.__tablename__} d
+ SELECT COUNT(*) AS candidate_count
+ FROM {KnowledgeBaseEmbeddings.__tablename__} e
+ JOIN {KnowledgeBaseDocuments.__tablename__} d ON e.document_id = d.id
WHERE d.knowledge_base_id = :kb_id
{filter_columns_clause}📝 Committable suggestion
Suggested change
🧰 Tools🪛 Ruff (0.16.3)[error] 526-531: Possible SQL injection vector through string-based query construction (S608) 🤖 Prompt for AI Agents |
||||||||||||||||||||
| """ | ||||||||||||||||||||
|
|
||||||||||||||||||||
| return sql_query, params | ||||||||||||||||||||
|
|
||||||||||||||||||||
| def get_documents_list_query( | ||||||||||||||||||||
| self, | ||||||||||||||||||||
| kb_id: str, | ||||||||||||||||||||
|
|
||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,6 +9,16 @@ | |
| from db_repo_module.repositories.sql_alchemy_repository import SQLAlchemyRepository | ||
| from sqlalchemy.exc import SQLAlchemyError | ||
|
|
||
| # Hard ceiling on exact_match_dino's candidate count -- no config/env var can | ||
| # exceed this, so a misconfiguration can't fully disable the safety guard. | ||
| EXACT_MATCH_HARD_CEILING = 5_000 | ||
|
|
||
| # Fallback candidate cap used when the caller doesn't supply one (e.g. older | ||
| # callers, or config missing the `knowledge_base.exact_match_max_candidates` | ||
| # key). Deliberately conservative; tune based on real p95 latency | ||
| # measurements against the target KB size. | ||
| DEFAULT_EXACT_MATCH_MAX_CANDIDATES = 1_000 | ||
|
|
||
|
|
||
| @dataclass | ||
| class ImageMatch: | ||
|
|
@@ -116,6 +126,7 @@ async def exact_match_dino( | |
| filter6: Optional[str] = None, | ||
| created_at_start=None, | ||
| created_at_end=None, | ||
| max_candidates: Optional[int] = None, | ||
| ) -> list[dict]: | ||
| """ | ||
| Exact (non-ANN) DINO similarity match, restricted to documents in | ||
|
|
@@ -133,7 +144,52 @@ async def exact_match_dino( | |
| (`QueryGenerator.get_image_embedding_dino_exact_match`) never engages the | ||
| HNSW index -- see that method's docstring -- so scores returned here | ||
| are always exact, not approximate. | ||
|
|
||
| Before any of that, runs a cheap count of matching documents and | ||
| rejects with a 422 if it exceeds `max_candidates` (clamped to | ||
| `EXACT_MATCH_HARD_CEILING`), so an oversized candidate set fails | ||
| fast instead of brute-forcing distances over it. | ||
| """ | ||
| effective_cap = min( | ||
| max_candidates or DEFAULT_EXACT_MATCH_MAX_CANDIDATES, | ||
| EXACT_MATCH_HARD_CEILING, | ||
| ) | ||
| try: | ||
| count_query, count_params = ( | ||
| self.query_generator.get_filtered_document_count_query( | ||
| kb_id, | ||
| filter1, | ||
| document_date_start, | ||
| document_date_end, | ||
| filter2, | ||
| filter3, | ||
| filter4, | ||
| filter5, | ||
| filter6, | ||
| created_at_start, | ||
| created_at_end, | ||
| ) | ||
| ) | ||
| count_rows = await self.knowledge_base_embeddings_repository.execute_query( | ||
| count_query, | ||
| count_params, | ||
| ) | ||
|
Comment on lines
+173
to
+176
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift Use one database snapshot for the cap check and exact query. Line 173 completes the count in one repository session. The exact query uses another session after the inference request. Documents can be inserted after the count and before the exact query. The exact query can then compute distances for more than Fetch the embedding first. Then run the count and exact query in one repeatable-read transaction, or enforce the cap in one SQL operation. 🤖 Prompt for AI Agents |
||
| except SQLAlchemyError as e: | ||
| raise RuntimeError( | ||
| f'Failed to execute the candidate-count query for exact match retrieval: {e}' | ||
| ) | ||
|
|
||
| candidate_count = int(count_rows[0]['candidate_count']) if count_rows else 0 | ||
| if candidate_count > effective_cap: | ||
| raise HTTPException( | ||
| status_code=status.HTTP_422_UNPROCESSABLE_ENTITY, | ||
| detail=( | ||
| f'{candidate_count} documents match the given filters, which ' | ||
| f'exceeds the exact-match safety limit of {effective_cap}. ' | ||
| 'Narrow your date range or filters and try again.' | ||
| ), | ||
| ) | ||
|
|
||
| data = {'image_data': image_data} | ||
| internal_api_url = f'{inference_url}/inference/v1/query/embeddings' | ||
| try: | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Normalize non-positive candidate caps.
If
KB_EXACT_MATCH_MAX_CANDIDATES=-1, the controller returns-1and the service retains it. Then even a zero candidate count is greater than the cap, so every exact-match request returns HTTP 422.wavefront/server/modules/knowledge_base_module/knowledge_base_module/controllers/rag_retreival_controller.py#L226-L226: replace non-positive configured values withDEFAULT_EXACT_MATCH_MAX_CANDIDATESbefore applying the hard ceiling.wavefront/server/modules/knowledge_base_module/knowledge_base_module/services/image_rag_retrieve.py#L153-L156: enforce the same positive-value invariant for direct callers.📍 Affects 2 files
wavefront/server/modules/knowledge_base_module/knowledge_base_module/controllers/rag_retreival_controller.py#L226-L226(this comment)wavefront/server/modules/knowledge_base_module/knowledge_base_module/services/image_rag_retrieve.py#L153-L156🤖 Prompt for AI Agents