Thread filter bindings through aggregate SQL composition#186
Merged
Conversation
Filter equality values now ride the driver's bound-parameter stream instead of inlining as literals via PDO::quote. The aggregate SQL helpers return a new `BoundFragment` (sql + bindings) value object; call sites merge bindings into connection->select/scalar or the builder's 'select'/'join' binding positions. Internal emitter signatures stay string-based — a `FragmentSplicer` adapter runs each emitter once with a sentinel placeholder, then counts and repeats predicate bindings to match splice occurrences. This avoids rewriting every variance/quantile/topk helper. `FilterValueQuoter` is removed; separator/JSON-key escaping in `AggregateSqlEmitter` switches to the Laravel-blessed `Connection::escape()`.
|
Wondering what really moved? Review this PR in Change Stack to inspect semantic changes, definitions, and references. Warning Review limit reached
More reviews will be available in 22 minutes and 4 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThis PR refactors aggregate SQL generation from inlined quoted values to composable bound SQL fragments. ChangesAggregate SQL binding refactor
🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 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 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 |
|
Bencher Report| Branch | refactor/filter-bindings |
| Testbed | ubuntu-latest |
⚠️ WARNING: Truncated view!The full continuous benchmarking report exceeds the maximum length allowed on this platform.
🐰 View full continuous benchmarking report in Bencher
⚠️ WARNING: No Threshold found!Without a Threshold, no Alerts will ever be generated.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #186 +/- ##
============================================
+ Coverage 93.82% 93.91% +0.09%
- Complexity 3779 3790 +11
============================================
Files 134 135 +1
Lines 11847 11924 +77
============================================
+ Hits 11116 11199 +83
+ Misses 731 725 -6
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Query/Aggregates/Read/FreshAggregateProjector.php (1)
585-630:⚠️ Potential issue | 🔴 CriticalFix reversed parameter binding order for filtered quantile/TopK scalar aggregates.
AggregateSqlEmitter::emitQuantileWindowSubquery()appends the filter predicate (AND ({$filterSql})) after the$innerFromClause(which already contains the?placeholders for bounds/scope), so the scalar bindings must be[...$bindings, ...$fragment->bindings](currently reversed).AggregateSqlEmitter::emitTopKCorrelatedSubqueryInternal()sets$innerWhere = $boundsAndScopefirst and only then appends the filter (AND ({$filterSql})), so bindings must be[...$innerBindings, ...$topKFragment->bindings](currently reversed).🤖 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/Query/Aggregates/Read/FreshAggregateProjector.php` around lines 585 - 630, The scalar calls pass bindings in the wrong order for filtered quantile/TopK aggregates; for AggregateSqlEmitter::emitQuantileWindowSubquery(...) return use $connection->scalar(..., [...$bindings, ...$fragment->bindings]) (swap the current order) and for AggregateSqlEmitter::emitTopKCorrelatedSubquery(...) return use $connection->scalar(..., [...$innerBindings, ...$topKFragment->bindings]) (swap the current order) so the positional ? placeholders from bounds/scope come before the filter fragment bindings; update the two scalar invocation arguments accordingly where $fragment->bindings/$topKFragment->bindings and $bindings/$innerBindings are combined.
🧹 Nitpick comments (1)
src/Aggregates/Strategy/RecomputeMaintenance.php (1)
176-227: ⚡ Quick winPlease add a unit test that locks down the new binding-order contract.
This path now depends on two easy-to-regress invariants: inner aggregate bindings must stay ahead of the outer bounds placeholders, and the nested
FragmentSplicerflow for filtered collection aggregates must duplicate predicate bindings exactly once per textual splice. Given the uncovered changed lines in this file, a small pure unit test around one filtered recompute query would add a lot of confidence here.As per coding guidelines, "For new/changed aggregates/filter-binding behavior, add/adjust pure unit tests where possible."
Also applies to: 341-483, 528-534
🤖 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/Aggregates/Strategy/RecomputeMaintenance.php` around lines 176 - 227, Add a unit test in the Strategy\RecomputeMaintenance spec that constructs a filtered recompute query (exercise the code path that calls innerAggregateExpression and the filtered FragmentSplicer / AggregateSqlEmitter flows) and asserts two binding-order invariants: (1) all inner-aggregate bindings (the bindings collected into $aggBindings from innerAggregateExpression / emitTopKCorrelatedSubquery) appear before the outer bounds placeholders ($bounds->lft, $bounds->rgt) in the final bindings array, and (2) the predicate bindings produced by FragmentSplicer are duplicated exactly once per textual splice (i.e., the filter predicate bindings appear the expected number of times in $aggBindings). Use the public API that triggers RecomputeMaintenance's select-building logic and assert on the returned SQL fragments and binding array to lock down this contract.Source: Coding guidelines
🤖 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/Query/Aggregates/Read/AggregateSqlFragmentsTest.php`:
- Around line 58-79: The inline(BoundFragment $fragment) helper currently stops
replacing when strpos($sql, '?') returns false and silently ignores any leftover
bindings; update inline to fail fast by validating that the number of '?'
placeholders matches the number of $fragment->bindings before returning (or
after the substitution loop) and throw a clear exception (e.g.,
InvalidArgumentException) if there are surplus bindings so tests can't silently
pass; reference the inline function and BoundFragment to locate and modify the
logic that counts placeholders and bindings.
---
Outside diff comments:
In `@src/Query/Aggregates/Read/FreshAggregateProjector.php`:
- Around line 585-630: The scalar calls pass bindings in the wrong order for
filtered quantile/TopK aggregates; for
AggregateSqlEmitter::emitQuantileWindowSubquery(...) return use
$connection->scalar(..., [...$bindings, ...$fragment->bindings]) (swap the
current order) and for AggregateSqlEmitter::emitTopKCorrelatedSubquery(...)
return use $connection->scalar(..., [...$innerBindings,
...$topKFragment->bindings]) (swap the current order) so the positional ?
placeholders from bounds/scope come before the filter fragment bindings; update
the two scalar invocation arguments accordingly where
$fragment->bindings/$topKFragment->bindings and $bindings/$innerBindings are
combined.
---
Nitpick comments:
In `@src/Aggregates/Strategy/RecomputeMaintenance.php`:
- Around line 176-227: Add a unit test in the Strategy\RecomputeMaintenance spec
that constructs a filtered recompute query (exercise the code path that calls
innerAggregateExpression and the filtered FragmentSplicer / AggregateSqlEmitter
flows) and asserts two binding-order invariants: (1) all inner-aggregate
bindings (the bindings collected into $aggBindings from innerAggregateExpression
/ emitTopKCorrelatedSubquery) appear before the outer bounds placeholders
($bounds->lft, $bounds->rgt) in the final bindings array, and (2) the predicate
bindings produced by FragmentSplicer are duplicated exactly once per textual
splice (i.e., the filter predicate bindings appear the expected number of times
in $aggBindings). Use the public API that triggers RecomputeMaintenance's
select-building logic and assert on the returned SQL fragments and binding array
to lock down this contract.
🪄 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 Plus
Run ID: f0400020-7e0c-4cdc-99d4-81826bacd2ed
📒 Files selected for processing (18)
src/Aggregates/Filters/BoundFragment.phpsrc/Aggregates/Filters/FilterPredicate.phpsrc/Aggregates/Filters/FilterValueQuoter.phpsrc/Aggregates/Filters/FragmentSplicer.phpsrc/Aggregates/Sql/AggregateSqlEmitter.phpsrc/Aggregates/Sql/DerivedAggregateFragments.phpsrc/Aggregates/Sql/VarianceSqlFragments.phpsrc/Aggregates/Strategy/DeltaMaintenance.phpsrc/Aggregates/Strategy/RecomputeMaintenance.phpsrc/Query/Aggregates/Maintenance/AggregateDiffer.phpsrc/Query/Aggregates/Read/AggregateSqlFragments.phpsrc/Query/Aggregates/Read/FreshAggregateProjector.phptests/Unit/Aggregates/Filters/FilterPredicateTest.phptests/Unit/Aggregates/Filters/FilterValueQuoterTest.phptests/Unit/Aggregates/Filters/FragmentSplicerTest.phptests/Unit/Aggregates/Sql/AggregateSqlEmitterTest.phptests/Unit/Aggregates/Sql/DerivedAggregateFragmentsTest.phptests/Unit/Query/Aggregates/Read/AggregateSqlFragmentsTest.php
💤 Files with no reviewable changes (2)
- src/Aggregates/Filters/FilterValueQuoter.php
- tests/Unit/Aggregates/Filters/FilterValueQuoterTest.php
The scalar fresh-read path placed filter bindings before bounds/scope bindings, but the textual order in the emitted SQL is bounds → scope → filter (the filter predicate sits inside the inner derived's WHERE). Positional `?` placeholders therefore picked up the wrong values. No feature test exercised the filtered scalar quantile or TopK path so the regression slipped past CI. Added `RecomputeBindingOrderTest` that captures the recompute SELECT via the query log and asserts: - predicate bindings precede outer bounds bindings (textual order) - predicate bindings appear once per textual splice Also hardened the snapshot-test `inline()` helper to fail fast when `?`-count and bindings-count diverge, so a future drop or stray would surface as an `InvalidArgumentException` instead of silently passing.
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
2 participants
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
PDO::quote. Addresses reviewer feedback about ergonomics + correctness on the read path.BoundFragmentvalue object (sql + bindings) flows through every aggregate-SQL helper.FragmentSpliceradapter runs each string-based emitter once with a sentinel and counts splices to repeat predicate bindings — avoids rewriting variance / quantile / topk / collection-aggregate internals.FilterValueQuoterremoved. Separator/JSON-key escaping inAggregateSqlEmitternow uses Laravel's blessedConnection::escape().Why
Inlined literals via
PDO::quotehad three issues:TRUE/FALSEvs1/0) thatConnection::escape()already handles correctly via driver overrides.Bindings via
?placeholders fix all three.Architecture
The cascade is mechanical but wide because the filter predicate gets spliced multiple times per aggregate expression (variance splices 3+×, stddev 6×, BoolAnd 3×, etc.). Two design choices kept the diff bounded:
BoundFragmentas the unified return type for every helper that previously returnedstringand embedded the predicate. Caller merges->bindingsinto its own binding stream.FragmentSplicersentinel pattern for emitters with conditional / per-driver internals. Each emitter has a thinBoundFragment-aware wrapper; the splicer feeds a sentinel into the internal?string $filterSqlpath, counts occurrences in the result, swaps for the real predicate, and repeats bindings N times. Internal SQL composition stays untouched.Result: bindings flow correctly through every read shape (scalar / derived / LATERAL / correlated) and through the recompute
UPDATEand differ paths.Verification
FragmentSplicerTestcovers null filter, single splice, N-splice binding repetition, and the literal-shortcut pathTest plan
composer fuzz)Summary by CodeRabbit
Release Notes
Breaking Changes
AggregateSqlEmitter,DerivedAggregateFragments, orVarianceSqlFragmentsmethods require updates.FilterValueQuoterclass; use connection's escape methods directly.Refactor