perf(transpiler): small improvements - #205
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
💤 Files with no reviewable changes (1)
📝 WalkthroughWalkthroughChangesThe planner now handles DISTINCT selection, user filter statements, and duplicate projections more explicitly. Planner and optimization tests use DB-free SQL snapshots across dialects, supported by new fixtures and formatting utilities. Snapshot generation tasks are split between inline and file snapshots. Planner optimization and snapshot workflow
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #205 +/- ##
==========================================
- Coverage 93.22% 93.20% -0.02%
==========================================
Files 72 72
Lines 6524 6565 +41
Branches 858 868 +10
==========================================
+ Hits 6082 6119 +37
- Misses 297 299 +2
- Partials 145 147 +2 ☔ View full report in Codecov by Harness. |
gazorby
changed the title
gazorby
changed the title
64fde62 to
33a8305
Compare
gazorby
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@mise.toml`:
- Around line 206-208: Update the test:add-new-snapshots task to run its inline
and file snapshot subtasks sequentially using a run step list instead of
parallel depends entries. Preserve both existing subtask commands and their
order, ensuring the inline writer completes before the file-based pytest task
starts.
In `@src/strawchemy/transpiler/_planner.py`:
- Around line 1079-1091: Update Planner.semijoin to project the root primary-key
attributes, preserve the original statement’s LIMIT/OFFSET semantics, and
deduplicate the projected rows before creating filter_alias so the inner join
cannot multiply outer results. Add an execution test covering a one-to-many join
that produces repeated root PKs and verifies each root result appears only once.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: ddb59d52-067f-4417-84ff-081e40244137
⛔ Files ignored due to path filters (6)
tests/integration/__snapshots__/test_aggregations.ambris excluded by!**/__snapshots__/**tests/integration/__snapshots__/test_custom_resolver.ambris excluded by!**/__snapshots__/**tests/integration/__snapshots__/test_distinct_on.ambris excluded by!**/__snapshots__/**tests/integration/__snapshots__/test_queries.ambris excluded by!**/__snapshots__/**tests/integration/__snapshots__/test_query_hooks.ambris excluded by!**/__snapshots__/**uv.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
mise.tomlpyproject.tomlsrc/strawchemy/transpiler/_planner.pytasks.mdtests/unit/transpiler/test_plan.pytests/unit/transpiler/test_planner.pytests/unit/transpiler/test_planner_helpers.pytests/utils.py
💤 Files with no reviewable changes (1)
- tests/unit/transpiler/test_planner.py
| [tasks."test:add-new-snapshots"] | ||
| description = "Run snapshot-based tests and add new snapshots" | ||
| depends = ["test:add-new-snapshots:inline", "test:add-new-snapshots:file"] |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== mise.toml context =="
git ls-files mise.toml
wc -l mise.toml
sed -n '190,220p' mise.toml
echo
echo "== local references to test:add-new-snapshots =="
rg -n 'test:add-new-snapshots|depends = \[|run = \[' mise.toml . || true
echo
echo "== if available, show mise docs mention in repo =="
rg -n 'depends.*concurrent|depends.*sequential|run = \[|task = ' README* docs* .github* . || trueRepository: gazorby/strawchemy
Length of output: 2898
🌐 Web query:
mise tasks depends run concurrently sequential task entries depends array run array documentation
💡 Result:
In mise, tasks defined in depends (and depends_post or wait_for) are executed by mise to fulfill prerequisites before the main task runs [1][2][3]. Mise automatically manages the execution of these dependencies, running as many as possible concurrently, subject to the global --jobs limit (which defaults to 4) [1][2][4]. The depends configuration accepts a list of task names or structured objects. The valid types for depends are: - String: e.g., depends = ["task1", "task2"] [2][5]. - Array of strings: A list of task names [2][5]. - Array of objects: For more granular control, you can provide objects including the task name, arguments, and environment variables, e.g., depends = [{ task = "build", args = ["--release"] }] [2][5][6]. Key points regarding concurrency and ordering: - Parallelism: Mise attempts to run available tasks (including dependencies) in parallel, up to the number of jobs configured [1][2]. - Controlling Execution Order: While mise handles concurrency automatically, you can explicitly force sequential execution for certain workflows by defining a master task that contains a list of steps in the run property [1][7]. Inside the run array, you can define steps that must complete before subsequent steps begin, or group tasks to run in parallel [1][7]. - Raw/Interactive Tasks: Tasks configured with raw = true or interactive = true acquire exclusive locks, forcing them to run without other tasks (including dependencies) running concurrently if they conflict with standard I/O [2][4][7]. - Disabling Concurrency: If you need to force total sequential execution for a specific command, you can set the --jobs 1 flag during execution [1][2][8]. There is currently no native property (like sequential = true) to force a specific depends array to run strictly in series rather than parallel, although this has been discussed as a potential future enhancement [2][9]. For complex orchestration, using the run array within a task definition is the recommended way to manage sequential versus parallel step execution [1][7].
Citations:
- 1: https://mise.jdx.dev/tasks/running-tasks.html
- 2: https://mise.jdx.dev/tasks/task-configuration.html
- 3: https://mise.en.dev/tasks/toml-tasks
- 4: https://github.com/jdx/mise/blob/v2026.4.0/docs/tasks/task-configuration.md
- 5: https://mise.en.dev/tasks/task-configuration.html
- 6: fix(schema): support structured objects in task depends jdx/mise#8463
- 7: https://mise.en.dev/tasks/running-tasks.html
- 8: https://github.com/jdx/mise/blob/5be2b8fd/docs/cli/run.md
- 9: method for running tasks in series jdx/mise#3761
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== test marker and snapshot config =="
git ls-files pyproject.toml pytest.ini tox.ini conftest.py tests || true
for f in pyproject.toml pytest.ini tox.ini conftest.py; do
if [ -f "$f" ]; then
echo
echo "---- $f ----"
sed -n '1,260p' "$f"
fi
done
echo
echo "== snapshot/inline snapshot references =="
rg -n 'inline_snapshot|snapshot-update-new-only|snapshot-update|markers?|pytest' . --glob '!tasks.md' --glob '!mise.toml' || trueRepository: gazorby/strawchemy
Length of output: 50375
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== inline snapshot usages =="
rg -n 'inline_snapshot|assert_match|snapshot' tests/unit tests/integration --glob '*.py' | head -n 200
echo
echo "== files with inline-snapshot markers or helpers =="
rg -n '`@pytest`\.mark\.inline_snapshot|inline-snapshot|inline snapshot|assert_snapshot|syrupy|snapshot' tests --glob '*.py' | head -n 200Repository: gazorby/strawchemy
Length of output: 36733
Run these snapshot writers sequentially. depends can execute in parallel, and both subtasks walk the same tests/ tree; the inline-snapshot writer also rewrites Python test files while the other pytest run is still collecting modules. Switch the wrapper to a run = [...] step list so they can’t race.
🤖 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 `@mise.toml` around lines 206 - 208, Update the test:add-new-snapshots task to
run its inline and file snapshot subtasks sequentially using a run step list
instead of parallel depends entries. Preserve both existing subtask commands and
their order, ensuring the inline writer completes before the file-based pytest
task starts.
| def semijoin(self) -> FilterSemiJoin: | ||
| """Builds the PK semi-join from the root alias to the filter-statement subquery. | ||
|
|
||
| Returns: | ||
| A FilterSemiJoin with the subquery alias and the PK-equality onclause. | ||
| """ | ||
| root_mapper = class_mapper(self.aliases.model) | ||
| pk_attributes = SQLAlchemyInspector.pk_attributes(root_mapper) | ||
| filter_alias = cast("Alias", self.statement.with_only_columns(*pk_attributes).subquery().alias()) | ||
| on_clause = and_( | ||
| *[getattr(self.aliases.root_alias, attr.key) == filter_alias.c[attr.key] for attr in pk_attributes] | ||
| ) | ||
| return bool(query_graph.distinct_on) | ||
| return FilterSemiJoin(alias=filter_alias, onclause=on_clause) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Deduplicate the PK relation before joining it.
This is an inner join rather than a true semijoin. If the user statement contains a one-to-many join, duplicate PK rows multiply the outer result. Wrap the projected PK statement and select distinct PKs from it, preserving the original statement’s LIMIT/OFFSET semantics.
Proposed fix
root_mapper = class_mapper(self.aliases.model)
pk_attributes = SQLAlchemyInspector.pk_attributes(root_mapper)
- filter_alias = cast("Alias", self.statement.with_only_columns(*pk_attributes).subquery().alias())
+ pk_rows = self.statement.with_only_columns(*pk_attributes).subquery()
+ filter_alias = cast(
+ "Alias",
+ select(*(pk_rows.c[attr.key] for attr in pk_attributes)).distinct().subquery(),
+ )Please add an execution test using a one-to-many join that produces repeated root PKs.
📝 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.
| def semijoin(self) -> FilterSemiJoin: | |
| """Builds the PK semi-join from the root alias to the filter-statement subquery. | |
| Returns: | |
| A FilterSemiJoin with the subquery alias and the PK-equality onclause. | |
| """ | |
| root_mapper = class_mapper(self.aliases.model) | |
| pk_attributes = SQLAlchemyInspector.pk_attributes(root_mapper) | |
| filter_alias = cast("Alias", self.statement.with_only_columns(*pk_attributes).subquery().alias()) | |
| on_clause = and_( | |
| *[getattr(self.aliases.root_alias, attr.key) == filter_alias.c[attr.key] for attr in pk_attributes] | |
| ) | |
| return bool(query_graph.distinct_on) | |
| return FilterSemiJoin(alias=filter_alias, onclause=on_clause) | |
| def semijoin(self) -> FilterSemiJoin: | |
| """Builds the PK semi-join from the root alias to the filter-statement subquery. | |
| Returns: | |
| A FilterSemiJoin with the subquery alias and the PK-equality onclause. | |
| """ | |
| root_mapper = class_mapper(self.aliases.model) | |
| pk_attributes = SQLAlchemyInspector.pk_attributes(root_mapper) | |
| pk_rows = self.statement.with_only_columns(*pk_attributes).subquery() | |
| filter_alias = cast( | |
| "Alias", | |
| select(*(pk_rows.c[attr.key] for attr in pk_attributes)).distinct().subquery(), | |
| ) | |
| on_clause = and_( | |
| *[getattr(self.aliases.root_alias, attr.key) == filter_alias.c[attr.key] for attr in pk_attributes] | |
| ) | |
| return FilterSemiJoin(alias=filter_alias, onclause=on_clause) |
🤖 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/strawchemy/transpiler/_planner.py` around lines 1079 - 1091, Update
Planner.semijoin to project the root primary-key attributes, preserve the
original statement’s LIMIT/OFFSET semantics, and deduplicate the projected rows
before creating filter_alias so the inner join cannot multiply outer results.
Add an execution test covering a one-to-many join that produces repeated root
PKs and verifies each root result appears only once.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@tests/unit/transpiler/conftest.py`:
- Line 76: Update the `_execute` methods in the synchronous and asynchronous
executor test doubles to replace `Any` session annotations with the
corresponding `AnySyncSession` and `AnyAsyncSession` protocols, matching the
original executor signatures and resolving Ruff ANN401.
In `@tests/unit/transpiler/test_optimizations.py`:
- Around line 34-60: Update the aggregate-only query generation so the parent
table is preserved when the child-grouped CTE has no matching rows, using a LEFT
OUTER JOIN or equivalent plan across the SQLite/MySQL cases and the additional
affected snapshots. Retain empty-set aggregate semantics, then update the
expected SQL snapshots to reflect the parent-preserving join.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: ce867dda-3e8f-473b-bff2-ef823b72dd0e
⛔ Files ignored due to path filters (1)
tests/integration/__snapshots__/test_optimizations.ambris excluded by!**/__snapshots__/**
📒 Files selected for processing (4)
tests/integration/test_optimizations.pytests/unit/schemas/optimizations.pytests/unit/transpiler/conftest.pytests/unit/transpiler/test_optimizations.py
💤 Files with no reviewable changes (1)
- tests/integration/test_optimizations.py
| """ | ||
| captured: list[Select[Any]] = [] | ||
|
|
||
| def _execute(self: SyncQueryExecutor[Any], session: Any) -> MagicMock: # noqa: ARG001 |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use the executor session protocols instead of Any.
Ruff reports ANN401 on both parameters. Match the original executor signatures with AnySyncSession and AnyAsyncSession.
Proposed fix
if TYPE_CHECKING:
from sqlalchemy import Select
- from strawchemy.typing import SupportedDialect
+ from strawchemy.typing import AnyAsyncSession, AnySyncSession, SupportedDialect
- def _execute(self: SyncQueryExecutor[Any], session: Any) -> MagicMock:
+ def _execute(self: SyncQueryExecutor[Any], session: AnySyncSession) -> MagicMock:
- async def _async_execute(self: AsyncQueryExecutor[Any], session: Any) -> MagicMock:
+ async def _async_execute(self: AsyncQueryExecutor[Any], session: AnyAsyncSession) -> MagicMock:Also applies to: 82-82
🧰 Tools
🪛 Ruff (0.15.21)
[warning] 76-76: Dynamically typed expressions (typing.Any) are disallowed in session
(ANN401)
🤖 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/unit/transpiler/conftest.py` at line 76, Update the `_execute` methods
in the synchronous and asynchronous executor test doubles to replace `Any`
session annotations with the corresponding `AnySyncSession` and
`AnyAsyncSession` protocols, matching the original executor signatures and
resolving Ruff ANN401.
Source: Linters/SAST tools
| "output-order-by-sqlite": [ | ||
| "WITH anon_1 AS (", | ||
| " SELECT count(*) AS count_1,", | ||
| " fruit_1.color_id AS color_id", | ||
| " FROM fruit AS fruit_1", | ||
| " WHERE fruit_1.color_id IS NOT NULL", | ||
| " GROUP BY fruit_1.color_id", | ||
| " ) SELECT color.id,", | ||
| " anon_1.count_1", | ||
| " FROM color AS color", | ||
| " JOIN anon_1", | ||
| " ON color.id = anon_1.color_id", | ||
| " ORDER BY anon_1.count_1 ASC", | ||
| ], | ||
| "output-order-by-mysql": [ | ||
| "WITH anon_1 AS (", | ||
| " SELECT count(*) AS count_1,", | ||
| " fruit_1.color_id AS color_id", | ||
| " FROM fruit AS fruit_1", | ||
| " WHERE fruit_1.color_id IS NOT NULL", | ||
| " GROUP BY fruit_1.color_id", | ||
| " ) SELECT color.id,", | ||
| " anon_1.count_1", | ||
| " FROM color AS color", | ||
| " INNER JOIN anon_1", | ||
| " ON color.id = anon_1.color_id", | ||
| " ORDER BY anon_1.count_1 ASC", |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Do not let aggregate-only queries drop parents without children.
These SQLite/MySQL snapshots inner-join a child-grouped CTE. Because that CTE has no row for colors without fruit, the unfiltered queries omit those colors, unlike the PostgreSQL lateral form. Emit a LEFT OUTER JOIN or equivalent parent-preserving plan and retain the empty-set aggregate semantics before updating these snapshots.
Also applies to: 229-259, 336-372
🤖 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/unit/transpiler/test_optimizations.py` around lines 34 - 60, Update the
aggregate-only query generation so the parent table is preserved when the
child-grouped CTE has no matching rows, using a LEFT OUTER JOIN or equivalent
plan across the SQLite/MySQL cases and the additional affected snapshots. Retain
empty-set aggregate semantics, then update the expected SQL snapshots to reflect
the parent-preserving join.
1 participant
Description
Types of Changes
Issues Fixed or Closed by This PR
Checklist
Summary by CodeRabbit
Bug Fixes
DISTINCT ONqueries across supported SQL dialects, including cases with ordering.Performance