Skip to content

Revert "Feat/refractor batch grpc async support"#49

Merged
pavanjava merged 1 commit into
mainfrom
revert-48-feat/refractor-batch-grpc-async-support
May 29, 2026
Merged

Revert "Feat/refractor batch grpc async support"#49
pavanjava merged 1 commit into
mainfrom
revert-48-feat/refractor-batch-grpc-async-support

Conversation

@srimon12
Copy link
Copy Markdown
Collaborator

@srimon12 srimon12 commented May 28, 2026

Reverts #48

Summary by CodeRabbit

Release Notes

  • Refactor

    • Removed async query execution support; only synchronous API available
    • Removed batch query execution capabilities
    • Removed parameterized query methods
    • Removed gRPC transport configuration option
    • Simplified Connection initialization
  • Documentation

    • Updated all documentation to reflect API simplifications
  • Tests

    • Updated test suite to align with API changes

Review Change Stack

@srimon12 srimon12 requested a review from pavanjava May 28, 2026 06:50
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 28, 2026

📝 Walkthrough

Walkthrough

This PR removes async support, batch operations (BEGIN BATCH...END BATCH), parameterized queries, gRPC transport, and semicolon syntax from QQL, consolidating to a single synchronous API (Connection.run_query()) and internalizing utility functions into the executor. Test count drops from 635 to 549.

Changes

QQL Sync-Only Simplification

Layer / File(s) Summary
Simplify public API surface
src/qql/__init__.py, src/qql/connection.py
Removes async/batch/parameterized exports and methods; Connection now accepts only verify (no prefer_grpc/grpc_port); updates __all__ to expose only synchronous Connection, Executor, Parser, Lexer, and error types.
Remove batch/semicolon syntax from grammar
src/qql/lexer.py, src/qql/parser.py, src/qql/script.py
Lexer removes BEGIN/BATCH/END/SEMICOLON token kinds; parser dispatches directly on statement keywords and enforces EOF (removes batch block and semicolon parsing); script splitter no longer groups BEGIN...END boundaries.
Tighten AST type contracts
src/qql/ast_nodes.py
SearchWith flags (exact/acorn/indexed_only) change from optional to non-nullable booleans with default False; InExpr/NotInExpr.values no longer accept None; BatchBlockStmt removed from ASTNode union.
Remove NULL literal support
src/qql/parser.py
_parse_literal and _parse_literal_list reject NULL as valid literal, affecting IN/NOT IN and comparison operator parsing.
Internalize utilities into Executor, remove batch dispatch
src/qql/executor.py
Removes BatchBlockStmt dispatch and batch execution; reimplements collection topology, filter building (_build_qdrant_filter, _wrap_as_filter), MMR detection/validation, dense query construction, insert id/payload extraction, recommend strategy parsing, and hybrid fusion resolution as private executor methods.
Remove entire async implementation
src/qql/async_connection.py (deleted), src/qql/async_executor.py (deleted)
Deletes AsyncConnection, QQLAsyncBatch, AsyncOperationProxy, and AsyncExecutor; removes all async query execution, batching, and lifecycle management code.
Update documentation for sync-only API
README.md, docs/getting-started.md, docs/programmatic.md, docs/reference.md, docs/scripts.md
Removes references to async execution, batch blocks, parameterized queries, gRPC transport; simplifies examples to Connection.run_query() only; updates expected test count (635→549); removes async/batch/gRPC sections from API reference.
Remove async tests and update sync tests
tests/test_async_connection.py (deleted), tests/test_connection.py, tests/test_executor.py, tests/test_parser.py
Deletes entire async connection test suite; removes parameterized query imports; removes NULL value filter test cases; deletes statement boundary and EXACT/WITH merge test cases.

🎯 4 (Complex) | ⏱️ ~60 minutes

Suggested reviewers

  • pavanjava

🐰 Batch blocks are gone, async awaits farewell,
gRPC routes closed, no parameterized spells,
One sync path remains, clean and direct,
Simpler, faster—the future we'll perfect.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 48.65% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title directly and accurately describes the main change: reverting a previous feature/refactor commit for batch, gRPC, and async support.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch revert-48-feat/refractor-batch-grpc-async-support

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
src/qql/script.py (1)

22-33: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Missing UPDATE in statement starters will break script splitting.

The _STMT_STARTERS set is missing TokenKind.UPDATE. This means scripts containing UPDATE statements will not correctly split at UPDATE boundaries - the UPDATE statement will be appended to the previous statement, causing parse failures.

