Skip to content

Store UUID filter values as compact binaries#4608

Merged
robacourt merged 6 commits into
mainfrom
rob/compress-uuid
Jun 17, 2026
Merged

Store UUID filter values as compact binaries#4608
robacourt merged 6 commits into
mainfrom
rob/compress-uuid

Conversation

@robacourt

@robacourt robacourt commented Jun 17, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Store parsed UUID values as 16-byte binaries (rather than 36 byte strings) inside the sync-service filter/eval representation, converting back to canonical UUID text at API and SQL output boundaries. This reduces memory use across the system, but especially in where-clause filter indexes, such as the equality and subquery index keys, by avoiding full canonical UUID strings for indexed values.
  • Bump the ShapeStatus metadata version so persisted shape comparable terms created with the old UUID string representation are ignored and rebuilt under the new representation.
  • Add a changeset for @core/sync-service.

Testing

  • mix test test/electric/shape_cache/shape_status_test.exs test/electric/shape_cache/shape_status/shape_db_test.exs
  • Temporarily patched the standard oracle schema from text/int IDs to UUID PK/FK columns and ran mix test --only oracle test/integration/oracle_property_test.exs successfully. This patch was not committed.
  • Temporarily ran the larger UUID oracle stress check successfully: CHECK_TIMEOUT=60000 SHAPE_COUNT=800 MUTATIONS_PER_TXN=10 TXNS_PER_BATCH=10 BATCH_COUNT=200 SKIP_REPATCH_PREWARM=true mix test --seed 8 --only oracle test/integration/oracle_property_test.exs. This patch was not committed.
  • Temporarily added route/subset coverage for id = $1::uuid and id = CAST($1 AS uuid) against the items.id UUID column; the four targeted tests passed and were not committed.

@codecov

codecov Bot commented Jun 17, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 59.00%. Comparing base (d2418d6) to head (68f03f5).
⚠️ Report is 6 commits behind head on main.
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4608      +/-   ##
==========================================
+ Coverage   57.43%   59.00%   +1.57%     
==========================================
  Files         342      385      +43     
  Lines       39928    42642    +2714     
  Branches    11597    12204     +607     
