fix(partition): support map keys in put_newer#21
Conversation
Signed-off-by: Sai Asish Y <say.apm35@gmail.com>
📝 WalkthroughWalkthrough
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with 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.
Inline comments:
In `@lib/partitioned_buffer/partition.ex`:
- Around line 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.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 6d511c27-ffd9-4e7b-89b2-7cca28c7c652
📒 Files selected for processing (3)
CHANGELOG.mdlib/partitioned_buffer/partition.extest/partitioned_buffer/map_test.exs
| 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 |
There was a problem hiding this comment.
🧩 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:
- 1: https://www.erlang.org/docs/29/apps/stdlib/ets.html
- 2: https://github.com/erlang/otp/blob/master/lib/stdlib/src/ets.erl
- 3: https://www.erlang.org/docs/24/man/ets
- 4: https://www.erlang.org/doc/apps/erts/match_spec.html
- 5: https://beta.erlang.org/docs/21/apps/erts/match_spec.html
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.
1 participant
Closes #18.
put_newer/5raisedArgumentError("not a valid match specification") whenever the key contained a map, because ETS rejects a raw map at the key position of aselect_replace/2match head. This binds the key to a match variable and compares it in the guard instead, which works for any key shape. Added a regression test alongside the existing tuple/list-key cases and updated the changelog.Summary by CodeRabbit