🐛 Proposed fix
 _STMT_STARTERS = {
     TokenKind.INSERT,
     TokenKind.CREATE,
     TokenKind.ALTER,
     TokenKind.DROP,
     TokenKind.SHOW,
     TokenKind.SELECT,
     TokenKind.SCROLL,
     TokenKind.SEARCH,
     TokenKind.RECOMMEND,
     TokenKind.DELETE,
+    TokenKind.UPDATE,
 }
🤖 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/script.py` around lines 22 - 33, The _STMT_STARTERS set is missing
TokenKind.UPDATE which prevents correct splitting at UPDATE boundaries; fix by
adding TokenKind.UPDATE to the _STMT_STARTERS collection in src/qql/script.py
(update the set that includes TokenKind.INSERT, TokenKind.CREATE, etc.) so
script splitting and parser behavior can recognize UPDATE as a statement
starter.
🤖 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 `@docs/reference.md`:
- Line 190: Update the documented test count by replacing the string "Expected
output: **604 tests passing**" in docs/reference.md with "Expected output: **549
tests passing**" so it matches the updated baseline in README.md; also scan
docs/reference.md for any other occurrences of "604 tests passing" and update
them to "549" to keep documentation consistent.
- Line 154: The fenced code block that begins with the text "qql/" is unlabeled
and triggers MD040; add a language specifier (for example "text") to the opening
triple backticks so the block becomes ```text to silence markdownlint. Locate
the fenced block containing "qql/" (the directory tree snippet) and change its
opening fence to include the language label.

---

Outside diff comments:
In `@src/qql/script.py`:
- Around line 22-33: The _STMT_STARTERS set is missing TokenKind.UPDATE which
prevents correct splitting at UPDATE boundaries; fix by adding TokenKind.UPDATE
to the _STMT_STARTERS collection in src/qql/script.py (update the set that
includes TokenKind.INSERT, TokenKind.CREATE, etc.) so script splitting and
parser behavior can recognize UPDATE as a statement starter.
🪄 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: 11f5085d-ca07-481c-8634-e2f36dcffbb8

📥 Commits

Reviewing files that changed from the base of the PR and between 4a1fa92 and b2a70bb.

📒 Files selected for processing (19)
  • README.md
  • docs/getting-started.md
  • docs/programmatic.md
  • docs/reference.md
  • docs/scripts.md
  • src/qql/__init__.py
  • src/qql/ast_nodes.py
  • src/qql/async_connection.py
  • src/qql/async_executor.py
  • src/qql/connection.py
  • src/qql/executor.py
  • src/qql/lexer.py
  • src/qql/parser.py
  • src/qql/script.py
  • src/qql/utils.py
  • tests/test_async_connection.py
  • tests/test_connection.py
  • tests/test_executor.py
  • tests/test_parser.py
💤 Files with no reviewable changes (8)
  • src/qql/async_executor.py
  • src/qql/async_connection.py
  • src/qql/utils.py
  • src/qql/lexer.py
  • tests/test_connection.py
  • tests/test_async_connection.py
  • tests/test_parser.py
  • tests/test_executor.py

Comment thread docs/reference.md
## Project Structure

```text
```
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Add a language to this fenced block to clear markdownlint warning.

Line 154 uses an unlabeled fenced code block, which triggers MD040.

Suggested patch
-```
+```text
 qql/
 ├── pyproject.toml          # Package config; installs the `qql` CLI command
 ...
-```
+```
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)

[warning] 154-154: Fenced code blocks should have a language specified

(MD040, fenced-code-language)

🤖 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 `@docs/reference.md` at line 154, The fenced code block that begins with the
text "qql/" is unlabeled and triggers MD040; add a language specifier (for
example "text") to the opening triple backticks so the block becomes ```text to
silence markdownlint. Locate the fenced block containing "qql/" (the directory
tree snippet) and change its opening fence to include the language label.

Comment thread docs/reference.md
```

Expected output: **635 tests passing**.
Expected output: **604 tests passing**.
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Align the documented test count with the current docs baseline.

Line 190 says 604 tests passing, but this PR’s updated docs elsewhere now state 549 (for example, README.md, Line 191). Keep this consistent to avoid confusion.

Suggested patch
-Expected output: **604 tests passing**.
+Expected output: **549 tests passing**.
📝 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
Expected output: **604 tests passing**.
Expected output: **549 tests passing**.
🤖 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 `@docs/reference.md` at line 190, Update the documented test count by replacing
the string "Expected output: **604 tests passing**" in docs/reference.md with
"Expected output: **549 tests passing**" so it matches the updated baseline in
README.md; also scan docs/reference.md for any other occurrences of "604 tests
passing" and update them to "549" to keep documentation consistent.

@pavanjava pavanjava merged commit 9172127 into main May 29, 2026
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants