fix: RAG filtering error with multiple where conditions (fixes #1728)#1729
fix: RAG filtering error with multiple where conditions (fixes #1728)#1729praisonai-triage-agent[bot] wants to merge 3 commits into
Conversation
…ator - Fix ChromaKnowledgeAdapter to handle multiple filter conditions properly - Use ChromaDB's operator when len(where_filter) > 1 - Resolves validation error: len(where) != 1 when filtering by user_id, agent_id, run_id - Maintains backward compatibility with single condition filters - Fixes #1728 Co-authored-by: praisonai-triage-agent[bot] <praisonai-triage-agent[bot]@users.noreply.github.com>
|
@coderabbitai review |
|
/review |
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
✅ Actions performedReview triggered.
|
|
Important Review skippedBot user detected. To trigger a single review, invoke the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe PR fixes a ChromaDB query construction bug where multiple scoped filters ( ChangesChromaDB Query Filter Normalization
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@copilot Do a thorough review of this PR. Read ALL existing reviewer comments above from Qodo, Coderabbit, and Gemini first — incorporate their findings. Review areas:
|
Greptile SummaryThis PR fixes a
Confidence Score: 5/5Safe to merge — the change is a minimal, targeted fix for a well-understood ChromaDB validation constraint. The fix correctly uses ChromaDB's documented $and logical operator to combine multiple metadata filter conditions. ChromaDB accepts simple key-value shorthand inside $and arrays (treated as implicit $eq), so the produced filter structure is valid. The single-condition path is unchanged, preserving backward compatibility. Two focused unit tests verify both branches. No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[search called with user_id / agent_id / run_id] --> B[Build where_filter dict]
B --> C[Merge with extra filters]
C --> D{len where_filter}
D -- "== 0" --> E[No where clause]
D -- "== 1" --> F["query_kwargs['where'] = where_filter as-is"]
D -- "> 1" --> G["and_conditions = [{k:v} for k,v in where_filter.items()]"]
G --> H["query_kwargs['where'] = {$and: and_conditions}"]
E --> I[collection.query]
F --> I
H --> I
I --> J[Return SearchResult]
Reviews (2): Last reviewed commit: "fix: add logging to ChromaDB search exce..." | Re-trigger Greptile |
Agent-Logs-Url: https://github.com/MervinPraison/PraisonAI/sessions/49aeab8d-0c2b-4a87-b55a-e30f79086d29 Co-authored-by: MervinPraison <454862+MervinPraison@users.noreply.github.com>
Completed a full review and incorporated all available reviewer outputs. Actionable gap found was test coverage, so I added focused unit tests in commit Review outcome: changes remain minimal and focused, no secrets/unsafe eval paths introduced, no hot-path import/perf regression, backward compatibility is preserved for single-condition filters, and code quality is consistent. Reviewer status incorporated: Qodo had no technical findings (reviews paused), CodeRabbit had no actionable issues, and Gemini had no posted code-review comments on this PR run. |
|
@claude You are the FINAL architecture reviewer. If the branch is under MervinPraison/PraisonAI (not a fork), you are able to make modifications to this branch and push directly. SCOPE: Focus ONLY on Python packages (praisonaiagents, praisonai). Do NOT modify praisonai-rust or praisonai-ts. Read ALL comments above from Gemini, Qodo, CodeRabbit, and Copilot carefully before responding. Phase 1: Review per AGENTS.md
Phase 2: FIX Valid Issues Phase 3: Final Verdict |
|
Claude finished @MervinPraison's task —— View job Final Architecture Review ✅Phase 1: Review per AGENTS.md
Phase 2: Address Reviewer Feedback
Phase 3: Final Verdict
Architecture Review Summary✅ Architectural ComplianceProtocol-Driven Design: ✅ PASSES
Backward Compatibility: ✅ PASSES
Performance: ✅ PASSES
✅ Code Quality ReviewCore Fix: The ChromaDB query bug fix is architecturally sound: # Before: {'user_id': 'u1', 'agent_id': 'a1'} - ChromaDB rejects this
# After: {'$and': [{'user_id': 'u1'}, {'agent_id': 'a1'}]} - ChromaDB accepts
if len(where_filter) == 1:
query_kwargs["where"] = where_filter # Single condition - pass through
else:
and_conditions = [{k: v} for k, v in where_filter.items()]
query_kwargs["where"] = {"$and": and_conditions} # Multi - wrap with $andTest Coverage: ✅ Adequate (lines 181-230 in test_adapters.py)
🔧 Issue ResolutionGreptile's Silent Exception Concern: ✅ ADDRESSED
Other Reviewer Feedback: ✅ NO ACTIONABLE ISSUES
📋 Files Modified
Final Verdict: ✅ APPROVEDThis PR successfully fixes the ChromaDB filtering bug while maintaining architectural principles. The solution is minimal, focused, and follows the protocol-driven design patterns established in AGENTS.md. Ready to merge - all architectural requirements satisfied and reviewer concerns addressed. |
Improves debugging for ChromaDB query failures by logging warnings instead of silently returning empty results. Co-authored-by: Mervin Praison <MervinPraison@users.noreply.github.com>
Summary
Fixes the RAG filtering error where len(where) != 1 fails when the where parameter contains multiple conditions.
Problem
When using RAG with multiple filter conditions (user_id, agent_id, run_id), the ChromaKnowledgeAdapter creates a dictionary with multiple keys, but ChromaDB's internal validation expects exactly one condition in the where clause.
Solution
Modified the ChromaKnowledgeAdapter to use ChromaDB's $and operator when multiple filter conditions are present:
Changes
Testing
Fixes
Fixes #1728
Generated with Claude Code
Summary by CodeRabbit