Support JSONB operators for shapes #3711
Conversation
This commit adds support for the two most fundamental JSONB operators: - `->` (get field/element as jsonb): Access object fields by key or array elements by index, returning jsonb - `->>` (get field/element as text): Same as above but returns text representation of the value Implementation details: - Added jsonb to BasicTypes with 'user' category - Added jsonb(text) -> jsonb parsing function - Added jsonbout(jsonb) -> text output function - Added jsonb equality operators (=, <>) - Comprehensive tests for all operators including: - Object field access by key - Array element access by index - Text extraction with type coercion - Chained operators for nested access - Comparison operations - Common filtering patterns like (data ->> 'status') = 'active' These operators enable WHERE clause filtering on JSONB columns, which was previously not possible despite JSONB being a supported type.
|
Warning Rate limit exceeded
⌛ 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. 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughAdds JSONB to the known types, implements PostgreSQL-like JSONB constructors, access, containment, comparison, and key-existence operators, refactors type parsing to support array syntax, and adds extensive evaluator tests plus a changeset entry. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
❌ 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 |
This comment has been minimized.
This comment has been minimized.
Co-Authored-By: Claude Opus 4.5 <[email protected]>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@packages/sync-service/lib/electric/replication/eval/env/known_functions.ex`:
- Around line 190-209: Extract the jsonb_value_to_text clauses into a single
module-level private helper and have both PostgreSQL operator implementations
call it: move the defp jsonb_value_to_text(...) definitions out of the first
defpostgres block to the module scope as a private function, then in
jsonb_get_text_by_key (the "jsonb ->> text -> text" implementation) and in the
second operator implementation ("jsonb ->> int4 -> text") replace the duplicated
inline conversion logic with calls to jsonb_value_to_text/1; ensure the function
signatures and nil handling remain identical so SQL NULL behavior is preserved.
- Around line 211-229: The JSON-to-text conversion in the defpostgres block for
the "jsonb ->> int4 -> text" operator duplicates logic; extract the conversion
into a module-level private helper (e.g., defp jsonb_value_to_text/1) and
replace the case branches inside jsonb_get_text_by_index/2 with a call to that
helper, ensuring the helper handles binaries, integers, floats, booleans, nil,
and fallback encoding (Jason.encode!); keep jsonb_get_text_by_index/2’s index
bounds and nil handling intact and reuse the same helper from the existing "->>
text" operator.
In `@packages/sync-service/test/electric/replication/eval/runner_test.exs`:
- Around line 307-340: The test block named "should work with jsonb comparison"
is misformatted and fails mix format; run `mix format` (or reformat the asserted
pipelines) so the chained pipelines using Parser.parse_and_validate_expression!
and Runner.execute align with the project's formatter rules and consistent
indentation for the multi-line assertions; after formatting, re-run CI to
confirm the mix format check passes.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
packages/sync-service/lib/electric/replication/eval/env/basic_types.expackages/sync-service/lib/electric/replication/eval/env/known_functions.expackages/sync-service/test/electric/replication/eval/runner_test.exs
🧰 Additional context used
🧬 Code graph analysis (1)
packages/sync-service/lib/electric/replication/eval/env/known_functions.ex (3)
examples/burn/lib/burn/serialize.ex (1)
to_string(2-4)packages/sync-service/lib/electric/postgres.ex (1)
defmodule Electric.Postgres do(1-66)packages/sync-service/lib/electric/shapes/querying.ex (1)
defp pg_coalesce_json_string(str), do: ~s[coalesce(#{str} , 'null')](268-268)
🪛 GitHub Actions: Elixir formatting
packages/sync-service/test/electric/replication/eval/runner_test.exs
[error] 309-336: Mix format check failed. The following files are not formatted. Run 'mix format' to format the code.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: Test packages/y-electric w/ sync-service
- GitHub Check: Test packages/typescript-client w/ sync-service
- GitHub Check: Test packages/react-hooks w/ sync-service
- GitHub Check: Test packages/experimental w/ sync-service
- GitHub Check: Test packages/start w/ sync-service
- GitHub Check: Build and test sync-service, pg17
- GitHub Check: Run Lux integration tests
- GitHub Check: Build and test sync-service, pg14
- GitHub Check: Build and test sync-service, pg15
- GitHub Check: Build and test sync-service, pg18
🔇 Additional comments (12)
packages/sync-service/lib/electric/replication/eval/env/basic_types.ex (1)
29-29: LGTM!The
jsonbtype is correctly added to the Known types catalog under theusercategory, consistent with other binary/opaque types likebyteaanduuid. This aligns with the existingsupported_types()inElectric.Postgreswhich already includes:jsonb.packages/sync-service/test/electric/replication/eval/runner_test.exs (6)
187-211: LGTM!Good coverage of the
->operator for object field access, including nested objects, missing fields, and null input handling.
213-236: LGTM!Comprehensive array index access tests correctly validate PostgreSQL's 0-based indexing and null return for out-of-bounds/negative indices.
238-271: LGTM!Thorough coverage of the
->>text extraction operator, including type coercion for strings, numbers, booleans, and nested structures.
273-285: LGTM!Array text extraction via
->>is properly tested for both string and numeric elements.
287-305: LGTM!Excellent coverage of chained operator patterns including nested object traversal and mixed object/array access.
342-353: LGTM!Good coverage of the primary use case: WHERE-clause filtering on JSONB columns using the
->>operator with string comparison.packages/sync-service/lib/electric/replication/eval/env/known_functions.ex (5)
161-165: LGTM!The nil guard is correct, and
Jason.encode!/1is safe here since the input is already a valid JSONB value from prior parsing.
167-174: LGTM!Clean implementation of object field access with appropriate nil fallback for type mismatches.
176-188: LGTM!Correctly implements PostgreSQL's 0-based array indexing with proper nil handling for negative indices and type mismatches.
231-233: LGTM!Using
Kernel.==/2for JSONB equality is correct. Elixir maps ignore key order (like PostgreSQL JSONB), so%{"a" => 1, "b" => 2} == %{"b" => 2, "a" => 1}returnstrue, matching PostgreSQL semantics.
156-159: The current implementation usingJason.decode!/1is consistent with the codebase's established error handling patterns. Other type parsing functions inCasting(e.g.,parse_int4,parse_date) and similar conversion functions in this module use bang functions that raise exceptions on invalid input. Additionally, PostgreSQL's behavior for invalid JSONB casts is to raise an error rather than return NULL. No changes needed.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| # Get object field or array element as text | ||
| defpostgres "jsonb ->> text -> text" do | ||
| def jsonb_get_text_by_key(json, key) when is_map(json) and is_binary(key) do | ||
| case Map.get(json, key) do | ||
| nil -> nil | ||
| value -> jsonb_value_to_text(value) | ||
| end | ||
| end | ||
|
|
||
| def jsonb_get_text_by_key(_, _), do: nil | ||
|
|
||
| defp jsonb_value_to_text(value) when is_binary(value), do: value | ||
| defp jsonb_value_to_text(value) when is_integer(value), do: Integer.to_string(value) | ||
| defp jsonb_value_to_text(value) when is_float(value), do: Float.to_string(value) | ||
| defp jsonb_value_to_text(true), do: "true" | ||
| defp jsonb_value_to_text(false), do: "false" | ||
| defp jsonb_value_to_text(nil), do: nil | ||
| # For nested objects/arrays, return JSON string | ||
| defp jsonb_value_to_text(value), do: Jason.encode!(value) | ||
| end |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check all occurrences of jsonb_value_to_text in the file
rg -n "jsonb_value_to_text" packages/sync-service/lib/electric/replication/eval/env/known_functions.ex -B2 -A2Repository: electric-sql/electric
Length of output: 798
🏁 Script executed:
#!/bin/bash
# Look at the jsonb ->> int4 operator implementation to see if it also uses jsonb_value_to_text
rg -n "jsonb.*int4|int4.*jsonb" packages/sync-service/lib/electric/replication/eval/env/known_functions.ex -A20Repository: electric-sql/electric
Length of output: 1521
🏁 Script executed:
#!/bin/bash
# Get broader context around lines 190-209 to understand the defpostgres structure
sed -n '180,220p' packages/sync-service/lib/electric/replication/eval/env/known_functions.exRepository: electric-sql/electric
Length of output: 1481
Extract the value-to-text conversion logic as a shared module-level helper to avoid duplication.
The jsonb_value_to_text helper (lines 201–208) handles the conversion logic, but it's scoped privately within the first defpostgres block and cannot be accessed by the second operator (jsonb ->> int4 -> text at line 211). Consequently, the same conversion logic is duplicated inline in lines 216–221 of the second operator. Extract this as a module-level private function to eliminate duplication and simplify maintenance.
The PostgreSQL null behavior is correctly implemented: jsonb_value_to_text(nil) returns Elixir nil, which correctly maps to SQL NULL.
🤖 Prompt for AI Agents
In `@packages/sync-service/lib/electric/replication/eval/env/known_functions.ex`
around lines 190 - 209, Extract the jsonb_value_to_text clauses into a single
module-level private helper and have both PostgreSQL operator implementations
call it: move the defp jsonb_value_to_text(...) definitions out of the first
defpostgres block to the module scope as a private function, then in
jsonb_get_text_by_key (the "jsonb ->> text -> text" implementation) and in the
second operator implementation ("jsonb ->> int4 -> text") replace the duplicated
inline conversion logic with calls to jsonb_value_to_text/1; ensure the function
signatures and nil handling remain identical so SQL NULL behavior is preserved.
Eliminates duplicate value-to-text conversion logic by reusing the existing helper function. Co-Authored-By: Claude Opus 4.5 <[email protected]>
Co-Authored-By: Claude Opus 4.5 <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/sync-service/test/electric/replication/eval/runner_test.exs`:
- Around line 187-347: Add tests covering jsonb type-mismatch behavior: create a
new test (e.g., "should return nil for type mismatches with jsonb -> operator")
that asserts {:ok, nil} for using -> with a string key on an array
(parse_and_validate_expression! with refs: %{["x"] => :jsonb} then
Runner.execute with %{["x"] => [1,2,3]}) and asserts {:ok, nil} for using ->
with an integer index on an object (same parse/execute but %{["x"] => %{"a" =>
1}}); place this alongside the existing jsonb operator tests to document the
expected nil result for these mismatches.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
.changeset/add-jsonb-operators.mdpackages/sync-service/lib/electric/replication/eval/env/known_functions.expackages/sync-service/test/electric/replication/eval/runner_test.exs
🧰 Additional context used
🧬 Code graph analysis (1)
packages/sync-service/lib/electric/replication/eval/env/known_functions.ex (3)
packages/sync-service/lib/electric/postgres.ex (2)
defmodule Electric.Postgres do(1-66)def supported_types do(10-24)packages/sync-service/lib/electric/replication/eval/parser.ex (1)
defmodule Func do(72-102)packages/sync-service/lib/electric/shapes/querying.ex (1)
defp pg_coalesce_json_string(str), do: ~s[coalesce(#{str} , 'null')](268-268)
🪛 markdownlint-cli2 (0.18.1)
.changeset/add-jsonb-operators.md
5-5: First line in a file should be a top-level heading
(MD041, first-line-heading, first-line-h1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: Test packages/typescript-client w/ sync-service
- GitHub Check: Test packages/start w/ sync-service
- GitHub Check: Test packages/experimental w/ sync-service
- GitHub Check: Test packages/react-hooks w/ sync-service
- GitHub Check: Test packages/y-electric w/ sync-service
- GitHub Check: Build and test sync-service, pg14
- GitHub Check: Build and test sync-service, pg17
- GitHub Check: Build and test sync-service, pg15
- GitHub Check: Build and test sync-service, pg18
- GitHub Check: Run Lux integration tests
🔇 Additional comments (15)
packages/sync-service/lib/electric/replication/eval/env/known_functions.ex (7)
162-165: LGTM - nil handling is correct.The
jsonb_out/1function correctly returnsnilfornilinput (preserving SQL NULL semantics) and delegates toJason.encode!/1for all other values.
167-174: LGTM - object field access operator.Correctly returns
nilfor non-map inputs or missing keys, matching PostgreSQL behavior.
176-188: LGTM - array index access with correct PostgreSQL semantics.The implementation correctly:
- Uses 0-based indexing per PostgreSQL JSON array convention
- Returns
nilfor negative indices (matching PostgreSQL, unlike Python)- Returns
nilfor non-list inputs
190-209: LGTM - text extraction with comprehensive type conversion.The
jsonb_value_to_text/1helper correctly handles all JSON value types:
- Strings passed through
- Numbers converted via standard library functions
- Booleans to lowercase strings ("true"/"false")
nilpreserved for SQL NULL- Nested structures encoded as JSON strings
223-225: LGTM - JSONB equality operators.Delegating to
Kernel.==/2andKernel.!=/2is appropriate for structural equality comparison of decoded JSON values (maps, lists, primitives).
157-159: Error handling is consistent with existing codebase patterns; no changes needed.
Jason.decode!/1raises on invalid JSON, but this is the standard approach used throughout the Casting module for parse functions (parse_int2,parse_uuid,parse_date,parse_timestamp, etc. all use!-raising variants). Exceptions from these functions are expected to propagate in this SQL evaluation context, making the current implementation appropriate and consistent.
211-221: Remove this comment — the code is correct.The
jsonb_value_to_text/1helper defined in the firstdefpostgresblock (lines 201–208) is accessible in the second block (line 214). Thedefpostgresmacro expands the entiredo:block as module-level definitions viaunquote(insertion), making all functions from both blocks part of the same module scope. Private functions (defp) are accessible throughout the module, so the call at line 214 works as intended..changeset/add-jsonb-operators.md (1)
1-5: LGTM - changeset is well-documented.The changeset correctly describes the new JSONB operators (
->and->>) and their functionality. The MD041 lint warning about missing first-line heading is a false positive—changeset files conventionally use YAML frontmatter.packages/sync-service/test/electric/replication/eval/runner_test.exs (7)
187-211: LGTM - comprehensive object field access tests.Good coverage including:
- Basic field access
- Nested object access
- Non-existent field returns
nilniljsonb input returnsnil
213-236: LGTM - array index access tests with proper edge cases.Tests correctly verify PostgreSQL behavior:
- 0-based indexing
- Out-of-bounds returns
nil- Negative indices return
nil(not Python-style wraparound)
238-271: LGTM - text extraction tests cover all value types.Excellent coverage of
->>operator including string, number, boolean, nested object (as JSON string), and missing field scenarios.
273-285: LGTM - array text extraction tests.Covers string and numeric element extraction.
287-305: LGTM - chained operator tests.Good coverage of operator chaining including:
- Nested object access (
-> ->)- Mixed access then extraction (
-> ->>)- Object-to-array navigation
307-334: LGTM - comparison operator tests.Tests equality and inequality for JSONB values.
336-347: LGTM - practical filtering pattern test.This test validates the common use case of
(x ->> 'field') = 'value'in WHERE clauses, which is the primary motivation for this PR.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
Add support for JSONB containment checking:
- `@>` (contains): checks if left JSONB contains right JSONB
- `<@` (contained by): checks if left JSONB is contained by right
Containment semantics match PostgreSQL:
- Objects: all key-value pairs in right must exist in left
- Arrays: all elements in right must exist somewhere in left
- Scalars: must be equal
Common use case: `data @> '{"type": "premium"}'::jsonb`
Add support for checking key/element existence in JSONB: - `?` (key exists): checks if key exists in object or string in array - `?|` (any key exists): checks if any of the keys exist - `?&` (all keys exist): checks if all of the keys exist Examples: - `data ? 'name'` - does the object have a 'name' key? - `data ?| ARRAY['email', 'phone']` - has email OR phone? - `data ?& ARRAY['name', 'age']` - has both name AND age?
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/sync-service/lib/electric/replication/eval/env/known_functions.ex`:
- Around line 253-273: The implementation of jsonb_contained_by?/2 duplicates
the recursive logic in do_jsonb_contained_by?/2 which is identical to
do_jsonb_contains?/2; replace the duplicated private helpers by delegating to
the existing do_jsonb_contains?/2 with swapped arguments (e.g., have
jsonb_contained_by?(left, right) call do_jsonb_contains?(right, left) or make
do_jsonb_contains?/2 a module-level private function and call it from the
defpostgres block), removing the redundant do_jsonb_contained_by?/2 definitions.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
.changeset/add-jsonb-operators.mdpackages/sync-service/lib/electric/replication/eval/env/known_functions.expackages/sync-service/test/electric/replication/eval/runner_test.exs
🧰 Additional context used
🧬 Code graph analysis (1)
packages/sync-service/test/electric/replication/eval/runner_test.exs (1)
packages/sync-service/lib/electric/replication/eval/runner.ex (1)
execute(54-58)
🪛 markdownlint-cli2 (0.18.1)
.changeset/add-jsonb-operators.md
5-5: First line in a file should be a top-level heading
(MD041, first-line-heading, first-line-h1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: Test packages/start w/ sync-service
- GitHub Check: Test packages/experimental w/ sync-service
- GitHub Check: Test packages/y-electric w/ sync-service
- GitHub Check: Test packages/typescript-client w/ sync-service
- GitHub Check: Test packages/react-hooks w/ sync-service
- GitHub Check: Build and test sync-service, pg18
- GitHub Check: Build and test sync-service, pg14
- GitHub Check: Build and test sync-service, pg15
- GitHub Check: Build and test sync-service, pg17
- GitHub Check: Run Lux integration tests
🔇 Additional comments (9)
packages/sync-service/lib/electric/replication/eval/env/known_functions.ex (3)
156-165: LGTM on jsonb parsing and output functions.The
jsonb(text)parser usingJason.decode!/1andjsonboutusingJason.encode!/1correctly handle the text-to-jsonb conversion and vice versa. The nil guard injsonb_out/1properly propagates SQL NULL.
167-188: LGTM on->operator implementations.Both object field access and array index access are correctly implemented:
jsonb_get_by_key/2properly returnsnilfor missing keysjsonb_get_by_index/2correctly handles 0-based indexing and returnsnilfor negative indices per PostgreSQL semantics- The catch-all clauses returning
nilhandle type mismatches gracefully
190-221: No refactoring needed. Thejsonb_value_to_text/1helper is already defined at module scope by the firstdefpostgresblock and is correctly accessible from the second block. Thedefpostgresmacro inserts the entire do_block directly into the module viaunquote(insertion), making all definitions module-level despite their local appearance. There is no duplication, and the code compiles without issues.Likely an incorrect or invalid review comment.
.changeset/add-jsonb-operators.md (1)
1-9: LGTM on changeset documentation.The changeset correctly documents the new JSONB operators (
->,->>,@>,<@) and their capabilities. The static analysis MD041 warning about missing first-line heading is a false positive since changesets use frontmatter format by design.packages/sync-service/test/electric/replication/eval/runner_test.exs (5)
187-236: Comprehensive test coverage for->operator.The tests cover:
- Object field access (string values, nested objects, missing keys, null input)
- Array index access (valid indices, out-of-bounds, negative indices)
This properly validates PostgreSQL-compatible behavior including 0-based indexing and
nilreturns for edge cases.
238-285: Good coverage for->>text extraction operator.Tests verify:
- String, number, and boolean extraction as text
- Nested object serialization to JSON string
- Missing field returns nil
- Array element extraction with type conversion
The test at lines 258-264 correctly validates that nested objects are JSON-encoded when extracted via
->>.
287-305: LGTM on chained operator tests.The tests validate important chaining scenarios:
->then->for deep object access->then->>for nested text extraction- Mixed object/array access paths
349-414: Thorough containment operator (@>) test coverage.Tests cover all PostgreSQL
@>semantics:
- Object key-value containment (match/mismatch/missing key)
- Array element containment (order-independent)
- Nested object containment
- Scalar equality fallback
416-462: Good coverage for<@and practical filtering patterns.The
<@tests verify the inverse containment relationship, and the practical pattern tests (lines 445-462) demonstrate real-world usage likex @> '{"type": "premium"}'::jsonb.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
packages/sync-service/lib/electric/replication/eval/env/known_functions.ex
Show resolved
Hide resolved
The regex patterns in known_definition.ex now support array type notation
like `text[]` for operator argument types. This enables operators like
`jsonb ?| text[] -> bool` to be properly parsed.
Changes:
- Add @type_pattern module attribute with regex supporting [] suffix
- Use Regex.compile!/1 instead of sigils for proper interpolation
- Add parse_type_string/1 to convert "text[]" to {:array, :text}
Add type_to_string/1 helper to properly convert array type tuples
like {:array, :text} to strings for category validation.
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)
packages/sync-service/lib/electric/replication/eval/known_definition.ex (1)
378-398: Fix compilation error:to_string/1fails on array type tuples.The pipeline fails because
validate_at_most_one_category/1callsto_string/1onargs, which now can contain tuples like{:array, :text}fromparse_type_string/1. TheString.Charsprotocol is not implemented for tuples.🐛 Proposed fix to handle array types
defp validate_at_most_one_category(%{defined_at: line, args: args} = map) do args - |> Enum.map(&to_string/1) + |> Enum.map(&type_to_string/1) |> Enum.filter(&Regex.match?(~r/\*[[:alnum:]_]+\*/, &1)) |> Enum.uniq() |> case do [_, _ | _] = keys -> raise CompileError, line: line, description: "defpostgres function cannot have more that one type category in definition, got #{inspect(keys)}" [category] when category not in ~w|*numeric_type* *integral_type*| -> raise CompileError, line: line, description: "unknown category #{category} - cannot expand definitions" _ -> map end end + + # Convert type representation to string for category validation + defp type_to_string({:array, base_type}), do: "#{base_type}[]" + defp type_to_string(type) when is_atom(type), do: Atom.to_string(type)
🤖 Fix all issues with AI agents
In `@packages/sync-service/test/electric/replication/eval/runner_test.exs`:
- Around line 464-533: The failing tests use ~S|...| sigils containing the
sequences ?| and ?& which make Elixir parse the | as part of a char literal;
update the three test cases in runner_test.exs (the "should work with jsonb ?|
any-key-exists operator" and "should work with jsonb ?& all-keys-exist operator"
tests) to use a different sigil delimiter (e.g., ~S(...) or ~S{...}) for the
SQL-expression strings passed into Parser.parse_and_validate_expression! so
expressions like ~S(... ?| ARRAY['name','missing'] ...) and ~S(... ?& ARRAY[...]
...) are parsed correctly before calling Runner.execute.
♻️ Duplicate comments (1)
packages/sync-service/lib/electric/replication/eval/env/known_functions.ex (1)
253-273: Consider consolidating containment logic to reduce duplication.The
do_jsonb_contained_by?/2implementation duplicatesdo_jsonb_contains?/2. Sincea <@ bis semantically equivalent tob @> a, you could eliminate duplication by either:
- Making
do_jsonb_contains?/2a module-level helper and callingdo_jsonb_contains?(right, left)- Combining both operators into a single
defpostgresblock
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
.changeset/add-jsonb-operators.mdpackages/sync-service/lib/electric/replication/eval/env/known_functions.expackages/sync-service/lib/electric/replication/eval/known_definition.expackages/sync-service/test/electric/replication/eval/runner_test.exs
🧰 Additional context used
🧬 Code graph analysis (2)
packages/sync-service/test/electric/replication/eval/runner_test.exs (2)
packages/sync-service/lib/electric/replication/eval/runner.ex (1)
execute(54-58)packages/elixir-client/lib/electric/client/fetch/response.ex (1)
decode!(28-34)
packages/sync-service/lib/electric/replication/eval/env/known_functions.ex (2)
examples/burn/lib/burn/serialize.ex (1)
to_string(2-4)packages/sync-service/lib/pg_interop/sublink.ex (1)
member?(6-8)
🪛 GitHub Actions: Elixir formatting
packages/sync-service/test/electric/replication/eval/runner_test.exs
[error] 493-493: mix format failed for file. SyntaxError: invalid syntax found on test/electric/replication/eval/runner_test.exs:493:24: error: syntax error before: 'ARRAY'
🪛 GitHub Actions: Lux Integration Tests
packages/sync-service/lib/electric/replication/eval/env/known_functions.ex
[error] 291-291: Elixir compile error: Protocol.UndefinedError: String.Chars not implemented for Tuple. Got value: {:array, :text}.
🪛 GitHub Actions: sync-service
packages/sync-service/lib/electric/replication/eval/env/known_functions.ex
[error] 1-1: Compilation error: Protocol String.Chars not implemented for Tuple {:array, :text}. Likely a type mismatch occurring in Electric.Replication.Eval.KnownDefinition.validate_at_most_one_category/1
🪛 markdownlint-cli2 (0.18.1)
.changeset/add-jsonb-operators.md
5-5: First line in a file should be a top-level heading
(MD041, first-line-heading, first-line-h1)
🔇 Additional comments (6)
.changeset/add-jsonb-operators.md (1)
1-10: LGTM!The changeset accurately documents the new JSONB operators being added. The static analysis hint about MD041 (first-line-heading) is a false positive since changeset files use YAML front matter by convention.
packages/sync-service/test/electric/replication/eval/runner_test.exs (1)
187-462: Comprehensive JSONB test coverage.The tests thoroughly cover the JSONB operators including:
- Object and array access with
->and->>- Chained access patterns
- NULL/nil handling for missing keys and out-of-bounds indices
- Containment operators
@>and<@- Equality comparisons
- Common filtering patterns
The test cases align well with PostgreSQL semantics (0-based indexing, negative indices returning NULL, etc.).
packages/sync-service/lib/electric/replication/eval/known_definition.ex (1)
187-264: Well-designed type parsing extension.The changes correctly extend the DSL to support array type syntax:
@type_patternhandles both simple types andtype[]notationparse_type_string/1properly converts strings to:typeor{:array, :type}- Regex patterns are precompiled as module attributes for efficiency
packages/sync-service/lib/electric/replication/eval/env/known_functions.ex (3)
154-188: JSONB accessor implementations look correct.The
->operators correctly implement PostgreSQL semantics:
- 0-based array indexing
- Negative indices return
nil(matching PostgreSQL NULL behavior)- Missing keys/out-of-bounds access return
nil- Proper type guards distinguish object vs array access
275-316: Key existence operators rely on array type support.The
?|and?&operators usetext[]type signatures which depend on theparse_type_string/1changes inknown_definition.ex. Ensure thevalidate_at_most_one_category/1fix is applied first, otherwise these operators will cause compilation errors.
211-221: No scoping issue exists. Thedefpostgresmacro expands all function definitions—includingdefphelpers—into module scope viaunquote(insertion). Both blocks compile into the same module (Electric.Replication.Eval.Env.KnownFunctions), making thejsonb_value_to_text/1helper defined at lines 201-208 accessible to the call at line 214.Likely an incorrect or invalid review comment.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
The ~S|...| sigil conflicts with the ?| operator in test strings. Using ~S(...) avoids this syntax error.
This comment has been minimized.
This comment has been minimized.
|
Found 5 test failures on Blacksmith runners: Failures
|
The <@ operator is just @> with swapped arguments. No need to duplicate the recursive containment logic. Co-Authored-By: Claude Opus 4.5 <[email protected]>
Co-Authored-By: Claude Opus 4.5 <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @.changeset/add-jsonb-operators.md:
- Around line 12-20: Add an HTTP language specifier to the fenced code block in
.changeset/add-jsonb-operators.md so the examples render with proper syntax
highlighting; replace the opening fence "```" with "```http" (or "```bash") for
the block containing the three GET examples that reference JSONB operators like
(metadata ->> 'status'), @>, and ? to ensure clients and reviewers see correct
highlighting.
♻️ Duplicate comments (3)
packages/sync-service/lib/electric/replication/eval/env/known_functions.ex (2)
190-221: LGTM - Previous duplication concern addressed.The
->>operators correctly extract values as text with proper type coercion. Thejsonb_value_to_texthelper (lines 201-208) is reused by the second operator on line 214, avoiding code duplication.Note: The private helper
jsonb_value_to_text/1defined inside the firstdefpostgresblock is accessible to subsequent blocks since they're all compiled into the same module scope. This is the correct approach.
253-255: Previous refactor suggestion implemented - Verify cross-block helper visibility.The
<@operator correctly reusesdo_jsonb_contains?/2with swapped arguments instead of duplicating the recursive containment logic.However, this relies on
do_jsonb_contains?/2(defined asdefpin the@>block on lines 233-250) being accessible from the<@block. While this should work since both blocks compile to the same module scope, this implicit dependency could be fragile if the block order changes or if someone refactors without noticing the dependency.Consider verifying this compiles correctly:
#!/bin/bash # Verify the module compiles without errors cd packages/sync-service mix compile --warnings-as-errors 2>&1 | head -50packages/sync-service/test/electric/replication/eval/runner_test.exs (1)
187-533: Consider adding type mismatch edge case tests.The test suite is comprehensive, but per a previous suggestion, consider adding explicit tests for type mismatch scenarios to document expected behavior:
- Using
->with a string key on a JSON array (e.g.,x -> 'foo'where x is[1,2,3])- Using
->with an integer index on a JSON object (e.g.,x -> 0where x is{"a":1})Both should return
nilbased on the implementation, but explicit tests would document this behavior.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
.changeset/add-jsonb-operators.mdpackages/sync-service/lib/electric/replication/eval/env/known_functions.expackages/sync-service/lib/electric/replication/eval/known_definition.expackages/sync-service/test/electric/replication/eval/runner_test.exs
🧰 Additional context used
🧬 Code graph analysis (2)
packages/sync-service/test/electric/replication/eval/runner_test.exs (2)
packages/sync-service/lib/electric/replication/eval/runner.ex (1)
execute(54-58)packages/elixir-client/lib/electric/client/fetch/response.ex (1)
decode!(28-34)
packages/sync-service/lib/electric/replication/eval/env/known_functions.ex (2)
examples/burn/lib/burn/serialize.ex (1)
to_string(2-4)packages/sync-service/lib/pg_interop/sublink.ex (1)
member?(6-8)
🪛 markdownlint-cli2 (0.18.1)
.changeset/add-jsonb-operators.md
5-5: First line in a file should be a top-level heading
(MD041, first-line-heading, first-line-h1)
12-12: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: Test packages/react-hooks w/ sync-service
- GitHub Check: Test packages/typescript-client w/ sync-service
- GitHub Check: Test packages/experimental w/ sync-service
- GitHub Check: Test packages/y-electric w/ sync-service
- GitHub Check: Test packages/start w/ sync-service
- GitHub Check: Build and test sync-service, pg17
- GitHub Check: Build and test sync-service, pg15
- GitHub Check: Build and test sync-service, pg14
- GitHub Check: Build and test sync-service, pg18
- GitHub Check: Run Lux integration tests
🔇 Additional comments (14)
packages/sync-service/lib/electric/replication/eval/known_definition.ex (5)
187-208: LGTM!The type pattern regex and its integration into the operator/function parsing regexes correctly support both simple types and array types. The pattern appropriately handles underscores and asterisks for type categories while making the
[]suffix optional for array types.
255-269: LGTM!The
parse_type_string/1andtype_to_string/1helpers correctly handle conversion between string representations and type tuples. The implementation properly handles single-dimension array types.Note: Multi-dimensional arrays (e.g.,
text[][]) are not supported by this implementation, which appears intentional for the current scope. If multi-dimensional support is needed later, this logic would need extension.
229-252: LGTM!The
parse_unary_operatorandparse_binary_operatorfunctions correctly useparse_type_string/1to handle both simple and array types in their argument and return type specifications.
271-290: LGTM!The
parse_functioncorrectly appliesparse_type_string/1to each argument type and the return type, enabling proper handling of array type arguments liketext[]in function signatures.
383-402: LGTM!The
validate_at_most_one_categoryfunction correctly usestype_to_string/1to convert type representations back to strings before performing category pattern matching. This is necessary since types can now be tuples like{:array, :text}instead of just atoms.packages/sync-service/lib/electric/replication/eval/env/known_functions.ex (4)
154-165: LGTM!The JSONB parsing and output functions are implemented correctly.
Jason.decode!will raise on invalid JSON (appropriate for strict parsing), andjsonb_outproperly handles the nil case.
167-188: LGTM!The
->operators correctly implement PostgreSQL JSONB semantics:
- Object field access returns the value or nil for missing keys
- Array index access uses 0-based indexing
- Negative indices correctly return nil (PostgreSQL behavior)
- Type mismatches (e.g., string key on array) correctly return nil
257-270: LGTM!The
?key existence operator correctly handles both objects (key existence) and arrays (string element membership).
272-284: LGTM!The
?|(any-key-exists) operator uses an efficient approach: creating aMapSetfrom keys and then iterating over the JSON array/object for O(1) lookups.packages/sync-service/test/electric/replication/eval/runner_test.exs (5)
187-236: LGTM!The
->operator tests provide excellent coverage including:
- Object field access (simple and nested)
- Missing key behavior (returns nil)
- Null JSONB handling
- Array index access with 0-based indexing
- Out-of-bounds and negative index edge cases
238-305: LGTM!Excellent coverage of the
->>operator including:
- Type coercion for strings, numbers, and booleans
- Nested objects correctly serialized as JSON strings (line 264)
- Chained operator combinations (
->then->>)- Mixed object/array access paths
307-347: LGTM!The comparison and filtering tests cover practical use cases that align well with the examples in the changeset documentation (e.g.,
(x ->> 'status') = 'active').
349-462: LGTM!Comprehensive containment operator tests covering:
- Object and array containment
- Nested structure containment
- Scalar equality semantics
- Practical filtering patterns with literal JSON casts
<@contained-by symmetry
464-533: LGTM!The key existence operator tests are well-structured with:
?operator testing both object keys and array string membership?|tests correctly using~S(...)delimiter to avoid sigil conflict with|?&tests correctly using~S|...|(no conflict since&doesn't include|)- Coverage for both objects and arrays
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| ``` | ||
| # Filter by nested field value | ||
| GET /v1/shapes?table=users&where=(metadata ->> 'status') = 'active' | ||
| # Filter by JSON containment | ||
| GET /v1/shapes?table=orders&where=data @> '{"type": "premium"}' | ||
| # Filter by key existence | ||
| GET /v1/shapes?table=events&where=payload ? 'user_id' |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding a language specifier for the code block.
The fenced code block could benefit from a language specifier for better syntax highlighting. Since these are HTTP requests, consider using http or bash.
-```
+```http
# Filter by nested field value
GET /v1/shapes?table=users&where=(metadata ->> 'status') = 'active'🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
12-12: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
In @.changeset/add-jsonb-operators.md around lines 12 - 20, Add an HTTP language
specifier to the fenced code block in .changeset/add-jsonb-operators.md so the
examples render with proper syntax highlighting; replace the opening fence "```"
with "```http" (or "```bash") for the block containing the three GET examples
that reference JSONB operators like (metadata ->> 'status'), @>, and ? to ensure
clients and reviewers see correct highlighting.
| # ?& checks if all of the keys exist | ||
| defpostgres "jsonb ?& text[] -> bool" do | ||
| def jsonb_all_keys_exist?(json, keys) when is_map(json) and is_list(keys) do | ||
| Enum.all?(keys, &Map.has_key?(json, &1)) | ||
| end | ||
|
|
||
| def jsonb_all_keys_exist?(json, keys) when is_list(json) and is_list(keys) do | ||
| key_set = MapSet.new(keys) | ||
| Enum.all?(key_set, &Enum.member?(json, &1)) | ||
| end | ||
|
|
||
| def jsonb_all_keys_exist?(_, _), do: false | ||
| end |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Performance inconsistency in ?& for arrays compared to ?|.
The array implementation at lines 292-294 iterates over key_set and performs Enum.member?(json, &1) which is O(n) for each key, resulting in O(keys × json_length) complexity.
For consistency with ?| (lines 278-280), which achieves O(json_length) by iterating over json and checking against a MapSet, consider refactoring:
♻️ Proposed performance improvement
def jsonb_all_keys_exist?(json, keys) when is_list(json) and is_list(keys) do
- key_set = MapSet.new(keys)
- Enum.all?(key_set, &Enum.member?(json, &1))
+ json_set = MapSet.new(json)
+ Enum.all?(keys, &MapSet.member?(json_set, &1))
end📝 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.
| # ?& checks if all of the keys exist | |
| defpostgres "jsonb ?& text[] -> bool" do | |
| def jsonb_all_keys_exist?(json, keys) when is_map(json) and is_list(keys) do | |
| Enum.all?(keys, &Map.has_key?(json, &1)) | |
| end | |
| def jsonb_all_keys_exist?(json, keys) when is_list(json) and is_list(keys) do | |
| key_set = MapSet.new(keys) | |
| Enum.all?(key_set, &Enum.member?(json, &1)) | |
| end | |
| def jsonb_all_keys_exist?(_, _), do: false | |
| end | |
| # ?& checks if all of the keys exist | |
| defpostgres "jsonb ?& text[] -> bool" do | |
| def jsonb_all_keys_exist?(json, keys) when is_map(json) and is_list(keys) do | |
| Enum.all?(keys, &Map.has_key?(json, &1)) | |
| end | |
| def jsonb_all_keys_exist?(json, keys) when is_list(json) and is_list(keys) do | |
| json_set = MapSet.new(json) | |
| Enum.all?(keys, &MapSet.member?(json_set, &1)) | |
| end | |
| def jsonb_all_keys_exist?(_, _), do: false | |
| end |
|
Might be worth leveraging the existing property testing tooling found/used in |
Fixes based on code review: 1. Negative index support for -> and ->> operators - Postgres supports negative indices counting from the end - [-1] returns last element, [-2] returns second-to-last, etc. - Elixir's Enum.at/2 already handles this correctly 2. Array-contains-primitive for @> operator - '["foo", "bar"]'::jsonb @> '"bar"'::jsonb returns true - This is a Postgres special-case for containment - Non-reciprocal: scalar @> array remains false 3. Refactored <@ to use @> implementation - x <@ y is semantically y @> x - Eliminates duplicate containment logic Added comprehensive tests for: - Negative indexing on both -> and ->> - Array-contains-primitive and non-reciprocal behavior - Structure matching in nested arrays - Structure mismatch extraction (returns nil)
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/sync-service/test/electric/replication/eval/runner_test.exs`:
- Around line 402-541: Add tests covering jsonb containment behavior with NULLs
by creating a new test (e.g., "should handle NULL in jsonb containment
operators") that uses Parser.parse_and_validate_expression!(refs: %{["x"] =>
:jsonb, ["y"] => :jsonb}) and Runner.execute to assert that when x is nil or y
is nil the result is {:ok, nil} for x @> y; place it alongside the existing
containment tests so Runner.execute and the parser paths are exercised the same
way as the other cases.
♻️ Duplicate comments (1)
packages/sync-service/lib/electric/replication/eval/env/known_functions.ex (1)
293-296: Performance inconsistency in array implementation compared to?|operator.The current implementation has O(keys × json_length) complexity because
Enum.member?/2on a list is O(n). The?|operator (lines 279-281) achieves O(json + keys) by converting to MapSet first.♻️ Proposed performance improvement
def jsonb_all_keys_exist?(json, keys) when is_list(json) and is_list(keys) do - key_set = MapSet.new(keys) - Enum.all?(key_set, &Enum.member?(json, &1)) + json_set = MapSet.new(json) + Enum.all?(keys, &MapSet.member?(json_set, &1)) end
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
packages/sync-service/lib/electric/replication/eval/env/known_functions.expackages/sync-service/test/electric/replication/eval/runner_test.exs
🧰 Additional context used
🧬 Code graph analysis (1)
packages/sync-service/lib/electric/replication/eval/env/known_functions.ex (5)
packages/elixir-client/lib/electric/client/fetch/response.ex (1)
decode!(28-34)examples/burn/lib/burn/serialize.ex (1)
to_string(2-4)packages/sync-service/lib/pg_interop/sublink.ex (1)
member?(6-8)packages/sync-service/lib/electric/shapes/shape/subset.ex (1)
new(23-45)packages/sync-service/lib/electric/replication/eval/env.ex (1)
new(101-105)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: Test packages/y-electric w/ sync-service
- GitHub Check: Test packages/typescript-client w/ sync-service
- GitHub Check: Test packages/react-hooks w/ sync-service
- GitHub Check: Test packages/start w/ sync-service
- GitHub Check: Test packages/experimental w/ sync-service
- GitHub Check: Build and test sync-service, pg17
- GitHub Check: Build and test sync-service, pg14
- GitHub Check: Build and test sync-service, pg18
- GitHub Check: Build and test sync-service, pg15
- GitHub Check: Run Lux integration tests
- GitHub Check: Check formatting of TS packages, examples and website
🔇 Additional comments (9)
packages/sync-service/lib/electric/replication/eval/env/known_functions.ex (6)
154-165: LGTM - JSONB parsing and output functions are correctly implemented.
Jason.decode!appropriately raises on invalid JSON (consistent with PostgreSQL behavior)jsonb_out(nil)returnsnil, correctly preserving SQL NULL semantics
167-184: LGTM - Field and array access operators correctly implemented.
Enum.at/2supports negative indices natively, matching PostgreSQL behavior- Type-mismatch fallbacks return
nilas expected
253-256: Verify cross-block function call fordo_jsonb_contains?/2.Similar to the
->>operator, this block callsdo_jsonb_contains?(right, left)which is defined in the@>operator'sdefpostgresblock (lines 226-250). Verify that this function is accessible from this block.
216-251: LGTM - Equality and containment operators are well implemented.
- Equality delegates to
Kernelfunctions appropriately- Containment logic correctly handles recursive structure matching
- Special case for array-contains-primitive (lines 242-247) matches PostgreSQL semantics
- Scalar fallback uses equality comparison
258-285: LGTM - Key existence operators correctly implemented.
?operator correctly distinguishes between map key lookup and array membership?|uses MapSet for efficient O(json + keys) any-key checking- Both return
false(notnil) for type mismatches, matching PostgreSQL behavior
207-214: No action needed—jsonb_value_to_text/1is correctly accessible acrossdefpostgresblocks.The
defpostgresmacro does not create isolated scopes. It directly inserts function definitions into the module at compile time. Sincejsonb_value_to_text/1is defined as a private function (defp) within the same module, it is accessible throughout the entire module, including from differentdefpostgresblocks. The presence of passing tests for thejsonb ->> int4 -> textoperator inrunner_test.exsconfirms this code compiles and executes correctly.Likely an incorrect or invalid review comment.
packages/sync-service/test/electric/replication/eval/runner_test.exs (3)
187-338: Excellent test coverage for JSONB access operators.Tests comprehensively cover:
- Object field access with nested structures
- Array index access with positive, negative, and out-of-bounds indices
- Text extraction for various scalar types and nested structures
- Type mismatch scenarios returning
nil
340-400: LGTM - Chained operators and comparison tests are thorough.Good coverage of practical patterns like
(x ->> 'status') = 'active'filtering.
543-612: LGTM - Key existence operator tests are well structured.Good use of
~S()delimiter for?|operator tests to avoid the sigil parsing conflict noted in past reviews. Coverage includes both object and array scenarios for all three operators.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
The <@ operator needs its own copy of the containment logic since defpostgres blocks are isolated. Also fixes the array-contains-primitive check to only apply at the top level, not recursively within array element comparisons. This ensures [1, 2, [1, 3]] @> [1, 3] returns false (structure must match) while ["foo", "bar"] @> "bar" returns true (top-level primitive check).
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@packages/sync-service/lib/electric/replication/eval/env/known_functions.ex`:
- Around line 207-214: The jsonb_get_text_by_index defpostgres block calls
jsonb_value_to_text/1 which is defined in a different defpostgres block and
therefore unavailable; inline the helper logic (or move jsonb_value_to_text/1 to
a module-level private function) into the same scope as jsonb_get_text_by_index
so the function exists at runtime, and adjust the index handling in
jsonb_get_text_by_index to return nil for negative indices (Postgres treats
negative indexes as NULL) instead of attempting Enum.at with negative values;
ensure you update the pattern-matching clause for jsonb_get_text_by_index and/or
add a guard to reject negative indices and call the inlined/shared
jsonb_value_to_text logic.
- Around line 176-184: The jsonb_get_by_index/2 implementation uses Enum.at/2
which treats negative indices as offsets from the end, but PostgreSQL semantics
require returning NULL for any negative index; update jsonb_get_by_index/2 (the
clause inside defpostgres "jsonb -> int4 -> jsonb") to explicitly return nil
when index is negative (e.g., guard or conditional index < 0), and only call
Enum.at(json, index) for non-negative indices while keeping the fallback clause
def jsonb_get_by_index(_, _), do: nil unchanged.
♻️ Duplicate comments (2)
packages/sync-service/lib/electric/replication/eval/env/known_functions.ex (2)
254-281: Containment logic duplication persists despite prior resolution.The
do_jsonb_contained_by?/2helper (lines 265-280) duplicates the exact logic fromdo_jsonb_contains?/2. A previous review suggested reusingdo_jsonb_contains?with swapped arguments sincea <@ b≡b @> a, and this was marked as addressed. However, the duplication remains in the current code.♻️ Proposed simplification
If
do_jsonb_contains?were extracted to module level:defpostgres "jsonb <@ jsonb -> bool" do def jsonb_contained_by?(left, right) when is_list(right) and not is_list(left) and not is_map(left) do Enum.member?(right, left) end - def jsonb_contained_by?(left, right), do: do_jsonb_contained_by?(right, left) - - defp do_jsonb_contained_by?(left, right) when is_map(left) and is_map(right) do - Enum.all?(right, fn {key, right_value} -> - case Map.fetch(left, key) do - {:ok, left_value} -> do_jsonb_contained_by?(left_value, right_value) - :error -> false - end - end) - end - - defp do_jsonb_contained_by?(left, right) when is_list(left) and is_list(right) do - Enum.all?(right, fn right_elem -> - Enum.any?(left, fn left_elem -> do_jsonb_contained_by?(left_elem, right_elem) end) - end) - end - - defp do_jsonb_contained_by?(left, right), do: left == right + def jsonb_contained_by?(left, right), do: do_jsonb_contains?(right, left) end
318-321: Performance inconsistency:?&for arrays uses O(n×m) vs O(n+m) in?|.The current implementation iterates over
key_setand callsEnum.member?(json, &1)which is O(n) per key, resulting in O(keys × json_length). The?|operator (lines 304-307) achieves O(json_length) by building aMapSetfromkeysand iterating overjson.For consistency and better performance on large arrays:
♻️ Proposed performance improvement
def jsonb_all_keys_exist?(json, keys) when is_list(json) and is_list(keys) do - key_set = MapSet.new(keys) - Enum.all?(key_set, &Enum.member?(json, &1)) + json_set = MapSet.new(json) + Enum.all?(keys, &MapSet.member?(json_set, &1)) end
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
packages/sync-service/lib/electric/replication/eval/env/known_functions.ex
🧰 Additional context used
🧬 Code graph analysis (1)
packages/sync-service/lib/electric/replication/eval/env/known_functions.ex (2)
examples/burn/lib/burn/serialize.ex (1)
to_string(2-4)packages/sync-service/lib/pg_interop/sublink.ex (1)
member?(6-8)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: Test packages/typescript-client w/ sync-service
- GitHub Check: Test packages/start w/ sync-service
- GitHub Check: Test packages/react-hooks w/ sync-service
- GitHub Check: Test packages/y-electric w/ sync-service
- GitHub Check: Test packages/experimental w/ sync-service
- GitHub Check: Build and test sync-service, pg15
- GitHub Check: Build and test sync-service, pg17
- GitHub Check: Build and test sync-service, pg18
- GitHub Check: Build and test sync-service, pg14
- GitHub Check: Run Lux integration tests
🔇 Additional comments (4)
packages/sync-service/lib/electric/replication/eval/env/known_functions.ex (4)
154-165: LGTM!The JSONB parse and output functions correctly handle NULL semantics and use the Jason library appropriately.
216-218: LGTM!JSONB equality operators appropriately delegate to Kernel functions, consistent with other type comparisons in this module.
220-252: LGTM!The
@>containment operator correctly implements PostgreSQL JSONB semantics, including the top-level array-contains-primitive special case and recursive containment for nested structures.
283-310: LGTM!The
?and?|key existence operators correctly implement PostgreSQL semantics for both objects and arrays, with efficient MapSet usage in?|.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| defpostgres "jsonb -> int4 -> jsonb" do | ||
| def jsonb_get_by_index(json, index) when is_list(json) and is_integer(index) do | ||
| # PostgreSQL uses 0-based indexing for JSON arrays | ||
| # Negative indices count from the end (e.g., -1 is last element) | ||
| Enum.at(json, index) | ||
| end | ||
|
|
||
| def jsonb_get_by_index(_, _), do: nil | ||
| end |
There was a problem hiding this comment.
Negative index handling does not match PostgreSQL semantics.
PostgreSQL's jsonb -> int4 operator returns NULL for negative indices, but Enum.at/2 in Elixir supports negative indexing (e.g., -1 returns the last element). The PR summary confirms the intended behavior is "negative indices return NULL."
🐛 Proposed fix
defpostgres "jsonb -> int4 -> jsonb" do
def jsonb_get_by_index(json, index) when is_list(json) and is_integer(index) do
- # PostgreSQL uses 0-based indexing for JSON arrays
- # Negative indices count from the end (e.g., -1 is last element)
- Enum.at(json, index)
+ # PostgreSQL uses 0-based indexing for JSON arrays
+ # Negative indices return NULL (unlike Elixir's Enum.at)
+ if index >= 0, do: Enum.at(json, index), else: nil
end
def jsonb_get_by_index(_, _), do: nil
end🤖 Prompt for AI Agents
In `@packages/sync-service/lib/electric/replication/eval/env/known_functions.ex`
around lines 176 - 184, The jsonb_get_by_index/2 implementation uses Enum.at/2
which treats negative indices as offsets from the end, but PostgreSQL semantics
require returning NULL for any negative index; update jsonb_get_by_index/2 (the
clause inside defpostgres "jsonb -> int4 -> jsonb") to explicitly return nil
when index is negative (e.g., guard or conditional index < 0), and only call
Enum.at(json, index) for non-negative indices while keeping the fallback clause
def jsonb_get_by_index(_, _), do: nil unchanged.
| defpostgres "jsonb ->> int4 -> text" do | ||
| def jsonb_get_text_by_index(json, index) when is_list(json) and is_integer(index) do | ||
| # Negative indices count from the end (e.g., -1 is last element) | ||
| json |> Enum.at(index) |> jsonb_value_to_text() | ||
| end | ||
|
|
||
| def jsonb_get_text_by_index(_, _), do: nil | ||
| end |
There was a problem hiding this comment.
Undefined function: jsonb_value_to_text/1 is not defined in this defpostgres block.
Line 210 calls jsonb_value_to_text(), but this helper is privately defined inside a separate defpostgres block (lines 197-204). Since each defpostgres block is isolated, this will cause an undefined function error at runtime.
Additionally, the same negative index issue exists here - the implementation should return NULL for negative indices per PostgreSQL semantics.
🐛 Proposed fix - inline the helper and fix negative index handling
defpostgres "jsonb ->> int4 -> text" do
def jsonb_get_text_by_index(json, index) when is_list(json) and is_integer(index) do
- # Negative indices count from the end (e.g., -1 is last element)
- json |> Enum.at(index) |> jsonb_value_to_text()
+ # Negative indices return NULL (PostgreSQL semantics)
+ if index >= 0 do
+ case Enum.at(json, index) do
+ nil -> nil
+ value -> jsonb_value_to_text(value)
+ end
+ else
+ nil
+ end
end
def jsonb_get_text_by_index(_, _), do: nil
+
+ defp jsonb_value_to_text(value) when is_binary(value), do: value
+ defp jsonb_value_to_text(value) when is_integer(value), do: Integer.to_string(value)
+ defp jsonb_value_to_text(value) when is_float(value), do: Float.to_string(value)
+ defp jsonb_value_to_text(true), do: "true"
+ defp jsonb_value_to_text(false), do: "false"
+ defp jsonb_value_to_text(nil), do: nil
+ defp jsonb_value_to_text(value), do: Jason.encode!(value)
endAlternatively, consider extracting jsonb_value_to_text/1 as a module-level private function outside the defpostgres blocks so both operators can share it, as suggested in previous reviews.
🤖 Prompt for AI Agents
In `@packages/sync-service/lib/electric/replication/eval/env/known_functions.ex`
around lines 207 - 214, The jsonb_get_text_by_index defpostgres block calls
jsonb_value_to_text/1 which is defined in a different defpostgres block and
therefore unavailable; inline the helper logic (or move jsonb_value_to_text/1 to
a module-level private function) into the same scope as jsonb_get_text_by_index
so the function exists at runtime, and adjust the index handling in
jsonb_get_text_by_index to return nil for negative indices (Postgres treats
negative indexes as NULL) instead of attempting Enum.at with negative values;
ensure you update the pattern-matching clause for jsonb_get_text_by_index and/or
add a guard to reject negative indices and call the inlined/shared
jsonb_value_to_text logic.
Add tests based on PostgreSQL documentation review: - Empty arrays/objects containment - Array order independence and duplicate handling - JSON null in containment and extraction - Top-level only behavior for ? operators - Case sensitivity for keys - Mixed primitive types in arrays - Boolean vs string distinction
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/sync-service/test/electric/replication/eval/runner_test.exs (1)
777-830: Consider extending property-based tests to include JSONB operators.The existing property-based testing framework validates expressions against a PostgreSQL oracle. As mentioned by reviewer msfstef, extending
PgExpressionGeneratorto generate JSONB expressions (->,->>,@>,<@,?,?|,?&) would provide additional fuzz-testing confidence for these new operators.This could be tracked as a follow-up task.
Would you like me to open an issue to track adding JSONB expression generation to the property-testing framework?
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
packages/sync-service/test/electric/replication/eval/runner_test.exs
🧰 Additional context used
🧬 Code graph analysis (1)
packages/sync-service/test/electric/replication/eval/runner_test.exs (1)
packages/sync-service/lib/electric/replication/eval/runner.ex (1)
execute(54-58)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: Test packages/experimental w/ sync-service
- GitHub Check: Test packages/react-hooks w/ sync-service
- GitHub Check: Test packages/typescript-client w/ sync-service
- GitHub Check: Test packages/y-electric w/ sync-service
- GitHub Check: Test packages/start w/ sync-service
- GitHub Check: Build and test sync-service, pg18
- GitHub Check: Build and test sync-service, pg17
- GitHub Check: Build and test sync-service, pg15
- GitHub Check: Build and test sync-service, pg14
- GitHub Check: Run Lux integration tests
🔇 Additional comments (8)
packages/sync-service/test/electric/replication/eval/runner_test.exs (8)
187-211: LGTM!Tests cover the essential cases for object field access: basic access, nested objects, missing keys returning nil, and null input propagation.
213-247: LGTM!Tests correctly cover 0-based array access including negative indices counting from the end (PostgreSQL behavior). The out-of-bounds cases (both positive and negative) correctly return nil.
249-313: LGTM!Thorough coverage of the
->>text extraction operator including type coercion (numbers and booleans to strings) and nested object serialization to JSON strings.
315-358: LGTM!Good coverage of structure mismatch scenarios (returning nil) and operator chaining for nested access patterns.
360-400: LGTM!Tests cover equality/inequality comparisons and the common filtering pattern of extracting a field as text and comparing against a string value.
402-541: LGTM!Excellent coverage of containment semantics including the PostgreSQL special case where an array can contain a primitive (line 469-473), non-reciprocal behavior, and nested structure matching requirements.
543-612: LGTM!Key existence operators are well-tested. The sigil delimiter handling is correct:
?|tests use~S(...)to avoid the pipe delimiter conflict, while?&tests safely use~S|...|.
614-746: LGTM!Excellent edge case coverage aligned with PostgreSQL documentation. The tests document important behaviors: empty structure handling, order independence, JSON null semantics, top-level-only key existence checks, case sensitivity, type distinctions, and mixed-type arrays.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
Implements KeyExistenceIndex for efficient filtering when shapes use the JSONB key existence operator (data ? 'key'). This allows O(1) lookups instead of evaluating every shape when records with JSONB fields are inserted/updated. Key changes: - Add key_exists_index_table to Filter struct - Add optimise_where pattern for ? operator in WhereCondition - Create KeyExistenceIndex module with add/remove/affected_shapes - Add dispatcher for ? in Index module - Add tests for correctness and performance Co-Authored-By: Claude Opus 4.5 <[email protected]>
Summary
Adds comprehensive JSONB operator support for where clauses, enabling filtering on JSON data stored in PostgreSQL.
User impact: Users can now write where clauses like
data -> 'user' ->> 'status' = 'active',data @> '{"type": "premium"}', ordata ? 'required_field'to filter shapes based on JSONB content.Example Shape Requests
Approach
Implemented three categories of PostgreSQL-compatible JSONB operators:
Field Access (
->,->>)->jsonb -> text -> jsonb->jsonb -> int4 -> jsonb->>jsonb ->> text -> text->>jsonb ->> int4 -> textContainment (
@>,<@)@><@Key Existence (
?,?|,?&)??&Also added
=and<>for direct JSONB comparison.Key implementation details:
Key invariants:
(x -> 'foo') ->> 'bar'DSL fixes included:
text[]) indefpostgressignaturesNon-goals:
#>,#>>)jsonb_path_query)Verification
Tests cover:
->,->>) for objects and arrays@>,<@) with nested structures?,?|,?&) for objects and arraysFiles Changed
lib/electric/replication/eval/env/known_functions.ex— JSONB operator implementations (~140 lines)lib/electric/replication/eval/known_definition.ex— DSL fixes for array types and pipe-containing sigilslib/electric/replication/eval/env/basic_types.ex— Addedjsonbto known types tabletest/electric/replication/eval/runner_test.exs— Comprehensive tests (~350 new lines).changeset/add-jsonb-operators.md— Changeset for releaseSummary by CodeRabbit
New Features
Tests
Documentation
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.