Better nested queries API#125
Merged
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughRuntime-provided multi-stage query lists are validated and auto-topologically ordered before execution (final entry remains the returned root). A new MCP ChangesMulti-Stage DAG Query Execution with Topological Auto-Sorting
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/test_mcp_server.py (1)
2932-2932: ⚡ Quick winMove
ToolErrorimports to module scope.These three function-local imports at lines 2932, 2972, and 2985 should be added to the file import block at the top for consistency with the repository guideline that imports must be placed at the top of files.
As per coding guidelines
**/*.py: Place imports at the top of files.🤖 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 `@tests/test_mcp_server.py` at line 2932, Three local imports of ToolError inside test functions must be moved to module scope; add a single top-level import "from mcp.server.fastmcp.exceptions import ToolError" to the file's import block and remove the three function-local import lines (the occurrences that currently import ToolError inside functions), so all tests reference the module-level ToolError instead of importing it locally.
🤖 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 `@slayer/engine/query_engine.py`:
- Around line 274-410: The _topologically_order_queries function is too complex;
split its logic into three private helpers: implement _validate_queries(rest:
List[SlayerQuery], root: SlayerQuery, rest_by_name: Dict[str, SlayerQuery]) to
perform non-final name checks, duplicates, self-reference (using the existing
_refs) and root-referrer validation; implement
_build_dependency_graph(rest_by_name: Dict[str, SlayerQuery], sibling_names:
set) to produce and return (in_degree: Dict[str,int], dependents:
Dict[str,List[str]]) by iterating over rest_by_name and calling _refs; and
implement _kahn_sort(in_degree, dependents) to run the Kahn traversal and return
sorted_names or raise the same cycle ValueError; then simplify
_topologically_order_queries to compute rest/rest_by_name/root/sibling_names,
call _validate_queries, call _build_dependency_graph, call _kahn_sort, and
finally return [rest_by_name[n] for n in sorted_names] + [root].
- Around line 435-443: The code assumes queries list is non-empty and does
queries[-1], which raises IndexError for empty staged-query lists; add a guard
after building/ordering queries (after calls to SlayerQuery.model_validate and
_topologically_order_queries) to detect empty queries and raise a user-facing
validation error (e.g., ValueError or a custom ValidationError) with a clear
message before accessing queries[-1]; ensure the same guard applies before
constructing named_queries and that any callers of this block still receive the
validation error rather than an IndexError.
---
Nitpick comments:
In `@tests/test_mcp_server.py`:
- Line 2932: Three local imports of ToolError inside test functions must be
moved to module scope; add a single top-level import "from
mcp.server.fastmcp.exceptions import ToolError" to the file's import block and
remove the three function-local import lines (the occurrences that currently
import ToolError inside functions), so all tests reference the module-level
ToolError instead of importing it locally.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 04e12485-f602-4624-a033-04fd69a544f3
📒 Files selected for processing (8)
.claude/skills/slayer-query.mdCLAUDE.mddocs/concepts/queries.mdpyproject.tomlslayer/engine/query_engine.pyslayer/mcp/server.pytests/test_mcp_server.pytests/test_query_backed_models.py
AivanF
force-pushed
the
aivan/2026-05-14-Nested-Queries-API
branch
from
8cd32b6 to
301c940
Compare
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
slayer/engine/query_engine.py (1)
399-409: 💤 Low valueConsider
dequefor O(1) pop from front.
frontier.pop(0)is O(n) on a list. For small DAGs this is negligible, butcollections.dequeprovides O(1)popleft()and aligns better with canonical Kahn's implementations.♻️ Optional optimization
+from collections import deque + `@staticmethod` def _kahn_sort( in_degree: Dict[str, int], dependents: Dict[str, List[str]], ) -> List[str]: - frontier: List[str] = sorted(n for n, d in in_degree.items() if d == 0) + frontier: deque = deque(sorted(n for n, d in in_degree.items() if d == 0)) sorted_names: List[str] = [] while frontier: - n = frontier.pop(0) + n = frontier.popleft() sorted_names.append(n) unlocked: List[str] = [] for dep in dependents[n]: in_degree[dep] -= 1 if in_degree[dep] == 0: unlocked.append(dep) frontier.extend(sorted(unlocked))🤖 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 `@slayer/engine/query_engine.py` around lines 399 - 409, Replace the list-based FIFO used in the Kahn topological sort with a deque to avoid O(n) pops: import collections.deque and change the frontier initialization to a deque(sorted(...)), replace frontier.pop(0) with frontier.popleft(), and keep the rest of the logic (appending to sorted_names and extending frontier with sorted(unlocked)) but use frontier.extend(sorted(unlocked)); this touches the variables/function block using frontier, sorted_names, unlocked, in_degree, and dependents in the topological-sort loop.
🤖 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 `@slayer/engine/query_engine.py`:
- Around line 274-311: The _extract_sibling_refs function is too complex due to
repeated branching for the three shapes of query.source_model; refactor by
normalizing source_model into a unified representation (e.g., derive a string
source_name and an iterable of join objects regardless of whether sm is str,
dict, or typed) and then run a single shared extraction loop that checks
source_name and iterates joins to collect target_model values; specifically,
inside _extract_sibling_refs pull out src = ... (handle str -> src=sm, dict ->
src=sm.get("source_name"), typed -> getattr(sm,"source_name",None)) and joins =
... (handle dict -> sm.get("joins") or [], typed -> getattr(sm,"joins", None) or
[], str -> empty), then run one set of checks that adds src if in against and
loops joins to add j.target_model / j.get("target_model") when a string in
against — this removes duplicated logic and lowers cognitive complexity.
---
Nitpick comments:
In `@slayer/engine/query_engine.py`:
- Around line 399-409: Replace the list-based FIFO used in the Kahn topological
sort with a deque to avoid O(n) pops: import collections.deque and change the
frontier initialization to a deque(sorted(...)), replace frontier.pop(0) with
frontier.popleft(), and keep the rest of the logic (appending to sorted_names
and extending frontier with sorted(unlocked)) but use
frontier.extend(sorted(unlocked)); this touches the variables/function block
using frontier, sorted_names, unlocked, in_degree, and dependents in the
topological-sort loop.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2acc8ac0-e507-4931-be0c-dd74aa6c70f5
📒 Files selected for processing (9)
.claude/skills/slayer-query.mdCLAUDE.mddocs/concepts/queries.mdpyproject.tomlslayer/engine/query_engine.pyslayer/mcp/server.pytests/integration/test_integration.pytests/test_mcp_server.pytests/test_query_backed_models.py
✅ Files skipped from review due to trivial changes (5)
- .claude/skills/slayer-query.md
- pyproject.toml
- tests/integration/test_integration.py
- CLAUDE.md
- docs/concepts/queries.md
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/test_mcp_server.py
- tests/test_query_backed_models.py
- slayer/mcp/server.py
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.
Summary by CodeRabbit
New Features
Documentation
Tests
Chores