==========================================
+ Hits        22931    25160    +2229     
- Misses      16960    17407     +447     
- Partials       37       75      +38     
Flag Coverage Δ
packages/agents 72.79% <ø> (ø)
packages/agents-mcp 77.70% <ø> (?)
packages/agents-mobile 80.67% <ø> (ø)
packages/agents-runtime 82.66% <ø> (-0.02%) ⬇️
packages/agents-server 75.29% <ø> (-0.21%) ⬇️
packages/agents-server-ui 7.55% <ø> (+0.05%) ⬆️
packages/electric-ax 47.62% <ø> (+1.19%) ⬆️
packages/experimental 87.73% <ø> (?)
packages/react-hooks 86.48% <ø> (?)
packages/start 82.83% <ø> (?)
packages/typescript-client 91.83% <ø> (?)
packages/y-electric 56.05% <ø> (?)
typescript 59.00% <ø> (+1.57%) ⬆️
unit-tests 59.00% <ø> (+1.57%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@robacourt
robacourt requested review from alco and magnetised June 17, 2026 10:17
end
end

def uuid_to_string(<<_::128>> = uuid), do: Ecto.UUID.load!(uuid)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How is this different from Casting.uuid_to_string(value)?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is not meaningfully different here: Ecto.UUID.cast/1 accepts both canonical UUID strings and raw 16-byte UUID binaries.

I removed the <<_::128>>/Ecto.UUID.load!/1 special case so uuid_to_string/1 uses the single Ecto.UUID.cast/1 path for both forms.

Verified with:

mix test test/electric/replication/eval/env_test.exs test/electric/replication/eval/sql_generator_test.exs test/electric/replication/eval/runner_test.exs test/electric/shapes/filter_test.exs

)
|> Filter.add_shape(
"s2",
Shape.new!("uuid_table", where: "id = '#{uuid2}'::uuid", inspector: inspector)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what happens if we don't cast the value to uuid? If I were writing that where filter I wouldn't cast to uuid...

@robacourt robacourt Jun 17, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. The uncast form works: the unknown string literal is inferred from the UUID column side of the equality, and the equality index still stores the 16-byte UUID key.

I updated this test to use id = '...' rather than id = '...'::uuid, and verified it with:

mix test test/electric/shapes/filter_test.exs

@claude

claude Bot commented Jun 17, 2026

Copy link
Copy Markdown

Claude Code Review

Summary

Iteration 4 review. This PR stores parsed UUIDs as compact 16-byte binaries inside the sync-service eval/filter representation, converting back to canonical text at SQL and output boundaries. Since iteration 3 the author landed commit 68f03f5 ("Reject invalid UUID text in eval parser"), which closes the last carried-over item. The implementation is now clean, consistent, and well covered — no outstanding blocking concerns.

Previous Review Status

  • Iteration-1 Important Working with Postgres WAL format from Elixir #1 (parse_uuid/1 16-byte fast-path skipped validation)Resolved in 68f03f5. The <<_::128>> fast-path is gone; parse_uuid/1 now always routes through Ecto.UUID.dump/1, so invalid input (including exactly-16-char garbage like 'abcdefghijklmnop') fails the {:ok, value} = ... match, raises, and parse_const's try/rescue converts it back to a clean :error at shape creation — restoring the pre-PR validation behavior. Removing the fast-path is safe: parse_uuid/1 is reachable only via the uuid(text) -> uuid delegate (known_functions.ex:71), which parse_const always calls with a text literal, so a raw 16-byte binary never reaches it. Two regression tests were added (env_test.exs:23 scalar, env_test.exs:35 array). This is exactly the fix recommended in prior iterations.
  • @alco: "How is this different from Casting.uuid_to_string(value)?" — Addressed in 3c6e4fe (iteration 3); single Ecto.UUID.cast/1 path.
  • @magnetised: "what happens if we don't cast the value to uuid?" — Addressed in 69e3342 (iteration 2); filter_test.exs uses uncast id = '...'.
  • Iteration-1 suggestion (committed SubqueryIndex UUID coverage) — Still only covered by the uncommitted oracle runs. Non-blocking.

What's Working Well

  • The parse/output paths are now symmetric and both validating: parse_uuid/1 is a single Ecto.UUID.dump/1 clause and uuid_to_string/1 a single Ecto.UUID.cast/1 clause. Both are smaller and stricter than the original two-branch versions — the kind of simplification that removes a whole class of "what about the other branch" questions.
  • Idempotent boundary handling remains consistent: value_to_postgrex(<<_::128>>, :uuid) and the new SQL-generator clauses tolerate the compact form without double-conversion.
  • Test coverage now spans the happy path (scalar + array parse, const_to_pg_string/3 round-trip, the equality-index assertion in filter_test.exs that the 16-byte key — not the 36-char string — is stored) and the rejection path (invalid scalar and invalid array literals). Codecov reports all modified lines covered.

Issues Found

Critical (Must Fix)

None.

Important (Should Fix)

None remaining.

Suggestions (Nice to Have)

  • Possible now-redundant list clause. The typed-array clause to_sql_prec(%Const{type: {:array, type}, value: value}) (sql_generator.ex:262) intercepts list-valued array consts ahead of the older untyped is_list(value) clause (sql_generator.ex:276). The new path renders UUID arrays correctly and produces identical SQL for int/text arrays, so this isn't a bug — but clause 276 may now be reachable only for consts whose type isn't {:array, _}. Worth confirming whether it's still needed or can be dropped/commented. (Carried over, non-blocking.)
  • parse_uuid/1 and uuid_to_string/1 lack @specs — consistent with the surrounding unspecced parse_* helpers, so optional.

Issue Conformance

No linked issue (per review context); per project convention PRs should reference the issue they address — minor process note. The PR description is accurate and matches the implemented scope (representation change, boundary conversions, metadata bump, changeset for @core/sync-service). The uncommitted oracle/stress testing described is appropriate validation but leaves the SubqueryIndex UUID path without committed regression coverage.

Overall: this iteration resolves the only remaining flagged issue. The PR looks ready to merge from a correctness standpoint; the two suggestions are optional cleanups.


Review iteration: 4 | 2026-06-17

@robacourt
robacourt merged commit 214e644 into main Jun 17, 2026
70 checks passed
@robacourt
robacourt deleted the rob/compress-uuid branch June 17, 2026 13:06
@robacourt robacourt self-assigned this Jun 17, 2026
@github-actions

Copy link
Copy Markdown
Contributor

This PR has been released! 🚀

The following packages include changes from this PR:

  • @core/sync-service@1.7.3

Thanks for contributing to Electric!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants