From 9cb9f5abdb828a2a473bf6873365c7ba910f2d2b Mon Sep 17 00:00:00 2001 From: rob Date: Wed, 27 Nov 2024 16:20:55 +0000 Subject: [PATCH 01/62] Add filter --- .../lib/electric/shapes/filter.ex | 117 +++++++++++++++++ .../sync-service/lib/electric/shapes/shape.ex | 4 +- .../test/electric/shapes/filter_test.exs | 120 ++++++++++++++++++ 3 files changed, 239 insertions(+), 2 deletions(-) create mode 100644 packages/sync-service/lib/electric/shapes/filter.ex create mode 100644 packages/sync-service/test/electric/shapes/filter_test.exs diff --git a/packages/sync-service/lib/electric/shapes/filter.ex b/packages/sync-service/lib/electric/shapes/filter.ex new file mode 100644 index 0000000000..3a4157cf7c --- /dev/null +++ b/packages/sync-service/lib/electric/shapes/filter.ex @@ -0,0 +1,117 @@ +defmodule Electric.Shapes.Filter do + alias Electric.Shapes.Shape + alias Electric.Replication.Changes.Transaction + alias Electric.Replication.Changes.NewRecord + + def new(shapes), do: new(shapes, empty()) + def new([shape | shapes], filter), do: new(shapes, add_shape(shape, filter)) + def new([], filter), do: filter + + defp empty, do: %{tables: %{}} + defp empty_table_filter, do: %{fields: %{}, other_shapes: []} + + defp add_shape(shape, %{tables: tables}) do + %{ + tables: + Map.update( + tables, + shape.shape.root_table, + add_shape_to_table_filter(shape, empty_table_filter()), + fn table_filter -> add_shape_to_table_filter(shape, table_filter) end + ) + } + end + + defp add_shape_to_table_filter(%{shape: %{where: where}} = shape, table_filter) do + case optimise_where(where) do + %{operation: "=", field: field, value: value, and_where: and_where} -> + %{ + table_filter + | fields: add_shape_to_field_filter(field, value, shape, table_filter.fields, and_where) + } + + :not_optimised -> + %{table_filter | other_shapes: [shape | table_filter.other_shapes]} + end + end + + defp add_shape_to_field_filter(field, value, shape, fields, and_where) do + Map.update( + fields, + field, + add_shape_to_value_filter(value, shape, and_where, %{}), + fn value_filter -> add_shape_to_value_filter(value, shape, and_where, value_filter) end + ) + end + + defp add_shape_to_value_filter(value, shape, and_where, value_filter) do + Map.update( + value_filter, + value, + [%{handle: shape.handle, and_where: and_where}], + fn shapes -> [%{handle: shape.handle, and_where: and_where} | shapes] end + ) + end + + alias Electric.Replication.Eval.Expr + alias Electric.Replication.Eval.Parser.Func + alias Electric.Replication.Eval.Parser.Ref + alias Electric.Replication.Eval.Parser.Const + + defp optimise_where(%Expr{ + eval: %Func{ + name: ~s("="), + args: [ + %Ref{path: [field]}, + %Const{} = const + ] + } + }) do + %{operation: "=", field: field, value: const_to_string(const), and_where: nil} + end + + defp optimise_where(_), do: :not_optimised + + defp const_to_string(%Const{value: value, type: :int4}), do: Integer.to_string(value) + defp const_to_string(%Const{value: value, type: :int8}), do: Integer.to_string(value) + + def affected_shapes(filter, %Transaction{changes: changes}) do + changes + |> Enum.map(&affected_shapes(filter, &1)) + |> Enum.reduce(MapSet.new(), &MapSet.union(&1, &2)) + end + + def affected_shapes(filter, %NewRecord{relation: relation, record: record}) do + case Map.get(filter.tables, relation) do + nil -> MapSet.new() + table_filter -> affected_shapes_by_table(table_filter, record) + end + end + + defp affected_shapes_by_table(%{fields: fields} = table_filter, record) do + fields + |> Enum.map(&affected_shapes_by_field(&1, record)) + |> Enum.reduce(MapSet.new(), &MapSet.union(&1, &2)) + |> MapSet.union(other_shapes_affected(table_filter, record)) + end + + def affected_shapes_by_field({field, values}, record) do + case values[record[field]] do + nil -> + MapSet.new() + + shapes -> + shapes + |> Enum.map(& &1.handle) + |> MapSet.new() + end + end + + defp other_shapes_affected(%{other_shapes: shapes}, record) do + for %{handle: handle, shape: shape} <- shapes, + Shape.record_in_shape?(shape.where, record), + into: MapSet.new() do + handle + end + end +end diff --git a/packages/sync-service/lib/electric/shapes/shape.ex b/packages/sync-service/lib/electric/shapes/shape.ex index a28faf7892..085c96917f 100644 --- a/packages/sync-service/lib/electric/shapes/shape.ex +++ b/packages/sync-service/lib/electric/shapes/shape.ex @@ -254,9 +254,9 @@ defmodule Electric.Shapes.Shape do defp filtered_columns_changed(_), do: true - defp record_in_shape?(nil, _record), do: true + def record_in_shape?(nil, _record), do: true - defp record_in_shape?(where, record) do + def record_in_shape?(where, record) do with {:ok, refs} <- Runner.record_to_ref_values(where.used_refs, record), {:ok, evaluated} <- Runner.execute(where, refs) do if is_nil(evaluated), do: false, else: evaluated diff --git a/packages/sync-service/test/electric/shapes/filter_test.exs b/packages/sync-service/test/electric/shapes/filter_test.exs new file mode 100644 index 0000000000..031025e2a1 --- /dev/null +++ b/packages/sync-service/test/electric/shapes/filter_test.exs @@ -0,0 +1,120 @@ +defmodule Electric.Shapes.FilterTest do + use ExUnit.Case + alias Electric.Replication.Changes.NewRecord + alias Electric.Replication.Changes.Transaction + alias Electric.Shapes.Filter + alias Electric.Shapes.Shape + alias Support.StubInspector + + @inspector StubInspector.new([%{name: "id", type: "int8", pk_position: 0}]) + + describe "new/1" do + test "with `field = constant` where clause" do + assert Filter.new([ + %{ + handle: "shape1", + shape: Shape.new!("the_table", where: "id = 1", inspector: @inspector) + } + ]) == %{ + tables: %{ + {"public", "the_table"} => %{ + fields: %{ + "id" => %{ + "1" => [ + %{handle: "shape1", and_where: nil} + ] + } + }, + other_shapes: [] + } + } + } + end + + test "with more complicated where clause" do + shape = %{ + handle: "shape1", + shape: Shape.new!("the_table", where: "id > 1", inspector: @inspector) + } + + assert Filter.new([shape]) == %{ + tables: %{ + {"public", "the_table"} => %{ + fields: %{}, + other_shapes: [shape] + } + } + } + end + end + + # TODO: relations + + describe "affected_shapes/2" do + test "shapes with same table and id are returned" do + filter = %{ + tables: %{ + {"public", "the_table"} => %{ + fields: %{ + "id" => %{ + "1" => [ + %{handle: "shape1", and_where: nil}, + %{handle: "shape2", and_where: nil} + ] + } + }, + other_shapes: [] + } + } + } + + transaction = + %Transaction{ + changes: [ + %NewRecord{ + relation: {"public", "the_table"}, + record: %{"id" => "1"} + } + ] + } + + assert Filter.affected_shapes(filter, transaction) == MapSet.new(["shape1", "shape2"]) + end + + test "shapes with more complicated where clauses are evaluated" do + filter = %{ + tables: %{ + {"public", "the_table"} => %{ + fields: %{}, + other_shapes: [ + %{ + handle: "shape1", + shape: Shape.new!("the_table", where: "id > 7", inspector: @inspector) + }, + %{ + handle: "shape2", + shape: Shape.new!("the_table", where: "id > 6", inspector: @inspector) + }, + %{ + handle: "shape3", + shape: Shape.new!("the_table", where: "id > 5", inspector: @inspector) + } + ] + } + } + } + + transaction = + %Transaction{ + changes: [ + %NewRecord{ + relation: {"public", "the_table"}, + record: %{"id" => "7"} + } + ] + } + + assert Filter.affected_shapes(filter, transaction) == MapSet.new(["shape2", "shape3"]) + end + end +end From d1d1adf665bd7589fd18e8033077a6b9737b64e3 Mon Sep 17 00:00:00 2001 From: rob Date: Wed, 27 Nov 2024 16:24:51 +0000 Subject: [PATCH 02/62] Make Shapes.record_in_shape? take a shape --- .../sync-service/lib/electric/shapes/filter.ex | 2 +- packages/sync-service/lib/electric/shapes/shape.ex | 14 +++++++------- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/packages/sync-service/lib/electric/shapes/filter.ex b/packages/sync-service/lib/electric/shapes/filter.ex index 3a4157cf7c..3895304eae 100644 --- a/packages/sync-service/lib/electric/shapes/filter.ex +++ b/packages/sync-service/lib/electric/shapes/filter.ex @@ -109,7 +109,7 @@ defmodule Electric.Shapes.Filter do defp other_shapes_affected(%{other_shapes: shapes}, record) do for %{handle: handle, shape: shape} <- shapes, - Shape.record_in_shape?(shape.where, record), + Shape.record_in_shape?(shape, record), into: MapSet.new() do handle end diff --git a/packages/sync-service/lib/electric/shapes/shape.ex b/packages/sync-service/lib/electric/shapes/shape.ex index 085c96917f..be2452cc7d 100644 --- a/packages/sync-service/lib/electric/shapes/shape.ex +++ b/packages/sync-service/lib/electric/shapes/shape.ex @@ -213,22 +213,22 @@ defmodule Electric.Shapes.Shape do def convert_change(%__MODULE__{}, %Changes.TruncatedRelation{} = change), do: [change] - def convert_change(%__MODULE__{where: where, selected_columns: selected_columns}, change) + def convert_change(%__MODULE__{selected_columns: selected_columns} = shape, change) when is_struct(change, Changes.NewRecord) when is_struct(change, Changes.DeletedRecord) do record = if is_struct(change, Changes.NewRecord), do: change.record, else: change.old_record - if record_in_shape?(where, record), + if record_in_shape?(shape, record), do: [filter_change_columns(selected_columns, change)], else: [] end def convert_change( - %__MODULE__{where: where, selected_columns: selected_columns}, + %__MODULE__{selected_columns: selected_columns} = shape, %Changes.UpdatedRecord{old_record: old_record, record: record} = change ) do - old_record_in_shape = record_in_shape?(where, old_record) - new_record_in_shape = record_in_shape?(where, record) + old_record_in_shape = record_in_shape?(shape, old_record) + new_record_in_shape = record_in_shape?(shape, record) converted_changes = case {old_record_in_shape, new_record_in_shape} do @@ -254,9 +254,9 @@ defmodule Electric.Shapes.Shape do defp filtered_columns_changed(_), do: true - def record_in_shape?(nil, _record), do: true + def record_in_shape?(%{where: nil}, _record), do: true - def record_in_shape?(where, record) do + def record_in_shape?(%{where: where}, record) do with {:ok, refs} <- Runner.record_to_ref_values(where.used_refs, record), {:ok, evaluated} <- Runner.execute(where, refs) do if is_nil(evaluated), do: false, else: evaluated From f299dc0edc3ed0dbe7e47f8291061a19537fd808 Mon Sep 17 00:00:00 2001 From: rob Date: Wed, 27 Nov 2024 16:27:10 +0000 Subject: [PATCH 03/62] Make add_shape public --- .../sync-service/lib/electric/shapes/filter.ex | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/packages/sync-service/lib/electric/shapes/filter.ex b/packages/sync-service/lib/electric/shapes/filter.ex index 3895304eae..8c6e3e6bea 100644 --- a/packages/sync-service/lib/electric/shapes/filter.ex +++ b/packages/sync-service/lib/electric/shapes/filter.ex @@ -1,16 +1,20 @@ defmodule Electric.Shapes.Filter do - alias Electric.Shapes.Shape - alias Electric.Replication.Changes.Transaction alias Electric.Replication.Changes.NewRecord + alias Electric.Replication.Changes.Transaction + alias Electric.Replication.Eval.Expr + alias Electric.Replication.Eval.Parser.Const + alias Electric.Replication.Eval.Parser.Func + alias Electric.Replication.Eval.Parser.Ref + alias Electric.Shapes.Shape def new(shapes), do: new(shapes, empty()) - def new([shape | shapes], filter), do: new(shapes, add_shape(shape, filter)) + def new([shape | shapes], filter), do: new(shapes, add_shape(filter, shape)) def new([], filter), do: filter defp empty, do: %{tables: %{}} defp empty_table_filter, do: %{fields: %{}, other_shapes: []} - defp add_shape(shape, %{tables: tables}) do + def add_shape(%{tables: tables}, shape) do %{ tables: Map.update( @@ -53,11 +57,6 @@ defmodule Electric.Shapes.Filter do ) end - alias Electric.Replication.Eval.Expr - alias Electric.Replication.Eval.Parser.Func - alias Electric.Replication.Eval.Parser.Ref - alias Electric.Replication.Eval.Parser.Const - defp optimise_where(%Expr{ eval: %Func{ name: ~s("="), From 9327bbdf65a450547827e1aabfca0c24aeb4354d Mon Sep 17 00:00:00 2001 From: rob Date: Wed, 27 Nov 2024 16:48:32 +0000 Subject: [PATCH 04/62] Support other types of change --- .../lib/electric/shapes/filter.ex | 21 +++++- .../test/electric/shapes/filter_test.exs | 71 +++++++++++++++++++ 2 files changed, 91 insertions(+), 1 deletion(-) diff --git a/packages/sync-service/lib/electric/shapes/filter.ex b/packages/sync-service/lib/electric/shapes/filter.ex index 8c6e3e6bea..8181e43e0a 100644 --- a/packages/sync-service/lib/electric/shapes/filter.ex +++ b/packages/sync-service/lib/electric/shapes/filter.ex @@ -1,5 +1,7 @@ defmodule Electric.Shapes.Filter do + alias Electric.Replication.Changes.DeletedRecord alias Electric.Replication.Changes.NewRecord + alias Electric.Replication.Changes.UpdatedRecord alias Electric.Replication.Changes.Transaction alias Electric.Replication.Eval.Expr alias Electric.Replication.Eval.Parser.Const @@ -80,8 +82,25 @@ defmodule Electric.Shapes.Filter do |> Enum.reduce(MapSet.new(), &MapSet.union(&1, &2)) end + # TODO: Optimisation: each time a shape is affected, take it out of `other_shapes` + def affected_shapes(filter, %NewRecord{relation: relation, record: record}) do - case Map.get(filter.tables, relation) do + affected_shapes_by_record(filter, relation, record) + end + + def affected_shapes(filter, %DeletedRecord{relation: relation, old_record: record}) do + affected_shapes_by_record(filter, relation, record) + end + + def affected_shapes(filter, %UpdatedRecord{relation: relation} = change) do + MapSet.union( + affected_shapes_by_record(filter, relation, change.record), + affected_shapes_by_record(filter, relation, change.old_record) + ) + end + + def affected_shapes_by_record(filter, table, record) do + case Map.get(filter.tables, table) do nil -> MapSet.new() table_filter -> affected_shapes_by_table(table_filter, record) end diff --git a/packages/sync-service/test/electric/shapes/filter_test.exs b/packages/sync-service/test/electric/shapes/filter_test.exs index 031025e2a1..6f5ac3cb5a 100644 --- a/packages/sync-service/test/electric/shapes/filter_test.exs +++ b/packages/sync-service/test/electric/shapes/filter_test.exs @@ -1,7 +1,9 @@ defmodule Electric.Shapes.FilterTest do use ExUnit.Case + alias Electric.Replication.Changes.DeletedRecord alias Electric.Replication.Changes.NewRecord alias Electric.Replication.Changes.Transaction + alias Electric.Replication.Changes.UpdatedRecord alias Electric.Shapes.Filter alias Electric.Shapes.Shape alias Support.StubInspector @@ -60,6 +62,10 @@ defmodule Electric.Shapes.FilterTest do "1" => [ %{handle: "shape1", and_where: nil}, %{handle: "shape2", and_where: nil} + ], + "2" => [ + %{handle: "shape3", and_where: nil}, + %{handle: "shape4", and_where: nil} ] } }, @@ -116,5 +122,70 @@ defmodule Electric.Shapes.FilterTest do assert Filter.affected_shapes(filter, transaction) == MapSet.new(["shape2", "shape3"]) end + + test "returns shapes affected by delete" do + filter = %{ + tables: %{ + {"public", "the_table"} => %{ + fields: %{ + "id" => %{ + "1" => [ + %{handle: "the-shape", and_where: nil} + ] + } + }, + other_shapes: [] + } + } + } + + transaction = + %Transaction{ + changes: [ + %DeletedRecord{ + relation: {"public", "the_table"}, + old_record: %{"id" => "1"} + } + ] + } + + assert Filter.affected_shapes(filter, transaction) == MapSet.new(["the-shape"]) + end + + test "returns shapes affected by update" do + filter = %{ + tables: %{ + {"public", "the_table"} => %{ + fields: %{ + "id" => %{ + "1" => [ + %{handle: "shape1", and_where: nil} + ], + "2" => [ + %{handle: "shape2", and_where: nil} + ], + "3" => [ + %{handle: "shape3", and_where: nil} + ] + } + }, + other_shapes: [] + } + } + } + + transaction = + %Transaction{ + changes: [ + %UpdatedRecord{ + relation: {"public", "the_table"}, + record: %{"id" => "1"}, + old_record: %{"id" => "2"} + } + ] + } + + assert Filter.affected_shapes(filter, transaction) == MapSet.new(["shape1", "shape2"]) + end end end From 5ca72675052d2e284f0023783a0e3009d3cc07f8 Mon Sep 17 00:00:00 2001 From: rob Date: Wed, 27 Nov 2024 16:52:40 +0000 Subject: [PATCH 05/62] Test id not found --- .../test/electric/shapes/filter_test.exs | 40 +++++++++++++++++++ 1 file changed, 40 insertions(+) diff --git a/packages/sync-service/test/electric/shapes/filter_test.exs b/packages/sync-service/test/electric/shapes/filter_test.exs index 6f5ac3cb5a..9cf381e94a 100644 --- a/packages/sync-service/test/electric/shapes/filter_test.exs +++ b/packages/sync-service/test/electric/shapes/filter_test.exs @@ -70,6 +70,16 @@ defmodule Electric.Shapes.FilterTest do } }, other_shapes: [] + }, + {"public", "another_table"} => %{ + fields: %{ + "id" => %{ + "1" => [ + %{handle: "shape5", and_where: nil} + ] + } + }, + other_shapes: [] } } } @@ -87,6 +97,36 @@ defmodule Electric.Shapes.FilterTest do assert Filter.affected_shapes(filter, transaction) == MapSet.new(["shape1", "shape2"]) end + test "shapes with same table but different id are not returned" do + filter = %{ + tables: %{ + {"public", "the_table"} => %{ + fields: %{ + "id" => %{ + "1" => [ + %{handle: "shape1", and_where: nil}, + %{handle: "shape2", and_where: nil} + ] + } + }, + other_shapes: [] + } + } + } + + transaction = + %Transaction{ + changes: [ + %NewRecord{ + relation: {"public", "the_table"}, + record: %{"id" => "2"} + } + ] + } + + assert Filter.affected_shapes(filter, transaction) == MapSet.new([]) + end + test "shapes with more complicated where clauses are evaluated" do filter = %{ tables: %{ From 285105d64dcf2eb56fd1417b3a6e4d9b160de9dd Mon Sep 17 00:00:00 2001 From: rob Date: Wed, 27 Nov 2024 16:59:42 +0000 Subject: [PATCH 06/62] Add Filter struct --- .../lib/electric/shapes/filter.ex | 28 ++++++++++--------- .../test/electric/shapes/filter_test.exs | 14 +++++----- 2 files changed, 22 insertions(+), 20 deletions(-) diff --git a/packages/sync-service/lib/electric/shapes/filter.ex b/packages/sync-service/lib/electric/shapes/filter.ex index 8181e43e0a..c63dd8b8b5 100644 --- a/packages/sync-service/lib/electric/shapes/filter.ex +++ b/packages/sync-service/lib/electric/shapes/filter.ex @@ -1,23 +1,23 @@ defmodule Electric.Shapes.Filter do alias Electric.Replication.Changes.DeletedRecord alias Electric.Replication.Changes.NewRecord - alias Electric.Replication.Changes.UpdatedRecord alias Electric.Replication.Changes.Transaction + alias Electric.Replication.Changes.UpdatedRecord alias Electric.Replication.Eval.Expr alias Electric.Replication.Eval.Parser.Const alias Electric.Replication.Eval.Parser.Func alias Electric.Replication.Eval.Parser.Ref + alias Electric.Shapes.Filter alias Electric.Shapes.Shape - def new(shapes), do: new(shapes, empty()) - def new([shape | shapes], filter), do: new(shapes, add_shape(filter, shape)) - def new([], filter), do: filter + defstruct tables: %{} - defp empty, do: %{tables: %{}} - defp empty_table_filter, do: %{fields: %{}, other_shapes: []} + def new(shapes), do: new(shapes, %Filter{}) + defp new([shape | shapes], filter), do: new(shapes, add_shape(filter, shape)) + defp new([], filter), do: filter - def add_shape(%{tables: tables}, shape) do - %{ + def add_shape(%Filter{tables: tables}, shape) do + %Filter{ tables: Map.update( tables, @@ -28,6 +28,8 @@ defmodule Electric.Shapes.Filter do } end + defp empty_table_filter, do: %{fields: %{}, other_shapes: []} + defp add_shape_to_table_filter(%{shape: %{where: where}} = shape, table_filter) do case optimise_where(where) do %{operation: "=", field: field, value: value, and_where: and_where} -> @@ -76,7 +78,7 @@ defmodule Electric.Shapes.Filter do defp const_to_string(%Const{value: value, type: :int4}), do: Integer.to_string(value) defp const_to_string(%Const{value: value, type: :int8}), do: Integer.to_string(value) - def affected_shapes(filter, %Transaction{changes: changes}) do + def affected_shapes(%Filter{} = filter, %Transaction{changes: changes}) do changes |> Enum.map(&affected_shapes(filter, &1)) |> Enum.reduce(MapSet.new(), &MapSet.union(&1, &2)) @@ -84,22 +86,22 @@ defmodule Electric.Shapes.Filter do # TODO: Optimisation: each time a shape is affected, take it out of `other_shapes` - def affected_shapes(filter, %NewRecord{relation: relation, record: record}) do + def affected_shapes(%Filter{} = filter, %NewRecord{relation: relation, record: record}) do affected_shapes_by_record(filter, relation, record) end - def affected_shapes(filter, %DeletedRecord{relation: relation, old_record: record}) do + def affected_shapes(%Filter{} = filter, %DeletedRecord{relation: relation, old_record: record}) do affected_shapes_by_record(filter, relation, record) end - def affected_shapes(filter, %UpdatedRecord{relation: relation} = change) do + def affected_shapes(%Filter{} = filter, %UpdatedRecord{relation: relation} = change) do MapSet.union( affected_shapes_by_record(filter, relation, change.record), affected_shapes_by_record(filter, relation, change.old_record) ) end - def affected_shapes_by_record(filter, table, record) do + defp affected_shapes_by_record(filter, table, record) do case Map.get(filter.tables, table) do nil -> MapSet.new() table_filter -> affected_shapes_by_table(table_filter, record) diff --git a/packages/sync-service/test/electric/shapes/filter_test.exs b/packages/sync-service/test/electric/shapes/filter_test.exs index 9cf381e94a..fdd1445bbd 100644 --- a/packages/sync-service/test/electric/shapes/filter_test.exs +++ b/packages/sync-service/test/electric/shapes/filter_test.exs @@ -17,7 +17,7 @@ defmodule Electric.Shapes.FilterTest do handle: "shape1", shape: Shape.new!("the_table", where: "id = 1", inspector: @inspector) } - ]) == %{ + ]) == %Filter{ tables: %{ {"public", "the_table"} => %{ fields: %{ @@ -39,7 +39,7 @@ defmodule Electric.Shapes.FilterTest do shape: Shape.new!("the_table", where: "id > 1", inspector: @inspector) } - assert Filter.new([shape]) == %{ + assert Filter.new([shape]) == %Filter{ tables: %{ {"public", "the_table"} => %{ fields: %{}, @@ -54,7 +54,7 @@ defmodule Electric.Shapes.FilterTest do describe "affected_shapes/2" do test "shapes with same table and id are returned" do - filter = %{ + filter = %Filter{ tables: %{ {"public", "the_table"} => %{ fields: %{ @@ -98,7 +98,7 @@ defmodule Electric.Shapes.FilterTest do end test "shapes with same table but different id are not returned" do - filter = %{ + filter = %Filter{ tables: %{ {"public", "the_table"} => %{ fields: %{ @@ -128,7 +128,7 @@ defmodule Electric.Shapes.FilterTest do end test "shapes with more complicated where clauses are evaluated" do - filter = %{ + filter = %Filter{ tables: %{ {"public", "the_table"} => %{ fields: %{}, @@ -164,7 +164,7 @@ defmodule Electric.Shapes.FilterTest do end test "returns shapes affected by delete" do - filter = %{ + filter = %Filter{ tables: %{ {"public", "the_table"} => %{ fields: %{ @@ -193,7 +193,7 @@ defmodule Electric.Shapes.FilterTest do end test "returns shapes affected by update" do - filter = %{ + filter = %Filter{ tables: %{ {"public", "the_table"} => %{ fields: %{ From f4a5e390369921bded35fcd574969039718bf937 Mon Sep 17 00:00:00 2001 From: rob Date: Wed, 27 Nov 2024 18:07:10 +0000 Subject: [PATCH 07/62] Turn other_shapes into a map --- .../lib/electric/shapes/filter.ex | 70 +++++++++---- .../test/electric/shapes/filter_test.exs | 99 +++++++++++++------ 2 files changed, 120 insertions(+), 49 deletions(-) diff --git a/packages/sync-service/lib/electric/shapes/filter.ex b/packages/sync-service/lib/electric/shapes/filter.ex index c63dd8b8b5..0af0900ba0 100644 --- a/packages/sync-service/lib/electric/shapes/filter.ex +++ b/packages/sync-service/lib/electric/shapes/filter.ex @@ -2,6 +2,7 @@ defmodule Electric.Shapes.Filter do alias Electric.Replication.Changes.DeletedRecord alias Electric.Replication.Changes.NewRecord alias Electric.Replication.Changes.Transaction + alias Electric.Replication.Changes.Relation alias Electric.Replication.Changes.UpdatedRecord alias Electric.Replication.Eval.Expr alias Electric.Replication.Eval.Parser.Const @@ -12,52 +13,61 @@ defmodule Electric.Shapes.Filter do defstruct tables: %{} - def new(shapes), do: new(shapes, %Filter{}) + def new(shapes), do: shapes |> Map.to_list() |> new(%Filter{}) defp new([shape | shapes], filter), do: new(shapes, add_shape(filter, shape)) defp new([], filter), do: filter - def add_shape(%Filter{tables: tables}, shape) do + def add_shape(%Filter{tables: tables}, {handle, shape}) do %Filter{ tables: Map.update( tables, - shape.shape.root_table, - add_shape_to_table_filter(shape, empty_table_filter()), - fn table_filter -> add_shape_to_table_filter(shape, table_filter) end + shape.root_table, + add_shape_to_table_filter({handle, shape}, empty_table_filter()), + fn table_filter -> add_shape_to_table_filter({handle, shape}, table_filter) end ) } end - defp empty_table_filter, do: %{fields: %{}, other_shapes: []} + defp empty_table_filter, do: %{fields: %{}, other_shapes: %{}} - defp add_shape_to_table_filter(%{shape: %{where: where}} = shape, table_filter) do - case optimise_where(where) do + defp add_shape_to_table_filter({handle, shape} = shape_instance, table_filter) do + case optimise_where(shape.where) do %{operation: "=", field: field, value: value, and_where: and_where} -> %{ table_filter - | fields: add_shape_to_field_filter(field, value, shape, table_filter.fields, and_where) + | fields: + add_shape_to_field_filter( + field, + value, + shape_instance, + table_filter.fields, + and_where + ) } :not_optimised -> - %{table_filter | other_shapes: [shape | table_filter.other_shapes]} + %{table_filter | other_shapes: Map.put(table_filter.other_shapes, handle, shape)} end end - defp add_shape_to_field_filter(field, value, shape, fields, and_where) do + defp add_shape_to_field_filter(field, value, shape_instance, fields, and_where) do Map.update( fields, field, - add_shape_to_value_filter(value, shape, and_where, %{}), - fn value_filter -> add_shape_to_value_filter(value, shape, and_where, value_filter) end + add_shape_to_value_filter(value, shape_instance, and_where, %{}), + fn value_filter -> + add_shape_to_value_filter(value, shape_instance, and_where, value_filter) + end ) end - defp add_shape_to_value_filter(value, shape, and_where, value_filter) do + defp add_shape_to_value_filter(value, {handle, _}, and_where, value_filter) do Map.update( value_filter, value, - [%{handle: shape.handle, and_where: and_where}], - fn shapes -> [%{handle: shape.handle, and_where: and_where} | shapes] end + [%{handle: handle, and_where: and_where}], + fn shapes -> [%{handle: handle, and_where: and_where} | shapes] end ) end @@ -78,6 +88,16 @@ defmodule Electric.Shapes.Filter do defp const_to_string(%Const{value: value, type: :int4}), do: Integer.to_string(value) defp const_to_string(%Const{value: value, type: :int8}), do: Integer.to_string(value) + # TODO: TruncateRelation and any others? + + def affected_shapes(%Filter{} = filter, %Relation{} = relation) do + for {handle, shape} <- all_shapes_for_table(filter, {relation.schema, relation.table}), + Shape.is_affected_by_relation_change?(shape, relation), + into: MapSet.new() do + handle + end + end + def affected_shapes(%Filter{} = filter, %Transaction{changes: changes}) do changes |> Enum.map(&affected_shapes(filter, &1)) @@ -128,10 +148,26 @@ defmodule Electric.Shapes.Filter do end defp other_shapes_affected(%{other_shapes: shapes}, record) do - for %{handle: handle, shape: shape} <- shapes, + for {handle, shape} <- shapes, Shape.record_in_shape?(shape, record), into: MapSet.new() do handle end end + + defp all_shapes_for_table(%Filter{} = filter, table) do + case Map.get(filter.tables, table) do + nil -> + %{} + + %{fields: fields, other_shapes: other_shapes} -> + for {_field, values} <- fields, + {_value, shapes} <- values, + %{handle: handle, shape: shape} <- shapes, + into: %{} do + {handle, shape} + end + |> Map.merge(other_shapes) + end + end end diff --git a/packages/sync-service/test/electric/shapes/filter_test.exs b/packages/sync-service/test/electric/shapes/filter_test.exs index fdd1445bbd..04d33cbea1 100644 --- a/packages/sync-service/test/electric/shapes/filter_test.exs +++ b/packages/sync-service/test/electric/shapes/filter_test.exs @@ -3,6 +3,7 @@ defmodule Electric.Shapes.FilterTest do alias Electric.Replication.Changes.DeletedRecord alias Electric.Replication.Changes.NewRecord alias Electric.Replication.Changes.Transaction + alias Electric.Replication.Changes.Relation alias Electric.Replication.Changes.UpdatedRecord alias Electric.Shapes.Filter alias Electric.Shapes.Shape @@ -12,12 +13,9 @@ defmodule Electric.Shapes.FilterTest do describe "new/1" do test "with `field = constant` where clause" do - assert Filter.new([ - %{ - handle: "shape1", - shape: Shape.new!("the_table", where: "id = 1", inspector: @inspector) - } - ]) == %Filter{ + assert Filter.new(%{ + "shape1" => Shape.new!("the_table", where: "id = 1", inspector: @inspector) + }) == %Filter{ tables: %{ {"public", "the_table"} => %{ fields: %{ @@ -27,23 +25,20 @@ defmodule Electric.Shapes.FilterTest do ] } }, - other_shapes: [] + other_shapes: %{} } } } end test "with more complicated where clause" do - shape = %{ - handle: "shape1", - shape: Shape.new!("the_table", where: "id > 1", inspector: @inspector) - } + shapes = %{"shape1" => Shape.new!("the_table", where: "id > 1", inspector: @inspector)} - assert Filter.new([shape]) == %Filter{ + assert Filter.new(shapes) == %Filter{ tables: %{ {"public", "the_table"} => %{ fields: %{}, - other_shapes: [shape] + other_shapes: shapes } } } @@ -69,7 +64,7 @@ defmodule Electric.Shapes.FilterTest do ] } }, - other_shapes: [] + other_shapes: %{} }, {"public", "another_table"} => %{ fields: %{ @@ -79,7 +74,7 @@ defmodule Electric.Shapes.FilterTest do ] } }, - other_shapes: [] + other_shapes: %{} } } } @@ -109,7 +104,7 @@ defmodule Electric.Shapes.FilterTest do ] } }, - other_shapes: [] + other_shapes: %{} } } } @@ -132,20 +127,11 @@ defmodule Electric.Shapes.FilterTest do tables: %{ {"public", "the_table"} => %{ fields: %{}, - other_shapes: [ - %{ - handle: "shape1", - shape: Shape.new!("the_table", where: "id > 7", inspector: @inspector) - }, - %{ - handle: "shape2", - shape: Shape.new!("the_table", where: "id > 6", inspector: @inspector) - }, - %{ - handle: "shape3", - shape: Shape.new!("the_table", where: "id > 5", inspector: @inspector) - } - ] + other_shapes: %{ + "shape1" => Shape.new!("the_table", where: "id > 7", inspector: @inspector), + "shape2" => Shape.new!("the_table", where: "id > 6", inspector: @inspector), + "shape3" => Shape.new!("the_table", where: "id > 5", inspector: @inspector) + } } } } @@ -174,7 +160,7 @@ defmodule Electric.Shapes.FilterTest do ] } }, - other_shapes: [] + other_shapes: %{} } } } @@ -209,7 +195,7 @@ defmodule Electric.Shapes.FilterTest do ] } }, - other_shapes: [] + other_shapes: %{} } } } @@ -227,5 +213,54 @@ defmodule Electric.Shapes.FilterTest do assert Filter.affected_shapes(filter, transaction) == MapSet.new(["shape1", "shape2"]) end + + @tag :skip + test "returns shapes affected by relation change" do + filter = %Filter{ + tables: %{ + {"public", "the_table"} => %{ + fields: %{ + "id" => %{ + "1" => [ + %{handle: "shape1", and_where: nil} + ], + "2" => [ + %{handle: "shape2", and_where: nil} + ] + } + }, + other_shapes: %{ + "shape3" => Shape.new!("the_table", where: "id > 7", inspector: @inspector), + "shape4" => Shape.new!("the_table", where: "id > 6", inspector: @inspector) + } + }, + {"public", "another_table"} => %{ + fields: %{ + "id" => %{ + "1" => [ + %{handle: "not-this-shape-1", and_where: nil} + ] + } + }, + other_shapes: %{ + "not-this-shape-1" => + Shape.new!("another_table", where: "id > 7", inspector: @inspector), + "not-this-shape-2" => + Shape.new!("another_table", where: "id > 6", inspector: @inspector) + } + } + } + } + + relation = + %Relation{ + id: "TODO", + schema: "public", + table: "the_table" + } + + assert Filter.affected_shapes(filter, relation) == + MapSet.new(["shape1", "shape2", "shape3", "shape4"]) + end end end From 5012b0c1b3972d1eb723a266f1d9753b88ebe904 Mon Sep 17 00:00:00 2001 From: rob Date: Wed, 27 Nov 2024 18:12:14 +0000 Subject: [PATCH 08/62] Support relation changes --- .../lib/electric/shapes/filter.ex | 4 ++-- .../test/electric/shapes/filter_test.exs | 22 ++++++++++++------- 2 files changed, 16 insertions(+), 10 deletions(-) diff --git a/packages/sync-service/lib/electric/shapes/filter.ex b/packages/sync-service/lib/electric/shapes/filter.ex index 0af0900ba0..122bfb23d1 100644 --- a/packages/sync-service/lib/electric/shapes/filter.ex +++ b/packages/sync-service/lib/electric/shapes/filter.ex @@ -62,11 +62,11 @@ defmodule Electric.Shapes.Filter do ) end - defp add_shape_to_value_filter(value, {handle, _}, and_where, value_filter) do + defp add_shape_to_value_filter(value, {handle, shape}, and_where, value_filter) do Map.update( value_filter, value, - [%{handle: handle, and_where: and_where}], + [%{handle: handle, and_where: and_where, shape: shape}], fn shapes -> [%{handle: handle, and_where: and_where} | shapes] end ) end diff --git a/packages/sync-service/test/electric/shapes/filter_test.exs b/packages/sync-service/test/electric/shapes/filter_test.exs index 04d33cbea1..1052a24e4a 100644 --- a/packages/sync-service/test/electric/shapes/filter_test.exs +++ b/packages/sync-service/test/electric/shapes/filter_test.exs @@ -13,15 +13,15 @@ defmodule Electric.Shapes.FilterTest do describe "new/1" do test "with `field = constant` where clause" do - assert Filter.new(%{ - "shape1" => Shape.new!("the_table", where: "id = 1", inspector: @inspector) - }) == %Filter{ + shape = Shape.new!("the_table", where: "id = 1", inspector: @inspector) + + assert Filter.new(%{"shape1" => shape}) == %Filter{ tables: %{ {"public", "the_table"} => %{ fields: %{ "id" => %{ "1" => [ - %{handle: "shape1", and_where: nil} + %{handle: "shape1", and_where: nil, shape: shape} ] } }, @@ -214,7 +214,6 @@ defmodule Electric.Shapes.FilterTest do assert Filter.affected_shapes(filter, transaction) == MapSet.new(["shape1", "shape2"]) end - @tag :skip test "returns shapes affected by relation change" do filter = %Filter{ tables: %{ @@ -222,10 +221,18 @@ defmodule Electric.Shapes.FilterTest do fields: %{ "id" => %{ "1" => [ - %{handle: "shape1", and_where: nil} + %{ + handle: "shape1", + and_where: nil, + shape: Shape.new!("the_table", where: "id = 1", inspector: @inspector) + } ], "2" => [ - %{handle: "shape2", and_where: nil} + %{ + handle: "shape2", + and_where: nil, + shape: Shape.new!("the_table", where: "id = 2", inspector: @inspector) + } ] } }, @@ -254,7 +261,6 @@ defmodule Electric.Shapes.FilterTest do relation = %Relation{ - id: "TODO", schema: "public", table: "the_table" } From 0586a0e08cdfad20e5b65a2a660025a63f0f766d Mon Sep 17 00:00:00 2001 From: rob Date: Wed, 27 Nov 2024 18:26:23 +0000 Subject: [PATCH 09/62] Support TruncateRelation --- .../lib/electric/shapes/filter.ex | 14 ++++- .../test/electric/shapes/filter_test.exs | 57 ++++++++++++++++++- 2 files changed, 67 insertions(+), 4 deletions(-) diff --git a/packages/sync-service/lib/electric/shapes/filter.ex b/packages/sync-service/lib/electric/shapes/filter.ex index 122bfb23d1..5b1f77436e 100644 --- a/packages/sync-service/lib/electric/shapes/filter.ex +++ b/packages/sync-service/lib/electric/shapes/filter.ex @@ -1,8 +1,9 @@ defmodule Electric.Shapes.Filter do alias Electric.Replication.Changes.DeletedRecord alias Electric.Replication.Changes.NewRecord - alias Electric.Replication.Changes.Transaction alias Electric.Replication.Changes.Relation + alias Electric.Replication.Changes.Transaction + alias Electric.Replication.Changes.TruncatedRelation alias Electric.Replication.Changes.UpdatedRecord alias Electric.Replication.Eval.Expr alias Electric.Replication.Eval.Parser.Const @@ -88,8 +89,6 @@ defmodule Electric.Shapes.Filter do defp const_to_string(%Const{value: value, type: :int4}), do: Integer.to_string(value) defp const_to_string(%Const{value: value, type: :int8}), do: Integer.to_string(value) - # TODO: TruncateRelation and any others? - def affected_shapes(%Filter{} = filter, %Relation{} = relation) do for {handle, shape} <- all_shapes_for_table(filter, {relation.schema, relation.table}), Shape.is_affected_by_relation_change?(shape, relation), @@ -121,6 +120,15 @@ defmodule Electric.Shapes.Filter do ) end + # TODO: Optimisation: Do TruncatedRelations first and then just process other changes for other tables + + def affected_shapes(%Filter{} = filter, %TruncatedRelation{relation: table}) do + for {handle, _shape} <- all_shapes_for_table(filter, table), + into: MapSet.new() do + handle + end + end + defp affected_shapes_by_record(filter, table, record) do case Map.get(filter.tables, table) do nil -> MapSet.new() diff --git a/packages/sync-service/test/electric/shapes/filter_test.exs b/packages/sync-service/test/electric/shapes/filter_test.exs index 1052a24e4a..00fb680fd1 100644 --- a/packages/sync-service/test/electric/shapes/filter_test.exs +++ b/packages/sync-service/test/electric/shapes/filter_test.exs @@ -2,8 +2,9 @@ defmodule Electric.Shapes.FilterTest do use ExUnit.Case alias Electric.Replication.Changes.DeletedRecord alias Electric.Replication.Changes.NewRecord - alias Electric.Replication.Changes.Transaction alias Electric.Replication.Changes.Relation + alias Electric.Replication.Changes.Transaction + alias Electric.Replication.Changes.TruncatedRelation alias Electric.Replication.Changes.UpdatedRecord alias Electric.Shapes.Filter alias Electric.Shapes.Shape @@ -268,5 +269,59 @@ defmodule Electric.Shapes.FilterTest do assert Filter.affected_shapes(filter, relation) == MapSet.new(["shape1", "shape2", "shape3", "shape4"]) end + + test "returns shapes affected by truncation" do + filter = %Filter{ + tables: %{ + {"public", "the_table"} => %{ + fields: %{ + "id" => %{ + "1" => [ + %{ + handle: "shape1", + and_where: nil, + shape: Shape.new!("the_table", where: "id = 1", inspector: @inspector) + } + ], + "2" => [ + %{ + handle: "shape2", + and_where: nil, + shape: Shape.new!("the_table", where: "id = 2", inspector: @inspector) + } + ] + } + }, + other_shapes: %{ + "shape3" => Shape.new!("the_table", where: "id > 7", inspector: @inspector), + "shape4" => Shape.new!("the_table", where: "id > 6", inspector: @inspector) + } + }, + {"public", "another_table"} => %{ + fields: %{ + "id" => %{ + "1" => [ + %{handle: "not-this-shape-1", and_where: nil} + ] + } + }, + other_shapes: %{ + "not-this-shape-1" => + Shape.new!("another_table", where: "id > 7", inspector: @inspector), + "not-this-shape-2" => + Shape.new!("another_table", where: "id > 6", inspector: @inspector) + } + } + } + } + + transaction = + %TruncatedRelation{ + relation: {"public", "the_table"} + } + + assert Filter.affected_shapes(filter, transaction) == + MapSet.new(["shape1", "shape2", "shape3", "shape4"]) + end end end From e97d8037a18d50f16fe0bff075857e3b5cdd74e8 Mon Sep 17 00:00:00 2001 From: rob Date: Wed, 27 Nov 2024 18:32:55 +0000 Subject: [PATCH 10/62] Optimise where clauses --- .../lib/electric/shapes/filter.ex | 13 +++++++++++++ .../test/electric/shapes/filter_test.exs | 19 +++++++++++++++++++ 2 files changed, 32 insertions(+) diff --git a/packages/sync-service/lib/electric/shapes/filter.ex b/packages/sync-service/lib/electric/shapes/filter.ex index 5b1f77436e..418ad7665b 100644 --- a/packages/sync-service/lib/electric/shapes/filter.ex +++ b/packages/sync-service/lib/electric/shapes/filter.ex @@ -76,6 +76,7 @@ defmodule Electric.Shapes.Filter do eval: %Func{ name: ~s("="), args: [ + # TODO: Is path really [field]? %Ref{path: [field]}, %Const{} = const ] @@ -84,6 +85,18 @@ defmodule Electric.Shapes.Filter do %{operation: "=", field: field, value: const_to_string(const), and_where: nil} end + defp optimise_where(%Expr{ + eval: %Func{ + name: ~s("="), + args: [ + %Const{} = const, + %Ref{path: [field]} + ] + } + }) do + %{operation: "=", field: field, value: const_to_string(const), and_where: nil} + end + defp optimise_where(_), do: :not_optimised defp const_to_string(%Const{value: value, type: :int4}), do: Integer.to_string(value) diff --git a/packages/sync-service/test/electric/shapes/filter_test.exs b/packages/sync-service/test/electric/shapes/filter_test.exs index 00fb680fd1..abaa2e092c 100644 --- a/packages/sync-service/test/electric/shapes/filter_test.exs +++ b/packages/sync-service/test/electric/shapes/filter_test.exs @@ -32,6 +32,25 @@ defmodule Electric.Shapes.FilterTest do } end + test "with `constant = field` where clause" do + shape = Shape.new!("the_table", where: "1 = id", inspector: @inspector) + + assert Filter.new(%{"shape1" => shape}) == %Filter{ + tables: %{ + {"public", "the_table"} => %{ + fields: %{ + "id" => %{ + "1" => [ + %{handle: "shape1", and_where: nil, shape: shape} + ] + } + }, + other_shapes: %{} + } + } + } + end + test "with more complicated where clause" do shapes = %{"shape1" => Shape.new!("the_table", where: "id > 1", inspector: @inspector)} From e4ccdcf6ed567aa1a6c6f458a37c3c198ee1e8b7 Mon Sep 17 00:00:00 2001 From: rob Date: Wed, 27 Nov 2024 22:00:20 +0000 Subject: [PATCH 11/62] Optimise where clauses --- .../lib/electric/replication/eval/parser.ex | 10 +- .../lib/electric/shapes/filter.ex | 53 +++++---- .../test/electric/shapes/filter_test.exs | 106 +++++++++++++++++- 3 files changed, 143 insertions(+), 26 deletions(-) diff --git a/packages/sync-service/lib/electric/replication/eval/parser.ex b/packages/sync-service/lib/electric/replication/eval/parser.ex index ad1a90cb49..20b776aba1 100644 --- a/packages/sync-service/lib/electric/replication/eval/parser.ex +++ b/packages/sync-service/lib/electric/replication/eval/parser.ex @@ -1063,14 +1063,14 @@ defmodule Electric.Replication.Eval.Parser do defp internal_node_to_error(%Func{type: type, name: name}), do: "function #{name} returning #{type}" - defp find_refs(tree, acc \\ %{}) - defp find_refs(%Const{}, acc), do: acc - defp find_refs(%Ref{path: path, type: type}, acc), do: Map.put_new(acc, path, type) + def find_refs(tree, acc \\ %{}) + def find_refs(%Const{}, acc), do: acc + def find_refs(%Ref{path: path, type: type}, acc), do: Map.put_new(acc, path, type) - defp find_refs(%Func{args: args, variadic_arg: nil}, acc), + def find_refs(%Func{args: args, variadic_arg: nil}, acc), do: Enum.reduce(args, acc, &find_refs/2) - defp find_refs(%Func{args: args, variadic_arg: position}, acc), + def find_refs(%Func{args: args, variadic_arg: position}, acc), do: Enum.reduce(Enum.with_index(args), acc, fn {arg, ^position}, acc -> Enum.reduce(arg, acc, &find_refs/2) diff --git a/packages/sync-service/lib/electric/shapes/filter.ex b/packages/sync-service/lib/electric/shapes/filter.ex index 418ad7665b..04162aed91 100644 --- a/packages/sync-service/lib/electric/shapes/filter.ex +++ b/packages/sync-service/lib/electric/shapes/filter.ex @@ -6,6 +6,7 @@ defmodule Electric.Shapes.Filter do alias Electric.Replication.Changes.TruncatedRelation alias Electric.Replication.Changes.UpdatedRecord alias Electric.Replication.Eval.Expr + alias Electric.Replication.Eval.Parser alias Electric.Replication.Eval.Parser.Const alias Electric.Replication.Eval.Parser.Func alias Electric.Replication.Eval.Parser.Ref @@ -72,33 +73,34 @@ defmodule Electric.Shapes.Filter do ) end - defp optimise_where(%Expr{ - eval: %Func{ - name: ~s("="), - args: [ - # TODO: Is path really [field]? - %Ref{path: [field]}, - %Const{} = const - ] - } - }) do + defp optimise_where(%Expr{eval: eval}), do: optimise_where(eval) + + # TODO: Is this really ~s("=") or is it just "="? + # TODO: Is path really [field]? + defp optimise_where(%Func{name: ~s("="), args: [%Ref{path: [field]}, %Const{} = const]}) do %{operation: "=", field: field, value: const_to_string(const), and_where: nil} end - defp optimise_where(%Expr{ - eval: %Func{ - name: ~s("="), - args: [ - %Const{} = const, - %Ref{path: [field]} - ] - } - }) do + defp optimise_where(%Func{name: ~s("="), args: [%Const{} = const, %Ref{path: [field]}]}) do %{operation: "=", field: field, value: const_to_string(const), and_where: nil} end + defp optimise_where(%Func{name: "and", args: [arg1, arg2]}) do + case {optimise_where(arg1), optimise_where(arg2)} do + {%{operation: "=", field: field, value: value, and_where: nil}, _} -> + %{operation: "=", field: field, value: value, and_where: arg2} + + {_, %{operation: "=", field: field, value: value, and_where: nil}} -> + %{operation: "=", field: field, value: value, and_where: arg1} + + _ -> + :not_optimised + end + end + defp optimise_where(_), do: :not_optimised + # TODO: Impliment other types, or is this not implimented elsewhere? defp const_to_string(%Const{value: value, type: :int4}), do: Integer.to_string(value) defp const_to_string(%Const{value: value, type: :int8}), do: Integer.to_string(value) @@ -163,13 +165,26 @@ defmodule Electric.Shapes.Filter do shapes -> shapes + |> Enum.filter(&record_in_where(&1.and_where, record)) |> Enum.map(& &1.handle) |> MapSet.new() end end + defp record_in_where(nil, _), do: true + + defp record_in_where(where_clause, record) do + # TODO: Move record_in_shape? out of shapes into Where module + # Keep full Expr in shape + Shape.record_in_shape?( + %{where: %Expr{eval: where_clause, used_refs: Parser.find_refs(where_clause)}}, + record + ) + end + defp other_shapes_affected(%{other_shapes: shapes}, record) do for {handle, shape} <- shapes, + # TODO: Test Shape.record_in_shape? is called Shape.record_in_shape?(shape, record), into: MapSet.new() do handle diff --git a/packages/sync-service/test/electric/shapes/filter_test.exs b/packages/sync-service/test/electric/shapes/filter_test.exs index abaa2e092c..8f289d6648 100644 --- a/packages/sync-service/test/electric/shapes/filter_test.exs +++ b/packages/sync-service/test/electric/shapes/filter_test.exs @@ -6,6 +6,9 @@ defmodule Electric.Shapes.FilterTest do alias Electric.Replication.Changes.Transaction alias Electric.Replication.Changes.TruncatedRelation alias Electric.Replication.Changes.UpdatedRecord + alias Electric.Replication.Eval.Parser.Const + alias Electric.Replication.Eval.Parser.Func + alias Electric.Replication.Eval.Parser.Ref alias Electric.Shapes.Filter alias Electric.Shapes.Shape alias Support.StubInspector @@ -51,6 +54,64 @@ defmodule Electric.Shapes.FilterTest do } end + test "with `field = constant AND another_condition` where clause" do + shape = Shape.new!("the_table", where: "id = 1 AND id > 0", inspector: @inspector) + + assert %Filter{ + tables: %{ + {"public", "the_table"} => %{ + fields: %{ + "id" => %{ + "1" => [ + %{ + handle: "shape1", + and_where: %Func{ + name: ~s(">"), + args: [ + %Ref{path: ["id"], type: :int8}, + %Const{value: 0, type: :int4} + ] + }, + shape: ^shape + } + ] + } + }, + other_shapes: %{} + } + } + } = Filter.new(%{"shape1" => shape}) + end + + test "with `some_condition AND field = constant` where clause" do + shape = Shape.new!("the_table", where: "id > 0 AND id = 1", inspector: @inspector) + + assert %Filter{ + tables: %{ + {"public", "the_table"} => %{ + fields: %{ + "id" => %{ + "1" => [ + %{ + handle: "shape1", + and_where: %Func{ + name: ~s(">"), + args: [ + %Ref{path: ["id"], type: :int8}, + %Const{value: 0, type: :int4} + ] + }, + shape: ^shape + } + ] + } + }, + other_shapes: %{} + } + } + } = Filter.new(%{"shape1" => shape}) + end + test "with more complicated where clause" do shapes = %{"shape1" => Shape.new!("the_table", where: "id > 1", inspector: @inspector)} @@ -65,8 +126,6 @@ defmodule Electric.Shapes.FilterTest do end end - # TODO: relations - describe "affected_shapes/2" do test "shapes with same table and id are returned" do filter = %Filter{ @@ -343,4 +402,47 @@ defmodule Electric.Shapes.FilterTest do MapSet.new(["shape1", "shape2", "shape3", "shape4"]) end end + + describe "where clause filtering" do + for test <- [ + %{where: "id = 7", record: %{"id" => "7"}, affected: true}, + %{where: "id = 7", record: %{"id" => "8"}, affected: false}, + %{where: "id = 7", record: %{"id" => nil}, affected: false}, + %{where: "7 = id", record: %{"id" => "7"}, affected: true}, + %{where: "7 = id", record: %{"id" => "8"}, affected: false}, + %{where: "7 = id", record: %{"id" => nil}, affected: false}, + %{where: "id = 7 AND id > 1", record: %{"id" => "7"}, affected: true}, + %{where: "id = 7 AND id > 1", record: %{"id" => "8"}, affected: false}, + %{where: "id = 7 AND id > 8", record: %{"id" => "7"}, affected: false}, + %{where: "id > 1 AND id = 7", record: %{"id" => "7"}, affected: true}, + %{where: "id > 1 AND id = 7", record: %{"id" => "8"}, affected: false}, + %{where: "id > 8 AND id = 7", record: %{"id" => "7"}, affected: false} + ] do + test "where: #{test.where}, record: #{inspect(test.record)}" do + %{where: where, record: record, affected: affected} = unquote(Macro.escape(test)) + + shape = Shape.new!("the_table", where: where, inspector: @inspector) + + transaction = + %Transaction{ + changes: [ + %NewRecord{ + relation: {"public", "the_table"}, + record: record + } + ] + } + + expected_affected_shapes = + if affected do + MapSet.new(["the-shape"]) + else + MapSet.new([]) + end + + assert Filter.new(%{"the-shape" => shape}) + |> Filter.affected_shapes(transaction) == expected_affected_shapes + end + end + end end From 54809eeab834e1017f794b46853a2caf67cdf792 Mon Sep 17 00:00:00 2001 From: rob Date: Thu, 28 Nov 2024 20:02:53 +0000 Subject: [PATCH 12/62] Add Filter.remove_shape --- .../lib/electric/shapes/filter.ex | 20 ++++- .../electric/shapes/filter/table_filter.ex | 24 ++++++ .../test/electric/shapes/filter_test.exs | 83 +++++++++++++++++++ 3 files changed, 124 insertions(+), 3 deletions(-) create mode 100644 packages/sync-service/lib/electric/shapes/filter/table_filter.ex diff --git a/packages/sync-service/lib/electric/shapes/filter.ex b/packages/sync-service/lib/electric/shapes/filter.ex index 04162aed91..9d56a073f6 100644 --- a/packages/sync-service/lib/electric/shapes/filter.ex +++ b/packages/sync-service/lib/electric/shapes/filter.ex @@ -11,15 +11,16 @@ defmodule Electric.Shapes.Filter do alias Electric.Replication.Eval.Parser.Func alias Electric.Replication.Eval.Parser.Ref alias Electric.Shapes.Filter + alias Electric.Shapes.Filter.TableFilter alias Electric.Shapes.Shape defstruct tables: %{} - def new(shapes), do: shapes |> Map.to_list() |> new(%Filter{}) - defp new([shape | shapes], filter), do: new(shapes, add_shape(filter, shape)) + def new(shapes), do: shapes |> Map.to_list() |> new(empty()) + defp new([{handle, shape} | shapes], filter), do: new(shapes, add_shape(filter, handle, shape)) defp new([], filter), do: filter - def add_shape(%Filter{tables: tables}, {handle, shape}) do + def add_shape(%Filter{tables: tables}, handle, shape) do %Filter{ tables: Map.update( @@ -31,6 +32,19 @@ defmodule Electric.Shapes.Filter do } end + def remove_shape(%Filter{tables: tables}, handle) do + %Filter{ + tables: + tables + |> Enum.map(fn {table, table_filter} -> + {table, TableFilter.remove_shape(table_filter, handle)} + end) + |> Enum.reject(fn {_table, table_filter} -> map_size(table_filter.fields) == 0 end) + |> Map.new() + } + end + + def empty, do: %Filter{} defp empty_table_filter, do: %{fields: %{}, other_shapes: %{}} defp add_shape_to_table_filter({handle, shape} = shape_instance, table_filter) do diff --git a/packages/sync-service/lib/electric/shapes/filter/table_filter.ex b/packages/sync-service/lib/electric/shapes/filter/table_filter.ex new file mode 100644 index 0000000000..e4ac8cfe0b --- /dev/null +++ b/packages/sync-service/lib/electric/shapes/filter/table_filter.ex @@ -0,0 +1,24 @@ +defmodule Electric.Shapes.Filter.TableFilter do + def remove_shape(%{fields: fields, other_shapes: other_shapes}, handle) do + %{ + fields: remove_shape_from_fields(fields, handle), + other_shapes: Map.delete(other_shapes, handle) + } + end + + defp remove_shape_from_fields(fields, handle) do + fields + |> Map.new(fn {field, value_filter} -> + {field, remove_shape_from_value_filter(value_filter, handle)} + end) + |> Enum.reject(fn {_field, value_filter} -> map_size(value_filter) == 0 end) + |> Map.new() + end + + defp remove_shape_from_value_filter(value_filter, handle) do + value_filter + |> Map.new(fn {value, shapes} -> {value, shapes |> Enum.reject(&(&1.handle == handle))} end) + |> Enum.reject(fn {_value, shapes} -> shapes == [] end) + |> Map.new() + end +end diff --git a/packages/sync-service/test/electric/shapes/filter_test.exs b/packages/sync-service/test/electric/shapes/filter_test.exs index 8f289d6648..6572d8cbef 100644 --- a/packages/sync-service/test/electric/shapes/filter_test.exs +++ b/packages/sync-service/test/electric/shapes/filter_test.exs @@ -126,6 +126,89 @@ defmodule Electric.Shapes.FilterTest do end end + describe "remove_shape/2" do + test "removes all shapes with the specified handle" do + filter = %Filter{ + tables: %{ + {"public", "the_table"} => %{ + fields: %{ + "id" => %{ + "1" => [ + %{ + handle: "shape1", + and_where: nil + } + ], + "2" => [ + %{ + handle: "shape2", + and_where: nil + } + ] + }, + "name" => %{ + "bill" => [ + %{ + handle: "shape1", + and_where: nil + }, + %{ + handle: "shape2", + and_where: nil + } + ] + } + }, + other_shapes: %{ + "shape1" => Shape.new!("the_table", where: "id = 1", inspector: @inspector), + "shape2" => Shape.new!("the_table", where: "id = 2", inspector: @inspector) + } + }, + {"public", "another_table"} => %{ + fields: %{ + "id" => %{ + "1" => [ + %{handle: "shape1", and_where: nil} + ] + } + }, + other_shapes: %{ + "shape1" => Shape.new!("another_table", where: "id = 1", inspector: @inspector) + } + } + } + } + + assert Filter.remove_shape(filter, "shape1") == %Filter{ + tables: %{ + {"public", "the_table"} => %{ + fields: %{ + "id" => %{ + "2" => [ + %{ + handle: "shape2", + and_where: nil + } + ] + }, + "name" => %{ + "bill" => [ + %{ + handle: "shape2", + and_where: nil + } + ] + } + }, + other_shapes: %{ + "shape2" => Shape.new!("the_table", where: "id = 2", inspector: @inspector) + } + } + } + } + end + end + describe "affected_shapes/2" do test "shapes with same table and id are returned" do filter = %Filter{ From c697ce8b1090a667caa7ee376cf23a9293716544 Mon Sep 17 00:00:00 2001 From: rob Date: Thu, 28 Nov 2024 20:09:07 +0000 Subject: [PATCH 13/62] Update dispatcher --- .../lib/electric/shapes/consumer.ex | 22 +----- .../lib/electric/shapes/dispatcher.ex | 69 ++++++++----------- .../test/electric/shapes/dispatcher_test.exs | 68 ++++++++++-------- 3 files changed, 71 insertions(+), 88 deletions(-) diff --git a/packages/sync-service/lib/electric/shapes/consumer.ex b/packages/sync-service/lib/electric/shapes/consumer.ex index 7e694eba33..5d067be7db 100644 --- a/packages/sync-service/lib/electric/shapes/consumer.ex +++ b/packages/sync-service/lib/electric/shapes/consumer.ex @@ -87,27 +87,7 @@ defmodule Electric.Shapes.Consumer do monitors: [] }) - {:consumer, state, - subscribe_to: [{producer, [max_demand: 1, selector: &selector(&1, config.shape)]}]} - end - - defp selector(event, shape) do - process_event?(event, shape) - rescue - # Swallow errors here to avoid crashing the ShapeLogCollector. - # Return `true` so the event is processed, which will then error - # for the same reason and cleanup the shape. - _ -> true - end - - defp process_event?(%Transaction{changes: changes}, shape) do - changes - |> Stream.flat_map(&Shape.convert_change(shape, &1)) - |> Enum.any?() - end - - defp process_event?(%Changes.Relation{} = relation_change, shape) do - Shape.is_affected_by_relation_change?(shape, relation_change) + {:consumer, state, subscribe_to: [{producer, [max_demand: 1, shape: config.shape]}]} end def handle_call(:initial_state, _from, %{snapshot_xmin: xmin, latest_offset: offset} = state) do diff --git a/packages/sync-service/lib/electric/shapes/dispatcher.ex b/packages/sync-service/lib/electric/shapes/dispatcher.ex index 92228bbdcd..b87a52786d 100644 --- a/packages/sync-service/lib/electric/shapes/dispatcher.ex +++ b/packages/sync-service/lib/electric/shapes/dispatcher.ex @@ -31,16 +31,17 @@ defmodule Electric.Shapes.Dispatcher do """ require Logger + alias Electric.Shapes.Filter @behaviour GenStage.Dispatcher @impl GenStage.Dispatcher def init(_opts) do - {:ok, {0, 0, nil, [], MapSet.new()}} + {:ok, {0, 0, nil, [], Filter.empty(), MapSet.new()}} end @impl GenStage.Dispatcher - def subscribe(opts, {pid, _ref} = from, {n, waiting, pending, subs, pids}) do + def subscribe(opts, {pid, _ref} = from, {n, waiting, pending, subs, filter, pids}) do if MapSet.member?(pids, pid) do Logger.error(fn -> "#{inspect(pid)} is already registered with #{inspect(self())}. " <> @@ -49,32 +50,25 @@ defmodule Electric.Shapes.Dispatcher do {:error, :already_subscribed} else - selector = - case Keyword.get(opts, :selector) do - nil -> - nil + shape = Keyword.fetch!(opts, :shape) - selector when is_function(selector, 1) -> - selector - - other -> - raise ArgumentError, - ":selector option must be passed a unary function, got: #{inspect(other)}" - end - - subs = [{from, selector} | subs] + subs = [{from, shape} | subs] demand = if n == 0, do: 1, else: 0 - {:ok, demand, {n + 1, waiting, pending, subs, MapSet.put(pids, pid)}} + filter = Filter.add_shape(filter, from, shape) + + {:ok, demand, {n + 1, waiting, pending, subs, filter, MapSet.put(pids, pid)}} end end @impl GenStage.Dispatcher - def cancel({pid, _ref} = from, {n, waiting, pending, subs, pids}) do + def cancel({pid, _ref} = from, {n, waiting, pending, subs, filter, pids}) do if MapSet.member?(pids, pid) do subs = List.keydelete(subs, from, 0) + filter = Filter.remove_shape(filter, from) + if pending && MapSet.member?(pending, from) do case waiting - 1 do 0 -> @@ -84,13 +78,14 @@ defmodule Electric.Shapes.Dispatcher do new_waiting -> {:ok, 0, - {n - 1, new_waiting, MapSet.delete(pending, from), subs, MapSet.delete(pids, pid)}} + {n - 1, new_waiting, MapSet.delete(pending, from), subs, filter, + MapSet.delete(pids, pid)}} end else - {:ok, 0, {n - 1, waiting, pending, subs, MapSet.delete(pids, pid)}} + {:ok, 0, {n - 1, waiting, pending, subs, filter, MapSet.delete(pids, pid)}} end else - {:ok, 0, {n, waiting, pending, subs, pids}} + {:ok, 0, {n, waiting, pending, subs, filter, pids}} end end @@ -98,34 +93,31 @@ defmodule Electric.Shapes.Dispatcher do # consumers sending demand before we have produced a message just ignore as # we have already sent initial demand of 1 to the producer when the first # consumer subscribed. - def ask(1, {_pid, _ref}, {n, 0, nil, subs, pids}) do - {:ok, 0, {n, 0, nil, subs, pids}} + def ask(1, {_pid, _ref}, {n, 0, nil, subs, filter, pids}) do + {:ok, 0, {n, 0, nil, subs, filter, pids}} end - def ask(1, {_pid, _ref}, {n, 1, _pending, subs, pids}) do - {:ok, 1, {n, 0, nil, subs, pids}} + def ask(1, {_pid, _ref}, {n, 1, _pending, subs, filter, pids}) do + {:ok, 1, {n, 0, nil, subs, filter, pids}} end - def ask(1, from, {n, waiting, pending, subs, pids}) when waiting > 1 do - {:ok, 0, {n, waiting - 1, MapSet.delete(pending, from), subs, pids}} + def ask(1, from, {n, waiting, pending, subs, filter, pids}) when waiting > 1 do + {:ok, 0, {n, waiting - 1, MapSet.delete(pending, from), subs, filter, pids}} end @impl GenStage.Dispatcher # handle the no subscribers case here to make the real dispatch impl easier - def dispatch([event], _length, {_n, 0, _pending, [], _pids} = state) do + def dispatch([event], _length, {_n, 0, _pending, [], _filter, _pids} = state) do {:ok, [event], state} end - def dispatch([event], _length, {n, 0, _pending, subs, pids}) do + def dispatch([event], _length, {n, 0, _pending, subs, filter, pids}) do {waiting, pending} = - subs - |> Enum.reduce({0, MapSet.new()}, fn {{pid, ref} = sub, selector}, {waiting, pending} -> - if subscriber_wants_message?(event, selector) do - Process.send(pid, {:"$gen_consumer", {self(), ref}, [event]}, [:noconnect]) - {waiting + 1, MapSet.put(pending, sub)} - else - {waiting, pending} - end + filter + |> Filter.affected_shapes(event) + |> Enum.reduce({0, MapSet.new()}, fn {pid, ref} = sub, {waiting, pending} -> + Process.send(pid, {:"$gen_consumer", {self(), ref}, [event]}, [:noconnect]) + {waiting + 1, MapSet.put(pending, sub)} end) |> case do {0, _pending} -> @@ -139,7 +131,7 @@ defmodule Electric.Shapes.Dispatcher do {waiting, pending} end - {:ok, [], {n, waiting, pending, subs, pids}} + {:ok, [], {n, waiting, pending, subs, filter, pids}} end @impl GenStage.Dispatcher @@ -147,7 +139,4 @@ defmodule Electric.Shapes.Dispatcher do send(self(), msg) {:ok, state} end - - defp subscriber_wants_message?(_event, nil), do: true - defp subscriber_wants_message?(event, selector), do: selector.(event) end diff --git a/packages/sync-service/test/electric/shapes/dispatcher_test.exs b/packages/sync-service/test/electric/shapes/dispatcher_test.exs index fa930dbb74..f1f8e0066d 100644 --- a/packages/sync-service/test/electric/shapes/dispatcher_test.exs +++ b/packages/sync-service/test/electric/shapes/dispatcher_test.exs @@ -1,7 +1,24 @@ defmodule Electric.Shapes.DispatcherTest do use ExUnit.Case, async: true + alias Electric.Shapes.Shape + alias Electric.Replication.Changes.NewRecord + alias Electric.Replication.Changes.Transaction alias Electric.Shapes.Dispatcher, as: D + alias Support.StubInspector + + @inspector StubInspector.new([%{name: "id", type: "int8", pk_position: 0}]) + @shape Shape.new!("the_table", where: "id = 1", inspector: @inspector) + @other_shape Shape.new!("the_table", where: "id = 2", inspector: @inspector) + + @transaction %Transaction{ + changes: [ + %NewRecord{ + relation: {"public", "the_table"}, + record: %{"id" => "1"} + } + ] + } defp dispatcher(opts \\ []) do {:ok, state} = D.init(opts) @@ -32,9 +49,6 @@ defmodule Electric.Shapes.DispatcherTest do {pid, ref} end - defp is_even(n), do: rem(n, 2) == 0 - defp is_odd(n), do: rem(n, 2) == 1 - test "demand is only sent to producer once all subscribers have processed the message" do dispatcher = dispatcher() @@ -43,11 +57,11 @@ defmodule Electric.Shapes.DispatcherTest do c3 = {_pid3, ref3} = consumer(3) # we only want to send a single event for any number of consumers - {:ok, 1, dispatcher} = D.subscribe([selector: &is_even/1], c1, dispatcher) - {:ok, 0, dispatcher} = D.subscribe([selector: &is_even/1], c2, dispatcher) - {:ok, 0, dispatcher} = D.subscribe([selector: &is_even/1], c3, dispatcher) + {:ok, 1, dispatcher} = D.subscribe([shape: @shape], c1, dispatcher) + {:ok, 0, dispatcher} = D.subscribe([shape: @shape], c2, dispatcher) + {:ok, 0, dispatcher} = D.subscribe([shape: @shape], c3, dispatcher) - event = 2 + event = @transaction {:ok, [], dispatcher} = D.dispatch([event], 1, dispatcher) @@ -68,11 +82,11 @@ defmodule Electric.Shapes.DispatcherTest do c2 = {_pid2, ref2} = consumer(2) c3 = {_pid3, ref3} = consumer(3) - {:ok, 1, dispatcher} = D.subscribe([selector: &is_odd/1], c1, dispatcher) - {:ok, 0, dispatcher} = D.subscribe([selector: &is_even/1], c2, dispatcher) - {:ok, 0, dispatcher} = D.subscribe([selector: &is_even/1], c3, dispatcher) + {:ok, 1, dispatcher} = D.subscribe([shape: @other_shape], c1, dispatcher) + {:ok, 0, dispatcher} = D.subscribe([shape: @shape], c2, dispatcher) + {:ok, 0, dispatcher} = D.subscribe([shape: @shape], c3, dispatcher) - event = 2 + event = @transaction {:ok, [], dispatcher} = D.dispatch([event], 1, dispatcher) @@ -91,11 +105,11 @@ defmodule Electric.Shapes.DispatcherTest do c2 = {_pid2, ref2} = consumer(2) c3 = {_pid3, ref3} = consumer(3) - {:ok, 1, dispatcher} = D.subscribe([selector: &is_even/1], c1, dispatcher) - {:ok, 0, dispatcher} = D.subscribe([selector: &is_even/1], c2, dispatcher) - {:ok, 0, dispatcher} = D.subscribe([selector: &is_even/1], c3, dispatcher) + {:ok, 1, dispatcher} = D.subscribe([shape: @shape], c1, dispatcher) + {:ok, 0, dispatcher} = D.subscribe([shape: @shape], c2, dispatcher) + {:ok, 0, dispatcher} = D.subscribe([shape: @shape], c3, dispatcher) - event = 2 + event = @transaction {:ok, [], dispatcher} = D.dispatch([event], 1, dispatcher) @@ -117,11 +131,11 @@ defmodule Electric.Shapes.DispatcherTest do c2 = {_pid2, ref2} = consumer(2) c3 = {_pid3, ref3} = consumer(3) - {:ok, 1, dispatcher} = D.subscribe([], c1, dispatcher) - {:ok, 0, dispatcher} = D.subscribe([selector: &is_even/1], c2, dispatcher) - {:ok, 0, dispatcher} = D.subscribe([selector: &is_even/1], c3, dispatcher) + {:ok, 1, dispatcher} = D.subscribe([shape: @shape], c1, dispatcher) + {:ok, 0, dispatcher} = D.subscribe([shape: @shape], c2, dispatcher) + {:ok, 0, dispatcher} = D.subscribe([shape: @shape], c3, dispatcher) - event = 2 + event = @transaction {:ok, [], dispatcher} = D.dispatch([event], 1, dispatcher) @@ -146,11 +160,11 @@ defmodule Electric.Shapes.DispatcherTest do c3 = {_pid3, _ref3} = consumer(3) # we only want to send a single event for any number of consumers - {:ok, 1, dispatcher} = D.subscribe([], c1, dispatcher) - {:ok, 0, dispatcher} = D.subscribe([selector: &is_even/1], c2, dispatcher) - {:ok, 0, dispatcher} = D.subscribe([selector: &is_even/1], c3, dispatcher) + {:ok, 1, dispatcher} = D.subscribe([shape: @shape], c1, dispatcher) + {:ok, 0, dispatcher} = D.subscribe([shape: @shape], c2, dispatcher) + {:ok, 0, dispatcher} = D.subscribe([shape: @shape], c3, dispatcher) - event = 2 + event = @transaction {:ok, [], dispatcher} = D.dispatch([event], 1, dispatcher) @@ -168,11 +182,11 @@ defmodule Electric.Shapes.DispatcherTest do c2 = {_pid2, ref2} = consumer(2) c3 = {_pid3, ref3} = consumer(3) - {:ok, 1, dispatcher} = D.subscribe([selector: &is_even/1], c1, dispatcher) - {:ok, 0, dispatcher} = D.subscribe([selector: &is_even/1], c2, dispatcher) - {:ok, 0, dispatcher} = D.subscribe([selector: &is_even/1], c3, dispatcher) + {:ok, 1, dispatcher} = D.subscribe([shape: @other_shape], c1, dispatcher) + {:ok, 0, dispatcher} = D.subscribe([shape: @other_shape], c2, dispatcher) + {:ok, 0, dispatcher} = D.subscribe([shape: @other_shape], c3, dispatcher) - event = 3 + event = @transaction {:ok, [], dispatcher} = D.dispatch([event], 1, dispatcher) refute_receive {C, ^ref1, [^event]} From 319ede51975f05723f8d10770cd6bb61aad9951a Mon Sep 17 00:00:00 2001 From: rob Date: Fri, 29 Nov 2024 09:09:52 +0000 Subject: [PATCH 14/62] Add TODOs --- packages/sync-service/test/electric/shapes/filter_test.exs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/packages/sync-service/test/electric/shapes/filter_test.exs b/packages/sync-service/test/electric/shapes/filter_test.exs index 6572d8cbef..2c0efb9d98 100644 --- a/packages/sync-service/test/electric/shapes/filter_test.exs +++ b/packages/sync-service/test/electric/shapes/filter_test.exs @@ -431,6 +431,10 @@ defmodule Electric.Shapes.FilterTest do MapSet.new(["shape1", "shape2", "shape3", "shape4"]) end + #TODO: Relation renames + #TODO: Also go through Shape.is_affected_by_relation_change? tests to see if all scenarious are covered here + #TODO: Also go through Shape.convert_change tests to see if all scenarious are covered here + test "returns shapes affected by truncation" do filter = %Filter{ tables: %{ From 1e7f050124f43f7c856e6c96bb42b82a0e930009 Mon Sep 17 00:00:00 2001 From: rob Date: Fri, 29 Nov 2024 10:02:59 +0000 Subject: [PATCH 15/62] Support renames --- .../lib/electric/shapes/filter.ex | 31 +++++++++++++------ .../test/electric/shapes/filter_test.exs | 1 - 2 files changed, 22 insertions(+), 10 deletions(-) diff --git a/packages/sync-service/lib/electric/shapes/filter.ex b/packages/sync-service/lib/electric/shapes/filter.ex index 9d56a073f6..1dfd9cffb4 100644 --- a/packages/sync-service/lib/electric/shapes/filter.ex +++ b/packages/sync-service/lib/electric/shapes/filter.ex @@ -119,7 +119,8 @@ defmodule Electric.Shapes.Filter do defp const_to_string(%Const{value: value, type: :int8}), do: Integer.to_string(value) def affected_shapes(%Filter{} = filter, %Relation{} = relation) do - for {handle, shape} <- all_shapes_for_table(filter, {relation.schema, relation.table}), + # Check all shapes is all tables becuase the table may have been renamed + for {handle, shape} <- all_shapes_in_filter(filter), Shape.is_affected_by_relation_change?(shape, relation), into: MapSet.new() do handle @@ -205,19 +206,31 @@ defmodule Electric.Shapes.Filter do end end + defp all_shapes_in_filter(%Filter{} = filter) do + for {_table, table_filter} <- filter.tables, + {handle, shape} <- all_shapes_in_table_filter(table_filter), + into: %{} do + {handle, shape} + end + end + + defp all_shapes_in_table_filter(%{fields: fields, other_shapes: other_shapes}) do + for {_field, values} <- fields, + {_value, shapes} <- values, + %{handle: handle, shape: shape} <- shapes, + into: %{} do + {handle, shape} + end + |> Map.merge(other_shapes) + end + defp all_shapes_for_table(%Filter{} = filter, table) do case Map.get(filter.tables, table) do nil -> %{} - %{fields: fields, other_shapes: other_shapes} -> - for {_field, values} <- fields, - {_value, shapes} <- values, - %{handle: handle, shape: shape} <- shapes, - into: %{} do - {handle, shape} - end - |> Map.merge(other_shapes) + table_filter -> + all_shapes_in_table_filter(table_filter) end end end diff --git a/packages/sync-service/test/electric/shapes/filter_test.exs b/packages/sync-service/test/electric/shapes/filter_test.exs index 2c0efb9d98..6186af9d88 100644 --- a/packages/sync-service/test/electric/shapes/filter_test.exs +++ b/packages/sync-service/test/electric/shapes/filter_test.exs @@ -431,7 +431,6 @@ defmodule Electric.Shapes.FilterTest do MapSet.new(["shape1", "shape2", "shape3", "shape4"]) end - #TODO: Relation renames #TODO: Also go through Shape.is_affected_by_relation_change? tests to see if all scenarious are covered here #TODO: Also go through Shape.convert_change tests to see if all scenarious are covered here From 1cb01dac09f3a5d15b9ef7a20e43aa7c7e100a78 Mon Sep 17 00:00:00 2001 From: rob Date: Fri, 29 Nov 2024 10:47:21 +0000 Subject: [PATCH 16/62] Fix remaining tests --- .../lib/electric/shapes/filter.ex | 2 +- .../replication/shape_log_collector_test.exs | 29 +++++++++++++++---- .../test/support/transaction_consumer.ex | 5 +++- 3 files changed, 28 insertions(+), 8 deletions(-) diff --git a/packages/sync-service/lib/electric/shapes/filter.ex b/packages/sync-service/lib/electric/shapes/filter.ex index 1dfd9cffb4..bda02c505e 100644 --- a/packages/sync-service/lib/electric/shapes/filter.ex +++ b/packages/sync-service/lib/electric/shapes/filter.ex @@ -83,7 +83,7 @@ defmodule Electric.Shapes.Filter do value_filter, value, [%{handle: handle, and_where: and_where, shape: shape}], - fn shapes -> [%{handle: handle, and_where: and_where} | shapes] end + fn shapes -> [%{handle: handle, and_where: and_where, shape: shape} | shapes] end ) end diff --git a/packages/sync-service/test/electric/replication/shape_log_collector_test.exs b/packages/sync-service/test/electric/replication/shape_log_collector_test.exs index 9d7cebcef7..3260248b44 100644 --- a/packages/sync-service/test/electric/replication/shape_log_collector_test.exs +++ b/packages/sync-service/test/electric/replication/shape_log_collector_test.exs @@ -6,8 +6,10 @@ defmodule Electric.Replication.ShapeLogCollectorTest do alias Electric.Replication.Changes.{Transaction, Relation} alias Electric.Replication.Changes alias Electric.Replication.LogOffset + alias Electric.Shapes.Shape alias Support.Mock + alias Support.StubInspector import Support.ComponentSetup, only: [with_in_memory_storage: 1, with_stack_id_from_test: 1] import Mox @@ -58,13 +60,21 @@ defmodule Electric.Replication.ShapeLogCollectorTest do end describe "store_transaction/2" do + @inspector StubInspector.new([%{name: "id", type: "int8", pk_position: 0}]) + @shape Shape.new!("test_table", where: "id = 2", inspector: @inspector) + setup ctx do parent = self() consumers = Enum.map(1..3, fn id -> {:ok, consumer} = - Support.TransactionConsumer.start_link(id: id, parent: parent, producer: ctx.server) + Support.TransactionConsumer.start_link( + id: id, + parent: parent, + producer: ctx.server, + shape: @shape + ) {id, consumer} end) @@ -88,7 +98,7 @@ defmodule Electric.Replication.ShapeLogCollectorTest do %Transaction{xid: xmin, lsn: lsn, last_log_offset: last_log_offset} |> Transaction.prepend_change(%Changes.NewRecord{ relation: {"public", "test_table"}, - record: %{"id" => "1"} + record: %{"id" => "2", "name" => "foo"} }) assert :ok = ShapeLogCollector.store_transaction(txn, ctx.server) @@ -102,7 +112,7 @@ defmodule Electric.Replication.ShapeLogCollectorTest do %Transaction{xid: xid, lsn: lsn, last_log_offset: last_log_offset} |> Transaction.prepend_change(%Changes.NewRecord{ relation: {"public", "test_table"}, - record: %{"id" => "2"} + record: %{"id" => "2", "name" => "bar"} }) assert :ok = ShapeLogCollector.store_transaction(txn2, ctx.server) @@ -120,7 +130,12 @@ defmodule Electric.Replication.ShapeLogCollectorTest do consumers = Enum.map(1..3, fn id -> {:ok, consumer} = - Support.TransactionConsumer.start_link(id: id, parent: parent, producer: ctx.server) + Support.TransactionConsumer.start_link( + id: id, + parent: parent, + producer: ctx.server, + shape: @shape + ) {id, consumer} end) @@ -129,11 +144,13 @@ defmodule Electric.Replication.ShapeLogCollectorTest do end test "should handle new relations", ctx do - relation1 = %Relation{id: 1, table: "test_table", schema: "public"} + id = @shape.root_table_id + + relation1 = %Relation{id: id, table: "test_table", schema: "public", columns: []} assert :ok = ShapeLogCollector.handle_relation_msg(relation1, ctx.server) - relation2 = %Relation{id: 2, table: "bar", schema: "public"} + relation2 = %Relation{id: id, table: "bar", schema: "public", columns: []} assert :ok = ShapeLogCollector.handle_relation_msg(relation2, ctx.server) diff --git a/packages/sync-service/test/support/transaction_consumer.ex b/packages/sync-service/test/support/transaction_consumer.ex index 6c51b61928..67e01c1bbe 100644 --- a/packages/sync-service/test/support/transaction_consumer.ex +++ b/packages/sync-service/test/support/transaction_consumer.ex @@ -35,9 +35,12 @@ defmodule Support.TransactionConsumer do {:ok, producer} = Keyword.fetch(opts, :producer) {:ok, parent} = Keyword.fetch(opts, :parent) {:ok, id} = Keyword.fetch(opts, :id) + # TODO: Remove partition = Keyword.get(opts, :partition, :transaction) + shape = Keyword.fetch!(opts, :shape) - {:consumer, {id, nil, parent}, subscribe_to: [{producer, [partition: partition]}]} + {:consumer, {id, nil, parent}, + subscribe_to: [{producer, [partition: partition, shape: shape]}]} end def handle_subscribe(:producer, _options, from, {id, _, parent}) do From 2881ad27189ca0dabfae080c770483f7d085f990 Mon Sep 17 00:00:00 2001 From: rob Date: Fri, 29 Nov 2024 10:48:41 +0000 Subject: [PATCH 17/62] Remove redundant arg --- packages/sync-service/test/support/transaction_consumer.ex | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/packages/sync-service/test/support/transaction_consumer.ex b/packages/sync-service/test/support/transaction_consumer.ex index 67e01c1bbe..d6fe94e9c0 100644 --- a/packages/sync-service/test/support/transaction_consumer.ex +++ b/packages/sync-service/test/support/transaction_consumer.ex @@ -35,12 +35,9 @@ defmodule Support.TransactionConsumer do {:ok, producer} = Keyword.fetch(opts, :producer) {:ok, parent} = Keyword.fetch(opts, :parent) {:ok, id} = Keyword.fetch(opts, :id) - # TODO: Remove - partition = Keyword.get(opts, :partition, :transaction) shape = Keyword.fetch!(opts, :shape) - {:consumer, {id, nil, parent}, - subscribe_to: [{producer, [partition: partition, shape: shape]}]} + {:consumer, {id, nil, parent}, subscribe_to: [{producer, [shape: shape]}]} end def handle_subscribe(:producer, _options, from, {id, _, parent}) do From 8c3bf5f17485fdb51a8df0b2c4efb163152d2216 Mon Sep 17 00:00:00 2001 From: rob Date: Fri, 29 Nov 2024 11:44:37 +0000 Subject: [PATCH 18/62] Pass inspector to dispatcher --- .../lib/electric/replication/shape_log_collector.ex | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/packages/sync-service/lib/electric/replication/shape_log_collector.ex b/packages/sync-service/lib/electric/replication/shape_log_collector.ex index 63c8bacab6..b5424e3d9a 100644 --- a/packages/sync-service/lib/electric/replication/shape_log_collector.ex +++ b/packages/sync-service/lib/electric/replication/shape_log_collector.ex @@ -56,7 +56,8 @@ defmodule Electric.Replication.ShapeLogCollector do state = Map.merge(opts, %{producer: nil, subscriptions: {0, MapSet.new()}}) # start in demand: :accumulate mode so that the ShapeCache is able to start # all active consumers before we start sending transactions - {:producer, state, dispatcher: Electric.Shapes.Dispatcher, demand: opts.demand} + {:producer, state, + dispatcher: {Electric.Shapes.Dispatcher, [inspector: state.inspector]}, demand: opts.demand} end def handle_subscribe(:consumer, _opts, from, state) do From 9f03c18c5c94ba13064702386b9b1957b4c7a724 Mon Sep 17 00:00:00 2001 From: rob Date: Fri, 29 Nov 2024 12:19:33 +0000 Subject: [PATCH 19/62] Refactor dispatcher so that state is a struct --- .../lib/electric/shapes/dispatcher.ex | 82 +++++++++++++------ 1 file changed, 58 insertions(+), 24 deletions(-) diff --git a/packages/sync-service/lib/electric/shapes/dispatcher.ex b/packages/sync-service/lib/electric/shapes/dispatcher.ex index b87a52786d..4976a5bc12 100644 --- a/packages/sync-service/lib/electric/shapes/dispatcher.ex +++ b/packages/sync-service/lib/electric/shapes/dispatcher.ex @@ -33,15 +33,25 @@ defmodule Electric.Shapes.Dispatcher do require Logger alias Electric.Shapes.Filter + defmodule State do + defstruct n: 0, + waiting: 0, + pending: nil, + subs: [], + filter: Filter.empty(), + pids: MapSet.new() + end + @behaviour GenStage.Dispatcher @impl GenStage.Dispatcher + def init(_opts) do - {:ok, {0, 0, nil, [], Filter.empty(), MapSet.new()}} + {:ok, %State{}} end @impl GenStage.Dispatcher - def subscribe(opts, {pid, _ref} = from, {n, waiting, pending, subs, filter, pids}) do + def subscribe(opts, {pid, _ref} = from, %State{n: n, pids: pids} = state) do if MapSet.member?(pids, pid) do Logger.error(fn -> "#{inspect(pid)} is already registered with #{inspect(self())}. " <> @@ -52,40 +62,64 @@ defmodule Electric.Shapes.Dispatcher do else shape = Keyword.fetch!(opts, :shape) - subs = [{from, shape} | subs] + subs = [{from, shape} | state.subs] demand = if n == 0, do: 1, else: 0 - filter = Filter.add_shape(filter, from, shape) + filter = Filter.add_shape(state.filter, from, shape) - {:ok, demand, {n + 1, waiting, pending, subs, filter, MapSet.put(pids, pid)}} + {:ok, demand, + %State{state | n: n + 1, subs: subs, filter: filter, pids: MapSet.put(state.pids, pid)}} end end @impl GenStage.Dispatcher - def cancel({pid, _ref} = from, {n, waiting, pending, subs, filter, pids}) do - if MapSet.member?(pids, pid) do - subs = List.keydelete(subs, from, 0) + def cancel({pid, _ref} = from, %State{n: n, waiting: waiting, pending: pending} = state) do + if MapSet.member?(state.pids, pid) do + subs = List.keydelete(state.subs, from, 0) - filter = Filter.remove_shape(filter, from) + filter = Filter.remove_shape(state.filter, from) if pending && MapSet.member?(pending, from) do case waiting - 1 do 0 -> # the only remaining unacked subscriber has cancelled, so we # return some demand - {:ok, 1, {n - 1, 0, nil, subs, MapSet.delete(pids, pid)}} + {:ok, 1, + %State{ + state + | n: n - 1, + waiting: 0, + pending: nil, + subs: subs, + filter: filter, + pids: MapSet.delete(state.pids, pid) + }} new_waiting -> {:ok, 0, - {n - 1, new_waiting, MapSet.delete(pending, from), subs, filter, - MapSet.delete(pids, pid)}} + %State{ + state + | n: n - 1, + waiting: new_waiting, + pending: MapSet.delete(pending, from), + subs: subs, + filter: filter, + pids: MapSet.delete(state.pids, pid) + }} end else - {:ok, 0, {n - 1, waiting, pending, subs, filter, MapSet.delete(pids, pid)}} + {:ok, 0, + %State{ + state + | n: n - 1, + subs: subs, + filter: filter, + pids: MapSet.delete(state.pids, pid) + }} end else - {:ok, 0, {n, waiting, pending, subs, filter, pids}} + {:ok, 0, state} end end @@ -93,27 +127,27 @@ defmodule Electric.Shapes.Dispatcher do # consumers sending demand before we have produced a message just ignore as # we have already sent initial demand of 1 to the producer when the first # consumer subscribed. - def ask(1, {_pid, _ref}, {n, 0, nil, subs, filter, pids}) do - {:ok, 0, {n, 0, nil, subs, filter, pids}} + def ask(1, {_pid, _ref}, %State{waiting: 0, pending: nil} = state) do + {:ok, 0, state} end - def ask(1, {_pid, _ref}, {n, 1, _pending, subs, filter, pids}) do - {:ok, 1, {n, 0, nil, subs, filter, pids}} + def ask(1, {_pid, _ref}, %State{waiting: 1} = state) do + {:ok, 1, %State{state | waiting: 0, pending: nil}} end - def ask(1, from, {n, waiting, pending, subs, filter, pids}) when waiting > 1 do - {:ok, 0, {n, waiting - 1, MapSet.delete(pending, from), subs, filter, pids}} + def ask(1, from, %State{waiting: waiting, pending: pending} = state) when waiting > 1 do + {:ok, 0, %State{state | waiting: waiting - 1, pending: MapSet.delete(pending, from)}} end @impl GenStage.Dispatcher # handle the no subscribers case here to make the real dispatch impl easier - def dispatch([event], _length, {_n, 0, _pending, [], _filter, _pids} = state) do + def dispatch([event], _length, %State{waiting: 0, subs: []} = state) do {:ok, [event], state} end - def dispatch([event], _length, {n, 0, _pending, subs, filter, pids}) do + def dispatch([event], _length, %State{waiting: 0, subs: subs} = state) do {waiting, pending} = - filter + state.filter |> Filter.affected_shapes(event) |> Enum.reduce({0, MapSet.new()}, fn {pid, ref} = sub, {waiting, pending} -> Process.send(pid, {:"$gen_consumer", {self(), ref}, [event]}, [:noconnect]) @@ -131,7 +165,7 @@ defmodule Electric.Shapes.Dispatcher do {waiting, pending} end - {:ok, [], {n, waiting, pending, subs, filter, pids}} + {:ok, [], %State{state | waiting: waiting, pending: pending}} end @impl GenStage.Dispatcher From bb2e4fa9eb0e1357f5b3840d7aa05294a8f16f2b Mon Sep 17 00:00:00 2001 From: rob Date: Fri, 29 Nov 2024 12:24:26 +0000 Subject: [PATCH 20/62] Add inspector to dispatcher state --- .../lib/electric/shapes/dispatcher.ex | 20 +++++++++++-------- .../test/electric/shapes/dispatcher_test.exs | 4 ++-- 2 files changed, 14 insertions(+), 10 deletions(-) diff --git a/packages/sync-service/lib/electric/shapes/dispatcher.ex b/packages/sync-service/lib/electric/shapes/dispatcher.ex index 4976a5bc12..f944f51a89 100644 --- a/packages/sync-service/lib/electric/shapes/dispatcher.ex +++ b/packages/sync-service/lib/electric/shapes/dispatcher.ex @@ -34,20 +34,24 @@ defmodule Electric.Shapes.Dispatcher do alias Electric.Shapes.Filter defmodule State do - defstruct n: 0, - waiting: 0, - pending: nil, - subs: [], - filter: Filter.empty(), - pids: MapSet.new() + defstruct [:n, :waiting, :pending, :subs, :filter, :pids, :inspector] end @behaviour GenStage.Dispatcher @impl GenStage.Dispatcher - def init(_opts) do - {:ok, %State{}} + def init(opts) do + {:ok, + %State{ + n: 0, + waiting: 0, + pending: nil, + subs: [], + filter: Filter.empty(), + pids: MapSet.new(), + inspector: Keyword.fetch!(opts, :inspector) + }} end @impl GenStage.Dispatcher diff --git a/packages/sync-service/test/electric/shapes/dispatcher_test.exs b/packages/sync-service/test/electric/shapes/dispatcher_test.exs index f1f8e0066d..bfdcad5a80 100644 --- a/packages/sync-service/test/electric/shapes/dispatcher_test.exs +++ b/packages/sync-service/test/electric/shapes/dispatcher_test.exs @@ -20,8 +20,8 @@ defmodule Electric.Shapes.DispatcherTest do ] } - defp dispatcher(opts \\ []) do - {:ok, state} = D.init(opts) + defp dispatcher() do + {:ok, state} = D.init(inspector: @inspector) state end From 55322a63ff74c359b01dc0b9fff1f757773e4421 Mon Sep 17 00:00:00 2001 From: rob Date: Fri, 29 Nov 2024 14:10:12 +0000 Subject: [PATCH 21/62] Add TODO --- packages/sync-service/lib/electric/shapes/dispatcher.ex | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/sync-service/lib/electric/shapes/dispatcher.ex b/packages/sync-service/lib/electric/shapes/dispatcher.ex index f944f51a89..bc2dc77641 100644 --- a/packages/sync-service/lib/electric/shapes/dispatcher.ex +++ b/packages/sync-service/lib/electric/shapes/dispatcher.ex @@ -50,6 +50,7 @@ defmodule Electric.Shapes.Dispatcher do subs: [], filter: Filter.empty(), pids: MapSet.new(), + # TODO: remove inspector as we don't need it anymore inspector: Keyword.fetch!(opts, :inspector) }} end From ecf54f9e46298c8d933a89d179e5e473b5f8bf93 Mon Sep 17 00:00:00 2001 From: rob Date: Fri, 29 Nov 2024 15:19:29 +0000 Subject: [PATCH 22/62] Parse record rather than converting the const to a string --- .../lib/electric/shapes/filter.ex | 79 +++-- .../electric/shapes/filter/table_filter.ex | 6 +- .../test/electric/shapes/filter_test.exs | 320 ++++++++++-------- 3 files changed, 237 insertions(+), 168 deletions(-) diff --git a/packages/sync-service/lib/electric/shapes/filter.ex b/packages/sync-service/lib/electric/shapes/filter.ex index bda02c505e..442a5052fd 100644 --- a/packages/sync-service/lib/electric/shapes/filter.ex +++ b/packages/sync-service/lib/electric/shapes/filter.ex @@ -5,6 +5,7 @@ defmodule Electric.Shapes.Filter do alias Electric.Replication.Changes.Transaction alias Electric.Replication.Changes.TruncatedRelation alias Electric.Replication.Changes.UpdatedRecord + alias Electric.Replication.Eval.Env alias Electric.Replication.Eval.Expr alias Electric.Replication.Eval.Parser alias Electric.Replication.Eval.Parser.Const @@ -46,15 +47,17 @@ defmodule Electric.Shapes.Filter do def empty, do: %Filter{} defp empty_table_filter, do: %{fields: %{}, other_shapes: %{}} + defp init_field_filter(type), do: %{type: type, values: %{}} defp add_shape_to_table_filter({handle, shape} = shape_instance, table_filter) do case optimise_where(shape.where) do - %{operation: "=", field: field, value: value, and_where: and_where} -> + %{operation: "=", field: field, type: type, value: value, and_where: and_where} -> %{ table_filter | fields: - add_shape_to_field_filter( + add_shape_to_fields( field, + type, value, shape_instance, table_filter.fields, @@ -67,45 +70,55 @@ defmodule Electric.Shapes.Filter do end end - defp add_shape_to_field_filter(field, value, shape_instance, fields, and_where) do + defp add_shape_to_fields(field, type, value, shape_instance, fields, and_where) do Map.update( fields, field, - add_shape_to_value_filter(value, shape_instance, and_where, %{}), - fn value_filter -> - add_shape_to_value_filter(value, shape_instance, and_where, value_filter) + add_shape_to_field_filter(value, shape_instance, and_where, init_field_filter(type)), + fn field_filter -> + add_shape_to_field_filter(value, shape_instance, and_where, field_filter) end ) end - defp add_shape_to_value_filter(value, {handle, shape}, and_where, value_filter) do - Map.update( - value_filter, - value, - [%{handle: handle, and_where: and_where, shape: shape}], - fn shapes -> [%{handle: handle, and_where: and_where, shape: shape} | shapes] end - ) + # TODO: Renmame handle to shape_id + defp add_shape_to_field_filter(value, {handle, shape}, and_where, field_filter) do + %{ + field_filter + | values: + Map.update( + field_filter.values, + value, + [%{handle: handle, and_where: and_where, shape: shape}], + fn shapes -> [%{handle: handle, and_where: and_where, shape: shape} | shapes] end + ) + } end defp optimise_where(%Expr{eval: eval}), do: optimise_where(eval) # TODO: Is this really ~s("=") or is it just "="? - # TODO: Is path really [field]? - defp optimise_where(%Func{name: ~s("="), args: [%Ref{path: [field]}, %Const{} = const]}) do - %{operation: "=", field: field, value: const_to_string(const), and_where: nil} + defp optimise_where(%Func{ + name: ~s("="), + args: [%Ref{path: [field], type: type}, %Const{value: value}] + }) do + %{operation: "=", field: field, type: type, value: value, and_where: nil} end - defp optimise_where(%Func{name: ~s("="), args: [%Const{} = const, %Ref{path: [field]}]}) do - %{operation: "=", field: field, value: const_to_string(const), and_where: nil} + defp optimise_where(%Func{ + name: ~s("="), + args: [%Const{value: value}, %Ref{path: [field], type: type}] + }) do + %{operation: "=", field: field, type: type, value: value, and_where: nil} end defp optimise_where(%Func{name: "and", args: [arg1, arg2]}) do case {optimise_where(arg1), optimise_where(arg2)} do - {%{operation: "=", field: field, value: value, and_where: nil}, _} -> - %{operation: "=", field: field, value: value, and_where: arg2} + {%{operation: "=", and_where: nil} = params, _} -> + %{params | and_where: arg2} - {_, %{operation: "=", field: field, value: value, and_where: nil}} -> - %{operation: "=", field: field, value: value, and_where: arg1} + {_, %{operation: "=", and_where: nil} = params} -> + %{params | and_where: arg1} _ -> :not_optimised @@ -114,10 +127,6 @@ defmodule Electric.Shapes.Filter do defp optimise_where(_), do: :not_optimised - # TODO: Impliment other types, or is this not implimented elsewhere? - defp const_to_string(%Const{value: value, type: :int4}), do: Integer.to_string(value) - defp const_to_string(%Const{value: value, type: :int8}), do: Integer.to_string(value) - def affected_shapes(%Filter{} = filter, %Relation{} = relation) do # Check all shapes is all tables becuase the table may have been renamed for {handle, shape} <- all_shapes_in_filter(filter), @@ -173,22 +182,28 @@ defmodule Electric.Shapes.Filter do |> MapSet.union(other_shapes_affected(table_filter, record)) end - def affected_shapes_by_field({field, values}, record) do - case values[record[field]] do + def affected_shapes_by_field({field, %{values: values, type: type}}, record) do + case values[value_from_record(record, field, type)] do nil -> MapSet.new() shapes -> shapes - |> Enum.filter(&record_in_where(&1.and_where, record)) + |> Enum.filter(&record_in_where?(&1.and_where, record)) |> Enum.map(& &1.handle) |> MapSet.new() end end - defp record_in_where(nil, _), do: true + defp value_from_record(record, field, type) do + # TODO: should we expect this to be ok? + {:ok, value} = Env.parse_const(Env.new(), record[field], type) + value + end + + defp record_in_where?(nil, _), do: true - defp record_in_where(where_clause, record) do + defp record_in_where?(where_clause, record) do # TODO: Move record_in_shape? out of shapes into Where module # Keep full Expr in shape Shape.record_in_shape?( @@ -215,7 +230,7 @@ defmodule Electric.Shapes.Filter do end defp all_shapes_in_table_filter(%{fields: fields, other_shapes: other_shapes}) do - for {_field, values} <- fields, + for {_field, %{values: values}} <- fields, {_value, shapes} <- values, %{handle: handle, shape: shape} <- shapes, into: %{} do diff --git a/packages/sync-service/lib/electric/shapes/filter/table_filter.ex b/packages/sync-service/lib/electric/shapes/filter/table_filter.ex index e4ac8cfe0b..4d7b721e6c 100644 --- a/packages/sync-service/lib/electric/shapes/filter/table_filter.ex +++ b/packages/sync-service/lib/electric/shapes/filter/table_filter.ex @@ -8,10 +8,10 @@ defmodule Electric.Shapes.Filter.TableFilter do defp remove_shape_from_fields(fields, handle) do fields - |> Map.new(fn {field, value_filter} -> - {field, remove_shape_from_value_filter(value_filter, handle)} + |> Map.new(fn {field, %{values: value_filter} = field_filter} -> + {field, %{field_filter | values: remove_shape_from_value_filter(value_filter, handle)}} end) - |> Enum.reject(fn {_field, value_filter} -> map_size(value_filter) == 0 end) + |> Enum.reject(fn {_field, %{values: value_filter}} -> map_size(value_filter) == 0 end) |> Map.new() end diff --git a/packages/sync-service/test/electric/shapes/filter_test.exs b/packages/sync-service/test/electric/shapes/filter_test.exs index 6186af9d88..964208e63d 100644 --- a/packages/sync-service/test/electric/shapes/filter_test.exs +++ b/packages/sync-service/test/electric/shapes/filter_test.exs @@ -24,9 +24,12 @@ defmodule Electric.Shapes.FilterTest do {"public", "the_table"} => %{ fields: %{ "id" => %{ - "1" => [ - %{handle: "shape1", and_where: nil, shape: shape} - ] + type: :int8, + values: %{ + 1 => [ + %{handle: "shape1", and_where: nil, shape: shape} + ] + } } }, other_shapes: %{} @@ -43,9 +46,12 @@ defmodule Electric.Shapes.FilterTest do {"public", "the_table"} => %{ fields: %{ "id" => %{ - "1" => [ - %{handle: "shape1", and_where: nil, shape: shape} - ] + type: :int8, + values: %{ + 1 => [ + %{handle: "shape1", and_where: nil, shape: shape} + ] + } } }, other_shapes: %{} @@ -62,19 +68,22 @@ defmodule Electric.Shapes.FilterTest do {"public", "the_table"} => %{ fields: %{ "id" => %{ - "1" => [ - %{ - handle: "shape1", - and_where: %Func{ - name: ~s(">"), - args: [ - %Ref{path: ["id"], type: :int8}, - %Const{value: 0, type: :int4} - ] - }, - shape: ^shape - } - ] + type: :int8, + values: %{ + 1 => [ + %{ + handle: "shape1", + and_where: %Func{ + name: ~s(">"), + args: [ + %Ref{path: ["id"], type: :int8}, + %Const{value: 0, type: :int4} + ] + }, + shape: ^shape + } + ] + } } }, other_shapes: %{} @@ -91,19 +100,22 @@ defmodule Electric.Shapes.FilterTest do {"public", "the_table"} => %{ fields: %{ "id" => %{ - "1" => [ - %{ - handle: "shape1", - and_where: %Func{ - name: ~s(">"), - args: [ - %Ref{path: ["id"], type: :int8}, - %Const{value: 0, type: :int4} - ] - }, - shape: ^shape - } - ] + type: :int8, + values: %{ + 1 => [ + %{ + handle: "shape1", + and_where: %Func{ + name: ~s(">"), + args: [ + %Ref{path: ["id"], type: :int8}, + %Const{value: 0, type: :int4} + ] + }, + shape: ^shape + } + ] + } } }, other_shapes: %{} @@ -133,30 +145,36 @@ defmodule Electric.Shapes.FilterTest do {"public", "the_table"} => %{ fields: %{ "id" => %{ - "1" => [ - %{ - handle: "shape1", - and_where: nil - } - ], - "2" => [ - %{ - handle: "shape2", - and_where: nil - } - ] + type: :int8, + values: %{ + 1 => [ + %{ + handle: "shape1", + and_where: nil + } + ], + 2 => [ + %{ + handle: "shape2", + and_where: nil + } + ] + } }, "name" => %{ - "bill" => [ - %{ - handle: "shape1", - and_where: nil - }, - %{ - handle: "shape2", - and_where: nil - } - ] + type: :text, + values: %{ + "bill" => [ + %{ + handle: "shape1", + and_where: nil + }, + %{ + handle: "shape2", + and_where: nil + } + ] + } } }, other_shapes: %{ @@ -167,9 +185,12 @@ defmodule Electric.Shapes.FilterTest do {"public", "another_table"} => %{ fields: %{ "id" => %{ - "1" => [ - %{handle: "shape1", and_where: nil} - ] + type: :int8, + values: %{ + 1 => [ + %{handle: "shape1", and_where: nil} + ] + } } }, other_shapes: %{ @@ -184,20 +205,26 @@ defmodule Electric.Shapes.FilterTest do {"public", "the_table"} => %{ fields: %{ "id" => %{ - "2" => [ - %{ - handle: "shape2", - and_where: nil - } - ] + type: :int8, + values: %{ + 2 => [ + %{ + handle: "shape2", + and_where: nil + } + ] + } }, "name" => %{ - "bill" => [ - %{ - handle: "shape2", - and_where: nil - } - ] + type: :text, + values: %{ + "bill" => [ + %{ + handle: "shape2", + and_where: nil + } + ] + } } }, other_shapes: %{ @@ -216,14 +243,17 @@ defmodule Electric.Shapes.FilterTest do {"public", "the_table"} => %{ fields: %{ "id" => %{ - "1" => [ - %{handle: "shape1", and_where: nil}, - %{handle: "shape2", and_where: nil} - ], - "2" => [ - %{handle: "shape3", and_where: nil}, - %{handle: "shape4", and_where: nil} - ] + type: :int8, + values: %{ + 1 => [ + %{handle: "shape1", and_where: nil}, + %{handle: "shape2", and_where: nil} + ], + 2 => [ + %{handle: "shape3", and_where: nil}, + %{handle: "shape4", and_where: nil} + ] + } } }, other_shapes: %{} @@ -231,9 +261,12 @@ defmodule Electric.Shapes.FilterTest do {"public", "another_table"} => %{ fields: %{ "id" => %{ - "1" => [ - %{handle: "shape5", and_where: nil} - ] + type: :int8, + values: %{ + 1 => [ + %{handle: "shape5", and_where: nil} + ] + } } }, other_shapes: %{} @@ -260,10 +293,13 @@ defmodule Electric.Shapes.FilterTest do {"public", "the_table"} => %{ fields: %{ "id" => %{ - "1" => [ - %{handle: "shape1", and_where: nil}, - %{handle: "shape2", and_where: nil} - ] + type: :int8, + values: %{ + 1 => [ + %{handle: "shape1", and_where: nil}, + %{handle: "shape2", and_where: nil} + ] + } } }, other_shapes: %{} @@ -317,9 +353,12 @@ defmodule Electric.Shapes.FilterTest do {"public", "the_table"} => %{ fields: %{ "id" => %{ - "1" => [ - %{handle: "the-shape", and_where: nil} - ] + type: :int8, + values: %{ + 1 => [ + %{handle: "the-shape", and_where: nil} + ] + } } }, other_shapes: %{} @@ -346,15 +385,18 @@ defmodule Electric.Shapes.FilterTest do {"public", "the_table"} => %{ fields: %{ "id" => %{ - "1" => [ - %{handle: "shape1", and_where: nil} - ], - "2" => [ - %{handle: "shape2", and_where: nil} - ], - "3" => [ - %{handle: "shape3", and_where: nil} - ] + type: :int8, + values: %{ + 1 => [ + %{handle: "shape1", and_where: nil} + ], + 2 => [ + %{handle: "shape2", and_where: nil} + ], + 3 => [ + %{handle: "shape3", and_where: nil} + ] + } } }, other_shapes: %{} @@ -382,20 +424,23 @@ defmodule Electric.Shapes.FilterTest do {"public", "the_table"} => %{ fields: %{ "id" => %{ - "1" => [ - %{ - handle: "shape1", - and_where: nil, - shape: Shape.new!("the_table", where: "id = 1", inspector: @inspector) - } - ], - "2" => [ - %{ - handle: "shape2", - and_where: nil, - shape: Shape.new!("the_table", where: "id = 2", inspector: @inspector) - } - ] + type: :int8, + values: %{ + 1 => [ + %{ + handle: "shape1", + and_where: nil, + shape: Shape.new!("the_table", where: "id = 1", inspector: @inspector) + } + ], + 2 => [ + %{ + handle: "shape2", + and_where: nil, + shape: Shape.new!("the_table", where: "id = 2", inspector: @inspector) + } + ] + } } }, other_shapes: %{ @@ -406,9 +451,12 @@ defmodule Electric.Shapes.FilterTest do {"public", "another_table"} => %{ fields: %{ "id" => %{ - "1" => [ - %{handle: "not-this-shape-1", and_where: nil} - ] + type: :int8, + values: %{ + 1 => [ + %{handle: "not-this-shape-1", and_where: nil} + ] + } } }, other_shapes: %{ @@ -431,8 +479,8 @@ defmodule Electric.Shapes.FilterTest do MapSet.new(["shape1", "shape2", "shape3", "shape4"]) end - #TODO: Also go through Shape.is_affected_by_relation_change? tests to see if all scenarious are covered here - #TODO: Also go through Shape.convert_change tests to see if all scenarious are covered here + # TODO: Also go through Shape.is_affected_by_relation_change? tests to see if all scenarious are covered here + # TODO: Also go through Shape.convert_change tests to see if all scenarious are covered here test "returns shapes affected by truncation" do filter = %Filter{ @@ -440,20 +488,23 @@ defmodule Electric.Shapes.FilterTest do {"public", "the_table"} => %{ fields: %{ "id" => %{ - "1" => [ - %{ - handle: "shape1", - and_where: nil, - shape: Shape.new!("the_table", where: "id = 1", inspector: @inspector) - } - ], - "2" => [ - %{ - handle: "shape2", - and_where: nil, - shape: Shape.new!("the_table", where: "id = 2", inspector: @inspector) - } - ] + type: :int8, + values: %{ + 1 => [ + %{ + handle: "shape1", + and_where: nil, + shape: Shape.new!("the_table", where: "id = 1", inspector: @inspector) + } + ], + 2 => [ + %{ + handle: "shape2", + and_where: nil, + shape: Shape.new!("the_table", where: "id = 2", inspector: @inspector) + } + ] + } } }, other_shapes: %{ @@ -463,10 +514,13 @@ defmodule Electric.Shapes.FilterTest do }, {"public", "another_table"} => %{ fields: %{ - "id" => %{ - "1" => [ - %{handle: "not-this-shape-1", and_where: nil} - ] + type: :int8, + values: %{ + "id" => %{ + 1 => [ + %{handle: "not-this-shape-1", and_where: nil} + ] + } } }, other_shapes: %{ From 33c8a8b7a1ed6e9f7d58cc225196c3ae61ea1638 Mon Sep 17 00:00:00 2001 From: rob Date: Fri, 29 Nov 2024 15:39:59 +0000 Subject: [PATCH 23/62] Create expr on Filter.new --- .../lib/electric/shapes/filter.ex | 17 +++++----- .../test/electric/shapes/filter_test.exs | 33 ++++++++++++------- 2 files changed, 30 insertions(+), 20 deletions(-) diff --git a/packages/sync-service/lib/electric/shapes/filter.ex b/packages/sync-service/lib/electric/shapes/filter.ex index 442a5052fd..caf264f08a 100644 --- a/packages/sync-service/lib/electric/shapes/filter.ex +++ b/packages/sync-service/lib/electric/shapes/filter.ex @@ -115,10 +115,10 @@ defmodule Electric.Shapes.Filter do defp optimise_where(%Func{name: "and", args: [arg1, arg2]}) do case {optimise_where(arg1), optimise_where(arg2)} do {%{operation: "=", and_where: nil} = params, _} -> - %{params | and_where: arg2} + %{params | and_where: where_expr(arg2)} {_, %{operation: "=", and_where: nil} = params} -> - %{params | and_where: arg1} + %{params | and_where: where_expr(arg1)} _ -> :not_optimised @@ -127,6 +127,10 @@ defmodule Electric.Shapes.Filter do defp optimise_where(_), do: :not_optimised + defp where_expr(eval) do + %Expr{eval: eval, used_refs: Parser.find_refs(eval), returns: :bool} + end + def affected_shapes(%Filter{} = filter, %Relation{} = relation) do # Check all shapes is all tables becuase the table may have been renamed for {handle, shape} <- all_shapes_in_filter(filter), @@ -195,9 +199,10 @@ defmodule Electric.Shapes.Filter do end end + @env Env.new() defp value_from_record(record, field, type) do # TODO: should we expect this to be ok? - {:ok, value} = Env.parse_const(Env.new(), record[field], type) + {:ok, value} = Env.parse_const(@env, record[field], type) value end @@ -205,11 +210,7 @@ defmodule Electric.Shapes.Filter do defp record_in_where?(where_clause, record) do # TODO: Move record_in_shape? out of shapes into Where module - # Keep full Expr in shape - Shape.record_in_shape?( - %{where: %Expr{eval: where_clause, used_refs: Parser.find_refs(where_clause)}}, - record - ) + Shape.record_in_shape?(%{where: where_clause}, record) end defp other_shapes_affected(%{other_shapes: shapes}, record) do diff --git a/packages/sync-service/test/electric/shapes/filter_test.exs b/packages/sync-service/test/electric/shapes/filter_test.exs index 964208e63d..a0ce8385cc 100644 --- a/packages/sync-service/test/electric/shapes/filter_test.exs +++ b/packages/sync-service/test/electric/shapes/filter_test.exs @@ -6,6 +6,7 @@ defmodule Electric.Shapes.FilterTest do alias Electric.Replication.Changes.Transaction alias Electric.Replication.Changes.TruncatedRelation alias Electric.Replication.Changes.UpdatedRecord + alias Electric.Replication.Eval.Expr alias Electric.Replication.Eval.Parser.Const alias Electric.Replication.Eval.Parser.Func alias Electric.Replication.Eval.Parser.Ref @@ -73,12 +74,16 @@ defmodule Electric.Shapes.FilterTest do 1 => [ %{ handle: "shape1", - and_where: %Func{ - name: ~s(">"), - args: [ - %Ref{path: ["id"], type: :int8}, - %Const{value: 0, type: :int4} - ] + and_where: %Expr{ + eval: %Func{ + name: ~s(">"), + args: [ + %Ref{path: ["id"], type: :int8}, + %Const{value: 0, type: :int4} + ] + }, + used_refs: %{["id"] => :int8}, + returns: :bool }, shape: ^shape } @@ -105,12 +110,16 @@ defmodule Electric.Shapes.FilterTest do 1 => [ %{ handle: "shape1", - and_where: %Func{ - name: ~s(">"), - args: [ - %Ref{path: ["id"], type: :int8}, - %Const{value: 0, type: :int4} - ] + and_where: %Expr{ + eval: %Func{ + name: ~s(">"), + args: [ + %Ref{path: ["id"], type: :int8}, + %Const{value: 0, type: :int4} + ] + }, + used_refs: %{["id"] => :int8}, + returns: :bool }, shape: ^shape } From 684cd47ed6927359a13fec2d0ed6288e1a1ddf8e Mon Sep 17 00:00:00 2001 From: rob Date: Sat, 30 Nov 2024 20:29:33 +0000 Subject: [PATCH 24/62] Move functions into TableFilter --- .../lib/electric/shapes/filter.ex | 144 +----------------- .../electric/shapes/filter/table_filter.ex | 136 +++++++++++++++++ 2 files changed, 144 insertions(+), 136 deletions(-) diff --git a/packages/sync-service/lib/electric/shapes/filter.ex b/packages/sync-service/lib/electric/shapes/filter.ex index caf264f08a..c1f3709251 100644 --- a/packages/sync-service/lib/electric/shapes/filter.ex +++ b/packages/sync-service/lib/electric/shapes/filter.ex @@ -5,12 +5,6 @@ defmodule Electric.Shapes.Filter do alias Electric.Replication.Changes.Transaction alias Electric.Replication.Changes.TruncatedRelation alias Electric.Replication.Changes.UpdatedRecord - alias Electric.Replication.Eval.Env - alias Electric.Replication.Eval.Expr - alias Electric.Replication.Eval.Parser - alias Electric.Replication.Eval.Parser.Const - alias Electric.Replication.Eval.Parser.Func - alias Electric.Replication.Eval.Parser.Ref alias Electric.Shapes.Filter alias Electric.Shapes.Filter.TableFilter alias Electric.Shapes.Shape @@ -27,8 +21,13 @@ defmodule Electric.Shapes.Filter do Map.update( tables, shape.root_table, - add_shape_to_table_filter({handle, shape}, empty_table_filter()), - fn table_filter -> add_shape_to_table_filter({handle, shape}, table_filter) end + TableFilter.add_shape_to_table_filter( + {handle, shape}, + TableFilter.empty_table_filter() + ), + fn table_filter -> + TableFilter.add_shape_to_table_filter({handle, shape}, table_filter) + end ) } end @@ -46,90 +45,6 @@ defmodule Electric.Shapes.Filter do end def empty, do: %Filter{} - defp empty_table_filter, do: %{fields: %{}, other_shapes: %{}} - defp init_field_filter(type), do: %{type: type, values: %{}} - - defp add_shape_to_table_filter({handle, shape} = shape_instance, table_filter) do - case optimise_where(shape.where) do - %{operation: "=", field: field, type: type, value: value, and_where: and_where} -> - %{ - table_filter - | fields: - add_shape_to_fields( - field, - type, - value, - shape_instance, - table_filter.fields, - and_where - ) - } - - :not_optimised -> - %{table_filter | other_shapes: Map.put(table_filter.other_shapes, handle, shape)} - end - end - - defp add_shape_to_fields(field, type, value, shape_instance, fields, and_where) do - Map.update( - fields, - field, - add_shape_to_field_filter(value, shape_instance, and_where, init_field_filter(type)), - fn field_filter -> - add_shape_to_field_filter(value, shape_instance, and_where, field_filter) - end - ) - end - - # TODO: Renmame handle to shape_id - defp add_shape_to_field_filter(value, {handle, shape}, and_where, field_filter) do - %{ - field_filter - | values: - Map.update( - field_filter.values, - value, - [%{handle: handle, and_where: and_where, shape: shape}], - fn shapes -> [%{handle: handle, and_where: and_where, shape: shape} | shapes] end - ) - } - end - - defp optimise_where(%Expr{eval: eval}), do: optimise_where(eval) - - # TODO: Is this really ~s("=") or is it just "="? - defp optimise_where(%Func{ - name: ~s("="), - args: [%Ref{path: [field], type: type}, %Const{value: value}] - }) do - %{operation: "=", field: field, type: type, value: value, and_where: nil} - end - - defp optimise_where(%Func{ - name: ~s("="), - args: [%Const{value: value}, %Ref{path: [field], type: type}] - }) do - %{operation: "=", field: field, type: type, value: value, and_where: nil} - end - - defp optimise_where(%Func{name: "and", args: [arg1, arg2]}) do - case {optimise_where(arg1), optimise_where(arg2)} do - {%{operation: "=", and_where: nil} = params, _} -> - %{params | and_where: where_expr(arg2)} - - {_, %{operation: "=", and_where: nil} = params} -> - %{params | and_where: where_expr(arg1)} - - _ -> - :not_optimised - end - end - - defp optimise_where(_), do: :not_optimised - - defp where_expr(eval) do - %Expr{eval: eval, used_refs: Parser.find_refs(eval), returns: :bool} - end def affected_shapes(%Filter{} = filter, %Relation{} = relation) do # Check all shapes is all tables becuase the table may have been renamed @@ -175,50 +90,7 @@ defmodule Electric.Shapes.Filter do defp affected_shapes_by_record(filter, table, record) do case Map.get(filter.tables, table) do nil -> MapSet.new() - table_filter -> affected_shapes_by_table(table_filter, record) - end - end - - defp affected_shapes_by_table(%{fields: fields} = table_filter, record) do - fields - |> Enum.map(&affected_shapes_by_field(&1, record)) - |> Enum.reduce(MapSet.new(), &MapSet.union(&1, &2)) - |> MapSet.union(other_shapes_affected(table_filter, record)) - end - - def affected_shapes_by_field({field, %{values: values, type: type}}, record) do - case values[value_from_record(record, field, type)] do - nil -> - MapSet.new() - - shapes -> - shapes - |> Enum.filter(&record_in_where?(&1.and_where, record)) - |> Enum.map(& &1.handle) - |> MapSet.new() - end - end - - @env Env.new() - defp value_from_record(record, field, type) do - # TODO: should we expect this to be ok? - {:ok, value} = Env.parse_const(@env, record[field], type) - value - end - - defp record_in_where?(nil, _), do: true - - defp record_in_where?(where_clause, record) do - # TODO: Move record_in_shape? out of shapes into Where module - Shape.record_in_shape?(%{where: where_clause}, record) - end - - defp other_shapes_affected(%{other_shapes: shapes}, record) do - for {handle, shape} <- shapes, - # TODO: Test Shape.record_in_shape? is called - Shape.record_in_shape?(shape, record), - into: MapSet.new() do - handle + table_filter -> TableFilter.affected_shapes_by_table(table_filter, record) end end diff --git a/packages/sync-service/lib/electric/shapes/filter/table_filter.ex b/packages/sync-service/lib/electric/shapes/filter/table_filter.ex index 4d7b721e6c..2416a29602 100644 --- a/packages/sync-service/lib/electric/shapes/filter/table_filter.ex +++ b/packages/sync-service/lib/electric/shapes/filter/table_filter.ex @@ -1,4 +1,97 @@ defmodule Electric.Shapes.Filter.TableFilter do + alias Electric.Replication.Eval.Env + alias Electric.Replication.Eval.Expr + alias Electric.Replication.Eval.Parser + alias Electric.Replication.Eval.Parser.Const + alias Electric.Replication.Eval.Parser.Func + alias Electric.Replication.Eval.Parser.Ref + alias Electric.Shapes.Shape + + def empty_table_filter, do: %{fields: %{}, other_shapes: %{}} + def init_field_filter(type), do: %{type: type, values: %{}} + + def add_shape_to_table_filter({handle, shape} = shape_instance, table_filter) do + case optimise_where(shape.where) do + %{operation: "=", field: field, type: type, value: value, and_where: and_where} -> + %{ + table_filter + | fields: + add_shape_to_fields( + field, + type, + value, + shape_instance, + table_filter.fields, + and_where + ) + } + + :not_optimised -> + %{table_filter | other_shapes: Map.put(table_filter.other_shapes, handle, shape)} + end + end + + defp add_shape_to_fields(field, type, value, shape_instance, fields, and_where) do + Map.update( + fields, + field, + add_shape_to_field_filter(value, shape_instance, and_where, init_field_filter(type)), + fn field_filter -> + add_shape_to_field_filter(value, shape_instance, and_where, field_filter) + end + ) + end + + # TODO: Renmame handle to shape_id + defp add_shape_to_field_filter(value, {handle, shape}, and_where, field_filter) do + %{ + field_filter + | values: + Map.update( + field_filter.values, + value, + [%{handle: handle, and_where: and_where, shape: shape}], + fn shapes -> [%{handle: handle, and_where: and_where, shape: shape} | shapes] end + ) + } + end + + defp optimise_where(%Expr{eval: eval}), do: optimise_where(eval) + + # TODO: Is this really ~s("=") or is it just "="? + defp optimise_where(%Func{ + name: ~s("="), + args: [%Ref{path: [field], type: type}, %Const{value: value}] + }) do + %{operation: "=", field: field, type: type, value: value, and_where: nil} + end + + defp optimise_where(%Func{ + name: ~s("="), + args: [%Const{value: value}, %Ref{path: [field], type: type}] + }) do + %{operation: "=", field: field, type: type, value: value, and_where: nil} + end + + defp optimise_where(%Func{name: "and", args: [arg1, arg2]}) do + case {optimise_where(arg1), optimise_where(arg2)} do + {%{operation: "=", and_where: nil} = params, _} -> + %{params | and_where: where_expr(arg2)} + + {_, %{operation: "=", and_where: nil} = params} -> + %{params | and_where: where_expr(arg1)} + + _ -> + :not_optimised + end + end + + defp optimise_where(_), do: :not_optimised + + defp where_expr(eval) do + %Expr{eval: eval, used_refs: Parser.find_refs(eval), returns: :bool} + end + def remove_shape(%{fields: fields, other_shapes: other_shapes}, handle) do %{ fields: remove_shape_from_fields(fields, handle), @@ -21,4 +114,47 @@ defmodule Electric.Shapes.Filter.TableFilter do |> Enum.reject(fn {_value, shapes} -> shapes == [] end) |> Map.new() end + + def affected_shapes_by_table(%{fields: fields} = table_filter, record) do + fields + |> Enum.map(&affected_shapes_by_field(&1, record)) + |> Enum.reduce(MapSet.new(), &MapSet.union(&1, &2)) + |> MapSet.union(other_shapes_affected(table_filter, record)) + end + + def affected_shapes_by_field({field, %{values: values, type: type}}, record) do + case values[value_from_record(record, field, type)] do + nil -> + MapSet.new() + + shapes -> + shapes + |> Enum.filter(&record_in_where?(&1.and_where, record)) + |> Enum.map(& &1.handle) + |> MapSet.new() + end + end + + @env Env.new() + defp value_from_record(record, field, type) do + # TODO: should we expect this to be ok? + {:ok, value} = Env.parse_const(@env, record[field], type) + value + end + + defp record_in_where?(nil, _), do: true + + defp record_in_where?(where_clause, record) do + # TODO: Move record_in_shape? out of shapes into Where module + Shape.record_in_shape?(%{where: where_clause}, record) + end + + defp other_shapes_affected(%{other_shapes: shapes}, record) do + for {handle, shape} <- shapes, + # TODO: Test Shape.record_in_shape? is called + Shape.record_in_shape?(shape, record), + into: MapSet.new() do + handle + end + end end From 04c04e9eeae2dc2153e044e943790b6ab0674ee5 Mon Sep 17 00:00:00 2001 From: rob Date: Sat, 30 Nov 2024 20:31:13 +0000 Subject: [PATCH 25/62] Rename TableFilter to Table --- packages/sync-service/lib/electric/shapes/filter.ex | 12 ++++++------ .../shapes/filter/{table_filter.ex => table.ex} | 2 +- 2 files changed, 7 insertions(+), 7 deletions(-) rename packages/sync-service/lib/electric/shapes/filter/{table_filter.ex => table.ex} (99%) diff --git a/packages/sync-service/lib/electric/shapes/filter.ex b/packages/sync-service/lib/electric/shapes/filter.ex index c1f3709251..886d1b03e9 100644 --- a/packages/sync-service/lib/electric/shapes/filter.ex +++ b/packages/sync-service/lib/electric/shapes/filter.ex @@ -6,7 +6,7 @@ defmodule Electric.Shapes.Filter do alias Electric.Replication.Changes.TruncatedRelation alias Electric.Replication.Changes.UpdatedRecord alias Electric.Shapes.Filter - alias Electric.Shapes.Filter.TableFilter + alias Electric.Shapes.Filter.Table alias Electric.Shapes.Shape defstruct tables: %{} @@ -21,12 +21,12 @@ defmodule Electric.Shapes.Filter do Map.update( tables, shape.root_table, - TableFilter.add_shape_to_table_filter( + Table.add_shape_to_table_filter( {handle, shape}, - TableFilter.empty_table_filter() + Table.empty_table_filter() ), fn table_filter -> - TableFilter.add_shape_to_table_filter({handle, shape}, table_filter) + Table.add_shape_to_table_filter({handle, shape}, table_filter) end ) } @@ -37,7 +37,7 @@ defmodule Electric.Shapes.Filter do tables: tables |> Enum.map(fn {table, table_filter} -> - {table, TableFilter.remove_shape(table_filter, handle)} + {table, Table.remove_shape(table_filter, handle)} end) |> Enum.reject(fn {_table, table_filter} -> map_size(table_filter.fields) == 0 end) |> Map.new() @@ -90,7 +90,7 @@ defmodule Electric.Shapes.Filter do defp affected_shapes_by_record(filter, table, record) do case Map.get(filter.tables, table) do nil -> MapSet.new() - table_filter -> TableFilter.affected_shapes_by_table(table_filter, record) + table_filter -> Table.affected_shapes_by_table(table_filter, record) end end diff --git a/packages/sync-service/lib/electric/shapes/filter/table_filter.ex b/packages/sync-service/lib/electric/shapes/filter/table.ex similarity index 99% rename from packages/sync-service/lib/electric/shapes/filter/table_filter.ex rename to packages/sync-service/lib/electric/shapes/filter/table.ex index 2416a29602..afd42faa8d 100644 --- a/packages/sync-service/lib/electric/shapes/filter/table_filter.ex +++ b/packages/sync-service/lib/electric/shapes/filter/table.ex @@ -1,4 +1,4 @@ -defmodule Electric.Shapes.Filter.TableFilter do +defmodule Electric.Shapes.Filter.Table do alias Electric.Replication.Eval.Env alias Electric.Replication.Eval.Expr alias Electric.Replication.Eval.Parser From ac11f0dcbdc19d53db50d3e804d543308cdacb3b Mon Sep 17 00:00:00 2001 From: rob Date: Sat, 30 Nov 2024 20:35:17 +0000 Subject: [PATCH 26/62] Rename public Table functions --- packages/sync-service/lib/electric/shapes/filter.ex | 9 +++------ .../sync-service/lib/electric/shapes/filter/table.ex | 6 +++--- 2 files changed, 6 insertions(+), 9 deletions(-) diff --git a/packages/sync-service/lib/electric/shapes/filter.ex b/packages/sync-service/lib/electric/shapes/filter.ex index 886d1b03e9..21ce222961 100644 --- a/packages/sync-service/lib/electric/shapes/filter.ex +++ b/packages/sync-service/lib/electric/shapes/filter.ex @@ -21,12 +21,9 @@ defmodule Electric.Shapes.Filter do Map.update( tables, shape.root_table, - Table.add_shape_to_table_filter( - {handle, shape}, - Table.empty_table_filter() - ), + Table.add_shape({handle, shape}, Table.empty()), fn table_filter -> - Table.add_shape_to_table_filter({handle, shape}, table_filter) + Table.add_shape({handle, shape}, table_filter) end ) } @@ -90,7 +87,7 @@ defmodule Electric.Shapes.Filter do defp affected_shapes_by_record(filter, table, record) do case Map.get(filter.tables, table) do nil -> MapSet.new() - table_filter -> Table.affected_shapes_by_table(table_filter, record) + table_filter -> Table.affected_shapes(table_filter, record) end end diff --git a/packages/sync-service/lib/electric/shapes/filter/table.ex b/packages/sync-service/lib/electric/shapes/filter/table.ex index afd42faa8d..db4b746fae 100644 --- a/packages/sync-service/lib/electric/shapes/filter/table.ex +++ b/packages/sync-service/lib/electric/shapes/filter/table.ex @@ -7,10 +7,10 @@ defmodule Electric.Shapes.Filter.Table do alias Electric.Replication.Eval.Parser.Ref alias Electric.Shapes.Shape - def empty_table_filter, do: %{fields: %{}, other_shapes: %{}} + def empty, do: %{fields: %{}, other_shapes: %{}} def init_field_filter(type), do: %{type: type, values: %{}} - def add_shape_to_table_filter({handle, shape} = shape_instance, table_filter) do + def add_shape({handle, shape} = shape_instance, table_filter) do case optimise_where(shape.where) do %{operation: "=", field: field, type: type, value: value, and_where: and_where} -> %{ @@ -115,7 +115,7 @@ defmodule Electric.Shapes.Filter.Table do |> Map.new() end - def affected_shapes_by_table(%{fields: fields} = table_filter, record) do + def affected_shapes(%{fields: fields} = table_filter, record) do fields |> Enum.map(&affected_shapes_by_field(&1, record)) |> Enum.reduce(MapSet.new(), &MapSet.union(&1, &2)) From 3db997015fdc250b3daaf2c090d380fcd7dc8774 Mon Sep 17 00:00:00 2001 From: rob Date: Sat, 30 Nov 2024 20:43:52 +0000 Subject: [PATCH 27/62] Introduce Table struct --- .../lib/electric/shapes/filter/table.ex | 19 ++++++---- .../test/electric/shapes/filter_test.exs | 37 ++++++++++--------- 2 files changed, 30 insertions(+), 26 deletions(-) diff --git a/packages/sync-service/lib/electric/shapes/filter/table.ex b/packages/sync-service/lib/electric/shapes/filter/table.ex index db4b746fae..13366f2de6 100644 --- a/packages/sync-service/lib/electric/shapes/filter/table.ex +++ b/packages/sync-service/lib/electric/shapes/filter/table.ex @@ -5,29 +5,32 @@ defmodule Electric.Shapes.Filter.Table do alias Electric.Replication.Eval.Parser.Const alias Electric.Replication.Eval.Parser.Func alias Electric.Replication.Eval.Parser.Ref + alias Electric.Shapes.Filter.Table alias Electric.Shapes.Shape - def empty, do: %{fields: %{}, other_shapes: %{}} + defstruct fields: %{}, other_shapes: %{} + + def empty, do: %Table{} def init_field_filter(type), do: %{type: type, values: %{}} - def add_shape({handle, shape} = shape_instance, table_filter) do + def add_shape({handle, shape} = shape_instance, table) do case optimise_where(shape.where) do %{operation: "=", field: field, type: type, value: value, and_where: and_where} -> %{ - table_filter + table | fields: add_shape_to_fields( field, type, value, shape_instance, - table_filter.fields, + table.fields, and_where ) } :not_optimised -> - %{table_filter | other_shapes: Map.put(table_filter.other_shapes, handle, shape)} + %{table | other_shapes: Map.put(table.other_shapes, handle, shape)} end end @@ -93,7 +96,7 @@ defmodule Electric.Shapes.Filter.Table do end def remove_shape(%{fields: fields, other_shapes: other_shapes}, handle) do - %{ + %Table{ fields: remove_shape_from_fields(fields, handle), other_shapes: Map.delete(other_shapes, handle) } @@ -115,11 +118,11 @@ defmodule Electric.Shapes.Filter.Table do |> Map.new() end - def affected_shapes(%{fields: fields} = table_filter, record) do + def affected_shapes(%{fields: fields} = table, record) do fields |> Enum.map(&affected_shapes_by_field(&1, record)) |> Enum.reduce(MapSet.new(), &MapSet.union(&1, &2)) - |> MapSet.union(other_shapes_affected(table_filter, record)) + |> MapSet.union(other_shapes_affected(table, record)) end def affected_shapes_by_field({field, %{values: values, type: type}}, record) do diff --git a/packages/sync-service/test/electric/shapes/filter_test.exs b/packages/sync-service/test/electric/shapes/filter_test.exs index a0ce8385cc..ec41ddb773 100644 --- a/packages/sync-service/test/electric/shapes/filter_test.exs +++ b/packages/sync-service/test/electric/shapes/filter_test.exs @@ -11,6 +11,7 @@ defmodule Electric.Shapes.FilterTest do alias Electric.Replication.Eval.Parser.Func alias Electric.Replication.Eval.Parser.Ref alias Electric.Shapes.Filter + alias Electric.Shapes.Filter.Table alias Electric.Shapes.Shape alias Support.StubInspector @@ -22,7 +23,7 @@ defmodule Electric.Shapes.FilterTest do assert Filter.new(%{"shape1" => shape}) == %Filter{ tables: %{ - {"public", "the_table"} => %{ + {"public", "the_table"} => %Table{ fields: %{ "id" => %{ type: :int8, @@ -44,7 +45,7 @@ defmodule Electric.Shapes.FilterTest do assert Filter.new(%{"shape1" => shape}) == %Filter{ tables: %{ - {"public", "the_table"} => %{ + {"public", "the_table"} => %Table{ fields: %{ "id" => %{ type: :int8, @@ -66,7 +67,7 @@ defmodule Electric.Shapes.FilterTest do assert %Filter{ tables: %{ - {"public", "the_table"} => %{ + {"public", "the_table"} => %Table{ fields: %{ "id" => %{ type: :int8, @@ -102,7 +103,7 @@ defmodule Electric.Shapes.FilterTest do assert %Filter{ tables: %{ - {"public", "the_table"} => %{ + {"public", "the_table"} => %Table{ fields: %{ "id" => %{ type: :int8, @@ -138,7 +139,7 @@ defmodule Electric.Shapes.FilterTest do assert Filter.new(shapes) == %Filter{ tables: %{ - {"public", "the_table"} => %{ + {"public", "the_table"} => %Table{ fields: %{}, other_shapes: shapes } @@ -151,7 +152,7 @@ defmodule Electric.Shapes.FilterTest do test "removes all shapes with the specified handle" do filter = %Filter{ tables: %{ - {"public", "the_table"} => %{ + {"public", "the_table"} => %Table{ fields: %{ "id" => %{ type: :int8, @@ -191,7 +192,7 @@ defmodule Electric.Shapes.FilterTest do "shape2" => Shape.new!("the_table", where: "id = 2", inspector: @inspector) } }, - {"public", "another_table"} => %{ + {"public", "another_table"} => %Table{ fields: %{ "id" => %{ type: :int8, @@ -211,7 +212,7 @@ defmodule Electric.Shapes.FilterTest do assert Filter.remove_shape(filter, "shape1") == %Filter{ tables: %{ - {"public", "the_table"} => %{ + {"public", "the_table"} => %Table{ fields: %{ "id" => %{ type: :int8, @@ -249,7 +250,7 @@ defmodule Electric.Shapes.FilterTest do test "shapes with same table and id are returned" do filter = %Filter{ tables: %{ - {"public", "the_table"} => %{ + {"public", "the_table"} => %Table{ fields: %{ "id" => %{ type: :int8, @@ -267,7 +268,7 @@ defmodule Electric.Shapes.FilterTest do }, other_shapes: %{} }, - {"public", "another_table"} => %{ + {"public", "another_table"} => %Table{ fields: %{ "id" => %{ type: :int8, @@ -299,7 +300,7 @@ defmodule Electric.Shapes.FilterTest do test "shapes with same table but different id are not returned" do filter = %Filter{ tables: %{ - {"public", "the_table"} => %{ + {"public", "the_table"} => %Table{ fields: %{ "id" => %{ type: :int8, @@ -332,7 +333,7 @@ defmodule Electric.Shapes.FilterTest do test "shapes with more complicated where clauses are evaluated" do filter = %Filter{ tables: %{ - {"public", "the_table"} => %{ + {"public", "the_table"} => %Table{ fields: %{}, other_shapes: %{ "shape1" => Shape.new!("the_table", where: "id > 7", inspector: @inspector), @@ -359,7 +360,7 @@ defmodule Electric.Shapes.FilterTest do test "returns shapes affected by delete" do filter = %Filter{ tables: %{ - {"public", "the_table"} => %{ + {"public", "the_table"} => %Table{ fields: %{ "id" => %{ type: :int8, @@ -391,7 +392,7 @@ defmodule Electric.Shapes.FilterTest do test "returns shapes affected by update" do filter = %Filter{ tables: %{ - {"public", "the_table"} => %{ + {"public", "the_table"} => %Table{ fields: %{ "id" => %{ type: :int8, @@ -430,7 +431,7 @@ defmodule Electric.Shapes.FilterTest do test "returns shapes affected by relation change" do filter = %Filter{ tables: %{ - {"public", "the_table"} => %{ + {"public", "the_table"} => %Table{ fields: %{ "id" => %{ type: :int8, @@ -457,7 +458,7 @@ defmodule Electric.Shapes.FilterTest do "shape4" => Shape.new!("the_table", where: "id > 6", inspector: @inspector) } }, - {"public", "another_table"} => %{ + {"public", "another_table"} => %Table{ fields: %{ "id" => %{ type: :int8, @@ -494,7 +495,7 @@ defmodule Electric.Shapes.FilterTest do test "returns shapes affected by truncation" do filter = %Filter{ tables: %{ - {"public", "the_table"} => %{ + {"public", "the_table"} => %Table{ fields: %{ "id" => %{ type: :int8, @@ -521,7 +522,7 @@ defmodule Electric.Shapes.FilterTest do "shape4" => Shape.new!("the_table", where: "id > 6", inspector: @inspector) } }, - {"public", "another_table"} => %{ + {"public", "another_table"} => %Table{ fields: %{ type: :int8, values: %{ From 753808740e0733cf68ddc4bb43963ec0d2476205 Mon Sep 17 00:00:00 2001 From: rob Date: Sat, 30 Nov 2024 21:00:55 +0000 Subject: [PATCH 28/62] Extract code into Field --- .../lib/electric/shapes/filter/field.ex | 56 +++++++++++++++++ .../lib/electric/shapes/filter/table.ex | 60 ++----------------- 2 files changed, 62 insertions(+), 54 deletions(-) create mode 100644 packages/sync-service/lib/electric/shapes/filter/field.ex diff --git a/packages/sync-service/lib/electric/shapes/filter/field.ex b/packages/sync-service/lib/electric/shapes/filter/field.ex new file mode 100644 index 0000000000..e3215225c0 --- /dev/null +++ b/packages/sync-service/lib/electric/shapes/filter/field.ex @@ -0,0 +1,56 @@ +defmodule Electric.Shapes.Filter.Field do + alias Electric.Replication.Eval.Env + alias Electric.Shapes.Shape + + defstruct fields: %{}, other_shapes: %{} + + def new(type), do: %{type: type, values: %{}} + + # TODO: Renmame handle to shape_id + def add_shape(value, {handle, shape}, and_where, field_filter) do + %{ + field_filter + | values: + Map.update( + field_filter.values, + value, + [%{handle: handle, and_where: and_where, shape: shape}], + fn shapes -> [%{handle: handle, and_where: and_where, shape: shape} | shapes] end + ) + } + end + + def remove_shape_from_value_filter(value_filter, handle) do + value_filter + |> Map.new(fn {value, shapes} -> {value, shapes |> Enum.reject(&(&1.handle == handle))} end) + |> Enum.reject(fn {_value, shapes} -> shapes == [] end) + |> Map.new() + end + + def affected_shapes({field, %{values: values, type: type}}, record) do + case values[value_from_record(record, field, type)] do + nil -> + MapSet.new() + + shapes -> + shapes + |> Enum.filter(&record_in_where?(&1.and_where, record)) + |> Enum.map(& &1.handle) + |> MapSet.new() + end + end + + @env Env.new() + defp value_from_record(record, field, type) do + # TODO: should we expect this to be ok? + {:ok, value} = Env.parse_const(@env, record[field], type) + value + end + + defp record_in_where?(nil, _), do: true + + defp record_in_where?(where_clause, record) do + # TODO: Move record_in_shape? out of shapes into Where module + Shape.record_in_shape?(%{where: where_clause}, record) + end +end diff --git a/packages/sync-service/lib/electric/shapes/filter/table.ex b/packages/sync-service/lib/electric/shapes/filter/table.ex index 13366f2de6..cdb146f31f 100644 --- a/packages/sync-service/lib/electric/shapes/filter/table.ex +++ b/packages/sync-service/lib/electric/shapes/filter/table.ex @@ -1,17 +1,16 @@ defmodule Electric.Shapes.Filter.Table do - alias Electric.Replication.Eval.Env alias Electric.Replication.Eval.Expr alias Electric.Replication.Eval.Parser alias Electric.Replication.Eval.Parser.Const alias Electric.Replication.Eval.Parser.Func alias Electric.Replication.Eval.Parser.Ref + alias Electric.Shapes.Filter.Field alias Electric.Shapes.Filter.Table alias Electric.Shapes.Shape defstruct fields: %{}, other_shapes: %{} def empty, do: %Table{} - def init_field_filter(type), do: %{type: type, values: %{}} def add_shape({handle, shape} = shape_instance, table) do case optimise_where(shape.where) do @@ -38,27 +37,13 @@ defmodule Electric.Shapes.Filter.Table do Map.update( fields, field, - add_shape_to_field_filter(value, shape_instance, and_where, init_field_filter(type)), + Field.add_shape(value, shape_instance, and_where, Field.new(type)), fn field_filter -> - add_shape_to_field_filter(value, shape_instance, and_where, field_filter) + Field.add_shape(value, shape_instance, and_where, field_filter) end ) end - # TODO: Renmame handle to shape_id - defp add_shape_to_field_filter(value, {handle, shape}, and_where, field_filter) do - %{ - field_filter - | values: - Map.update( - field_filter.values, - value, - [%{handle: handle, and_where: and_where, shape: shape}], - fn shapes -> [%{handle: handle, and_where: and_where, shape: shape} | shapes] end - ) - } - end - defp optimise_where(%Expr{eval: eval}), do: optimise_where(eval) # TODO: Is this really ~s("=") or is it just "="? @@ -105,53 +90,20 @@ defmodule Electric.Shapes.Filter.Table do defp remove_shape_from_fields(fields, handle) do fields |> Map.new(fn {field, %{values: value_filter} = field_filter} -> - {field, %{field_filter | values: remove_shape_from_value_filter(value_filter, handle)}} + {field, + %{field_filter | values: Field.remove_shape_from_value_filter(value_filter, handle)}} end) |> Enum.reject(fn {_field, %{values: value_filter}} -> map_size(value_filter) == 0 end) |> Map.new() end - defp remove_shape_from_value_filter(value_filter, handle) do - value_filter - |> Map.new(fn {value, shapes} -> {value, shapes |> Enum.reject(&(&1.handle == handle))} end) - |> Enum.reject(fn {_value, shapes} -> shapes == [] end) - |> Map.new() - end - def affected_shapes(%{fields: fields} = table, record) do fields - |> Enum.map(&affected_shapes_by_field(&1, record)) + |> Enum.map(&Field.affected_shapes(&1, record)) |> Enum.reduce(MapSet.new(), &MapSet.union(&1, &2)) |> MapSet.union(other_shapes_affected(table, record)) end - def affected_shapes_by_field({field, %{values: values, type: type}}, record) do - case values[value_from_record(record, field, type)] do - nil -> - MapSet.new() - - shapes -> - shapes - |> Enum.filter(&record_in_where?(&1.and_where, record)) - |> Enum.map(& &1.handle) - |> MapSet.new() - end - end - - @env Env.new() - defp value_from_record(record, field, type) do - # TODO: should we expect this to be ok? - {:ok, value} = Env.parse_const(@env, record[field], type) - value - end - - defp record_in_where?(nil, _), do: true - - defp record_in_where?(where_clause, record) do - # TODO: Move record_in_shape? out of shapes into Where module - Shape.record_in_shape?(%{where: where_clause}, record) - end - defp other_shapes_affected(%{other_shapes: shapes}, record) do for {handle, shape} <- shapes, # TODO: Test Shape.record_in_shape? is called From 8a484ab2798bde86491fad936af49a78e2b47e7d Mon Sep 17 00:00:00 2001 From: rob Date: Sat, 30 Nov 2024 21:11:15 +0000 Subject: [PATCH 29/62] Rename table_filter to table --- .../lib/electric/shapes/filter.ex | 34 +++++++++---------- 1 file changed, 17 insertions(+), 17 deletions(-) diff --git a/packages/sync-service/lib/electric/shapes/filter.ex b/packages/sync-service/lib/electric/shapes/filter.ex index 21ce222961..2a87a72d16 100644 --- a/packages/sync-service/lib/electric/shapes/filter.ex +++ b/packages/sync-service/lib/electric/shapes/filter.ex @@ -22,8 +22,8 @@ defmodule Electric.Shapes.Filter do tables, shape.root_table, Table.add_shape({handle, shape}, Table.empty()), - fn table_filter -> - Table.add_shape({handle, shape}, table_filter) + fn table -> + Table.add_shape({handle, shape}, table) end ) } @@ -33,10 +33,10 @@ defmodule Electric.Shapes.Filter do %Filter{ tables: tables - |> Enum.map(fn {table, table_filter} -> - {table, Table.remove_shape(table_filter, handle)} + |> Enum.map(fn {table_name, table} -> + {table_name, Table.remove_shape(table, handle)} end) - |> Enum.reject(fn {_table, table_filter} -> map_size(table_filter.fields) == 0 end) + |> Enum.reject(fn {_table, table} -> map_size(table.fields) == 0 end) |> Map.new() } end @@ -77,29 +77,29 @@ defmodule Electric.Shapes.Filter do # TODO: Optimisation: Do TruncatedRelations first and then just process other changes for other tables - def affected_shapes(%Filter{} = filter, %TruncatedRelation{relation: table}) do - for {handle, _shape} <- all_shapes_for_table(filter, table), + def affected_shapes(%Filter{} = filter, %TruncatedRelation{relation: table_name}) do + for {handle, _shape} <- all_shapes_for_table(filter, table_name), into: MapSet.new() do handle end end - defp affected_shapes_by_record(filter, table, record) do - case Map.get(filter.tables, table) do + defp affected_shapes_by_record(filter, table_name, record) do + case Map.get(filter.tables, table_name) do nil -> MapSet.new() - table_filter -> Table.affected_shapes(table_filter, record) + table -> Table.affected_shapes(table, record) end end defp all_shapes_in_filter(%Filter{} = filter) do - for {_table, table_filter} <- filter.tables, - {handle, shape} <- all_shapes_in_table_filter(table_filter), + for {_table, table} <- filter.tables, + {handle, shape} <- all_shapes_in_table(table), into: %{} do {handle, shape} end end - defp all_shapes_in_table_filter(%{fields: fields, other_shapes: other_shapes}) do + defp all_shapes_in_table(%{fields: fields, other_shapes: other_shapes}) do for {_field, %{values: values}} <- fields, {_value, shapes} <- values, %{handle: handle, shape: shape} <- shapes, @@ -109,13 +109,13 @@ defmodule Electric.Shapes.Filter do |> Map.merge(other_shapes) end - defp all_shapes_for_table(%Filter{} = filter, table) do - case Map.get(filter.tables, table) do + defp all_shapes_for_table(%Filter{} = filter, table_name) do + case Map.get(filter.tables, table_name) do nil -> %{} - table_filter -> - all_shapes_in_table_filter(table_filter) + table -> + all_shapes_in_table(table) end end end From 2d2c3f60c48c7b77fee67e09875b473c83cb9663 Mon Sep 17 00:00:00 2001 From: rob Date: Sat, 30 Nov 2024 21:18:33 +0000 Subject: [PATCH 30/62] Rename field to index --- .../lib/electric/shapes/filter.ex | 6 ++-- .../shapes/filter/{field.ex => index.ex} | 10 +++--- .../lib/electric/shapes/filter/table.ex | 34 +++++++++--------- .../test/electric/shapes/filter_test.exs | 36 +++++++++---------- 4 files changed, 43 insertions(+), 43 deletions(-) rename packages/sync-service/lib/electric/shapes/filter/{field.ex => index.ex} (87%) diff --git a/packages/sync-service/lib/electric/shapes/filter.ex b/packages/sync-service/lib/electric/shapes/filter.ex index 2a87a72d16..f834a4ee43 100644 --- a/packages/sync-service/lib/electric/shapes/filter.ex +++ b/packages/sync-service/lib/electric/shapes/filter.ex @@ -36,7 +36,7 @@ defmodule Electric.Shapes.Filter do |> Enum.map(fn {table_name, table} -> {table_name, Table.remove_shape(table, handle)} end) - |> Enum.reject(fn {_table, table} -> map_size(table.fields) == 0 end) + |> Enum.reject(fn {_table, table} -> map_size(table.indexes) == 0 end) |> Map.new() } end @@ -99,8 +99,8 @@ defmodule Electric.Shapes.Filter do end end - defp all_shapes_in_table(%{fields: fields, other_shapes: other_shapes}) do - for {_field, %{values: values}} <- fields, + defp all_shapes_in_table(%{indexes: indexes, other_shapes: other_shapes}) do + for {_field, %{values: values}} <- indexes, {_value, shapes} <- values, %{handle: handle, shape: shape} <- shapes, into: %{} do diff --git a/packages/sync-service/lib/electric/shapes/filter/field.ex b/packages/sync-service/lib/electric/shapes/filter/index.ex similarity index 87% rename from packages/sync-service/lib/electric/shapes/filter/field.ex rename to packages/sync-service/lib/electric/shapes/filter/index.ex index e3215225c0..d740b05145 100644 --- a/packages/sync-service/lib/electric/shapes/filter/field.ex +++ b/packages/sync-service/lib/electric/shapes/filter/index.ex @@ -1,18 +1,18 @@ -defmodule Electric.Shapes.Filter.Field do +defmodule Electric.Shapes.Filter.Index do alias Electric.Replication.Eval.Env alias Electric.Shapes.Shape - defstruct fields: %{}, other_shapes: %{} + defstruct indexes: %{}, other_shapes: %{} def new(type), do: %{type: type, values: %{}} # TODO: Renmame handle to shape_id - def add_shape(value, {handle, shape}, and_where, field_filter) do + def add_shape(value, {handle, shape}, and_where, index) do %{ - field_filter + index | values: Map.update( - field_filter.values, + index.values, value, [%{handle: handle, and_where: and_where, shape: shape}], fn shapes -> [%{handle: handle, and_where: and_where, shape: shape} | shapes] end diff --git a/packages/sync-service/lib/electric/shapes/filter/table.ex b/packages/sync-service/lib/electric/shapes/filter/table.ex index cdb146f31f..09b0c71f13 100644 --- a/packages/sync-service/lib/electric/shapes/filter/table.ex +++ b/packages/sync-service/lib/electric/shapes/filter/table.ex @@ -4,11 +4,11 @@ defmodule Electric.Shapes.Filter.Table do alias Electric.Replication.Eval.Parser.Const alias Electric.Replication.Eval.Parser.Func alias Electric.Replication.Eval.Parser.Ref - alias Electric.Shapes.Filter.Field + alias Electric.Shapes.Filter.Index alias Electric.Shapes.Filter.Table alias Electric.Shapes.Shape - defstruct fields: %{}, other_shapes: %{} + defstruct indexes: %{}, other_shapes: %{} def empty, do: %Table{} @@ -17,13 +17,13 @@ defmodule Electric.Shapes.Filter.Table do %{operation: "=", field: field, type: type, value: value, and_where: and_where} -> %{ table - | fields: - add_shape_to_fields( + | indexes: + add_shape_to_indexes( field, type, value, shape_instance, - table.fields, + table.indexes, and_where ) } @@ -33,13 +33,13 @@ defmodule Electric.Shapes.Filter.Table do end end - defp add_shape_to_fields(field, type, value, shape_instance, fields, and_where) do + defp add_shape_to_indexes(field, type, value, shape_instance, indexes, and_where) do Map.update( - fields, + indexes, field, - Field.add_shape(value, shape_instance, and_where, Field.new(type)), + Index.add_shape(value, shape_instance, and_where, Index.new(type)), fn field_filter -> - Field.add_shape(value, shape_instance, and_where, field_filter) + Index.add_shape(value, shape_instance, and_where, field_filter) end ) end @@ -80,26 +80,26 @@ defmodule Electric.Shapes.Filter.Table do %Expr{eval: eval, used_refs: Parser.find_refs(eval), returns: :bool} end - def remove_shape(%{fields: fields, other_shapes: other_shapes}, handle) do + def remove_shape(%{indexes: indexes, other_shapes: other_shapes}, handle) do %Table{ - fields: remove_shape_from_fields(fields, handle), + indexes: remove_shape_from_indexes(indexes, handle), other_shapes: Map.delete(other_shapes, handle) } end - defp remove_shape_from_fields(fields, handle) do - fields + defp remove_shape_from_indexes(indexes, handle) do + indexes |> Map.new(fn {field, %{values: value_filter} = field_filter} -> {field, - %{field_filter | values: Field.remove_shape_from_value_filter(value_filter, handle)}} + %{field_filter | values: Index.remove_shape_from_value_filter(value_filter, handle)}} end) |> Enum.reject(fn {_field, %{values: value_filter}} -> map_size(value_filter) == 0 end) |> Map.new() end - def affected_shapes(%{fields: fields} = table, record) do - fields - |> Enum.map(&Field.affected_shapes(&1, record)) + def affected_shapes(%{indexes: indexes} = table, record) do + indexes + |> Enum.map(&Index.affected_shapes(&1, record)) |> Enum.reduce(MapSet.new(), &MapSet.union(&1, &2)) |> MapSet.union(other_shapes_affected(table, record)) end diff --git a/packages/sync-service/test/electric/shapes/filter_test.exs b/packages/sync-service/test/electric/shapes/filter_test.exs index ec41ddb773..acd505d90e 100644 --- a/packages/sync-service/test/electric/shapes/filter_test.exs +++ b/packages/sync-service/test/electric/shapes/filter_test.exs @@ -24,7 +24,7 @@ defmodule Electric.Shapes.FilterTest do assert Filter.new(%{"shape1" => shape}) == %Filter{ tables: %{ {"public", "the_table"} => %Table{ - fields: %{ + indexes: %{ "id" => %{ type: :int8, values: %{ @@ -46,7 +46,7 @@ defmodule Electric.Shapes.FilterTest do assert Filter.new(%{"shape1" => shape}) == %Filter{ tables: %{ {"public", "the_table"} => %Table{ - fields: %{ + indexes: %{ "id" => %{ type: :int8, values: %{ @@ -68,7 +68,7 @@ defmodule Electric.Shapes.FilterTest do assert %Filter{ tables: %{ {"public", "the_table"} => %Table{ - fields: %{ + indexes: %{ "id" => %{ type: :int8, values: %{ @@ -104,7 +104,7 @@ defmodule Electric.Shapes.FilterTest do assert %Filter{ tables: %{ {"public", "the_table"} => %Table{ - fields: %{ + indexes: %{ "id" => %{ type: :int8, values: %{ @@ -140,7 +140,7 @@ defmodule Electric.Shapes.FilterTest do assert Filter.new(shapes) == %Filter{ tables: %{ {"public", "the_table"} => %Table{ - fields: %{}, + indexes: %{}, other_shapes: shapes } } @@ -153,7 +153,7 @@ defmodule Electric.Shapes.FilterTest do filter = %Filter{ tables: %{ {"public", "the_table"} => %Table{ - fields: %{ + indexes: %{ "id" => %{ type: :int8, values: %{ @@ -193,7 +193,7 @@ defmodule Electric.Shapes.FilterTest do } }, {"public", "another_table"} => %Table{ - fields: %{ + indexes: %{ "id" => %{ type: :int8, values: %{ @@ -213,7 +213,7 @@ defmodule Electric.Shapes.FilterTest do assert Filter.remove_shape(filter, "shape1") == %Filter{ tables: %{ {"public", "the_table"} => %Table{ - fields: %{ + indexes: %{ "id" => %{ type: :int8, values: %{ @@ -251,7 +251,7 @@ defmodule Electric.Shapes.FilterTest do filter = %Filter{ tables: %{ {"public", "the_table"} => %Table{ - fields: %{ + indexes: %{ "id" => %{ type: :int8, values: %{ @@ -269,7 +269,7 @@ defmodule Electric.Shapes.FilterTest do other_shapes: %{} }, {"public", "another_table"} => %Table{ - fields: %{ + indexes: %{ "id" => %{ type: :int8, values: %{ @@ -301,7 +301,7 @@ defmodule Electric.Shapes.FilterTest do filter = %Filter{ tables: %{ {"public", "the_table"} => %Table{ - fields: %{ + indexes: %{ "id" => %{ type: :int8, values: %{ @@ -334,7 +334,7 @@ defmodule Electric.Shapes.FilterTest do filter = %Filter{ tables: %{ {"public", "the_table"} => %Table{ - fields: %{}, + indexes: %{}, other_shapes: %{ "shape1" => Shape.new!("the_table", where: "id > 7", inspector: @inspector), "shape2" => Shape.new!("the_table", where: "id > 6", inspector: @inspector), @@ -361,7 +361,7 @@ defmodule Electric.Shapes.FilterTest do filter = %Filter{ tables: %{ {"public", "the_table"} => %Table{ - fields: %{ + indexes: %{ "id" => %{ type: :int8, values: %{ @@ -393,7 +393,7 @@ defmodule Electric.Shapes.FilterTest do filter = %Filter{ tables: %{ {"public", "the_table"} => %Table{ - fields: %{ + indexes: %{ "id" => %{ type: :int8, values: %{ @@ -432,7 +432,7 @@ defmodule Electric.Shapes.FilterTest do filter = %Filter{ tables: %{ {"public", "the_table"} => %Table{ - fields: %{ + indexes: %{ "id" => %{ type: :int8, values: %{ @@ -459,7 +459,7 @@ defmodule Electric.Shapes.FilterTest do } }, {"public", "another_table"} => %Table{ - fields: %{ + indexes: %{ "id" => %{ type: :int8, values: %{ @@ -496,7 +496,7 @@ defmodule Electric.Shapes.FilterTest do filter = %Filter{ tables: %{ {"public", "the_table"} => %Table{ - fields: %{ + indexes: %{ "id" => %{ type: :int8, values: %{ @@ -523,7 +523,7 @@ defmodule Electric.Shapes.FilterTest do } }, {"public", "another_table"} => %Table{ - fields: %{ + indexes: %{ type: :int8, values: %{ "id" => %{ From 47fc530359dd1a8daeee51ae134595fa61fb7543 Mon Sep 17 00:00:00 2001 From: rob Date: Sat, 30 Nov 2024 21:28:43 +0000 Subject: [PATCH 31/62] Ensure table is first param of Table public functions --- packages/sync-service/lib/electric/shapes/filter.ex | 4 ++-- packages/sync-service/lib/electric/shapes/filter/table.ex | 6 +++--- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/packages/sync-service/lib/electric/shapes/filter.ex b/packages/sync-service/lib/electric/shapes/filter.ex index f834a4ee43..0cfb06e471 100644 --- a/packages/sync-service/lib/electric/shapes/filter.ex +++ b/packages/sync-service/lib/electric/shapes/filter.ex @@ -21,9 +21,9 @@ defmodule Electric.Shapes.Filter do Map.update( tables, shape.root_table, - Table.add_shape({handle, shape}, Table.empty()), + Table.add_shape(Table.empty(), {handle, shape}), fn table -> - Table.add_shape({handle, shape}, table) + Table.add_shape(table, {handle, shape}) end ) } diff --git a/packages/sync-service/lib/electric/shapes/filter/table.ex b/packages/sync-service/lib/electric/shapes/filter/table.ex index 09b0c71f13..e562934d1f 100644 --- a/packages/sync-service/lib/electric/shapes/filter/table.ex +++ b/packages/sync-service/lib/electric/shapes/filter/table.ex @@ -12,7 +12,7 @@ defmodule Electric.Shapes.Filter.Table do def empty, do: %Table{} - def add_shape({handle, shape} = shape_instance, table) do + def add_shape(%Table{} = table, {handle, shape} = shape_instance) do case optimise_where(shape.where) do %{operation: "=", field: field, type: type, value: value, and_where: and_where} -> %{ @@ -80,7 +80,7 @@ defmodule Electric.Shapes.Filter.Table do %Expr{eval: eval, used_refs: Parser.find_refs(eval), returns: :bool} end - def remove_shape(%{indexes: indexes, other_shapes: other_shapes}, handle) do + def remove_shape(%Table{indexes: indexes, other_shapes: other_shapes}, handle) do %Table{ indexes: remove_shape_from_indexes(indexes, handle), other_shapes: Map.delete(other_shapes, handle) @@ -97,7 +97,7 @@ defmodule Electric.Shapes.Filter.Table do |> Map.new() end - def affected_shapes(%{indexes: indexes} = table, record) do + def affected_shapes(%Table{indexes: indexes} = table, record) do indexes |> Enum.map(&Index.affected_shapes(&1, record)) |> Enum.reduce(MapSet.new(), &MapSet.union(&1, &2)) From ba275c597b42897ceb2cc796a7bfc1c7b65ec463 Mon Sep 17 00:00:00 2001 From: rob Date: Sat, 30 Nov 2024 21:36:14 +0000 Subject: [PATCH 32/62] Neaten up remove_shape --- .../lib/electric/shapes/filter/index.ex | 18 +++++++++++++----- .../lib/electric/shapes/filter/table.ex | 7 ++----- 2 files changed, 15 insertions(+), 10 deletions(-) diff --git a/packages/sync-service/lib/electric/shapes/filter/index.ex b/packages/sync-service/lib/electric/shapes/filter/index.ex index d740b05145..dab7a6967e 100644 --- a/packages/sync-service/lib/electric/shapes/filter/index.ex +++ b/packages/sync-service/lib/electric/shapes/filter/index.ex @@ -6,6 +6,8 @@ defmodule Electric.Shapes.Filter.Index do def new(type), do: %{type: type, values: %{}} + def empty?(%{values: values}), do: values == %{} + # TODO: Renmame handle to shape_id def add_shape(value, {handle, shape}, and_where, index) do %{ @@ -20,11 +22,17 @@ defmodule Electric.Shapes.Filter.Index do } end - def remove_shape_from_value_filter(value_filter, handle) do - value_filter - |> Map.new(fn {value, shapes} -> {value, shapes |> Enum.reject(&(&1.handle == handle))} end) - |> Enum.reject(fn {_value, shapes} -> shapes == [] end) - |> Map.new() + def remove_shape(index, handle) do + %{ + index + | values: + index.values + |> Map.new(fn {value, shapes} -> + {value, shapes |> Enum.reject(&(&1.handle == handle))} + end) + |> Enum.reject(fn {_value, shapes} -> shapes == [] end) + |> Map.new() + } end def affected_shapes({field, %{values: values, type: type}}, record) do diff --git a/packages/sync-service/lib/electric/shapes/filter/table.ex b/packages/sync-service/lib/electric/shapes/filter/table.ex index e562934d1f..aa131b6a08 100644 --- a/packages/sync-service/lib/electric/shapes/filter/table.ex +++ b/packages/sync-service/lib/electric/shapes/filter/table.ex @@ -89,11 +89,8 @@ defmodule Electric.Shapes.Filter.Table do defp remove_shape_from_indexes(indexes, handle) do indexes - |> Map.new(fn {field, %{values: value_filter} = field_filter} -> - {field, - %{field_filter | values: Index.remove_shape_from_value_filter(value_filter, handle)}} - end) - |> Enum.reject(fn {_field, %{values: value_filter}} -> map_size(value_filter) == 0 end) + |> Map.new(fn {field, index} -> {field, Index.remove_shape(index, handle)} end) + |> Enum.reject(fn {_field, index} -> Index.empty?(index) end) |> Map.new() end From f8c2703ed8409a44c6321edc246ae6fbde7c2d81 Mon Sep 17 00:00:00 2001 From: rob Date: Sat, 30 Nov 2024 21:43:35 +0000 Subject: [PATCH 33/62] Refactor affected_shapes function to improve argument structure and usage. --- packages/sync-service/lib/electric/shapes/filter/index.ex | 2 +- packages/sync-service/lib/electric/shapes/filter/table.ex | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/sync-service/lib/electric/shapes/filter/index.ex b/packages/sync-service/lib/electric/shapes/filter/index.ex index dab7a6967e..f62b6b3be9 100644 --- a/packages/sync-service/lib/electric/shapes/filter/index.ex +++ b/packages/sync-service/lib/electric/shapes/filter/index.ex @@ -35,7 +35,7 @@ defmodule Electric.Shapes.Filter.Index do } end - def affected_shapes({field, %{values: values, type: type}}, record) do + def affected_shapes(%{values: values, type: type}, field, record) do case values[value_from_record(record, field, type)] do nil -> MapSet.new() diff --git a/packages/sync-service/lib/electric/shapes/filter/table.ex b/packages/sync-service/lib/electric/shapes/filter/table.ex index aa131b6a08..2a9c3782d6 100644 --- a/packages/sync-service/lib/electric/shapes/filter/table.ex +++ b/packages/sync-service/lib/electric/shapes/filter/table.ex @@ -96,7 +96,7 @@ defmodule Electric.Shapes.Filter.Table do def affected_shapes(%Table{indexes: indexes} = table, record) do indexes - |> Enum.map(&Index.affected_shapes(&1, record)) + |> Enum.map(fn {field, index} -> Index.affected_shapes(index, field, record) end) |> Enum.reduce(MapSet.new(), &MapSet.union(&1, &2)) |> MapSet.union(other_shapes_affected(table, record)) end From c4af87881b2371a4a52d97125e18bfbec03f7efe Mon Sep 17 00:00:00 2001 From: rob Date: Sun, 1 Dec 2024 10:07:18 +0000 Subject: [PATCH 34/62] Refactor: Use Table.empty?/1 for cleaner table emptiness check --- packages/sync-service/lib/electric/shapes/filter.ex | 2 +- packages/sync-service/lib/electric/shapes/filter/table.ex | 4 ++++ 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/packages/sync-service/lib/electric/shapes/filter.ex b/packages/sync-service/lib/electric/shapes/filter.ex index 0cfb06e471..d262cd2240 100644 --- a/packages/sync-service/lib/electric/shapes/filter.ex +++ b/packages/sync-service/lib/electric/shapes/filter.ex @@ -36,7 +36,7 @@ defmodule Electric.Shapes.Filter do |> Enum.map(fn {table_name, table} -> {table_name, Table.remove_shape(table, handle)} end) - |> Enum.reject(fn {_table, table} -> map_size(table.indexes) == 0 end) + |> Enum.reject(fn {_table, table} -> Table.empty?(table) end) |> Map.new() } end diff --git a/packages/sync-service/lib/electric/shapes/filter/table.ex b/packages/sync-service/lib/electric/shapes/filter/table.ex index 2a9c3782d6..bedada4fb9 100644 --- a/packages/sync-service/lib/electric/shapes/filter/table.ex +++ b/packages/sync-service/lib/electric/shapes/filter/table.ex @@ -12,6 +12,10 @@ defmodule Electric.Shapes.Filter.Table do def empty, do: %Table{} + def empty?(%Table{indexes: indexes, other_shapes: other_shapes}) do + indexes == %{} && other_shapes == %{} + end + def add_shape(%Table{} = table, {handle, shape} = shape_instance) do case optimise_where(shape.where) do %{operation: "=", field: field, type: type, value: value, and_where: and_where} -> From b6044829f46e0fee6f9dc7437088e2b802a95632 Mon Sep 17 00:00:00 2001 From: rob Date: Sun, 1 Dec 2024 10:14:17 +0000 Subject: [PATCH 35/62] Use Index struct --- .../lib/electric/shapes/filter/index.ex | 13 +++--- .../test/electric/shapes/filter_test.exs | 46 ++++++++++--------- 2 files changed, 32 insertions(+), 27 deletions(-) diff --git a/packages/sync-service/lib/electric/shapes/filter/index.ex b/packages/sync-service/lib/electric/shapes/filter/index.ex index f62b6b3be9..8a5e61471f 100644 --- a/packages/sync-service/lib/electric/shapes/filter/index.ex +++ b/packages/sync-service/lib/electric/shapes/filter/index.ex @@ -1,15 +1,16 @@ defmodule Electric.Shapes.Filter.Index do alias Electric.Replication.Eval.Env + alias Electric.Shapes.Filter.Index alias Electric.Shapes.Shape - defstruct indexes: %{}, other_shapes: %{} + defstruct [:type, :values] - def new(type), do: %{type: type, values: %{}} + def new(type), do: %Index{type: type, values: %{}} - def empty?(%{values: values}), do: values == %{} + def empty?(%Index{values: values}), do: values == %{} # TODO: Renmame handle to shape_id - def add_shape(value, {handle, shape}, and_where, index) do + def add_shape(value, {handle, shape}, and_where, %Index{} = index) do %{ index | values: @@ -22,7 +23,7 @@ defmodule Electric.Shapes.Filter.Index do } end - def remove_shape(index, handle) do + def remove_shape(%Index{} = index, handle) do %{ index | values: @@ -35,7 +36,7 @@ defmodule Electric.Shapes.Filter.Index do } end - def affected_shapes(%{values: values, type: type}, field, record) do + def affected_shapes(%Index{values: values, type: type}, field, record) do case values[value_from_record(record, field, type)] do nil -> MapSet.new() diff --git a/packages/sync-service/test/electric/shapes/filter_test.exs b/packages/sync-service/test/electric/shapes/filter_test.exs index acd505d90e..e73cbe7a3f 100644 --- a/packages/sync-service/test/electric/shapes/filter_test.exs +++ b/packages/sync-service/test/electric/shapes/filter_test.exs @@ -11,6 +11,7 @@ defmodule Electric.Shapes.FilterTest do alias Electric.Replication.Eval.Parser.Func alias Electric.Replication.Eval.Parser.Ref alias Electric.Shapes.Filter + alias Electric.Shapes.Filter.Index alias Electric.Shapes.Filter.Table alias Electric.Shapes.Shape alias Support.StubInspector @@ -25,7 +26,7 @@ defmodule Electric.Shapes.FilterTest do tables: %{ {"public", "the_table"} => %Table{ indexes: %{ - "id" => %{ + "id" => %Index{ type: :int8, values: %{ 1 => [ @@ -47,7 +48,7 @@ defmodule Electric.Shapes.FilterTest do tables: %{ {"public", "the_table"} => %Table{ indexes: %{ - "id" => %{ + "id" => %Index{ type: :int8, values: %{ 1 => [ @@ -69,7 +70,7 @@ defmodule Electric.Shapes.FilterTest do tables: %{ {"public", "the_table"} => %Table{ indexes: %{ - "id" => %{ + "id" => %Index{ type: :int8, values: %{ 1 => [ @@ -105,7 +106,7 @@ defmodule Electric.Shapes.FilterTest do tables: %{ {"public", "the_table"} => %Table{ indexes: %{ - "id" => %{ + "id" => %Index{ type: :int8, values: %{ 1 => [ @@ -154,7 +155,7 @@ defmodule Electric.Shapes.FilterTest do tables: %{ {"public", "the_table"} => %Table{ indexes: %{ - "id" => %{ + "id" => %Index{ type: :int8, values: %{ 1 => [ @@ -171,7 +172,7 @@ defmodule Electric.Shapes.FilterTest do ] } }, - "name" => %{ + "name" => %Index{ type: :text, values: %{ "bill" => [ @@ -194,7 +195,7 @@ defmodule Electric.Shapes.FilterTest do }, {"public", "another_table"} => %Table{ indexes: %{ - "id" => %{ + "id" => %Index{ type: :int8, values: %{ 1 => [ @@ -214,7 +215,7 @@ defmodule Electric.Shapes.FilterTest do tables: %{ {"public", "the_table"} => %Table{ indexes: %{ - "id" => %{ + "id" => %Index{ type: :int8, values: %{ 2 => [ @@ -225,7 +226,7 @@ defmodule Electric.Shapes.FilterTest do ] } }, - "name" => %{ + "name" => %Index{ type: :text, values: %{ "bill" => [ @@ -252,7 +253,7 @@ defmodule Electric.Shapes.FilterTest do tables: %{ {"public", "the_table"} => %Table{ indexes: %{ - "id" => %{ + "id" => %Index{ type: :int8, values: %{ 1 => [ @@ -270,7 +271,7 @@ defmodule Electric.Shapes.FilterTest do }, {"public", "another_table"} => %Table{ indexes: %{ - "id" => %{ + "id" => %Index{ type: :int8, values: %{ 1 => [ @@ -302,7 +303,7 @@ defmodule Electric.Shapes.FilterTest do tables: %{ {"public", "the_table"} => %Table{ indexes: %{ - "id" => %{ + "id" => %Index{ type: :int8, values: %{ 1 => [ @@ -362,7 +363,7 @@ defmodule Electric.Shapes.FilterTest do tables: %{ {"public", "the_table"} => %Table{ indexes: %{ - "id" => %{ + "id" => %Index{ type: :int8, values: %{ 1 => [ @@ -394,7 +395,7 @@ defmodule Electric.Shapes.FilterTest do tables: %{ {"public", "the_table"} => %Table{ indexes: %{ - "id" => %{ + "id" => %Index{ type: :int8, values: %{ 1 => [ @@ -433,7 +434,7 @@ defmodule Electric.Shapes.FilterTest do tables: %{ {"public", "the_table"} => %Table{ indexes: %{ - "id" => %{ + "id" => %Index{ type: :int8, values: %{ 1 => [ @@ -460,7 +461,7 @@ defmodule Electric.Shapes.FilterTest do }, {"public", "another_table"} => %Table{ indexes: %{ - "id" => %{ + "id" => %Index{ type: :int8, values: %{ 1 => [ @@ -497,7 +498,7 @@ defmodule Electric.Shapes.FilterTest do tables: %{ {"public", "the_table"} => %Table{ indexes: %{ - "id" => %{ + "id" => %Index{ type: :int8, values: %{ 1 => [ @@ -526,10 +527,13 @@ defmodule Electric.Shapes.FilterTest do indexes: %{ type: :int8, values: %{ - "id" => %{ - 1 => [ - %{handle: "not-this-shape-1", and_where: nil} - ] + "id" => %Index{ + type: :int8, + values: %{ + 1 => [ + %{handle: "not-this-shape-1", and_where: nil} + ] + } } } }, From 001e0d37b01e170f72d171eb2838fc1ef08876dc Mon Sep 17 00:00:00 2001 From: rob Date: Sun, 1 Dec 2024 10:18:36 +0000 Subject: [PATCH 36/62] Refactor add_shape function to reorder parameters for consistency. --- packages/sync-service/lib/electric/shapes/filter/index.ex | 2 +- packages/sync-service/lib/electric/shapes/filter/table.ex | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/packages/sync-service/lib/electric/shapes/filter/index.ex b/packages/sync-service/lib/electric/shapes/filter/index.ex index 8a5e61471f..04e7eed969 100644 --- a/packages/sync-service/lib/electric/shapes/filter/index.ex +++ b/packages/sync-service/lib/electric/shapes/filter/index.ex @@ -10,7 +10,7 @@ defmodule Electric.Shapes.Filter.Index do def empty?(%Index{values: values}), do: values == %{} # TODO: Renmame handle to shape_id - def add_shape(value, {handle, shape}, and_where, %Index{} = index) do + def add_shape(%Index{} = index, value, {handle, shape}, and_where) do %{ index | values: diff --git a/packages/sync-service/lib/electric/shapes/filter/table.ex b/packages/sync-service/lib/electric/shapes/filter/table.ex index bedada4fb9..6681c3aa0c 100644 --- a/packages/sync-service/lib/electric/shapes/filter/table.ex +++ b/packages/sync-service/lib/electric/shapes/filter/table.ex @@ -41,9 +41,9 @@ defmodule Electric.Shapes.Filter.Table do Map.update( indexes, field, - Index.add_shape(value, shape_instance, and_where, Index.new(type)), - fn field_filter -> - Index.add_shape(value, shape_instance, and_where, field_filter) + Index.add_shape(Index.new(type), value, shape_instance, and_where), + fn index -> + Index.add_shape(index, value, shape_instance, and_where) end ) end From f4d3f9b1cffdc5c1c1d5e9c78c3acd9648c0ce9e Mon Sep 17 00:00:00 2001 From: rob Date: Sun, 1 Dec 2024 10:24:52 +0000 Subject: [PATCH 37/62] Remove unused inspector option from Dispatcher and related modules. --- .../lib/electric/replication/shape_log_collector.ex | 3 +-- packages/sync-service/lib/electric/shapes/dispatcher.ex | 9 ++++----- .../test/electric/shapes/dispatcher_test.exs | 2 +- 3 files changed, 6 insertions(+), 8 deletions(-) diff --git a/packages/sync-service/lib/electric/replication/shape_log_collector.ex b/packages/sync-service/lib/electric/replication/shape_log_collector.ex index b5424e3d9a..63c8bacab6 100644 --- a/packages/sync-service/lib/electric/replication/shape_log_collector.ex +++ b/packages/sync-service/lib/electric/replication/shape_log_collector.ex @@ -56,8 +56,7 @@ defmodule Electric.Replication.ShapeLogCollector do state = Map.merge(opts, %{producer: nil, subscriptions: {0, MapSet.new()}}) # start in demand: :accumulate mode so that the ShapeCache is able to start # all active consumers before we start sending transactions - {:producer, state, - dispatcher: {Electric.Shapes.Dispatcher, [inspector: state.inspector]}, demand: opts.demand} + {:producer, state, dispatcher: Electric.Shapes.Dispatcher, demand: opts.demand} end def handle_subscribe(:consumer, _opts, from, state) do diff --git a/packages/sync-service/lib/electric/shapes/dispatcher.ex b/packages/sync-service/lib/electric/shapes/dispatcher.ex index bc2dc77641..000a045218 100644 --- a/packages/sync-service/lib/electric/shapes/dispatcher.ex +++ b/packages/sync-service/lib/electric/shapes/dispatcher.ex @@ -1,4 +1,5 @@ defmodule Electric.Shapes.Dispatcher do + # TODO : Update @moduledoc @moduledoc """ Dispatches transactions and relations to consumers filtered according to the subscriber's `selector` function. @@ -34,14 +35,14 @@ defmodule Electric.Shapes.Dispatcher do alias Electric.Shapes.Filter defmodule State do - defstruct [:n, :waiting, :pending, :subs, :filter, :pids, :inspector] + defstruct [:n, :waiting, :pending, :subs, :filter, :pids] end @behaviour GenStage.Dispatcher @impl GenStage.Dispatcher - def init(opts) do + def init(_opts) do {:ok, %State{ n: 0, @@ -49,9 +50,7 @@ defmodule Electric.Shapes.Dispatcher do pending: nil, subs: [], filter: Filter.empty(), - pids: MapSet.new(), - # TODO: remove inspector as we don't need it anymore - inspector: Keyword.fetch!(opts, :inspector) + pids: MapSet.new() }} end diff --git a/packages/sync-service/test/electric/shapes/dispatcher_test.exs b/packages/sync-service/test/electric/shapes/dispatcher_test.exs index bfdcad5a80..db55d9c532 100644 --- a/packages/sync-service/test/electric/shapes/dispatcher_test.exs +++ b/packages/sync-service/test/electric/shapes/dispatcher_test.exs @@ -21,7 +21,7 @@ defmodule Electric.Shapes.DispatcherTest do } defp dispatcher() do - {:ok, state} = D.init(inspector: @inspector) + {:ok, state} = D.init([]) state end From be416742a543d761b2d211dfc3160e0dcfb2a83c Mon Sep 17 00:00:00 2001 From: rob Date: Sun, 1 Dec 2024 12:03:09 +0000 Subject: [PATCH 38/62] Refactor: Rename handle to shape_id --- .../lib/electric/shapes/filter.ex | 31 +++++----- .../lib/electric/shapes/filter/index.ex | 13 ++--- .../lib/electric/shapes/filter/table.ex | 18 +++--- .../test/electric/shapes/filter_test.exs | 58 +++++++++---------- 4 files changed, 61 insertions(+), 59 deletions(-) diff --git a/packages/sync-service/lib/electric/shapes/filter.ex b/packages/sync-service/lib/electric/shapes/filter.ex index d262cd2240..413534fe14 100644 --- a/packages/sync-service/lib/electric/shapes/filter.ex +++ b/packages/sync-service/lib/electric/shapes/filter.ex @@ -12,29 +12,32 @@ defmodule Electric.Shapes.Filter do defstruct tables: %{} def new(shapes), do: shapes |> Map.to_list() |> new(empty()) - defp new([{handle, shape} | shapes], filter), do: new(shapes, add_shape(filter, handle, shape)) + + defp new([{shape_id, shape} | shapes], filter), + do: new(shapes, add_shape(filter, shape_id, shape)) + defp new([], filter), do: filter - def add_shape(%Filter{tables: tables}, handle, shape) do + def add_shape(%Filter{tables: tables}, shape_id, shape) do %Filter{ tables: Map.update( tables, shape.root_table, - Table.add_shape(Table.empty(), {handle, shape}), + Table.add_shape(Table.empty(), {shape_id, shape}), fn table -> - Table.add_shape(table, {handle, shape}) + Table.add_shape(table, {shape_id, shape}) end ) } end - def remove_shape(%Filter{tables: tables}, handle) do + def remove_shape(%Filter{tables: tables}, shape_id) do %Filter{ tables: tables |> Enum.map(fn {table_name, table} -> - {table_name, Table.remove_shape(table, handle)} + {table_name, Table.remove_shape(table, shape_id)} end) |> Enum.reject(fn {_table, table} -> Table.empty?(table) end) |> Map.new() @@ -45,10 +48,10 @@ defmodule Electric.Shapes.Filter do def affected_shapes(%Filter{} = filter, %Relation{} = relation) do # Check all shapes is all tables becuase the table may have been renamed - for {handle, shape} <- all_shapes_in_filter(filter), + for {shape_id, shape} <- all_shapes_in_filter(filter), Shape.is_affected_by_relation_change?(shape, relation), into: MapSet.new() do - handle + shape_id end end @@ -78,9 +81,9 @@ defmodule Electric.Shapes.Filter do # TODO: Optimisation: Do TruncatedRelations first and then just process other changes for other tables def affected_shapes(%Filter{} = filter, %TruncatedRelation{relation: table_name}) do - for {handle, _shape} <- all_shapes_for_table(filter, table_name), + for {shape_id, _shape} <- all_shapes_for_table(filter, table_name), into: MapSet.new() do - handle + shape_id end end @@ -93,18 +96,18 @@ defmodule Electric.Shapes.Filter do defp all_shapes_in_filter(%Filter{} = filter) do for {_table, table} <- filter.tables, - {handle, shape} <- all_shapes_in_table(table), + {shape_id, shape} <- all_shapes_in_table(table), into: %{} do - {handle, shape} + {shape_id, shape} end end defp all_shapes_in_table(%{indexes: indexes, other_shapes: other_shapes}) do for {_field, %{values: values}} <- indexes, {_value, shapes} <- values, - %{handle: handle, shape: shape} <- shapes, + %{shape_id: shape_id, shape: shape} <- shapes, into: %{} do - {handle, shape} + {shape_id, shape} end |> Map.merge(other_shapes) end diff --git a/packages/sync-service/lib/electric/shapes/filter/index.ex b/packages/sync-service/lib/electric/shapes/filter/index.ex index 04e7eed969..74a8a2a512 100644 --- a/packages/sync-service/lib/electric/shapes/filter/index.ex +++ b/packages/sync-service/lib/electric/shapes/filter/index.ex @@ -9,27 +9,26 @@ defmodule Electric.Shapes.Filter.Index do def empty?(%Index{values: values}), do: values == %{} - # TODO: Renmame handle to shape_id - def add_shape(%Index{} = index, value, {handle, shape}, and_where) do + def add_shape(%Index{} = index, value, {shape_id, shape}, and_where) do %{ index | values: Map.update( index.values, value, - [%{handle: handle, and_where: and_where, shape: shape}], - fn shapes -> [%{handle: handle, and_where: and_where, shape: shape} | shapes] end + [%{shape_id: shape_id, and_where: and_where, shape: shape}], + fn shapes -> [%{shape_id: shape_id, and_where: and_where, shape: shape} | shapes] end ) } end - def remove_shape(%Index{} = index, handle) do + def remove_shape(%Index{} = index, shape_id) do %{ index | values: index.values |> Map.new(fn {value, shapes} -> - {value, shapes |> Enum.reject(&(&1.handle == handle))} + {value, shapes |> Enum.reject(&(&1.shape_id == shape_id))} end) |> Enum.reject(fn {_value, shapes} -> shapes == [] end) |> Map.new() @@ -44,7 +43,7 @@ defmodule Electric.Shapes.Filter.Index do shapes -> shapes |> Enum.filter(&record_in_where?(&1.and_where, record)) - |> Enum.map(& &1.handle) + |> Enum.map(& &1.shape_id) |> MapSet.new() end end diff --git a/packages/sync-service/lib/electric/shapes/filter/table.ex b/packages/sync-service/lib/electric/shapes/filter/table.ex index 6681c3aa0c..9dcc70c457 100644 --- a/packages/sync-service/lib/electric/shapes/filter/table.ex +++ b/packages/sync-service/lib/electric/shapes/filter/table.ex @@ -16,7 +16,7 @@ defmodule Electric.Shapes.Filter.Table do indexes == %{} && other_shapes == %{} end - def add_shape(%Table{} = table, {handle, shape} = shape_instance) do + def add_shape(%Table{} = table, {shape_id, shape} = shape_instance) do case optimise_where(shape.where) do %{operation: "=", field: field, type: type, value: value, and_where: and_where} -> %{ @@ -33,7 +33,7 @@ defmodule Electric.Shapes.Filter.Table do } :not_optimised -> - %{table | other_shapes: Map.put(table.other_shapes, handle, shape)} + %{table | other_shapes: Map.put(table.other_shapes, shape_id, shape)} end end @@ -84,16 +84,16 @@ defmodule Electric.Shapes.Filter.Table do %Expr{eval: eval, used_refs: Parser.find_refs(eval), returns: :bool} end - def remove_shape(%Table{indexes: indexes, other_shapes: other_shapes}, handle) do + def remove_shape(%Table{indexes: indexes, other_shapes: other_shapes}, shape_id) do %Table{ - indexes: remove_shape_from_indexes(indexes, handle), - other_shapes: Map.delete(other_shapes, handle) + indexes: remove_shape_from_indexes(indexes, shape_id), + other_shapes: Map.delete(other_shapes, shape_id) } end - defp remove_shape_from_indexes(indexes, handle) do + defp remove_shape_from_indexes(indexes, shape_id) do indexes - |> Map.new(fn {field, index} -> {field, Index.remove_shape(index, handle)} end) + |> Map.new(fn {field, index} -> {field, Index.remove_shape(index, shape_id)} end) |> Enum.reject(fn {_field, index} -> Index.empty?(index) end) |> Map.new() end @@ -106,11 +106,11 @@ defmodule Electric.Shapes.Filter.Table do end defp other_shapes_affected(%{other_shapes: shapes}, record) do - for {handle, shape} <- shapes, + for {shape_id, shape} <- shapes, # TODO: Test Shape.record_in_shape? is called Shape.record_in_shape?(shape, record), into: MapSet.new() do - handle + shape_id end end end diff --git a/packages/sync-service/test/electric/shapes/filter_test.exs b/packages/sync-service/test/electric/shapes/filter_test.exs index e73cbe7a3f..bbe81e4b1d 100644 --- a/packages/sync-service/test/electric/shapes/filter_test.exs +++ b/packages/sync-service/test/electric/shapes/filter_test.exs @@ -30,7 +30,7 @@ defmodule Electric.Shapes.FilterTest do type: :int8, values: %{ 1 => [ - %{handle: "shape1", and_where: nil, shape: shape} + %{shape_id: "shape1", and_where: nil, shape: shape} ] } } @@ -52,7 +52,7 @@ defmodule Electric.Shapes.FilterTest do type: :int8, values: %{ 1 => [ - %{handle: "shape1", and_where: nil, shape: shape} + %{shape_id: "shape1", and_where: nil, shape: shape} ] } } @@ -75,7 +75,7 @@ defmodule Electric.Shapes.FilterTest do values: %{ 1 => [ %{ - handle: "shape1", + shape_id: "shape1", and_where: %Expr{ eval: %Func{ name: ~s(">"), @@ -111,7 +111,7 @@ defmodule Electric.Shapes.FilterTest do values: %{ 1 => [ %{ - handle: "shape1", + shape_id: "shape1", and_where: %Expr{ eval: %Func{ name: ~s(">"), @@ -150,7 +150,7 @@ defmodule Electric.Shapes.FilterTest do end describe "remove_shape/2" do - test "removes all shapes with the specified handle" do + test "removes all shapes with the specified shape_id" do filter = %Filter{ tables: %{ {"public", "the_table"} => %Table{ @@ -160,13 +160,13 @@ defmodule Electric.Shapes.FilterTest do values: %{ 1 => [ %{ - handle: "shape1", + shape_id: "shape1", and_where: nil } ], 2 => [ %{ - handle: "shape2", + shape_id: "shape2", and_where: nil } ] @@ -177,11 +177,11 @@ defmodule Electric.Shapes.FilterTest do values: %{ "bill" => [ %{ - handle: "shape1", + shape_id: "shape1", and_where: nil }, %{ - handle: "shape2", + shape_id: "shape2", and_where: nil } ] @@ -199,7 +199,7 @@ defmodule Electric.Shapes.FilterTest do type: :int8, values: %{ 1 => [ - %{handle: "shape1", and_where: nil} + %{shape_id: "shape1", and_where: nil} ] } } @@ -220,7 +220,7 @@ defmodule Electric.Shapes.FilterTest do values: %{ 2 => [ %{ - handle: "shape2", + shape_id: "shape2", and_where: nil } ] @@ -231,7 +231,7 @@ defmodule Electric.Shapes.FilterTest do values: %{ "bill" => [ %{ - handle: "shape2", + shape_id: "shape2", and_where: nil } ] @@ -257,12 +257,12 @@ defmodule Electric.Shapes.FilterTest do type: :int8, values: %{ 1 => [ - %{handle: "shape1", and_where: nil}, - %{handle: "shape2", and_where: nil} + %{shape_id: "shape1", and_where: nil}, + %{shape_id: "shape2", and_where: nil} ], 2 => [ - %{handle: "shape3", and_where: nil}, - %{handle: "shape4", and_where: nil} + %{shape_id: "shape3", and_where: nil}, + %{shape_id: "shape4", and_where: nil} ] } } @@ -275,7 +275,7 @@ defmodule Electric.Shapes.FilterTest do type: :int8, values: %{ 1 => [ - %{handle: "shape5", and_where: nil} + %{shape_id: "shape5", and_where: nil} ] } } @@ -307,8 +307,8 @@ defmodule Electric.Shapes.FilterTest do type: :int8, values: %{ 1 => [ - %{handle: "shape1", and_where: nil}, - %{handle: "shape2", and_where: nil} + %{shape_id: "shape1", and_where: nil}, + %{shape_id: "shape2", and_where: nil} ] } } @@ -367,7 +367,7 @@ defmodule Electric.Shapes.FilterTest do type: :int8, values: %{ 1 => [ - %{handle: "the-shape", and_where: nil} + %{shape_id: "the-shape", and_where: nil} ] } } @@ -399,13 +399,13 @@ defmodule Electric.Shapes.FilterTest do type: :int8, values: %{ 1 => [ - %{handle: "shape1", and_where: nil} + %{shape_id: "shape1", and_where: nil} ], 2 => [ - %{handle: "shape2", and_where: nil} + %{shape_id: "shape2", and_where: nil} ], 3 => [ - %{handle: "shape3", and_where: nil} + %{shape_id: "shape3", and_where: nil} ] } } @@ -439,14 +439,14 @@ defmodule Electric.Shapes.FilterTest do values: %{ 1 => [ %{ - handle: "shape1", + shape_id: "shape1", and_where: nil, shape: Shape.new!("the_table", where: "id = 1", inspector: @inspector) } ], 2 => [ %{ - handle: "shape2", + shape_id: "shape2", and_where: nil, shape: Shape.new!("the_table", where: "id = 2", inspector: @inspector) } @@ -465,7 +465,7 @@ defmodule Electric.Shapes.FilterTest do type: :int8, values: %{ 1 => [ - %{handle: "not-this-shape-1", and_where: nil} + %{shape_id: "not-this-shape-1", and_where: nil} ] } } @@ -503,14 +503,14 @@ defmodule Electric.Shapes.FilterTest do values: %{ 1 => [ %{ - handle: "shape1", + shape_id: "shape1", and_where: nil, shape: Shape.new!("the_table", where: "id = 1", inspector: @inspector) } ], 2 => [ %{ - handle: "shape2", + shape_id: "shape2", and_where: nil, shape: Shape.new!("the_table", where: "id = 2", inspector: @inspector) } @@ -531,7 +531,7 @@ defmodule Electric.Shapes.FilterTest do type: :int8, values: %{ 1 => [ - %{handle: "not-this-shape-1", and_where: nil} + %{shape_id: "not-this-shape-1", and_where: nil} ] } } From 7c22ad7d62371c0c4440eef76415de28c6e599e7 Mon Sep 17 00:00:00 2001 From: rob Date: Mon, 2 Dec 2024 08:53:47 +0000 Subject: [PATCH 39/62] Refactor all_shapes --- .../lib/electric/shapes/filter.ex | 19 +++---------------- .../lib/electric/shapes/filter/index.ex | 8 ++++++++ .../lib/electric/shapes/filter/table.ex | 7 +++++++ 3 files changed, 18 insertions(+), 16 deletions(-) diff --git a/packages/sync-service/lib/electric/shapes/filter.ex b/packages/sync-service/lib/electric/shapes/filter.ex index 413534fe14..78e1e82b99 100644 --- a/packages/sync-service/lib/electric/shapes/filter.ex +++ b/packages/sync-service/lib/electric/shapes/filter.ex @@ -96,29 +96,16 @@ defmodule Electric.Shapes.Filter do defp all_shapes_in_filter(%Filter{} = filter) do for {_table, table} <- filter.tables, - {shape_id, shape} <- all_shapes_in_table(table), + {shape_id, shape} <- Table.all_shapes(table), into: %{} do {shape_id, shape} end end - defp all_shapes_in_table(%{indexes: indexes, other_shapes: other_shapes}) do - for {_field, %{values: values}} <- indexes, - {_value, shapes} <- values, - %{shape_id: shape_id, shape: shape} <- shapes, - into: %{} do - {shape_id, shape} - end - |> Map.merge(other_shapes) - end - defp all_shapes_for_table(%Filter{} = filter, table_name) do case Map.get(filter.tables, table_name) do - nil -> - %{} - - table -> - all_shapes_in_table(table) + nil -> %{} + table -> Table.all_shapes(table) end end end diff --git a/packages/sync-service/lib/electric/shapes/filter/index.ex b/packages/sync-service/lib/electric/shapes/filter/index.ex index 74a8a2a512..748a9b4907 100644 --- a/packages/sync-service/lib/electric/shapes/filter/index.ex +++ b/packages/sync-service/lib/electric/shapes/filter/index.ex @@ -61,4 +61,12 @@ defmodule Electric.Shapes.Filter.Index do # TODO: Move record_in_shape? out of shapes into Where module Shape.record_in_shape?(%{where: where_clause}, record) end + + def all_shapes(%Index{values: values}) do + for {_value, shapes} <- values, + %{shape_id: shape_id, shape: shape} <- shapes, + into: %{} do + {shape_id, shape} + end + end end diff --git a/packages/sync-service/lib/electric/shapes/filter/table.ex b/packages/sync-service/lib/electric/shapes/filter/table.ex index 9dcc70c457..b862e4ad38 100644 --- a/packages/sync-service/lib/electric/shapes/filter/table.ex +++ b/packages/sync-service/lib/electric/shapes/filter/table.ex @@ -113,4 +113,11 @@ defmodule Electric.Shapes.Filter.Table do shape_id end end + + def all_shapes(%Table{indexes: indexes, other_shapes: other_shapes}) do + for {_field, index} <- indexes, {shape_id, shape} <- Index.all_shapes(index), into: %{} do + {shape_id, shape} + end + |> Map.merge(other_shapes) + end end From dbf1192c8f8388f63dbfc734c865f2108fb9223d Mon Sep 17 00:00:00 2001 From: rob Date: Mon, 2 Dec 2024 09:07:53 +0000 Subject: [PATCH 40/62] Gracefully handle parsing errors --- .../lib/electric/shapes/filter/index.ex | 23 +++++++++++++++---- .../test/electric/shapes/filter_test.exs | 3 ++- 2 files changed, 20 insertions(+), 6 deletions(-) diff --git a/packages/sync-service/lib/electric/shapes/filter/index.ex b/packages/sync-service/lib/electric/shapes/filter/index.ex index 748a9b4907..8165f66da1 100644 --- a/packages/sync-service/lib/electric/shapes/filter/index.ex +++ b/packages/sync-service/lib/electric/shapes/filter/index.ex @@ -2,6 +2,7 @@ defmodule Electric.Shapes.Filter.Index do alias Electric.Replication.Eval.Env alias Electric.Shapes.Filter.Index alias Electric.Shapes.Shape + require Logger defstruct [:type, :values] @@ -35,8 +36,22 @@ defmodule Electric.Shapes.Filter.Index do } end - def affected_shapes(%Index{values: values, type: type}, field, record) do - case values[value_from_record(record, field, type)] do + def affected_shapes(%Index{values: values, type: type} = index, field, record) do + case value_from_record(record, field, type) do + {:ok, value} -> + shapes_for_value(value, values, record) + + :error -> + Logger.error("Could not parse value for field #{inspect(field)} of type #{inspect(type)}") + # We can't tell which shapes are affected, the safest thing to do is return all shapes + index + |> all_shapes() + |> MapSet.new(fn {shape_id, _shape} -> shape_id end) + end + end + + defp shapes_for_value(value, values, record) do + case values[value] do nil -> MapSet.new() @@ -50,9 +65,7 @@ defmodule Electric.Shapes.Filter.Index do @env Env.new() defp value_from_record(record, field, type) do - # TODO: should we expect this to be ok? - {:ok, value} = Env.parse_const(@env, record[field], type) - value + Env.parse_const(@env, record[field], type) end defp record_in_where?(nil, _), do: true diff --git a/packages/sync-service/test/electric/shapes/filter_test.exs b/packages/sync-service/test/electric/shapes/filter_test.exs index bbe81e4b1d..8aca503230 100644 --- a/packages/sync-service/test/electric/shapes/filter_test.exs +++ b/packages/sync-service/test/electric/shapes/filter_test.exs @@ -570,7 +570,8 @@ defmodule Electric.Shapes.FilterTest do %{where: "id = 7 AND id > 8", record: %{"id" => "7"}, affected: false}, %{where: "id > 1 AND id = 7", record: %{"id" => "7"}, affected: true}, %{where: "id > 1 AND id = 7", record: %{"id" => "8"}, affected: false}, - %{where: "id > 8 AND id = 7", record: %{"id" => "7"}, affected: false} + %{where: "id > 8 AND id = 7", record: %{"id" => "7"}, affected: false}, + %{where: "id = 7", record: %{"id" => "invalid_value"}, affected: true} ] do test "where: #{test.where}, record: #{inspect(test.record)}" do %{where: where, record: record, affected: affected} = unquote(Macro.escape(test)) From 32e75e9435e0faabbe67cd6a27d2d46b6426fb5e Mon Sep 17 00:00:00 2001 From: rob Date: Mon, 2 Dec 2024 09:09:34 +0000 Subject: [PATCH 41/62] Hide deliberate error in test --- packages/sync-service/test/electric/shapes/filter_test.exs | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/sync-service/test/electric/shapes/filter_test.exs b/packages/sync-service/test/electric/shapes/filter_test.exs index 8aca503230..e7bef9cb03 100644 --- a/packages/sync-service/test/electric/shapes/filter_test.exs +++ b/packages/sync-service/test/electric/shapes/filter_test.exs @@ -573,6 +573,7 @@ defmodule Electric.Shapes.FilterTest do %{where: "id > 8 AND id = 7", record: %{"id" => "7"}, affected: false}, %{where: "id = 7", record: %{"id" => "invalid_value"}, affected: true} ] do + @tag :capture_log test "where: #{test.where}, record: #{inspect(test.record)}" do %{where: where, record: record, affected: affected} = unquote(Macro.escape(test)) From b150fed7bb3fe7097d8a11cb5077667e6be3c9b8 Mon Sep 17 00:00:00 2001 From: rob Date: Mon, 2 Dec 2024 09:20:54 +0000 Subject: [PATCH 42/62] Test error logging for invalid error --- .../test/electric/shapes/filter_test.exs | 49 ++++++++++--------- 1 file changed, 27 insertions(+), 22 deletions(-) diff --git a/packages/sync-service/test/electric/shapes/filter_test.exs b/packages/sync-service/test/electric/shapes/filter_test.exs index e7bef9cb03..62bf7b38d0 100644 --- a/packages/sync-service/test/electric/shapes/filter_test.exs +++ b/packages/sync-service/test/electric/shapes/filter_test.exs @@ -1,5 +1,6 @@ defmodule Electric.Shapes.FilterTest do use ExUnit.Case + import ExUnit.CaptureLog alias Electric.Replication.Changes.DeletedRecord alias Electric.Replication.Changes.NewRecord alias Electric.Replication.Changes.Relation @@ -570,35 +571,39 @@ defmodule Electric.Shapes.FilterTest do %{where: "id = 7 AND id > 8", record: %{"id" => "7"}, affected: false}, %{where: "id > 1 AND id = 7", record: %{"id" => "7"}, affected: true}, %{where: "id > 1 AND id = 7", record: %{"id" => "8"}, affected: false}, - %{where: "id > 8 AND id = 7", record: %{"id" => "7"}, affected: false}, - %{where: "id = 7", record: %{"id" => "invalid_value"}, affected: true} + %{where: "id > 8 AND id = 7", record: %{"id" => "7"}, affected: false} ] do - @tag :capture_log test "where: #{test.where}, record: #{inspect(test.record)}" do %{where: where, record: record, affected: affected} = unquote(Macro.escape(test)) - shape = Shape.new!("the_table", where: where, inspector: @inspector) + assert affected?(where, record) == affected + end + end - transaction = - %Transaction{ - changes: [ - %NewRecord{ - relation: {"public", "the_table"}, - record: record - } - ] - } + test "Invalid record value logs an error and says all shapes are affected" do + log = + capture_log(fn -> + assert affected?("id = 7", %{"id" => "invalid_value"}) + end) - expected_affected_shapes = - if affected do - MapSet.new(["the-shape"]) - else - MapSet.new([]) - end + assert log =~ ~s(Could not parse value for field "id" of type :int8) + end - assert Filter.new(%{"the-shape" => shape}) - |> Filter.affected_shapes(transaction) == expected_affected_shapes - end + defp affected?(where, record) do + shape = Shape.new!("the_table", where: where, inspector: @inspector) + + transaction = + %Transaction{ + changes: [ + %NewRecord{ + relation: {"public", "the_table"}, + record: record + } + ] + } + + Filter.new(%{"the-shape" => shape}) + |> Filter.affected_shapes(transaction) == MapSet.new(["the-shape"]) end end end From c6604497c78f0735a0610ca58258e99f65f4375b Mon Sep 17 00:00:00 2001 From: rob Date: Mon, 2 Dec 2024 09:39:49 +0000 Subject: [PATCH 43/62] Update TODOs --- packages/sync-service/lib/electric/shapes/filter.ex | 4 ---- packages/sync-service/lib/electric/shapes/filter/table.ex | 1 - 2 files changed, 5 deletions(-) diff --git a/packages/sync-service/lib/electric/shapes/filter.ex b/packages/sync-service/lib/electric/shapes/filter.ex index 78e1e82b99..e1656fb01d 100644 --- a/packages/sync-service/lib/electric/shapes/filter.ex +++ b/packages/sync-service/lib/electric/shapes/filter.ex @@ -61,8 +61,6 @@ defmodule Electric.Shapes.Filter do |> Enum.reduce(MapSet.new(), &MapSet.union(&1, &2)) end - # TODO: Optimisation: each time a shape is affected, take it out of `other_shapes` - def affected_shapes(%Filter{} = filter, %NewRecord{relation: relation, record: record}) do affected_shapes_by_record(filter, relation, record) end @@ -78,8 +76,6 @@ defmodule Electric.Shapes.Filter do ) end - # TODO: Optimisation: Do TruncatedRelations first and then just process other changes for other tables - def affected_shapes(%Filter{} = filter, %TruncatedRelation{relation: table_name}) do for {shape_id, _shape} <- all_shapes_for_table(filter, table_name), into: MapSet.new() do diff --git a/packages/sync-service/lib/electric/shapes/filter/table.ex b/packages/sync-service/lib/electric/shapes/filter/table.ex index b862e4ad38..9e1696066f 100644 --- a/packages/sync-service/lib/electric/shapes/filter/table.ex +++ b/packages/sync-service/lib/electric/shapes/filter/table.ex @@ -107,7 +107,6 @@ defmodule Electric.Shapes.Filter.Table do defp other_shapes_affected(%{other_shapes: shapes}, record) do for {shape_id, shape} <- shapes, - # TODO: Test Shape.record_in_shape? is called Shape.record_in_shape?(shape, record), into: MapSet.new() do shape_id From 6dd4a487dc8e1ee361946b2d8ef28e50f21c1512 Mon Sep 17 00:00:00 2001 From: rob Date: Mon, 2 Dec 2024 10:45:16 +0000 Subject: [PATCH 44/62] Refactor code into WhereClause module --- .../lib/electric/shapes/filter/index.ex | 11 ++--------- .../lib/electric/shapes/filter/table.ex | 4 ++-- .../sync-service/lib/electric/shapes/shape.ex | 19 ++++--------------- .../lib/electric/shapes/where_clause.ex | 14 ++++++++++++++ 4 files changed, 22 insertions(+), 26 deletions(-) create mode 100644 packages/sync-service/lib/electric/shapes/where_clause.ex diff --git a/packages/sync-service/lib/electric/shapes/filter/index.ex b/packages/sync-service/lib/electric/shapes/filter/index.ex index 8165f66da1..5eab54d456 100644 --- a/packages/sync-service/lib/electric/shapes/filter/index.ex +++ b/packages/sync-service/lib/electric/shapes/filter/index.ex @@ -1,7 +1,7 @@ defmodule Electric.Shapes.Filter.Index do alias Electric.Replication.Eval.Env alias Electric.Shapes.Filter.Index - alias Electric.Shapes.Shape + alias Electric.Shapes.WhereClause require Logger defstruct [:type, :values] @@ -57,7 +57,7 @@ defmodule Electric.Shapes.Filter.Index do shapes -> shapes - |> Enum.filter(&record_in_where?(&1.and_where, record)) + |> Enum.filter(&WhereClause.includes_record?(&1.and_where, record)) |> Enum.map(& &1.shape_id) |> MapSet.new() end @@ -68,13 +68,6 @@ defmodule Electric.Shapes.Filter.Index do Env.parse_const(@env, record[field], type) end - defp record_in_where?(nil, _), do: true - - defp record_in_where?(where_clause, record) do - # TODO: Move record_in_shape? out of shapes into Where module - Shape.record_in_shape?(%{where: where_clause}, record) - end - def all_shapes(%Index{values: values}) do for {_value, shapes} <- values, %{shape_id: shape_id, shape: shape} <- shapes, diff --git a/packages/sync-service/lib/electric/shapes/filter/table.ex b/packages/sync-service/lib/electric/shapes/filter/table.ex index 9e1696066f..c091de923a 100644 --- a/packages/sync-service/lib/electric/shapes/filter/table.ex +++ b/packages/sync-service/lib/electric/shapes/filter/table.ex @@ -6,7 +6,7 @@ defmodule Electric.Shapes.Filter.Table do alias Electric.Replication.Eval.Parser.Ref alias Electric.Shapes.Filter.Index alias Electric.Shapes.Filter.Table - alias Electric.Shapes.Shape + alias Electric.Shapes.WhereClause defstruct indexes: %{}, other_shapes: %{} @@ -107,7 +107,7 @@ defmodule Electric.Shapes.Filter.Table do defp other_shapes_affected(%{other_shapes: shapes}, record) do for {shape_id, shape} <- shapes, - Shape.record_in_shape?(shape, record), + WhereClause.includes_record?(shape.where, record), into: MapSet.new() do shape_id end diff --git a/packages/sync-service/lib/electric/shapes/shape.ex b/packages/sync-service/lib/electric/shapes/shape.ex index be2452cc7d..96859c2735 100644 --- a/packages/sync-service/lib/electric/shapes/shape.ex +++ b/packages/sync-service/lib/electric/shapes/shape.ex @@ -5,8 +5,8 @@ defmodule Electric.Shapes.Shape do require Logger alias Electric.Postgres.Inspector alias Electric.Replication.Eval.Parser - alias Electric.Replication.Eval.Runner alias Electric.Replication.Changes + alias Electric.Shapes.WhereClause @enforce_keys [:root_table, :root_table_id] defstruct [ @@ -218,7 +218,7 @@ defmodule Electric.Shapes.Shape do when is_struct(change, Changes.DeletedRecord) do record = if is_struct(change, Changes.NewRecord), do: change.record, else: change.old_record - if record_in_shape?(shape, record), + if WhereClause.includes_record?(shape.where, record), do: [filter_change_columns(selected_columns, change)], else: [] end @@ -227,8 +227,8 @@ defmodule Electric.Shapes.Shape do %__MODULE__{selected_columns: selected_columns} = shape, %Changes.UpdatedRecord{old_record: old_record, record: record} = change ) do - old_record_in_shape = record_in_shape?(shape, old_record) - new_record_in_shape = record_in_shape?(shape, record) + old_record_in_shape = WhereClause.includes_record?(shape.where, old_record) + new_record_in_shape = WhereClause.includes_record?(shape.where, record) converted_changes = case {old_record_in_shape, new_record_in_shape} do @@ -254,17 +254,6 @@ defmodule Electric.Shapes.Shape do defp filtered_columns_changed(_), do: true - def record_in_shape?(%{where: nil}, _record), do: true - - def record_in_shape?(%{where: where}, record) do - with {:ok, refs} <- Runner.record_to_ref_values(where.used_refs, record), - {:ok, evaluated} <- Runner.execute(where, refs) do - if is_nil(evaluated), do: false, else: evaluated - else - _ -> false - end - end - # If relation OID matches, but qualified table name does not, then shape is affected def is_affected_by_relation_change?( %__MODULE__{root_table_id: id, root_table: {shape_schema, shape_table}}, diff --git a/packages/sync-service/lib/electric/shapes/where_clause.ex b/packages/sync-service/lib/electric/shapes/where_clause.ex new file mode 100644 index 0000000000..f3bb276cea --- /dev/null +++ b/packages/sync-service/lib/electric/shapes/where_clause.ex @@ -0,0 +1,14 @@ +defmodule Electric.Shapes.WhereClause do + alias Electric.Replication.Eval.Runner + + def includes_record?(nil = _where_clause, _record), do: true + + def includes_record?(where_clause, record) do + with {:ok, refs} <- Runner.record_to_ref_values(where_clause.used_refs, record), + {:ok, evaluated} <- Runner.execute(where_clause, refs) do + if is_nil(evaluated), do: false, else: evaluated + else + _ -> false + end + end +end From 81be707410f18ad7425c6e80e7cc88226329e575 Mon Sep 17 00:00:00 2001 From: rob Date: Mon, 2 Dec 2024 10:56:50 +0000 Subject: [PATCH 45/62] Remove Filter.new/1 --- .../lib/electric/shapes/filter.ex | 9 +-------- .../test/electric/shapes/filter_test.exs | 19 ++++++++++--------- 2 files changed, 11 insertions(+), 17 deletions(-) diff --git a/packages/sync-service/lib/electric/shapes/filter.ex b/packages/sync-service/lib/electric/shapes/filter.ex index e1656fb01d..5915353193 100644 --- a/packages/sync-service/lib/electric/shapes/filter.ex +++ b/packages/sync-service/lib/electric/shapes/filter.ex @@ -11,12 +11,7 @@ defmodule Electric.Shapes.Filter do defstruct tables: %{} - def new(shapes), do: shapes |> Map.to_list() |> new(empty()) - - defp new([{shape_id, shape} | shapes], filter), - do: new(shapes, add_shape(filter, shape_id, shape)) - - defp new([], filter), do: filter + def empty, do: %Filter{} def add_shape(%Filter{tables: tables}, shape_id, shape) do %Filter{ @@ -44,8 +39,6 @@ defmodule Electric.Shapes.Filter do } end - def empty, do: %Filter{} - def affected_shapes(%Filter{} = filter, %Relation{} = relation) do # Check all shapes is all tables becuase the table may have been renamed for {shape_id, shape} <- all_shapes_in_filter(filter), diff --git a/packages/sync-service/test/electric/shapes/filter_test.exs b/packages/sync-service/test/electric/shapes/filter_test.exs index 62bf7b38d0..65152d964c 100644 --- a/packages/sync-service/test/electric/shapes/filter_test.exs +++ b/packages/sync-service/test/electric/shapes/filter_test.exs @@ -19,11 +19,11 @@ defmodule Electric.Shapes.FilterTest do @inspector StubInspector.new([%{name: "id", type: "int8", pk_position: 0}]) - describe "new/1" do + describe "add_shape/2" do test "with `field = constant` where clause" do shape = Shape.new!("the_table", where: "id = 1", inspector: @inspector) - assert Filter.new(%{"shape1" => shape}) == %Filter{ + assert Filter.add_shape(Filter.empty(), "shape1", shape) == %Filter{ tables: %{ {"public", "the_table"} => %Table{ indexes: %{ @@ -45,7 +45,7 @@ defmodule Electric.Shapes.FilterTest do test "with `constant = field` where clause" do shape = Shape.new!("the_table", where: "1 = id", inspector: @inspector) - assert Filter.new(%{"shape1" => shape}) == %Filter{ + assert Filter.add_shape(Filter.empty(), "shape1", shape) == %Filter{ tables: %{ {"public", "the_table"} => %Table{ indexes: %{ @@ -97,7 +97,7 @@ defmodule Electric.Shapes.FilterTest do other_shapes: %{} } } - } = Filter.new(%{"shape1" => shape}) + } = Filter.add_shape(Filter.empty(), "shape1", shape) end test "with `some_condition AND field = constant` where clause" do @@ -133,17 +133,17 @@ defmodule Electric.Shapes.FilterTest do other_shapes: %{} } } - } = Filter.new(%{"shape1" => shape}) + } = Filter.add_shape(Filter.empty(), "shape1", shape) end test "with more complicated where clause" do - shapes = %{"shape1" => Shape.new!("the_table", where: "id > 1", inspector: @inspector)} + shape = Shape.new!("the_table", where: "id > 1", inspector: @inspector) - assert Filter.new(shapes) == %Filter{ + assert Filter.add_shape(Filter.empty(), "the-shape", shape) == %Filter{ tables: %{ {"public", "the_table"} => %Table{ indexes: %{}, - other_shapes: shapes + other_shapes: %{"the-shape" => shape} } } } @@ -602,7 +602,8 @@ defmodule Electric.Shapes.FilterTest do ] } - Filter.new(%{"the-shape" => shape}) + Filter.empty() + |> Filter.add_shape("the-shape", shape) |> Filter.affected_shapes(transaction) == MapSet.new(["the-shape"]) end end From e9822d6771a4729509a68d79b8b44314b0043465 Mon Sep 17 00:00:00 2001 From: rob Date: Mon, 2 Dec 2024 15:41:14 +0000 Subject: [PATCH 46/62] Test arrays --- .../test/electric/shapes/filter_test.exs | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/packages/sync-service/test/electric/shapes/filter_test.exs b/packages/sync-service/test/electric/shapes/filter_test.exs index 65152d964c..7b097483dd 100644 --- a/packages/sync-service/test/electric/shapes/filter_test.exs +++ b/packages/sync-service/test/electric/shapes/filter_test.exs @@ -17,7 +17,10 @@ defmodule Electric.Shapes.FilterTest do alias Electric.Shapes.Shape alias Support.StubInspector - @inspector StubInspector.new([%{name: "id", type: "int8", pk_position: 0}]) + @inspector StubInspector.new([ + %{name: "id", type: "int8", pk_position: 0}, + %{name: "an_array", array_type: "int8"} + ]) describe "add_shape/2" do test "with `field = constant` where clause" do @@ -42,6 +45,7 @@ defmodule Electric.Shapes.FilterTest do } end + # TODO optimise nil where clause test "with `constant = field` where clause" do shape = Shape.new!("the_table", where: "1 = id", inspector: @inspector) @@ -491,7 +495,6 @@ defmodule Electric.Shapes.FilterTest do MapSet.new(["shape1", "shape2", "shape3", "shape4"]) end - # TODO: Also go through Shape.is_affected_by_relation_change? tests to see if all scenarious are covered here # TODO: Also go through Shape.convert_change tests to see if all scenarious are covered here test "returns shapes affected by truncation" do @@ -571,7 +574,10 @@ defmodule Electric.Shapes.FilterTest do %{where: "id = 7 AND id > 8", record: %{"id" => "7"}, affected: false}, %{where: "id > 1 AND id = 7", record: %{"id" => "7"}, affected: true}, %{where: "id > 1 AND id = 7", record: %{"id" => "8"}, affected: false}, - %{where: "id > 8 AND id = 7", record: %{"id" => "7"}, affected: false} + %{where: "id > 8 AND id = 7", record: %{"id" => "7"}, affected: false}, + %{where: "an_array = '{1}'", record: %{"an_array" => "{1}"}, affected: true}, + %{where: "an_array = '{1}'", record: %{"an_array" => "{2}"}, affected: false}, + %{where: "an_array = '{1}'", record: %{"an_array" => "{1,2}"}, affected: false} ] do test "where: #{test.where}, record: #{inspect(test.record)}" do %{where: where, record: record, affected: affected} = unquote(Macro.escape(test)) From 7f474a1f400f3e1745e60f2fb6eca8a509980ac1 Mon Sep 17 00:00:00 2001 From: rob Date: Mon, 2 Dec 2024 15:52:04 +0000 Subject: [PATCH 47/62] Test shape with no where clause --- .../test/electric/shapes/filter_test.exs | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/packages/sync-service/test/electric/shapes/filter_test.exs b/packages/sync-service/test/electric/shapes/filter_test.exs index 7b097483dd..2580fdfabe 100644 --- a/packages/sync-service/test/electric/shapes/filter_test.exs +++ b/packages/sync-service/test/electric/shapes/filter_test.exs @@ -562,6 +562,24 @@ defmodule Electric.Shapes.FilterTest do end describe "where clause filtering" do + test "shape with no where clause is always affected" do + shape = Shape.new!("the_table", inspector: @inspector) + + transaction = + %Transaction{ + changes: [ + %NewRecord{ + relation: {"public", "the_table"}, + record: %{"id" => "7"} + } + ] + } + + assert Filter.empty() + |> Filter.add_shape("the-shape", shape) + |> Filter.affected_shapes(transaction) == MapSet.new(["the-shape"]) + end + for test <- [ %{where: "id = 7", record: %{"id" => "7"}, affected: true}, %{where: "id = 7", record: %{"id" => "8"}, affected: false}, From ab3b970ae1a5112e4068ef357ca913e59015a2b7 Mon Sep 17 00:00:00 2001 From: rob Date: Mon, 2 Dec 2024 15:56:07 +0000 Subject: [PATCH 48/62] Update TODOs --- packages/sync-service/test/electric/shapes/filter_test.exs | 3 --- 1 file changed, 3 deletions(-) diff --git a/packages/sync-service/test/electric/shapes/filter_test.exs b/packages/sync-service/test/electric/shapes/filter_test.exs index 2580fdfabe..9d09525875 100644 --- a/packages/sync-service/test/electric/shapes/filter_test.exs +++ b/packages/sync-service/test/electric/shapes/filter_test.exs @@ -45,7 +45,6 @@ defmodule Electric.Shapes.FilterTest do } end - # TODO optimise nil where clause test "with `constant = field` where clause" do shape = Shape.new!("the_table", where: "1 = id", inspector: @inspector) @@ -495,8 +494,6 @@ defmodule Electric.Shapes.FilterTest do MapSet.new(["shape1", "shape2", "shape3", "shape4"]) end - # TODO: Also go through Shape.convert_change tests to see if all scenarious are covered here - test "returns shapes affected by truncation" do filter = %Filter{ tables: %{ From 29fb39d7642c4b9222afa1a97be03c2ed860c133 Mon Sep 17 00:00:00 2001 From: rob Date: Mon, 2 Dec 2024 15:58:54 +0000 Subject: [PATCH 49/62] Update moduledoc --- .../sync-service/lib/electric/shapes/dispatcher.ex | 14 ++------------ 1 file changed, 2 insertions(+), 12 deletions(-) diff --git a/packages/sync-service/lib/electric/shapes/dispatcher.ex b/packages/sync-service/lib/electric/shapes/dispatcher.ex index 000a045218..cd314186e9 100644 --- a/packages/sync-service/lib/electric/shapes/dispatcher.ex +++ b/packages/sync-service/lib/electric/shapes/dispatcher.ex @@ -1,17 +1,7 @@ defmodule Electric.Shapes.Dispatcher do - # TODO : Update @moduledoc @moduledoc """ - Dispatches transactions and relations to consumers filtered according to the - subscriber's `selector` function. - - To receive all messages, don't pass a selector function or use `nil`, e.g. - - ``` - def init(producer) do - {:consumer, :nostate, subscribe_to: [{producer, [max_demand: 1, selector: nil]}]} - end - - ``` + Dispatches transactions and relations to consumers filtered using `Filter` + and the consumer's shape. The essential behaviour is that the dispatcher only asks the producer for more demand once all relevant subscribers have processed the last message and From c452bc4d2c37d7f0d8f122fb8ee872b1f101e382 Mon Sep 17 00:00:00 2001 From: rob Date: Mon, 2 Dec 2024 16:07:01 +0000 Subject: [PATCH 50/62] Add moduledoc --- packages/sync-service/lib/electric/shapes/filter.ex | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/packages/sync-service/lib/electric/shapes/filter.ex b/packages/sync-service/lib/electric/shapes/filter.ex index 5915353193..c217185796 100644 --- a/packages/sync-service/lib/electric/shapes/filter.ex +++ b/packages/sync-service/lib/electric/shapes/filter.ex @@ -1,4 +1,10 @@ defmodule Electric.Shapes.Filter do + @moduledoc """ + Responsible for knowing which shapes are affected by a change. + + `affected_shapes(filter, change)` will return a set shape IDs that are affected by a change. + """ + alias Electric.Replication.Changes.DeletedRecord alias Electric.Replication.Changes.NewRecord alias Electric.Replication.Changes.Relation From 0ac30a6bee2201d7d82f0de80474897e886cb8a9 Mon Sep 17 00:00:00 2001 From: rob Date: Mon, 2 Dec 2024 16:43:50 +0000 Subject: [PATCH 51/62] Add moduledocs --- packages/sync-service/lib/electric/shapes/filter.ex | 7 ++++++- .../sync-service/lib/electric/shapes/filter/index.ex | 10 ++++++++++ .../sync-service/lib/electric/shapes/filter/table.ex | 8 ++++++++ 3 files changed, 24 insertions(+), 1 deletion(-) diff --git a/packages/sync-service/lib/electric/shapes/filter.ex b/packages/sync-service/lib/electric/shapes/filter.ex index c217185796..ba2be00750 100644 --- a/packages/sync-service/lib/electric/shapes/filter.ex +++ b/packages/sync-service/lib/electric/shapes/filter.ex @@ -2,7 +2,12 @@ defmodule Electric.Shapes.Filter do @moduledoc """ Responsible for knowing which shapes are affected by a change. - `affected_shapes(filter, change)` will return a set shape IDs that are affected by a change. + `affected_shapes(filter, change)` will return a set of IDs for the shapes that are affected by the change + considering all the shapes that have been added to the filter using `add_shape/3`. + + + The `Filter` module keeps track of what tables are referenced by the shapes and changes and delegates + the table specific logic to the `Filter.Table` module. """ alias Electric.Replication.Changes.DeletedRecord diff --git a/packages/sync-service/lib/electric/shapes/filter/index.ex b/packages/sync-service/lib/electric/shapes/filter/index.ex index 5eab54d456..d877780665 100644 --- a/packages/sync-service/lib/electric/shapes/filter/index.ex +++ b/packages/sync-service/lib/electric/shapes/filter/index.ex @@ -1,4 +1,14 @@ defmodule Electric.Shapes.Filter.Index do + @moduledoc """ + Responsible for knowing which shapes are affected by a change to a specific field. + + The `%Table{}` struct contains `values` a map of values for a specific field to shapes that are affected by that field value. + This acts as an index for the shapes, providing a fast way to know which shapes have been affected without having to + iterate over all the shapes. + + Currently only `=` operations are indexed. + """ + alias Electric.Replication.Eval.Env alias Electric.Shapes.Filter.Index alias Electric.Shapes.WhereClause diff --git a/packages/sync-service/lib/electric/shapes/filter/table.ex b/packages/sync-service/lib/electric/shapes/filter/table.ex index c091de923a..d25e7e03ac 100644 --- a/packages/sync-service/lib/electric/shapes/filter/table.ex +++ b/packages/sync-service/lib/electric/shapes/filter/table.ex @@ -1,4 +1,12 @@ defmodule Electric.Shapes.Filter.Table do + @moduledoc """ + Responsible for knowing which shapes are affected by a change to a specific table. + + The `%Table{}` struct contains `indexes`, a map of indexes for shapes that have been optimised, and `other_shapes` for shapes + that have not been optimised. The logic for specific indexes is delegated to the `Filter.Index` module. + + """ + alias Electric.Replication.Eval.Expr alias Electric.Replication.Eval.Parser alias Electric.Replication.Eval.Parser.Const From efe361384600491f72c8066545b510d256a441be Mon Sep 17 00:00:00 2001 From: rob Date: Mon, 2 Dec 2024 16:46:59 +0000 Subject: [PATCH 52/62] Remove TODO --- packages/sync-service/lib/electric/shapes/filter/table.ex | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/sync-service/lib/electric/shapes/filter/table.ex b/packages/sync-service/lib/electric/shapes/filter/table.ex index d25e7e03ac..54656f5d8a 100644 --- a/packages/sync-service/lib/electric/shapes/filter/table.ex +++ b/packages/sync-service/lib/electric/shapes/filter/table.ex @@ -58,7 +58,6 @@ defmodule Electric.Shapes.Filter.Table do defp optimise_where(%Expr{eval: eval}), do: optimise_where(eval) - # TODO: Is this really ~s("=") or is it just "="? defp optimise_where(%Func{ name: ~s("="), args: [%Ref{path: [field], type: type}, %Const{value: value}] From aa4e7c23d6615e79c9a24497fc5e1ec7f54fe76c Mon Sep 17 00:00:00 2001 From: rob Date: Mon, 2 Dec 2024 17:24:42 +0000 Subject: [PATCH 53/62] Rescue all errors in affected_shapes --- .../lib/electric/shapes/filter.ex | 48 ++++++++++++++----- 1 file changed, 35 insertions(+), 13 deletions(-) diff --git a/packages/sync-service/lib/electric/shapes/filter.ex b/packages/sync-service/lib/electric/shapes/filter.ex index ba2be00750..1429682fa9 100644 --- a/packages/sync-service/lib/electric/shapes/filter.ex +++ b/packages/sync-service/lib/electric/shapes/filter.ex @@ -19,6 +19,7 @@ defmodule Electric.Shapes.Filter do alias Electric.Shapes.Filter alias Electric.Shapes.Filter.Table alias Electric.Shapes.Shape + require Logger defstruct tables: %{} @@ -50,51 +51,72 @@ defmodule Electric.Shapes.Filter do } end - def affected_shapes(%Filter{} = filter, %Relation{} = relation) do + def affected_shapes(%Filter{} = filter, change) do + shapes_affected_by_change(filter, change) + rescue + error -> + Logger.error(""" + Unexpected error in Filter.affected_shapes: + #{Exception.format(:error, error, __STACKTRACE__)} + """) + + # We can't tell which shapes are affected, the safest thing to do is return all shapes + filter + |> all_shapes() + |> MapSet.new(fn {shape_id, _shape} -> shape_id end) + end + + defp shapes_affected_by_change(%Filter{} = filter, %Relation{} = relation) do # Check all shapes is all tables becuase the table may have been renamed - for {shape_id, shape} <- all_shapes_in_filter(filter), + for {shape_id, shape} <- all_shapes(filter), Shape.is_affected_by_relation_change?(shape, relation), into: MapSet.new() do shape_id end end - def affected_shapes(%Filter{} = filter, %Transaction{changes: changes}) do + defp shapes_affected_by_change(%Filter{} = filter, %Transaction{changes: changes}) do changes |> Enum.map(&affected_shapes(filter, &1)) |> Enum.reduce(MapSet.new(), &MapSet.union(&1, &2)) end - def affected_shapes(%Filter{} = filter, %NewRecord{relation: relation, record: record}) do - affected_shapes_by_record(filter, relation, record) + defp shapes_affected_by_change(%Filter{} = filter, %NewRecord{ + relation: relation, + record: record + }) do + shapes_affected_by_record(filter, relation, record) end - def affected_shapes(%Filter{} = filter, %DeletedRecord{relation: relation, old_record: record}) do - affected_shapes_by_record(filter, relation, record) + defp shapes_affected_by_change(%Filter{} = filter, %DeletedRecord{ + relation: relation, + old_record: record + }) do + shapes_affected_by_record(filter, relation, record) end - def affected_shapes(%Filter{} = filter, %UpdatedRecord{relation: relation} = change) do + defp shapes_affected_by_change(%Filter{} = filter, %UpdatedRecord{relation: relation} = change) do MapSet.union( - affected_shapes_by_record(filter, relation, change.record), - affected_shapes_by_record(filter, relation, change.old_record) + shapes_affected_by_record(filter, relation, change.record), + shapes_affected_by_record(filter, relation, change.old_record) ) end - def affected_shapes(%Filter{} = filter, %TruncatedRelation{relation: table_name}) do + defp shapes_affected_by_change(%Filter{} = filter, %TruncatedRelation{relation: table_name}) do for {shape_id, _shape} <- all_shapes_for_table(filter, table_name), into: MapSet.new() do shape_id end end - defp affected_shapes_by_record(filter, table_name, record) do + defp shapes_affected_by_record(filter, table_name, record) do case Map.get(filter.tables, table_name) do nil -> MapSet.new() table -> Table.affected_shapes(table, record) end end - defp all_shapes_in_filter(%Filter{} = filter) do + defp all_shapes(%Filter{} = filter) do for {_table, table} <- filter.tables, {shape_id, shape} <- Table.all_shapes(table), into: %{} do From f91c830ba961b0948cb7903c247e4d92ece00041 Mon Sep 17 00:00:00 2001 From: rob Date: Mon, 2 Dec 2024 17:39:58 +0000 Subject: [PATCH 54/62] Remove unnecessary field from dispatcher state --- .../lib/electric/shapes/dispatcher.ex | 49 ++++++++++--------- 1 file changed, 25 insertions(+), 24 deletions(-) diff --git a/packages/sync-service/lib/electric/shapes/dispatcher.ex b/packages/sync-service/lib/electric/shapes/dispatcher.ex index cd314186e9..653764bcfd 100644 --- a/packages/sync-service/lib/electric/shapes/dispatcher.ex +++ b/packages/sync-service/lib/electric/shapes/dispatcher.ex @@ -25,7 +25,7 @@ defmodule Electric.Shapes.Dispatcher do alias Electric.Shapes.Filter defmodule State do - defstruct [:n, :waiting, :pending, :subs, :filter, :pids] + defstruct [:waiting, :pending, :subscribers, :filter, :pids] end @behaviour GenStage.Dispatcher @@ -35,17 +35,16 @@ defmodule Electric.Shapes.Dispatcher do def init(_opts) do {:ok, %State{ - n: 0, waiting: 0, pending: nil, - subs: [], + subscribers: [], filter: Filter.empty(), pids: MapSet.new() }} end @impl GenStage.Dispatcher - def subscribe(opts, {pid, _ref} = from, %State{n: n, pids: pids} = state) do + def subscribe(opts, {pid, _ref} = from, %State{pids: pids} = state) do if MapSet.member?(pids, pid) do Logger.error(fn -> "#{inspect(pid)} is already registered with #{inspect(self())}. " <> @@ -56,21 +55,26 @@ defmodule Electric.Shapes.Dispatcher do else shape = Keyword.fetch!(opts, :shape) - subs = [{from, shape} | state.subs] + demand = if state.subscribers == [], do: 1, else: 0 - demand = if n == 0, do: 1, else: 0 + subscribers = [{from, shape} | state.subscribers] filter = Filter.add_shape(state.filter, from, shape) {:ok, demand, - %State{state | n: n + 1, subs: subs, filter: filter, pids: MapSet.put(state.pids, pid)}} + %State{ + state + | subscribers: subscribers, + filter: filter, + pids: MapSet.put(state.pids, pid) + }} end end @impl GenStage.Dispatcher - def cancel({pid, _ref} = from, %State{n: n, waiting: waiting, pending: pending} = state) do + def cancel({pid, _ref} = from, %State{waiting: waiting, pending: pending} = state) do if MapSet.member?(state.pids, pid) do - subs = List.keydelete(state.subs, from, 0) + subscribers = List.keydelete(state.subscribers, from, 0) filter = Filter.remove_shape(state.filter, from) @@ -82,10 +86,9 @@ defmodule Electric.Shapes.Dispatcher do {:ok, 1, %State{ state - | n: n - 1, - waiting: 0, + | waiting: 0, pending: nil, - subs: subs, + subscribers: subscribers, filter: filter, pids: MapSet.delete(state.pids, pid) }} @@ -94,10 +97,9 @@ defmodule Electric.Shapes.Dispatcher do {:ok, 0, %State{ state - | n: n - 1, - waiting: new_waiting, + | waiting: new_waiting, pending: MapSet.delete(pending, from), - subs: subs, + subscribers: subscribers, filter: filter, pids: MapSet.delete(state.pids, pid) }} @@ -106,8 +108,7 @@ defmodule Electric.Shapes.Dispatcher do {:ok, 0, %State{ state - | n: n - 1, - subs: subs, + | subscribers: subscribers, filter: filter, pids: MapSet.delete(state.pids, pid) }} @@ -135,25 +136,25 @@ defmodule Electric.Shapes.Dispatcher do @impl GenStage.Dispatcher # handle the no subscribers case here to make the real dispatch impl easier - def dispatch([event], _length, %State{waiting: 0, subs: []} = state) do + def dispatch([event], _length, %State{waiting: 0, subscribers: []} = state) do {:ok, [event], state} end - def dispatch([event], _length, %State{waiting: 0, subs: subs} = state) do + def dispatch([event], _length, %State{waiting: 0, subscribers: subscribers} = state) do {waiting, pending} = state.filter |> Filter.affected_shapes(event) - |> Enum.reduce({0, MapSet.new()}, fn {pid, ref} = sub, {waiting, pending} -> + |> Enum.reduce({0, MapSet.new()}, fn {pid, ref} = subscriber, {waiting, pending} -> Process.send(pid, {:"$gen_consumer", {self(), ref}, [event]}, [:noconnect]) - {waiting + 1, MapSet.put(pending, sub)} + {waiting + 1, MapSet.put(pending, subscriber)} end) |> case do {0, _pending} -> # even though no subscriber wants the event, we still need to generate demand # so that we can complete the loop in the log collector - [{sub, _selector} | _] = subs - send(self(), {:"$gen_producer", sub, {:ask, 1}}) - {1, MapSet.new([sub])} + [{subscriber, _selector} | _] = subscribers + send(self(), {:"$gen_producer", subscriber, {:ask, 1}}) + {1, MapSet.new([subscriber])} {waiting, pending} -> {waiting, pending} From c0fba1a788ca68a4132f05c1b4cd47c747dbc8c4 Mon Sep 17 00:00:00 2001 From: rob Date: Mon, 2 Dec 2024 17:47:52 +0000 Subject: [PATCH 55/62] Revert unnecessary change --- packages/sync-service/lib/electric/shapes/shape.ex | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/packages/sync-service/lib/electric/shapes/shape.ex b/packages/sync-service/lib/electric/shapes/shape.ex index 96859c2735..9333ffa398 100644 --- a/packages/sync-service/lib/electric/shapes/shape.ex +++ b/packages/sync-service/lib/electric/shapes/shape.ex @@ -213,22 +213,22 @@ defmodule Electric.Shapes.Shape do def convert_change(%__MODULE__{}, %Changes.TruncatedRelation{} = change), do: [change] - def convert_change(%__MODULE__{selected_columns: selected_columns} = shape, change) + def convert_change(%__MODULE__{where: where, selected_columns: selected_columns}, change) when is_struct(change, Changes.NewRecord) when is_struct(change, Changes.DeletedRecord) do record = if is_struct(change, Changes.NewRecord), do: change.record, else: change.old_record - if WhereClause.includes_record?(shape.where, record), + if WhereClause.includes_record?(where, record), do: [filter_change_columns(selected_columns, change)], else: [] end def convert_change( - %__MODULE__{selected_columns: selected_columns} = shape, + %__MODULE__{where: where, selected_columns: selected_columns}, %Changes.UpdatedRecord{old_record: old_record, record: record} = change ) do - old_record_in_shape = WhereClause.includes_record?(shape.where, old_record) - new_record_in_shape = WhereClause.includes_record?(shape.where, record) + old_record_in_shape = WhereClause.includes_record?(where, old_record) + new_record_in_shape = WhereClause.includes_record?(where, record) converted_changes = case {old_record_in_shape, new_record_in_shape} do From 8997ce69e40439ffacb89c46882c93003ad51177 Mon Sep 17 00:00:00 2001 From: rob Date: Mon, 2 Dec 2024 21:18:09 +0000 Subject: [PATCH 56/62] Add typespecs --- packages/sync-service/lib/electric/shapes/filter.ex | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/packages/sync-service/lib/electric/shapes/filter.ex b/packages/sync-service/lib/electric/shapes/filter.ex index 1429682fa9..60082a4786 100644 --- a/packages/sync-service/lib/electric/shapes/filter.ex +++ b/packages/sync-service/lib/electric/shapes/filter.ex @@ -10,6 +10,7 @@ defmodule Electric.Shapes.Filter do the table specific logic to the `Filter.Table` module. """ + alias Electric.Replication.Changes alias Electric.Replication.Changes.DeletedRecord alias Electric.Replication.Changes.NewRecord alias Electric.Replication.Changes.Relation @@ -23,8 +24,13 @@ defmodule Electric.Shapes.Filter do defstruct tables: %{} + @type t :: %Filter{} + @type shape_id :: any() + + @spec empty() :: Filter.t() def empty, do: %Filter{} + @spec add_shape(Filter.t(), shape_id(), Shape.t()) :: Filter.t() def add_shape(%Filter{tables: tables}, shape_id, shape) do %Filter{ tables: @@ -39,6 +45,7 @@ defmodule Electric.Shapes.Filter do } end + @spec remove_shape(Filter.t(), shape_id()) :: Filter.t() def remove_shape(%Filter{tables: tables}, shape_id) do %Filter{ tables: @@ -51,6 +58,7 @@ defmodule Electric.Shapes.Filter do } end + @spec affected_shapes(Filter.t(), Changes.change()) :: MapSet.t(shape_id()) def affected_shapes(%Filter{} = filter, change) do shapes_affected_by_change(filter, change) rescue From 3e40fd7e5290b88c10d1d831977970aa29383325 Mon Sep 17 00:00:00 2001 From: rob Date: Mon, 2 Dec 2024 21:23:54 +0000 Subject: [PATCH 57/62] Add documentation --- packages/sync-service/lib/electric/shapes/filter.ex | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/packages/sync-service/lib/electric/shapes/filter.ex b/packages/sync-service/lib/electric/shapes/filter.ex index 60082a4786..07faf6741c 100644 --- a/packages/sync-service/lib/electric/shapes/filter.ex +++ b/packages/sync-service/lib/electric/shapes/filter.ex @@ -30,6 +30,12 @@ defmodule Electric.Shapes.Filter do @spec empty() :: Filter.t() def empty, do: %Filter{} + @doc """ + Add a shape for the filter to track. + + The `shape_id` can be any term you like to identify the shape. Whatever you use will be returned + by `affected_shapes/2` when the shape is affected by a change. + """ @spec add_shape(Filter.t(), shape_id(), Shape.t()) :: Filter.t() def add_shape(%Filter{tables: tables}, shape_id, shape) do %Filter{ @@ -45,6 +51,9 @@ defmodule Electric.Shapes.Filter do } end + @doc """ + Remove a shape from the filter. + """ @spec remove_shape(Filter.t(), shape_id()) :: Filter.t() def remove_shape(%Filter{tables: tables}, shape_id) do %Filter{ @@ -58,6 +67,10 @@ defmodule Electric.Shapes.Filter do } end + @doc """ + Returns the shape IDs for all shapes that have been added to the filter + that are affected by the given change. + """ @spec affected_shapes(Filter.t(), Changes.change()) :: MapSet.t(shape_id()) def affected_shapes(%Filter{} = filter, change) do shapes_affected_by_change(filter, change) From e478fee8abd857a730bcd1d4d6ea8af127a1c7da Mon Sep 17 00:00:00 2001 From: rob Date: Tue, 3 Dec 2024 08:28:50 +0000 Subject: [PATCH 58/62] Decouple Filter tests from the Filter data structure --- .../lib/electric/shapes/filter/index.ex | 27 +- .../lib/electric/shapes/filter/table.ex | 13 + .../test/electric/shapes/filter_test.exs | 735 +++++------------- 3 files changed, 199 insertions(+), 576 deletions(-) diff --git a/packages/sync-service/lib/electric/shapes/filter/index.ex b/packages/sync-service/lib/electric/shapes/filter/index.ex index d877780665..7a374e481a 100644 --- a/packages/sync-service/lib/electric/shapes/filter/index.ex +++ b/packages/sync-service/lib/electric/shapes/filter/index.ex @@ -46,22 +46,8 @@ defmodule Electric.Shapes.Filter.Index do } end - def affected_shapes(%Index{values: values, type: type} = index, field, record) do - case value_from_record(record, field, type) do - {:ok, value} -> - shapes_for_value(value, values, record) - - :error -> - Logger.error("Could not parse value for field #{inspect(field)} of type #{inspect(type)}") - # We can't tell which shapes are affected, the safest thing to do is return all shapes - index - |> all_shapes() - |> MapSet.new(fn {shape_id, _shape} -> shape_id end) - end - end - - defp shapes_for_value(value, values, record) do - case values[value] do + def affected_shapes(%Index{values: values, type: type}, field, record) do + case Map.get(values, value_from_record(record, field, type)) do nil -> MapSet.new() @@ -75,7 +61,14 @@ defmodule Electric.Shapes.Filter.Index do @env Env.new() defp value_from_record(record, field, type) do - Env.parse_const(@env, record[field], type) + case Env.parse_const(@env, record[field], type) do + {:ok, value} -> + value + + :error -> + raise RuntimeError, + message: "Could not parse value for field #{inspect(field)} of type #{inspect(type)}" + end end def all_shapes(%Index{values: values}) do diff --git a/packages/sync-service/lib/electric/shapes/filter/table.ex b/packages/sync-service/lib/electric/shapes/filter/table.ex index 54656f5d8a..4998d5b066 100644 --- a/packages/sync-service/lib/electric/shapes/filter/table.ex +++ b/packages/sync-service/lib/electric/shapes/filter/table.ex @@ -16,6 +16,8 @@ defmodule Electric.Shapes.Filter.Table do alias Electric.Shapes.Filter.Table alias Electric.Shapes.WhereClause + require Logger + defstruct indexes: %{}, other_shapes: %{} def empty, do: %Table{} @@ -110,6 +112,17 @@ defmodule Electric.Shapes.Filter.Table do |> Enum.map(fn {field, index} -> Index.affected_shapes(index, field, record) end) |> Enum.reduce(MapSet.new(), &MapSet.union(&1, &2)) |> MapSet.union(other_shapes_affected(table, record)) + rescue + error -> + Logger.error(""" + Unexpected error in Filter.Table.affected_shapes: + #{Exception.format(:error, error, __STACKTRACE__)} + """) + + # We can't tell which shapes are affected, the safest thing to do is return all shapes + table + |> all_shapes() + |> MapSet.new(fn {shape_id, _shape} -> shape_id end) end defp other_shapes_affected(%{other_shapes: shapes}, record) do diff --git a/packages/sync-service/test/electric/shapes/filter_test.exs b/packages/sync-service/test/electric/shapes/filter_test.exs index 9d09525875..11340a5f9f 100644 --- a/packages/sync-service/test/electric/shapes/filter_test.exs +++ b/packages/sync-service/test/electric/shapes/filter_test.exs @@ -1,19 +1,15 @@ defmodule Electric.Shapes.FilterTest do use ExUnit.Case + import ExUnit.CaptureLog + alias Electric.Replication.Changes.DeletedRecord alias Electric.Replication.Changes.NewRecord alias Electric.Replication.Changes.Relation alias Electric.Replication.Changes.Transaction alias Electric.Replication.Changes.TruncatedRelation alias Electric.Replication.Changes.UpdatedRecord - alias Electric.Replication.Eval.Expr - alias Electric.Replication.Eval.Parser.Const - alias Electric.Replication.Eval.Parser.Func - alias Electric.Replication.Eval.Parser.Ref alias Electric.Shapes.Filter - alias Electric.Shapes.Filter.Index - alias Electric.Shapes.Filter.Table alias Electric.Shapes.Shape alias Support.StubInspector @@ -22,610 +18,231 @@ defmodule Electric.Shapes.FilterTest do %{name: "an_array", array_type: "int8"} ]) - describe "add_shape/2" do - test "with `field = constant` where clause" do - shape = Shape.new!("the_table", where: "id = 1", inspector: @inspector) - - assert Filter.add_shape(Filter.empty(), "shape1", shape) == %Filter{ - tables: %{ - {"public", "the_table"} => %Table{ - indexes: %{ - "id" => %Index{ - type: :int8, - values: %{ - 1 => [ - %{shape_id: "shape1", and_where: nil, shape: shape} - ] - } - } - }, - other_shapes: %{} - } - } - } - end - - test "with `constant = field` where clause" do - shape = Shape.new!("the_table", where: "1 = id", inspector: @inspector) - - assert Filter.add_shape(Filter.empty(), "shape1", shape) == %Filter{ - tables: %{ - {"public", "the_table"} => %Table{ - indexes: %{ - "id" => %Index{ - type: :int8, - values: %{ - 1 => [ - %{shape_id: "shape1", and_where: nil, shape: shape} - ] - } - } - }, - other_shapes: %{} - } - } - } - end - - test "with `field = constant AND another_condition` where clause" do - shape = Shape.new!("the_table", where: "id = 1 AND id > 0", inspector: @inspector) - - assert %Filter{ - tables: %{ - {"public", "the_table"} => %Table{ - indexes: %{ - "id" => %Index{ - type: :int8, - values: %{ - 1 => [ - %{ - shape_id: "shape1", - and_where: %Expr{ - eval: %Func{ - name: ~s(">"), - args: [ - %Ref{path: ["id"], type: :int8}, - %Const{value: 0, type: :int4} - ] - }, - used_refs: %{["id"] => :int8}, - returns: :bool - }, - shape: ^shape - } - ] - } - } - }, - other_shapes: %{} - } - } - } = Filter.add_shape(Filter.empty(), "shape1", shape) - end - - test "with `some_condition AND field = constant` where clause" do - shape = Shape.new!("the_table", where: "id > 0 AND id = 1", inspector: @inspector) - - assert %Filter{ - tables: %{ - {"public", "the_table"} => %Table{ - indexes: %{ - "id" => %Index{ - type: :int8, - values: %{ - 1 => [ - %{ - shape_id: "shape1", - and_where: %Expr{ - eval: %Func{ - name: ~s(">"), - args: [ - %Ref{path: ["id"], type: :int8}, - %Const{value: 0, type: :int4} - ] - }, - used_refs: %{["id"] => :int8}, - returns: :bool - }, - shape: ^shape - } - ] - } - } - }, - other_shapes: %{} - } - } - } = Filter.add_shape(Filter.empty(), "shape1", shape) - end - - test "with more complicated where clause" do - shape = Shape.new!("the_table", where: "id > 1", inspector: @inspector) - - assert Filter.add_shape(Filter.empty(), "the-shape", shape) == %Filter{ - tables: %{ - {"public", "the_table"} => %Table{ - indexes: %{}, - other_shapes: %{"the-shape" => shape} - } - } - } - end - end - - describe "remove_shape/2" do - test "removes all shapes with the specified shape_id" do - filter = %Filter{ - tables: %{ - {"public", "the_table"} => %Table{ - indexes: %{ - "id" => %Index{ - type: :int8, - values: %{ - 1 => [ - %{ - shape_id: "shape1", - and_where: nil - } - ], - 2 => [ - %{ - shape_id: "shape2", - and_where: nil - } - ] - } - }, - "name" => %Index{ - type: :text, - values: %{ - "bill" => [ - %{ - shape_id: "shape1", - and_where: nil - }, - %{ - shape_id: "shape2", - and_where: nil - } - ] - } - } - }, - other_shapes: %{ - "shape1" => Shape.new!("the_table", where: "id = 1", inspector: @inspector), - "shape2" => Shape.new!("the_table", where: "id = 2", inspector: @inspector) - } - }, - {"public", "another_table"} => %Table{ - indexes: %{ - "id" => %Index{ - type: :int8, - values: %{ - 1 => [ - %{shape_id: "shape1", and_where: nil} - ] - } - } - }, - other_shapes: %{ - "shape1" => Shape.new!("another_table", where: "id = 1", inspector: @inspector) - } - } - } - } - - assert Filter.remove_shape(filter, "shape1") == %Filter{ - tables: %{ - {"public", "the_table"} => %Table{ - indexes: %{ - "id" => %Index{ - type: :int8, - values: %{ - 2 => [ - %{ - shape_id: "shape2", - and_where: nil - } - ] - } - }, - "name" => %Index{ - type: :text, - values: %{ - "bill" => [ - %{ - shape_id: "shape2", - and_where: nil - } - ] - } - } - }, - other_shapes: %{ - "shape2" => Shape.new!("the_table", where: "id = 2", inspector: @inspector) - } - } - } - } - end - end - describe "affected_shapes/2" do - test "shapes with same table and id are returned" do - filter = %Filter{ - tables: %{ - {"public", "the_table"} => %Table{ - indexes: %{ - "id" => %Index{ - type: :int8, - values: %{ - 1 => [ - %{shape_id: "shape1", and_where: nil}, - %{shape_id: "shape2", and_where: nil} - ], - 2 => [ - %{shape_id: "shape3", and_where: nil}, - %{shape_id: "shape4", and_where: nil} - ] - } - } - }, - other_shapes: %{} - }, - {"public", "another_table"} => %Table{ - indexes: %{ - "id" => %Index{ - type: :int8, - values: %{ - 1 => [ - %{shape_id: "shape5", and_where: nil} - ] - } - } - }, - other_shapes: %{} - } - } - } - - transaction = + test "returns shapes affected by insert" do + filter = + Filter.empty() + |> Filter.add_shape("s1", Shape.new!("t1", where: "id = 1", inspector: @inspector)) + |> Filter.add_shape("s2", Shape.new!("t1", where: "id = 2", inspector: @inspector)) + |> Filter.add_shape("s3", Shape.new!("t1", where: "id = 3", inspector: @inspector)) + |> Filter.add_shape("s4", Shape.new!("t2", where: "id = 2", inspector: @inspector)) + + insert = %Transaction{ changes: [ %NewRecord{ - relation: {"public", "the_table"}, - record: %{"id" => "1"} - } - ] - } - - assert Filter.affected_shapes(filter, transaction) == MapSet.new(["shape1", "shape2"]) - end - - test "shapes with same table but different id are not returned" do - filter = %Filter{ - tables: %{ - {"public", "the_table"} => %Table{ - indexes: %{ - "id" => %Index{ - type: :int8, - values: %{ - 1 => [ - %{shape_id: "shape1", and_where: nil}, - %{shape_id: "shape2", and_where: nil} - ] - } - } - }, - other_shapes: %{} - } - } - } - - transaction = - %Transaction{ - changes: [ - %NewRecord{ - relation: {"public", "the_table"}, + relation: {"public", "t1"}, record: %{"id" => "2"} } ] } - assert Filter.affected_shapes(filter, transaction) == MapSet.new([]) - end - - test "shapes with more complicated where clauses are evaluated" do - filter = %Filter{ - tables: %{ - {"public", "the_table"} => %Table{ - indexes: %{}, - other_shapes: %{ - "shape1" => Shape.new!("the_table", where: "id > 7", inspector: @inspector), - "shape2" => Shape.new!("the_table", where: "id > 6", inspector: @inspector), - "shape3" => Shape.new!("the_table", where: "id > 5", inspector: @inspector) - } - } - } - } - - transaction = - %Transaction{ - changes: [ - %NewRecord{ - relation: {"public", "the_table"}, - record: %{"id" => "7"} - } - ] - } - - assert Filter.affected_shapes(filter, transaction) == MapSet.new(["shape2", "shape3"]) + assert Filter.affected_shapes(filter, insert) == MapSet.new(["s2"]) end test "returns shapes affected by delete" do - filter = %Filter{ - tables: %{ - {"public", "the_table"} => %Table{ - indexes: %{ - "id" => %Index{ - type: :int8, - values: %{ - 1 => [ - %{shape_id: "the-shape", and_where: nil} - ] - } - } - }, - other_shapes: %{} - } - } - } - - transaction = + filter = + Filter.empty() + |> Filter.add_shape("s1", Shape.new!("t1", where: "id = 1", inspector: @inspector)) + |> Filter.add_shape("s2", Shape.new!("t1", where: "id = 2", inspector: @inspector)) + |> Filter.add_shape("s3", Shape.new!("t1", where: "id = 3", inspector: @inspector)) + |> Filter.add_shape("s4", Shape.new!("t2", where: "id = 2", inspector: @inspector)) + + delete = %Transaction{ changes: [ %DeletedRecord{ - relation: {"public", "the_table"}, - old_record: %{"id" => "1"} + relation: {"public", "t1"}, + old_record: %{"id" => "2"} } ] } - assert Filter.affected_shapes(filter, transaction) == MapSet.new(["the-shape"]) + assert Filter.affected_shapes(filter, delete) == MapSet.new(["s2"]) end test "returns shapes affected by update" do - filter = %Filter{ - tables: %{ - {"public", "the_table"} => %Table{ - indexes: %{ - "id" => %Index{ - type: :int8, - values: %{ - 1 => [ - %{shape_id: "shape1", and_where: nil} - ], - 2 => [ - %{shape_id: "shape2", and_where: nil} - ], - 3 => [ - %{shape_id: "shape3", and_where: nil} - ] - } - } - }, - other_shapes: %{} - } - } - } - - transaction = + filter = + Filter.empty() + |> Filter.add_shape("s1", Shape.new!("t1", where: "id = 1", inspector: @inspector)) + |> Filter.add_shape("s2", Shape.new!("t1", where: "id = 2", inspector: @inspector)) + |> Filter.add_shape("s3", Shape.new!("t1", where: "id = 3", inspector: @inspector)) + |> Filter.add_shape("s4", Shape.new!("t1", where: "id = 4", inspector: @inspector)) + |> Filter.add_shape("s2", Shape.new!("t2", where: "id = 2", inspector: @inspector)) + + update = %Transaction{ changes: [ %UpdatedRecord{ - relation: {"public", "the_table"}, - record: %{"id" => "1"}, - old_record: %{"id" => "2"} + relation: {"public", "t1"}, + record: %{"id" => "2"}, + old_record: %{"id" => "3"} } ] } - assert Filter.affected_shapes(filter, transaction) == MapSet.new(["shape1", "shape2"]) + assert Filter.affected_shapes(filter, update) == MapSet.new(["s2", "s3"]) end test "returns shapes affected by relation change" do - filter = %Filter{ - tables: %{ - {"public", "the_table"} => %Table{ - indexes: %{ - "id" => %Index{ - type: :int8, - values: %{ - 1 => [ - %{ - shape_id: "shape1", - and_where: nil, - shape: Shape.new!("the_table", where: "id = 1", inspector: @inspector) - } - ], - 2 => [ - %{ - shape_id: "shape2", - and_where: nil, - shape: Shape.new!("the_table", where: "id = 2", inspector: @inspector) - } - ] - } - } - }, - other_shapes: %{ - "shape3" => Shape.new!("the_table", where: "id > 7", inspector: @inspector), - "shape4" => Shape.new!("the_table", where: "id > 6", inspector: @inspector) - } - }, - {"public", "another_table"} => %Table{ - indexes: %{ - "id" => %Index{ - type: :int8, - values: %{ - 1 => [ - %{shape_id: "not-this-shape-1", and_where: nil} - ] - } - } - }, - other_shapes: %{ - "not-this-shape-1" => - Shape.new!("another_table", where: "id > 7", inspector: @inspector), - "not-this-shape-2" => - Shape.new!("another_table", where: "id > 6", inspector: @inspector) - } - } - } - } + filter = + Filter.empty() + |> Filter.add_shape("s1", Shape.new!("t1", where: "id = 1", inspector: @inspector)) + |> Filter.add_shape("s2", Shape.new!("t1", where: "id = 2", inspector: @inspector)) + |> Filter.add_shape("s3", Shape.new!("t1", where: "id > 7", inspector: @inspector)) + |> Filter.add_shape("s4", Shape.new!("t1", where: "id > 8", inspector: @inspector)) + |> Filter.add_shape("s5", Shape.new!("t2", where: "id = 1", inspector: @inspector)) + |> Filter.add_shape("s6", Shape.new!("t2", where: "id = 2", inspector: @inspector)) + |> Filter.add_shape("s7", Shape.new!("t2", where: "id > 7", inspector: @inspector)) + |> Filter.add_shape("s8", Shape.new!("t2", where: "id > 8", inspector: @inspector)) - relation = - %Relation{ - schema: "public", - table: "the_table" - } + relation = %Relation{schema: "public", table: "t1"} + + assert Filter.affected_shapes(filter, relation) == MapSet.new(["s1", "s2", "s3", "s4"]) + end + + test "returns shapes affected by relation rename" do + table_id = 123 + s1 = Shape.new!("t1", inspector: @inspector) + s2 = Shape.new!("t2", inspector: @inspector) |> Map.put(:root_table_id, table_id) + s3 = Shape.new!("t3", inspector: @inspector) + + filter = + Filter.empty() + |> Filter.add_shape("s1", s1) + |> Filter.add_shape("s2", s2) + |> Filter.add_shape("s3", s3) + + rename = %Relation{schema: "public", table: "new_name", id: table_id} - assert Filter.affected_shapes(filter, relation) == - MapSet.new(["shape1", "shape2", "shape3", "shape4"]) + assert Filter.affected_shapes(filter, rename) == MapSet.new(["s2"]) end test "returns shapes affected by truncation" do - filter = %Filter{ - tables: %{ - {"public", "the_table"} => %Table{ - indexes: %{ - "id" => %Index{ - type: :int8, - values: %{ - 1 => [ - %{ - shape_id: "shape1", - and_where: nil, - shape: Shape.new!("the_table", where: "id = 1", inspector: @inspector) - } - ], - 2 => [ - %{ - shape_id: "shape2", - and_where: nil, - shape: Shape.new!("the_table", where: "id = 2", inspector: @inspector) - } - ] - } - } - }, - other_shapes: %{ - "shape3" => Shape.new!("the_table", where: "id > 7", inspector: @inspector), - "shape4" => Shape.new!("the_table", where: "id > 6", inspector: @inspector) - } - }, - {"public", "another_table"} => %Table{ - indexes: %{ - type: :int8, - values: %{ - "id" => %Index{ - type: :int8, - values: %{ - 1 => [ - %{shape_id: "not-this-shape-1", and_where: nil} - ] - } - } - } - }, - other_shapes: %{ - "not-this-shape-1" => - Shape.new!("another_table", where: "id > 7", inspector: @inspector), - "not-this-shape-2" => - Shape.new!("another_table", where: "id > 6", inspector: @inspector) - } - } - } - } + filter = + Filter.empty() + |> Filter.add_shape("s1", Shape.new!("t1", where: "id = 1", inspector: @inspector)) + |> Filter.add_shape("s2", Shape.new!("t1", where: "id = 2", inspector: @inspector)) + |> Filter.add_shape("s3", Shape.new!("t1", where: "id > 7", inspector: @inspector)) + |> Filter.add_shape("s4", Shape.new!("t1", where: "id > 8", inspector: @inspector)) + |> Filter.add_shape("s5", Shape.new!("t2", where: "id = 1", inspector: @inspector)) + |> Filter.add_shape("s6", Shape.new!("t2", where: "id = 2", inspector: @inspector)) + |> Filter.add_shape("s7", Shape.new!("t2", where: "id > 7", inspector: @inspector)) + |> Filter.add_shape("s8", Shape.new!("t2", where: "id > 8", inspector: @inspector)) - transaction = - %TruncatedRelation{ - relation: {"public", "the_table"} - } + truncation = %Transaction{changes: [%TruncatedRelation{relation: {"public", "t1"}}]} - assert Filter.affected_shapes(filter, transaction) == - MapSet.new(["shape1", "shape2", "shape3", "shape4"]) + assert Filter.affected_shapes(filter, truncation) == MapSet.new(["s1", "s2", "s3", "s4"]) end end - describe "where clause filtering" do - test "shape with no where clause is always affected" do - shape = Shape.new!("the_table", inspector: @inspector) + test "shape with no where clause is affected by all changes for the same table" do + shape = Shape.new!("t1", inspector: @inspector) + filter = Filter.empty() |> Filter.add_shape("s", shape) - transaction = - %Transaction{ - changes: [ - %NewRecord{ - relation: {"public", "the_table"}, - record: %{"id" => "7"} - } - ] - } + assert Filter.affected_shapes(filter, change("t1", %{"id" => "7"})) == MapSet.new(["s"]) + assert Filter.affected_shapes(filter, change("t1", %{"id" => "8"})) == MapSet.new(["s"]) + assert Filter.affected_shapes(filter, change("t2", %{"id" => "8"})) == MapSet.new([]) + end - assert Filter.empty() - |> Filter.add_shape("the-shape", shape) - |> Filter.affected_shapes(transaction) == MapSet.new(["the-shape"]) - end + test "shape with a where clause is affected by changes that match that where clause" do + shape = Shape.new!("t1", where: "id = 7", inspector: @inspector) + filter = Filter.empty() |> Filter.add_shape("s", shape) - for test <- [ - %{where: "id = 7", record: %{"id" => "7"}, affected: true}, - %{where: "id = 7", record: %{"id" => "8"}, affected: false}, - %{where: "id = 7", record: %{"id" => nil}, affected: false}, - %{where: "7 = id", record: %{"id" => "7"}, affected: true}, - %{where: "7 = id", record: %{"id" => "8"}, affected: false}, - %{where: "7 = id", record: %{"id" => nil}, affected: false}, - %{where: "id = 7 AND id > 1", record: %{"id" => "7"}, affected: true}, - %{where: "id = 7 AND id > 1", record: %{"id" => "8"}, affected: false}, - %{where: "id = 7 AND id > 8", record: %{"id" => "7"}, affected: false}, - %{where: "id > 1 AND id = 7", record: %{"id" => "7"}, affected: true}, - %{where: "id > 1 AND id = 7", record: %{"id" => "8"}, affected: false}, - %{where: "id > 8 AND id = 7", record: %{"id" => "7"}, affected: false}, - %{where: "an_array = '{1}'", record: %{"an_array" => "{1}"}, affected: true}, - %{where: "an_array = '{1}'", record: %{"an_array" => "{2}"}, affected: false}, - %{where: "an_array = '{1}'", record: %{"an_array" => "{1,2}"}, affected: false} - ] do - test "where: #{test.where}, record: #{inspect(test.record)}" do - %{where: where, record: record, affected: affected} = unquote(Macro.escape(test)) - - assert affected?(where, record) == affected - end - end + assert Filter.affected_shapes(filter, change("t1", %{"id" => "7"})) == MapSet.new(["s"]) + assert Filter.affected_shapes(filter, change("t1", %{"id" => "8"})) == MapSet.new([]) + assert Filter.affected_shapes(filter, change("t2", %{"id" => "8"})) == MapSet.new([]) + end + + test "invalid record value logs an error and says all shapes for the table are affected" do + filter = + Filter.empty() + |> Filter.add_shape("shape1", Shape.new!("table", inspector: @inspector)) + |> Filter.add_shape("shape2", Shape.new!("table", where: "id = 7", inspector: @inspector)) + |> Filter.add_shape("shape3", Shape.new!("table", where: "id = 8", inspector: @inspector)) + |> Filter.add_shape("shape4", Shape.new!("table", where: "id > 9", inspector: @inspector)) + |> Filter.add_shape("shape5", Shape.new!("another_table", inspector: @inspector)) + + log = + capture_log(fn -> + assert Filter.affected_shapes(filter, change("table", %{"id" => "invalid_value"})) == + MapSet.new(["shape1", "shape2", "shape3", "shape4"]) + end) + + assert log =~ ~s(Could not parse value for field "id" of type :int8) + end - test "Invalid record value logs an error and says all shapes are affected" do - log = - capture_log(fn -> - assert affected?("id = 7", %{"id" => "invalid_value"}) - end) + test "Filter.remove_shape/2" do + filter1 = + Filter.empty() + |> Filter.add_shape("shape1", Shape.new!("table", inspector: @inspector)) + + filter2 = + filter1 + |> Filter.add_shape("shape2", Shape.new!("another_table", inspector: @inspector)) + + filter3 = + filter2 + |> Filter.add_shape("shape3", Shape.new!("table", where: "id = 1", inspector: @inspector)) + + filter4 = + filter3 + |> Filter.add_shape("shape4", Shape.new!("table", where: "id = 2", inspector: @inspector)) + + filter5 = + filter4 + |> Filter.add_shape("shape5", Shape.new!("table", where: "id > 2", inspector: @inspector)) + + filter6 = + filter5 + |> Filter.add_shape("shape6", Shape.new!("table", where: "id > 7", inspector: @inspector)) + + assert Filter.remove_shape(filter6, "shape6") == filter5 + assert Filter.remove_shape(filter5, "shape5") == filter4 + assert Filter.remove_shape(filter4, "shape4") == filter3 + assert Filter.remove_shape(filter3, "shape3") == filter2 + assert Filter.remove_shape(filter2, "shape2") == filter1 + assert Filter.remove_shape(filter1, "shape1") == Filter.empty() + end - assert log =~ ~s(Could not parse value for field "id" of type :int8) - end + for test <- [ + %{where: "id = 7", record: %{"id" => "7"}, affected: true}, + %{where: "id = 7", record: %{"id" => "8"}, affected: false}, + %{where: "id = 7", record: %{"id" => nil}, affected: false}, + %{where: "7 = id", record: %{"id" => "7"}, affected: true}, + %{where: "7 = id", record: %{"id" => "8"}, affected: false}, + %{where: "7 = id", record: %{"id" => nil}, affected: false}, + %{where: "id = 7 AND id > 1", record: %{"id" => "7"}, affected: true}, + %{where: "id = 7 AND id > 1", record: %{"id" => "8"}, affected: false}, + %{where: "id = 7 AND id > 8", record: %{"id" => "7"}, affected: false}, + %{where: "id > 1 AND id = 7", record: %{"id" => "7"}, affected: true}, + %{where: "id > 1 AND id = 7", record: %{"id" => "8"}, affected: false}, + %{where: "id > 8 AND id = 7", record: %{"id" => "7"}, affected: false}, + %{where: "an_array = '{1}'", record: %{"an_array" => "{1}"}, affected: true}, + %{where: "an_array = '{1}'", record: %{"an_array" => "{2}"}, affected: false}, + %{where: "an_array = '{1}'", record: %{"an_array" => "{1,2}"}, affected: false} + ] do + test "where: #{test.where}, record: #{inspect(test.record)}" do + %{where: where, record: record, affected: affected} = unquote(Macro.escape(test)) - defp affected?(where, record) do shape = Shape.new!("the_table", where: where, inspector: @inspector) - transaction = - %Transaction{ - changes: [ - %NewRecord{ - relation: {"public", "the_table"}, - record: record - } - ] - } + transaction = change("the_table", record) - Filter.empty() - |> Filter.add_shape("the-shape", shape) - |> Filter.affected_shapes(transaction) == MapSet.new(["the-shape"]) + assert Filter.empty() + |> Filter.add_shape("the-shape", shape) + |> Filter.affected_shapes(transaction) == MapSet.new(["the-shape"]) == affected end end + + defp change(table, record) do + %Transaction{ + changes: [ + %NewRecord{ + relation: {"public", table}, + record: record + } + ] + } + end end From 93ee95e69971249197ecf0fa33bf7621ad33925c Mon Sep 17 00:00:00 2001 From: rob Date: Tue, 3 Dec 2024 09:45:25 +0000 Subject: [PATCH 59/62] Add optimisation tests --- .../test/electric/shapes/filter_test.exs | 60 +++++++++++++++++++ 1 file changed, 60 insertions(+) diff --git a/packages/sync-service/test/electric/shapes/filter_test.exs b/packages/sync-service/test/electric/shapes/filter_test.exs index 11340a5f9f..35fe44908b 100644 --- a/packages/sync-service/test/electric/shapes/filter_test.exs +++ b/packages/sync-service/test/electric/shapes/filter_test.exs @@ -134,6 +134,59 @@ defmodule Electric.Shapes.FilterTest do assert Filter.affected_shapes(filter, truncation) == MapSet.new(["s1", "s2", "s3", "s4"]) end + + test "where clause in the form `field = const` is optimised" do + filter = + 1..1000 + |> Enum.reduce(Filter.empty(), fn i, filter -> + Filter.add_shape(filter, i, Shape.new!("t1", where: "id = #{i}", inspector: @inspector)) + end) + + reductions = + reductions(fn -> + assert Filter.affected_shapes(filter, change("t1", %{"id" => "7"})) == MapSet.new([7]) + end) + + assert reductions < 500 + end + + test "where clause in the form `field = const AND another_condition` is optimised" do + filter = + 1..1000 + |> Enum.reduce(Filter.empty(), fn i, filter -> + Filter.add_shape( + filter, + i, + Shape.new!("t1", where: "id = #{i} AND id > 6", inspector: @inspector) + ) + end) + + reductions = + reductions(fn -> + assert Filter.affected_shapes(filter, change("t1", %{"id" => "7"})) == MapSet.new([7]) + end) + + assert reductions < 500 + end + + test "where clause in the form `a_condition AND field = const` is optimised" do + filter = + 1..1000 + |> Enum.reduce(Filter.empty(), fn i, filter -> + Filter.add_shape( + filter, + i, + Shape.new!("t1", where: "id > 6 AND id = #{i}", inspector: @inspector) + ) + end) + + reductions = + reductions(fn -> + assert Filter.affected_shapes(filter, change("t1", %{"id" => "7"})) == MapSet.new([7]) + end) + + assert reductions < 500 + end end test "shape with no where clause is affected by all changes for the same table" do @@ -245,4 +298,11 @@ defmodule Electric.Shapes.FilterTest do ] } end + + defp reductions(fun) do + {:reductions, reductions} = :erlang.process_info(self(), :reductions) + fun.() + {:reductions, new_reductions} = :erlang.process_info(self(), :reductions) + new_reductions - reductions + end end From 51cd0832f8b4043557b0f694536da5d83c3bd44e Mon Sep 17 00:00:00 2001 From: rob Date: Tue, 3 Dec 2024 10:41:06 +0000 Subject: [PATCH 60/62] Refactor reductions/1 --- packages/sync-service/test/electric/shapes/filter_test.exs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/sync-service/test/electric/shapes/filter_test.exs b/packages/sync-service/test/electric/shapes/filter_test.exs index 35fe44908b..ad195dec72 100644 --- a/packages/sync-service/test/electric/shapes/filter_test.exs +++ b/packages/sync-service/test/electric/shapes/filter_test.exs @@ -300,9 +300,9 @@ defmodule Electric.Shapes.FilterTest do end defp reductions(fun) do - {:reductions, reductions} = :erlang.process_info(self(), :reductions) + {:reductions, reductions_before} = :erlang.process_info(self(), :reductions) fun.() - {:reductions, new_reductions} = :erlang.process_info(self(), :reductions) - new_reductions - reductions + {:reductions, reductions_after} = :erlang.process_info(self(), :reductions) + reductions_after - reductions_before end end From 0f8ef279786887bddf75dafd16d61de5cc4bde3f Mon Sep 17 00:00:00 2001 From: rob Date: Tue, 3 Dec 2024 10:54:29 +0000 Subject: [PATCH 61/62] Rename Filter.empty() to Filter.new() --- .../lib/electric/shapes/dispatcher.ex | 2 +- .../lib/electric/shapes/filter.ex | 6 ++-- .../lib/electric/shapes/filter/table.ex | 2 +- .../test/electric/shapes/filter_test.exs | 32 ++++++++++--------- 4 files changed, 22 insertions(+), 20 deletions(-) diff --git a/packages/sync-service/lib/electric/shapes/dispatcher.ex b/packages/sync-service/lib/electric/shapes/dispatcher.ex index 653764bcfd..65743dc6d1 100644 --- a/packages/sync-service/lib/electric/shapes/dispatcher.ex +++ b/packages/sync-service/lib/electric/shapes/dispatcher.ex @@ -38,7 +38,7 @@ defmodule Electric.Shapes.Dispatcher do waiting: 0, pending: nil, subscribers: [], - filter: Filter.empty(), + filter: Filter.new(), pids: MapSet.new() }} end diff --git a/packages/sync-service/lib/electric/shapes/filter.ex b/packages/sync-service/lib/electric/shapes/filter.ex index 07faf6741c..e43de40e18 100644 --- a/packages/sync-service/lib/electric/shapes/filter.ex +++ b/packages/sync-service/lib/electric/shapes/filter.ex @@ -27,8 +27,8 @@ defmodule Electric.Shapes.Filter do @type t :: %Filter{} @type shape_id :: any() - @spec empty() :: Filter.t() - def empty, do: %Filter{} + @spec new() :: Filter.t() + def new, do: %Filter{} @doc """ Add a shape for the filter to track. @@ -43,7 +43,7 @@ defmodule Electric.Shapes.Filter do Map.update( tables, shape.root_table, - Table.add_shape(Table.empty(), {shape_id, shape}), + Table.add_shape(Table.new(), {shape_id, shape}), fn table -> Table.add_shape(table, {shape_id, shape}) end diff --git a/packages/sync-service/lib/electric/shapes/filter/table.ex b/packages/sync-service/lib/electric/shapes/filter/table.ex index 4998d5b066..898fe4c049 100644 --- a/packages/sync-service/lib/electric/shapes/filter/table.ex +++ b/packages/sync-service/lib/electric/shapes/filter/table.ex @@ -20,7 +20,7 @@ defmodule Electric.Shapes.Filter.Table do defstruct indexes: %{}, other_shapes: %{} - def empty, do: %Table{} + def new, do: %Table{} def empty?(%Table{indexes: indexes, other_shapes: other_shapes}) do indexes == %{} && other_shapes == %{} diff --git a/packages/sync-service/test/electric/shapes/filter_test.exs b/packages/sync-service/test/electric/shapes/filter_test.exs index ad195dec72..4b114d184f 100644 --- a/packages/sync-service/test/electric/shapes/filter_test.exs +++ b/packages/sync-service/test/electric/shapes/filter_test.exs @@ -21,7 +21,7 @@ defmodule Electric.Shapes.FilterTest do describe "affected_shapes/2" do test "returns shapes affected by insert" do filter = - Filter.empty() + Filter.new() |> Filter.add_shape("s1", Shape.new!("t1", where: "id = 1", inspector: @inspector)) |> Filter.add_shape("s2", Shape.new!("t1", where: "id = 2", inspector: @inspector)) |> Filter.add_shape("s3", Shape.new!("t1", where: "id = 3", inspector: @inspector)) @@ -42,7 +42,7 @@ defmodule Electric.Shapes.FilterTest do test "returns shapes affected by delete" do filter = - Filter.empty() + Filter.new() |> Filter.add_shape("s1", Shape.new!("t1", where: "id = 1", inspector: @inspector)) |> Filter.add_shape("s2", Shape.new!("t1", where: "id = 2", inspector: @inspector)) |> Filter.add_shape("s3", Shape.new!("t1", where: "id = 3", inspector: @inspector)) @@ -63,7 +63,7 @@ defmodule Electric.Shapes.FilterTest do test "returns shapes affected by update" do filter = - Filter.empty() + Filter.new() |> Filter.add_shape("s1", Shape.new!("t1", where: "id = 1", inspector: @inspector)) |> Filter.add_shape("s2", Shape.new!("t1", where: "id = 2", inspector: @inspector)) |> Filter.add_shape("s3", Shape.new!("t1", where: "id = 3", inspector: @inspector)) @@ -86,7 +86,7 @@ defmodule Electric.Shapes.FilterTest do test "returns shapes affected by relation change" do filter = - Filter.empty() + Filter.new() |> Filter.add_shape("s1", Shape.new!("t1", where: "id = 1", inspector: @inspector)) |> Filter.add_shape("s2", Shape.new!("t1", where: "id = 2", inspector: @inspector)) |> Filter.add_shape("s3", Shape.new!("t1", where: "id > 7", inspector: @inspector)) @@ -108,7 +108,7 @@ defmodule Electric.Shapes.FilterTest do s3 = Shape.new!("t3", inspector: @inspector) filter = - Filter.empty() + Filter.new() |> Filter.add_shape("s1", s1) |> Filter.add_shape("s2", s2) |> Filter.add_shape("s3", s3) @@ -120,7 +120,7 @@ defmodule Electric.Shapes.FilterTest do test "returns shapes affected by truncation" do filter = - Filter.empty() + Filter.new() |> Filter.add_shape("s1", Shape.new!("t1", where: "id = 1", inspector: @inspector)) |> Filter.add_shape("s2", Shape.new!("t1", where: "id = 2", inspector: @inspector)) |> Filter.add_shape("s3", Shape.new!("t1", where: "id > 7", inspector: @inspector)) @@ -138,7 +138,7 @@ defmodule Electric.Shapes.FilterTest do test "where clause in the form `field = const` is optimised" do filter = 1..1000 - |> Enum.reduce(Filter.empty(), fn i, filter -> + |> Enum.reduce(Filter.new(), fn i, filter -> Filter.add_shape(filter, i, Shape.new!("t1", where: "id = #{i}", inspector: @inspector)) end) @@ -153,7 +153,7 @@ defmodule Electric.Shapes.FilterTest do test "where clause in the form `field = const AND another_condition` is optimised" do filter = 1..1000 - |> Enum.reduce(Filter.empty(), fn i, filter -> + |> Enum.reduce(Filter.new(), fn i, filter -> Filter.add_shape( filter, i, @@ -172,7 +172,7 @@ defmodule Electric.Shapes.FilterTest do test "where clause in the form `a_condition AND field = const` is optimised" do filter = 1..1000 - |> Enum.reduce(Filter.empty(), fn i, filter -> + |> Enum.reduce(Filter.new(), fn i, filter -> Filter.add_shape( filter, i, @@ -191,7 +191,7 @@ defmodule Electric.Shapes.FilterTest do test "shape with no where clause is affected by all changes for the same table" do shape = Shape.new!("t1", inspector: @inspector) - filter = Filter.empty() |> Filter.add_shape("s", shape) + filter = Filter.new() |> Filter.add_shape("s", shape) assert Filter.affected_shapes(filter, change("t1", %{"id" => "7"})) == MapSet.new(["s"]) assert Filter.affected_shapes(filter, change("t1", %{"id" => "8"})) == MapSet.new(["s"]) @@ -200,7 +200,7 @@ defmodule Electric.Shapes.FilterTest do test "shape with a where clause is affected by changes that match that where clause" do shape = Shape.new!("t1", where: "id = 7", inspector: @inspector) - filter = Filter.empty() |> Filter.add_shape("s", shape) + filter = Filter.new() |> Filter.add_shape("s", shape) assert Filter.affected_shapes(filter, change("t1", %{"id" => "7"})) == MapSet.new(["s"]) assert Filter.affected_shapes(filter, change("t1", %{"id" => "8"})) == MapSet.new([]) @@ -209,7 +209,7 @@ defmodule Electric.Shapes.FilterTest do test "invalid record value logs an error and says all shapes for the table are affected" do filter = - Filter.empty() + Filter.new() |> Filter.add_shape("shape1", Shape.new!("table", inspector: @inspector)) |> Filter.add_shape("shape2", Shape.new!("table", where: "id = 7", inspector: @inspector)) |> Filter.add_shape("shape3", Shape.new!("table", where: "id = 8", inspector: @inspector)) @@ -226,8 +226,10 @@ defmodule Electric.Shapes.FilterTest do end test "Filter.remove_shape/2" do + empty = Filter.new() + filter1 = - Filter.empty() + empty |> Filter.add_shape("shape1", Shape.new!("table", inspector: @inspector)) filter2 = @@ -255,7 +257,7 @@ defmodule Electric.Shapes.FilterTest do assert Filter.remove_shape(filter4, "shape4") == filter3 assert Filter.remove_shape(filter3, "shape3") == filter2 assert Filter.remove_shape(filter2, "shape2") == filter1 - assert Filter.remove_shape(filter1, "shape1") == Filter.empty() + assert Filter.remove_shape(filter1, "shape1") == empty end for test <- [ @@ -282,7 +284,7 @@ defmodule Electric.Shapes.FilterTest do transaction = change("the_table", record) - assert Filter.empty() + assert Filter.new() |> Filter.add_shape("the-shape", shape) |> Filter.affected_shapes(transaction) == MapSet.new(["the-shape"]) == affected end From 12c0644feb37952e024af1d1bae991e0da2a161a Mon Sep 17 00:00:00 2001 From: rob Date: Tue, 3 Dec 2024 13:05:38 +0000 Subject: [PATCH 62/62] Add changeset --- .changeset/spotty-ears-build.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/spotty-ears-build.md diff --git a/.changeset/spotty-ears-build.md b/.changeset/spotty-ears-build.md new file mode 100644 index 0000000000..14dae5cd32 --- /dev/null +++ b/.changeset/spotty-ears-build.md @@ -0,0 +1,5 @@ +--- +"@core/sync-service": patch +--- + +Improved replication steam processing for where clauses in the form `field = const` or `field = const AND another_condition`