Arbitrary Where Clauses with Subqueries: Revised RFC and Plan#3852
Open
Arbitrary Where Clauses with Subqueries: Revised RFC and Plan#3852
Conversation
❌ 2 Tests Failed:
View the top 2 failed test(s) by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
✅ Deploy Preview for electric-next ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Documents completed work: - DNF Decomposer module with tests - Shape module DNF integration - WhereClause active conditions computation - Consumer invalidation updates - Integration test scaffold Documents remaining work: - NOT inversion in MoveHandling - OR move-out when all disjuncts false - Deduplication for rows in multiple disjuncts - Existing test updates Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
All critical functionality for DNF-based subquery handling is now complete: - NOT inversion in MoveHandling - OR deduplication via exclusion clauses - Router tests updated to expect proper behavior instead of 409 - All 1389 tests pass Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Consolidates three conflicting plan updates into one: - DnfContext architectural constraint (no new Shape struct fields) - Effective-value negation semantics in active_conditions and SQL - Phase 11 Elixir Client updates and wire format handling Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Column-name matching for sublink index resolution is ambiguous when multiple subqueries reference the same column (e.g., OR of two IN subqueries on parent_id). Add explicit extract_sublink_index/1 to Phase 7, a corresponding test case, and Risk #9 documenting the pitfall. Align Phase 5a naming for consistency. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The plan had a gap where non-subquery conditions in active_conditions SELECT columns were left as a vague `sql_for_subexpression` call with no implementation detail. This led to a stub returning "true", which is wrong for OR shapes where a row can match one disjunct but not another. Adds Phase 1a with a new `Electric.Replication.Eval.SqlGenerator` module alongside parser.ex, with comprehensive tests. Updates Phase 7 to explicitly branch on subquery vs non-subquery conditions, and rewrites Risk #7 to explain the actual bug. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Clarify the three distinct tag representations (condition_hashes,
2D internal tags, wire-format slash-delimited tags) and document
that all three are computed independently from column values by
Postgres SQL — none is derived from another in code.
Key changes:
- Add "Data Formats" section documenting condition_hashes, tags,
and wire format with their computation points
- Rename binary snapshot file contents from "tags" to
"condition_hashes" (one hash per DNF position, no nils)
- Change moved_out_tags type from %{name => {pos, MapSet}} to
%{name => %{pos => MapSet}} for multi-position support
- Replace Enum.at with map-based O(1) positional access throughout
- Update Phase 12 filtering to ANY-match semantics with map
pattern matching
- Update query_move_in to SELECT condition_hashes as separate
text[] column alongside JSON with embedded wire tags
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Async move-in queries can cause clients to see updates to existing rows before newly-eligible rows appear, breaking causal ordering. Note plan to restore consistency by delaying up-to-date markers (future work). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Align RFC examples with the implementation plan and codebase
(shape.ex Enum.join("/")). Tags are now shown as slash-delimited
strings (e.g. "hash1/hash2/", "//hash3") instead of nested JSON
arrays with nulls.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The structures that need O(1) positional access (condition_hashes, moved_out_tags) are already specified as maps where defined. active_conditions is fine as a list given the bounded size. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The "Revised Order of Operations" was identical to the earlier "Order of Operations" section. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Move each addendum section (§A-H) into the phase it belongs to so readers following phases in order see all details inline. §A was redundant with Phase 5 and was deleted. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Phase 10 removes the entire should_invalidate? block, but only discussed removing or_with_subquery? and not_with_subquery?. The length(shape_dependencies) > 1 check is equally significant since it's the guard preventing any multi-subquery shape from working. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replace ~200 lines of inline test code in Phase 1a with a prose summary of coverage and three representative examples showing the construct-AST-assert-SQL pattern. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
SqlGenerator must handle every AST node Parser can produce. Add a property-based test that generates arbitrary WHERE clauses, parses them, regenerates SQL via SqlGenerator, and asserts the normalized output matches the normalized input. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Remove misleading process_change_for_changes_only/3 pseudocode that implied a new function was needed. The existing do_process_changes/5 pipeline is already log_mode-agnostic. Add client-side implication note about simplified DNF evaluation without snapshot rows. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Remove the negation branch from generate_subquery_condition_sql so it always returns the un-negated condition. Negation is already applied by build_active_conditions_select's outer wrapper, so having both produced NOT (NOT (...)) for negated subquery positions. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The RFC previously said "no limit" on disjuncts, but the plan pragmatically introduces @max_disjuncts 100. Reconcile by updating the RFC — DNF expansion is exponential and unbounded disjuncts would overwhelm tag arrays and clients. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The old code called an undefined find_dnf_position_for_dep_index that assumed a single position per dependency. A dependency can map to multiple DNF positions (same subquery in different parts of the WHERE clause). Define the function returning a list of positions and update the disjunct partitioning to check membership against all trigger positions. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The existing has_dependencies guard means change_handling's DNF path is only reached for shapes that have a DnfContext. Remove misleading "or nil for non-DNF shapes" language that implied callers need a fallback. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The decomposition type already has a position_count field — use it in compute_active_conditions and build_active_conditions_select rather than recomputing via map_size. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add disjuncts_positions (polarity-stripped [[position()]]) to the decomposition type. evaluate_dnf only needs positions — polarity is irrelevant since active_conditions already has negation applied. This makes the API self-documenting: move_handling uses disjuncts (with polarity), evaluate_dnf uses disjuncts_positions (without). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The subexpression type only has {ast, is_subquery, negated} — no column
or columns fields. Add extract_columns_from_ast/1 to pull column names
from the AST at SQL generation time, and simplify to a single code path
that handles any number of columns uniformly.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…al annotations Mark every code block as either "final" (follow as written) or "directional" (shape of the solution, details left to implementer). Add a legend at the top of the implementation steps section and a TL;DR summary for Phase 12 to aid navigation. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replaces bare map pattern-matching (%{left: %{name: ...}}) with
Walker.reduce! collecting Eval.Parser.Ref nodes, consistent with the
rest of the codebase. Addresses review point 1.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Adds worked example showing that non-subquery positions (e.g. status = 'active') store column names in tag_structure just like subquery positions, and that fill_move_tags hashes the actual column value. Addresses review point 2. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…l WHERE For multi-disjunct shapes, the move-in WHERE clause is reconstructed from the triggering disjunct's conditions via SqlGenerator, not derived from shape.where.query by string replacement. The full-WHERE approach would over-fetch rows already present via other disjuncts and make exclusion clauses redundant. Updates both RFC (move-in query section) and implementation plan (Phase 7a). Addresses review point 3. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
robacourt
force-pushed
the
rob/subqueries-plan
branch
from
f5426f0 to
a532f01
Compare
Cherry-picked plan changes from cac7d4963 (Unify tag format to slash-delimited strings everywhere) - plan portion only. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
robacourt
force-pushed
the
rob/subqueries-plan
branch
from
a532f01 to
fba32ce
Compare
Contributor
|
Found 2 test failures on Blacksmith runners: Failures
|
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.
No description provided.