Open
Conversation
The versionselector pushdown initially used github.com/spandigital/cel2sql/v3
to translate CEL selectors into SQL WHERE fragments. The codebase already
ships an SQL extractor at pkg/celutil/cel_sql.go (used by the resource
store) that fits this use case better:
- Parameterized output. cel2sql inlines escaped string literals into the
emitted SQL; celutil.SQLExtractor returns $N placeholders + an args
slice. SQL injection becomes structurally impossible rather than
verified-by-test, and the integration matches the pgx convention used
everywhere else in the binary.
- Native JSONB key access. version.metadata["env"] == "prod" now pushes
down as metadata->>$N = $N+1; cel2sql with our flat schema couldn't
translate this shape at all.
- One fewer third-party dependency.
The trade-off is OR composition: celutil.SQLExtractor only flattens
top-level && conjuncts, so OR-bearing selectors fall back to the
row-by-row CEL evaluator. Real-world version selectors are
overwhelmingly AND-shaped, and runtime CEL still produces the correct
result for any selector — pushdown is a candidate-set narrowing
optimization, not a correctness boundary.
Wire-up changes:
- versionselector.TryPushDown returns (clause, args, ok) and accepts a
startParam so multiple rules can share a single argument list with
non-overlapping placeholders.
- desiredrelease.IterCandidateVersions takes (pushdownClauses,
pushdownArgs); base parameters $1-$4 are reserved for deploymentID,
limit, and the keyset cursor; pushdown args land at $5+.
- collectPushdownClauses tracks the running placeholder offset and
silently skips untranslatable rules without consuming a slot.
- Singleflight cache key continues to hash the clause set; identical
selector text deterministically produces identical args, so no
separate args fingerprint is needed.
go.mod loses github.com/spandigital/cel2sql/v3 via go mod tidy.
* log.Info in charmbracelet/log expects key/value pairs after the message,
not printf-style formatting. The "%s" placeholder was rendered literally
("...filter: %s filter=<clause>") instead of being substituted. Drop
the placeholder and let the structured "filter" key carry the clause.
* Provides direnv integration so contributors get the project's flox environment auto-activated on cd into the repo. Local overrides land in .env.local (dotenv) or .envrc.local (full bash sourcing) so per-machine secrets and tweaks stay out of source control. Setting USE_FLOX=n in .env.local opts out for anyone not using flox.
Contributor
|
Warning Rate limit exceeded
To continue reviewing without waiting, purchase usage credits in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, 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 have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR migrates the SQL pushdown mechanism in the workspace-engine from CEL2SQL to a celutil-based parameterized approach. It updates the versionselector pushdown API to accept a parameter offset and return clause with arguments separately, cascades signature changes through getter and reconcile interfaces, and adds corresponding test coverage. The PR also adds a ChangesEnvironment Configuration
SQL Pushdown Refactoring
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![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: 1
🧹 Nitpick comments (2)
apps/workspace-engine/svc/controllers/desiredrelease/getters.go (1)
31-45: 💤 Low valueConsider documenting the placeholder offset as a named constant.
The contract pins
$5and the meaning of$1–$4directly in the interface comment, but the actual offset is computed inreconcile.goaspushdownBaseParamand consumed bygetters_postgres.go. If the reserved-param layout ever changes (e.g., a workspace filter is added), three files have to stay in lockstep, and this comment can silently drift.Not blocking — but a small
pushdownBaseParamconstant exported (or referenced) from a single source of truth would make the contract self-checking. Optional, defer if you prefer.🤖 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 `@apps/workspace-engine/svc/controllers/desiredrelease/getters.go` around lines 31 - 45, Add a single exported constant for the reserved-parameter offset (e.g., pushdownBaseParam) and use it as the canonical source of truth instead of hardcoding “$5” in comments and code: declare the constant where the reserved-parameter layout is defined (currently pushdownBaseParam in reconcile.go), export it (or move it to a shared package) so getters_postgres.go and the IterCandidateVersions interface comment can reference it, update reconcile.go to consume that exported constant, and update the IterCandidateVersions comment to mention the named constant rather than hardcoded $5 so all three locations stay in sync.apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/versionselector/pushdown_test.go (1)
94-102: 💤 Low valuePartial-pushdown test silently passes either way.
t.Skipwhenok=falsemeans the test passes whether the extractor pushes theversion.tagconjunct down or refuses entirely. If the goal is to document current behavior and lock it in, this is fine; if the goal is to guard against the extractor losing partial-extraction support, consider asserting one branch deterministically (the current celutil behavior) so a future regression fails loudly.Minor — feel free to keep as-is if intentional.
🤖 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 `@apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/versionselector/pushdown_test.go` around lines 94 - 102, The test TestTryPushDown_PartialAndDoesPushSubset currently calls TryPushDown and t.Skip() if ok==false, which silently lets the test pass whether partial pushdown is supported or not; decide on deterministic behavior and enforce it: replace the t.Skip branch with an explicit assertion (e.g., require.True(t, ok) or t.Fatalf) to lock in the current behavior (or alternatively assert require.False if you want to assert refusal), so the test fails on regressions; keep the subsequent assertions on clause and args the same to validate that TryPushDown produced the expected partial pushdown.
🤖 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 `@apps/workspace-engine/svc/controllers/desiredrelease/getters_postgres.go`:
- Around line 193-202: The singleflight key in PostgresGetter.fetchFirstBatch
uses deploymentID + hashClauses(pushdownClauses) which ignores pushdownArgs,
causing different selectors that compile to the same clause shape to collide;
update the key construction in fetchFirstBatch (and the equivalent block around
the other occurrence that spans the logic at lines ~309-321) to incorporate a
stable representation of pushdownArgs (e.g., serialize or hash pushdownArgs) so
the key becomes deploymentID + hashClauses(pushdownClauses) + ":" +
hashArgs(pushdownArgs); ensure the chosen serialization/hashing is deterministic
and safe for any arg types so concurrent fetches are correctly distinguished.
---
Nitpick comments:
In
`@apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/versionselector/pushdown_test.go`:
- Around line 94-102: The test TestTryPushDown_PartialAndDoesPushSubset
currently calls TryPushDown and t.Skip() if ok==false, which silently lets the
test pass whether partial pushdown is supported or not; decide on deterministic
behavior and enforce it: replace the t.Skip branch with an explicit assertion
(e.g., require.True(t, ok) or t.Fatalf) to lock in the current behavior (or
alternatively assert require.False if you want to assert refusal), so the test
fails on regressions; keep the subsequent assertions on clause and args the same
to validate that TryPushDown produced the expected partial pushdown.
In `@apps/workspace-engine/svc/controllers/desiredrelease/getters.go`:
- Around line 31-45: Add a single exported constant for the reserved-parameter
offset (e.g., pushdownBaseParam) and use it as the canonical source of truth
instead of hardcoding “$5” in comments and code: declare the constant where the
reserved-parameter layout is defined (currently pushdownBaseParam in
reconcile.go), export it (or move it to a shared package) so getters_postgres.go
and the IterCandidateVersions interface comment can reference it, update
reconcile.go to consume that exported constant, and update the
IterCandidateVersions comment to mention the named constant rather than
hardcoded $5 so all three locations stay in sync.
🪄 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: 85600f21-571b-41e4-bfde-63de58325d3f
⛔ Files ignored due to path filters (1)
apps/workspace-engine/go.sumis excluded by!**/*.sum
📒 Files selected for processing (12)
.envrcapps/workspace-engine/go.modapps/workspace-engine/pkg/store/resources/get_resources.goapps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/versionselector/pushdown.goapps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/versionselector/pushdown_test.goapps/workspace-engine/svc/controllers/desiredrelease/getters.goapps/workspace-engine/svc/controllers/desiredrelease/getters_postgres.goapps/workspace-engine/svc/controllers/desiredrelease/getters_postgres_test.goapps/workspace-engine/svc/controllers/desiredrelease/pushdown_test.goapps/workspace-engine/svc/controllers/desiredrelease/reconcile.goapps/workspace-engine/svc/controllers/desiredrelease/reconcile_test.goapps/workspace-engine/test/controllers/harness/mocks.go
Two CEL selectors can compile to the same SQL clause text while binding different values — `version.tag == "v1"` and `version.tag == "v2"` both emit `tag = $5`, the only difference being the arg slice. The fetchFirstBatch singleflight key previously hashed only the clauses, so concurrent reconciles for different release targets of the same deployment whose selectors collapsed to identical clause shapes would share the same cache slot and receive each other's filtered results. Extend the key with hashArgs(pushdownArgs) so any difference in bound values produces a distinct slot. Args use fmt %v for stable string representation, which is deterministic for the string-typed values celutil.SQLExtractor currently emits.
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.
The versionselector pushdown initially used github.com/spandigital/cel2sql/v3
to translate CEL selectors into SQL WHERE fragments. The codebase already
ships an SQL extractor at pkg/celutil/cel_sql.go (used by the resource
store) that fits this use case better:
Parameterized output. cel2sql inlines escaped string literals into the
emitted SQL; celutil.SQLExtractor returns $N placeholders + an args
slice. SQL injection becomes structurally impossible rather than
verified-by-test, and the integration matches the pgx convention used
everywhere else in the binary.
Native JSONB key access. version.metadata["env"] == "prod" now pushes
down as metadata->>$N = $N+1; cel2sql with our flat schema couldn't
translate this shape at all.
One fewer third-party dependency.
The trade-off is OR composition: celutil.SQLExtractor only flattens
top-level && conjuncts, so OR-bearing selectors fall back to the
row-by-row CEL evaluator. Real-world version selectors are
overwhelmingly AND-shaped, and runtime CEL still produces the correct
result for any selector — pushdown is a candidate-set narrowing
optimization, not a correctness boundary.
Wire-up changes:
startParam so multiple rules can share a single argument list with
non-overlapping placeholders.
pushdownArgs); base parameters $1-$4 are reserved for deploymentID,
limit, and the keyset cursor; pushdown args land at $5+.
silently skips untranslatable rules without consuming a slot.
selector text deterministically produces identical args, so no
separate args fingerprint is needed.
go.mod loses github.com/spandigital/cel2sql/v3 via go mod tidy.
Summary by CodeRabbit
Release Notes
Chores
Refactor