feat: add gRPC support with configurable options for Qdrant connection#51
Open
srimon12 wants to merge 2 commits into
Open
feat: add gRPC support with configurable options for Qdrant connection#51srimon12 wants to merge 2 commits into
srimon12 wants to merge 2 commits into
Conversation
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/qql/ast_nodes.py`:
- Around line 28-30: Parser currently initializes exact/acorn/indexed_only to
False and merges with "or", which loses explicit false; update
src/qql/parser.py::_parse_with_clause to initialize
SearchWith.exact/acorn/indexed_only as None (not False) and change merging logic
to coalesce using "if parsed_with.<field> is not None then parsed_with.<field>
else with_clause.<field>" (or equivalent is not None check) instead of "or" so
explicit False wins; also update src/qql/executor.py::_build_search_params to
map these three fields by checking "is not None" and passing True/False through
as-is (return value = field if field is not None else None) so False remains an
explicit disable for exact, acorn, and indexed_only.
In `@src/qql/cli.py`:
- Around line 243-250: The QdrantClient created in the try block may not be
closed if client.get_collections() raises; ensure the client is always closed by
restructuring to either use a context manager for QdrantClient or add a
try/finally that calls client.close() after creating the client (i.e., create
client = QdrantClient(**client_kwargs) then in finally call client.close()),
keeping the error logging in the except block and sys.exit(1) after the finally.
In `@src/qql/executor.py`:
- Around line 264-269: The current except block in executor.py that inspects
ValueError (the block referencing variable name and raising QQLRuntimeError)
uses a brittle literal substring check; replace it with a robust,
case-insensitive regex (or two-part check) that looks for "not found" and the
collection name allowing optional surrounding quotes (e.g. use
re.search(fr"collection\s+['\"]?{re.escape(name)}['\"]?\s+not\s+found", str(e),
re.I) or fall back to checking "not found" in str(e). If the regex matches,
return None; otherwise raise the QQLRuntimeError as before (preserving the
original exception chain).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 2d7d6693-1448-4233-ab2e-d177e9819b16
📒 Files selected for processing (9)
src/qql/__init__.pysrc/qql/ast_nodes.pysrc/qql/cli.pysrc/qql/config.pysrc/qql/connection.pysrc/qql/executor.pysrc/qql/parser.pytests/test_connection.pytests/test_executor.py
Comment on lines
+28
to
+30
| exact: bool | None = None | ||
| acorn: bool | None = None | ||
| indexed_only: bool | None = None |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check all references to exact/acorn/indexed_only fields in SearchWith
rg -n -C3 'with_clause\.(exact|acorn|indexed_only)' --type=pyRepository: pavanjava/qql
Length of output: 5734
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show SearchWith definition around the reported lines
sed -n '1,120p' src/qql/ast_nodes.py | nl -ba | sed -n '1,120p'
# Show executor section building SearchParams
sed -n '1300,1405p' src/qql/executor.py | nl -ba
# Show parser around merging/propagating with_clause options
sed -n '740,860p' src/qql/parser.py | nl -ba
# Search for explicit parsing of indexed_only/acorn/exact values
rg -n "indexed_only|acorn|exact" src/qql/parser.py | head -n 200Repository: pavanjava/qql
Length of output: 100
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== src/qql/ast_nodes.py (SearchWith definition) ==="
sed -n '1,120p' src/qql/ast_nodes.py | nl -ba | sed -n '1,120p'
echo "=== src/qql/executor.py (SearchParams construction) ==="
sed -n '1300,1405p' src/qql/executor.py | nl -ba
echo "=== src/qql/parser.py (with_clause merging) ==="
sed -n '740,860p' src/qql/parser.py | nl -ba
echo "=== src/qql/parser.py (mentions of indexed_only/acorn/exact) ==="
rg -n "indexed_only|acorn|exact" src/qql/parser.py | head -n 200Repository: pavanjava/qql
Length of output: 171
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== src/qql/ast_nodes.py (SearchWith definition) ==="
sed -n '1,120p' src/qql/ast_nodes.py | nl -ba | sed -n '1,120p'
echo "=== src/qql/executor.py (SearchParams construction) ==="
sed -n '1300,1405p' src/qql/executor.py | nl -ba
echo "=== src/qql/parser.py (with_clause merging) ==="
sed -n '740,860p' src/qql/parser.py | nl -ba
echo "=== src/qql/parser.py (mentions of indexed_only/acorn/exact) ==="
rg -n "indexed_only|acorn|exact" src/qql/parser.py | head -n 200Repository: pavanjava/qql
Length of output: 171
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== src/qql/ast_nodes.py (SearchWith definition) ==="
sed -n '1,120p' src/qql/ast_nodes.py | nl -ba | sed -n '1,120p'
echo "=== src/qql/executor.py (SearchParams construction) ==="
sed -n '1300,1405p' src/qql/executor.py | nl -ba
echo "=== src/qql/parser.py (with_clause merging) ==="
sed -n '740,860p' src/qql/parser.py | nl -ba
echo "=== src/qql/parser.py (mentions of indexed_only/acorn/exact) ==="
rg -n "indexed_only|acorn|exact" src/qql/parser.py | head -n 200Repository: pavanjava/qql
Length of output: 171
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== src/qql/ast_nodes.py (SearchWith definition) ==="
sed -n '1,120p' src/qql/ast_nodes.py | nl -ba | sed -n '1,120p'
echo "=== src/qql/executor.py (SearchParams construction) ==="
sed -n '1300,1405p' src/qql/executor.py | nl -ba
echo "=== src/qql/parser.py (with_clause merging) ==="
sed -n '740,860p' src/qql/parser.py | nl -ba
echo "=== src/qql/parser.py (mentions of indexed_only/acorn/exact) ==="
rg -n "indexed_only|acorn|exact" src/qql/parser.py | head -n 200Repository: pavanjava/qql
Length of output: 171
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== src/qql/ast_nodes.py (SearchWith definition) ==="
sed -n '1,120p' src/qql/ast_nodes.py | nl -ba | sed -n '1,120p'
echo "=== src/qql/executor.py (SearchParams construction) ==="
sed -n '1300,1405p' src/qql/executor.py | nl -ba
echo "=== src/qql/parser.py (with_clause merging) ==="
sed -n '740,860p' src/qql/parser.py | nl -ba
echo "=== src/qql/parser.py (mentions of indexed_only/acorn/exact) ==="
rg -n "indexed_only|acorn|exact" src/qql/parser.py | head -n 200Repository: pavanjava/qql
Length of output: 171
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== src/qql/ast_nodes.py (SearchWith definition) ==="
sed -n '1,120p' src/qql/ast_nodes.py | nl -ba | sed -n '1,120p'
echo "=== src/qql/executor.py (SearchParams construction) ==="
sed -n '1300,1405p' src/qql/executor.py | nl -ba
echo "=== src/qql/parser.py (with_clause merging) ==="
sed -n '740,860p' src/qql/parser.py | nl -ba
echo "=== src/qql/parser.py (mentions of indexed_only/acorn/exact) ==="
rg -n "indexed_only|acorn|exact" src/qql/parser.py | head -n 200Repository: pavanjava/qql
Length of output: 171
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== src/qql/ast_nodes.py (SearchWith fields around lines 20-50) ==="
sed -n '1,120p' src/qql/ast_nodes.py | cat -n | sed -n '20,70p'
echo "=== src/qql/ast_nodes.py (SearchWith definition area) ==="
rg -n "class SearchWith|SearchWith\s*\(" src/qql/ast_nodes.py
echo "=== src/qql/executor.py (SearchParams construction around 1320-1370) ==="
sed -n '1310,1370p' src/qql/executor.py | cat -n
echo "=== src/qql/parser.py (with_clause merging around 760-825) ==="
sed -n '760,830p' src/qql/parser.py | cat -n
echo "=== src/qql/parser.py (lines mentioning exact/acorn/indexed_only) ==="
rg -n "exact\s*=|acorn\s*=|indexed_only\s*=|\.exact\b|\.acorn\b|\.indexed_only\b|parsed_with\.(exact|acorn|indexed_only)|with_clause\.(exact|acorn|indexed_only)" src/qql/parser.pyRepository: pavanjava/qql
Length of output: 9807
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== src/qql/parser.py (parse_with_clause around 1360-1450) ==="
sed -n '1340,1460p' src/qql/parser.py | cat -n
echo "=== tests for indexed_only false / acorn false ==="
rg -n "indexed_only:\s*false|acorn:\s*false" tests -S || true
echo "=== tests for indexed_only false / acorn false (with_clause keywords) ==="
rg -n "indexed_only.*false|acorn.*false" tests/test_parser.py -S || trueRepository: pavanjava/qql
Length of output: 5912
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== rg: SearchWith( constructions in src/qql/parser.py ==="
rg -n "SearchWith\(" src/qql/parser.py
echo "=== rg: SearchWith( constructions in src/qql/**/*.py ==="
rg -n "SearchWith\(" src/qql
echo "=== rg: _build_search_params call sites / with_clause usage ==="
rg -n "_build_search_params\(" -S src/qql/executor.py src/qql -SRepository: pavanjava/qql
Length of output: 1503
Fix SearchWith.exact/acorn/indexed_only handling of false vs None end-to-end
src/qql/ast_nodes.pynow definesSearchWith.exact,SearchWith.acorn, andSearchWith.indexed_onlyasbool | None = None, butsrc/qql/parser.py::_parse_with_clauseinitializesexact/acorn/indexed_onlytoFalseand always returns booleans—so omitting those keys inWITH { ... }yieldsFalse(notNone).src/qql/parser.pymergeswith_clausevalues usingparsed_with.<field> or with_clause.<field>, which makes explicitfalseunable to override a previously-settrue.src/qql/executor.py::_build_search_paramstreatsindexed_onlyandacornasTrue if ... else None, sofalseandNoneboth become “unset” for Qdrant (whileexactis passed through directly).
Adjust parsing/merging/executor mapping to use is not None / coalescing semantics so None remains “unset” and false remains an explicit disable for all three fields.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/qql/ast_nodes.py` around lines 28 - 30, Parser currently initializes
exact/acorn/indexed_only to False and merges with "or", which loses explicit
false; update src/qql/parser.py::_parse_with_clause to initialize
SearchWith.exact/acorn/indexed_only as None (not False) and change merging logic
to coalesce using "if parsed_with.<field> is not None then parsed_with.<field>
else with_clause.<field>" (or equivalent is not None check) instead of "or" so
explicit False wins; also update src/qql/executor.py::_build_search_params to
map these three fields by checking "is not None" and passing True/False through
as-is (return value = field if field is not None else None) so False remains an
explicit disable for exact, acorn, and indexed_only.
Comment on lines
243
to
250
| try: | ||
| client = QdrantClient(url=url, api_key=secret, verify=verify_val) | ||
| client = QdrantClient(**client_kwargs) | ||
| client.get_collections() | ||
| except Exception as e: | ||
| err_console.print(f"[bold red]Connection failed:[/bold red] {e}") | ||
| sys.exit(1) | ||
| else: | ||
| client.close() |
There was a problem hiding this comment.
Client not closed on connection test failure.
When the connection test at line 244 succeeds, client.close() is called at line 250. However, if client.get_collections() at line 245 raises an exception, the except block at line 246 prints an error and exits, but the client connection is never closed.
🔒 Proposed fix to ensure client is closed
try:
client = QdrantClient(**client_kwargs)
client.get_collections()
except Exception as e:
+ client.close()
err_console.print(f"[bold red]Connection failed:[/bold red] {e}")
sys.exit(1)
else:
client.close()Alternatively, use a context manager or try/finally.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| try: | |
| client = QdrantClient(url=url, api_key=secret, verify=verify_val) | |
| client = QdrantClient(**client_kwargs) | |
| client.get_collections() | |
| except Exception as e: | |
| err_console.print(f"[bold red]Connection failed:[/bold red] {e}") | |
| sys.exit(1) | |
| else: | |
| client.close() | |
| client = None | |
| try: | |
| client = QdrantClient(**client_kwargs) | |
| client.get_collections() | |
| except Exception as e: | |
| if client: | |
| client.close() | |
| err_console.print(f"[bold red]Connection failed:[/bold red] {e}") | |
| sys.exit(1) | |
| else: | |
| client.close() |
🧰 Tools
🪛 Ruff (0.15.15)
[warning] 246-246: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/qql/cli.py` around lines 243 - 250, The QdrantClient created in the try
block may not be closed if client.get_collections() raises; ensure the client is
always closed by restructuring to either use a context manager for QdrantClient
or add a try/finally that calls client.close() after creating the client (i.e.,
create client = QdrantClient(**client_kwargs) then in finally call
client.close()), keeping the error logging in the except block and sys.exit(1)
after the finally.
Comment on lines
+264
to
+269
| except ValueError as e: | ||
| if f"Collection {name} not found" in str(e): | ||
| return None | ||
| raise QQLRuntimeError( | ||
| f"Qdrant error fetching collection '{name}': {e}" | ||
| ) from e |
There was a problem hiding this comment.
String matching may miss collection-not-found ValueError.
Line 265 checks if f"Collection {name} not found" is in str(e), but this literal string interpolation happens at check-time with the actual collection name. If the ValueError message uses a different format (e.g., quotes around the name: Collection 'docs' not found), this check will fail.
🛡️ Proposed fix for robust pattern matching
except ValueError as e:
- if f"Collection {name} not found" in str(e):
+ if "not found" in str(e).lower() and name in str(e):
return None
raise QQLRuntimeError(
f"Qdrant error fetching collection '{name}': {e}"
) from eThis checks for both "not found" (case-insensitive) and the collection name, making it more robust to message format variations.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/qql/executor.py` around lines 264 - 269, The current except block in
executor.py that inspects ValueError (the block referencing variable name and
raising QQLRuntimeError) uses a brittle literal substring check; replace it with
a robust, case-insensitive regex (or two-part check) that looks for "not found"
and the collection name allowing optional surrounding quotes (e.g. use
re.search(fr"collection\s+['\"]?{re.escape(name)}['\"]?\s+not\s+found", str(e),
re.I) or fall back to checking "not found" in str(e). If the regex matches,
return None; otherwise raise the QQLRuntimeError as before (preserving the
original exception chain).
…ction and executor
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
feat: add gRPC transport support and NULL filter handling
What
Adds optional gRPC transport for faster Qdrant communication and proper SQL-style
NULLcomparison handling in QQL filter expressions.Why
NULLcomparisons (field = NULL,field != NULL,field IN (val, NULL),field NOT IN (NULL)) were silently broken or missing. QQL hadIS NULL/IS NOT NULLsyntax but couldn't handleNULLas a literal in=,!=,IN, orNOT INoperators.Changes
gRPC transport:
Connectionacceptsprefer_grpcandgrpc_portparametersQQLConfigpersists gRPC settings across sessionsqql connectexposes--prefer-grpcand--grpc-portflagsrun_query()standalone function passes gRPC params throughconnect,execute,dump,_launch_repl) use a shared_client_kwargs_from_cfg()helperNULL handling:
NULLas a literal value in comparisons andIN/NOT INlistsfield = NULLbuildsIsNullConditionfield != NULLbuildsFilter(must_not=[IsNullCondition])field IN (val, NULL)buildsFilter(should=[IsNullCondition, MatchAny])field NOT IN (NULL)buildsFilter(must_not=[IsNullCondition])field > NULL(or<,>=,<=) raises a clear error:Cannot use operator '>' with nullCompareExpr,InExpr,NotInExprvalue types now includeNoneSearchWithboolean fields (exact,acorn,indexed_only) changed frombool = Falsetobool | None = Noneto distinguish "not set" from "explicitly false"Error handling:
_fetch_collection_infocatchesValueErrorfor client versions that throw string-based "Collection not found" errors_fetch_collection_infocatches gRPCRpcErrorwithStatusCode.NOT_FOUND(same semantics as HTTP 404)ValueErrors now wrap asQQLRuntimeErrorinstead of leaking raw exceptionsTests
_fetch_collection_infoerror paths (HTTP 404, ValueError, gRPC NOT_FOUND, gRPC other, generic exception)QdrantClientrun_querydelegation test for new signatureSummary by CodeRabbit
Release Notes
--prefer-grpcand--grpc-portCLI options or when initializing connections