Skip to content

fix(partition): support map keys in put_newer #21

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
SAY-5 wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix(partition): support map keys in put_newer #21

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,16 @@ All notable changes to this project will be documented in this file.
The format is based on [Keep a Changelog](http://keepachangelog.com/)
and this project adheres to [Semantic Versioning](http://semver.org/).

## Unreleased

### Bug Fixes

- Fixed `put_newer/5` and `put_all_newer/3` raising `ArgumentError` ("not a
valid match specification") when the key contains a map. The key is now
bound to a match variable and compared in the guard, since ETS rejects raw
maps at the key position of a `select_replace/2` match head.
[#18](https://github.com/appcues/partitioned_buffer/issues/18).

## Release 0.4.1

### Bug Fixes
Expand Down
28 changes: 15 additions & 13 deletions lib/partitioned_buffer/partition.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -518,25 +518,27 @@ defmodule PartitionedBuffer.Partition do
end

defp replace_match_spec(key, value, version) do
# Performance note: The key in the match head is a literal (bound value),
# not a pattern variable. This allows ETS to use its hash index for O(1)
# lookup rather than scanning the entire table.
# The key is bound to "$3" and compared in the guard rather than embedded
# as a literal in the match head. ETS rejects raw maps in a match head at
# the key position (see :ets.select_replace/2), so keys that contain a map
# would raise "not a valid match specification". Comparing via the guard
# works for any term shape.
#
# In match spec bodies, bare tuples are interpreted as operations/function
# calls, NOT as literal data. We wrap key and value with ms_literal/1 so
# tuples use the {{...}} constructor form and maps use {:const, map} that
# ETS understands. This handles tuples, maps, and lists (including nested
# combinations).
# In match spec bodies and guards, bare tuples are interpreted as
# operations/function calls, NOT as literal data. We wrap key and value
# with ms_literal/1 so tuples use the {{...}} constructor form and maps use
# {:const, map} that ETS understands. This handles tuples, maps, and lists
# (including nested combinations).
[
{
# Match: {entry, key, value, existing_version, updates} where key is literal
entry(key: key, value: :_, version: :"$1", updates: :"$2"),
# Guard (update only if): new_version > existing_version
[{:>, version, :"$1"}],
# Match: {entry, key, value, existing_version, updates}
entry(key: :"$3", value: :_, version: :"$1", updates: :"$2"),
# Guard (update only if): key matches and new_version > existing_version
[{:>, version, :"$1"}, {:"=:=", :"$3", ms_literal(key)}],
# Result: the new entry with incremented updates counter
Comment on lines 520 to 538

@coderabbitai coderabbitai Bot Jun 18, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🌐 Web query:

In Erlang/OTP docs for :ets.select_replace/2, does using a key match variable plus guard equality disable key-based lookup optimization versus a literal key in the match head for :set tables?

💡 Result:

Yes, using a key match variable in the match head instead of a literal key does disable key-based lookup optimization for:set tables [1][2][3]. In Erlang/OTP, ETS match specifications (used by functions such as:ets.select_replace/2) rely on the match head to optimize search operations [4][2]. The documentation explicitly states that a match pattern with a fully bound key—meaning a key specified as a literal without any match variables—is optimized to a single key lookup, bypassing the need for a full table scan [1][2]. If the key position in the match head contains a match variable (e.g., '$1') or an underscore ('_'), the system cannot perform a direct key-based lookup [2][3]. Moving an equality check (e.g., K =:= 123) into the match specification's guard does not restore this optimization because the guard is only evaluated after an object has already been matched and extracted from the table, often necessitating a scan of the entire table [4][5]. Consequently, for large:set tables, using a variable in the match head will result in a significantly less efficient full table scan compared to using the exact key [1][3].

Citations:

Guard-only key equality disables ETS keyed lookup optimization.

The change to use key: :"$3" with guard equality removes the literal-key match head that enables O(1) lookups on :set tables. Even though the guard binds the key, ETS still evaluates the guard after extracting candidates, forcing a full scan instead of a direct key lookup. For put_newer/2 on existing keys, this degrades performance significantly. Consider a conditional match spec: use the literal-key head for non-map keys (where the original code worked) and the guard-based path only for map-containing keys that triggered the crash.

🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@lib/partitioned_buffer/partition.ex` around lines 520 - 538, The current
implementation of replace_match_spec uses a guard-based key equality check with
key: :"$3", which prevents ETS from using its O(1) keyed lookup optimization on
:set tables and forces a full table scan instead. To fix this, implement a
conditional match spec that uses the original literal-key approach in the match
head for non-map keys (which enables the fast lookup), and only falls back to
the guard-based approach with ms_literal(key) for keys that contain maps (which
would otherwise cause ETS to reject the match spec). This way, the fast path is
preserved for the common case while still handling map-containing keys
correctly.

[
{entry(
key: ms_literal(key),
key: :"$3",
value: ms_literal(value),
version: version,
updates: {:+, :"$2", 1}
Expand Down
16 changes: 16 additions & 0 deletions test/partitioned_buffer/map_test.exs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -514,6 +514,22 @@ defmodule PartitionedBuffer.MapTest do
M.put_all_newer(buff, entries)
end
end

test "ok: updates existing entry with a key containing a map", %{buffer: buff} do
key = {:user, %{id: 1}}

assert M.put_newer(buff, key, "v1", 100) == :ok
assert M.put_newer(buff, key, "v2", 200) == :ok

assert M.size(buff) == 1
assert M.get(buff, key) == "v2"

assert_receive {@processing_stop_event, %{duration: _, size: 1},
%{buffer: ^buff, partition: _}},
@default_timeout

assert_receive {:process_completed, [{^key, "v2", 200, 1}]}, @default_timeout
end
end

describe "processing" do
Expand Down