Support duplicate bag in insert and lookup #1718

jgonet · 2025-06-17T14:53:08Z

~~Merge after #1614. New code after "ets hash table: rename status enum".~~

These changes are made under both the "Apache 2.0" and the "GNU Lesser General
Public License 2.1 or later" license terms (dual license).

SPDX-License-Identifier: Apache-2.0 OR LGPL-2.1-or-later

jgonet · 2025-06-17T14:55:32Z

src/libAtomVM/nifs.c

+        switch (key_atom) {
+            case NAMED_TABLE_ATOM: {
+                is_named = true;
+                break;
+            }
+            case PRIVATE_ATOM: {
+                private = true;
+                public = false;
+                break;
+            }
+            case PUBLIC_ATOM: {
+                private = false;
+                public = true;
+                break;
+            }
+            case SET_ATOM: {
+                is_duplicate_bag = false;
+                break;
+            }
+            case DUPLICATE_BAG_ATOM: {
+                is_duplicate_bag = true;
+                break;
+            }
+            case KEYPOS_ATOM: {
+                VALIDATE_VALUE(value, term_is_integer);
+                avm_int_t keypos = term_to_int(value);
+                if (UNLIKELY(keypos < 1)) {
+                    RAISE_ERROR(BADARG_ATOM);
+                }
+                key_index = keypos - 1;
+                break;
+            }
+            default:
+                RAISE_ERROR(BADARG_ATOM);
+        }
+        options = term_get_list_tail(options);


I was thinking about adding interop_parse_kw_opts(term kw, &opts) but this loop relies on ability to override keys when they're repeated (e.g. [set, duplicate_bag, set]). Maybe in the future.

jgonet · 2025-06-17T14:57:57Z

src/libAtomVM/ets_hashtable.c

+}
+
+// TODO: create list elsewhere, by copying terms from orig heap, appending new copied tuple and using ets_hashtable_new_node
+struct HNode *ets_hashtable_new_node_from_list(term old_tuples, term tuple, size_t key_index)


This copies old tuple list from previous node's heap, copies tuple from process' heap and merges them together on the new heap. TODO is for removing this function and using utility function to do that and then just pass new heap to new_node.

jgonet · 2025-06-17T14:59:45Z

src/libAtomVM/ets.c

@@ -248,78 +243,105 @@ static bool true_pred(struct EtsTable *table, void *data)
    return true;
 }

-static void ets_delete_all_tables(struct Ets *ets, GlobalContext *global)
+static void delete_all_tables(struct Ets *ets, GlobalContext *global)


I was constantly confusing exported functions and internal functions so I opted for rename removing prefix for internal ones (previously: ets_lookup_maybe_gc vs ets_lookup_table_maybe_gc, now ets_lookup_maybe_gc vs lookup_maybe_gc).

Sounds a reasonable point, I will note this on the idea for the updated coding style.

The alternative we have in the fork is to use internal prefix/suffix but it's get lengthy.

jgonet · 2025-06-17T15:00:43Z

src/libAtomVM/ets.c

@@ -52,7 +58,7 @@ struct EtsTable
    term name;
    bool is_named;
    int32_t owner_process_id;
-    size_t keypos;
+    size_t key_index;


This was already using 0-based indexes but was named position which suggests 1-based indexes. Cleaned all up.

good catch.

bettio

I'm adding a first round of comments here, I did not yet finished this review, so more comments will follow.
A rebase on latest main would defintely help, since the previous ets PR has been merged.

bettio · 2025-07-01T17:22:46Z

src/libAtomVM/ets.c

@@ -52,7 +58,7 @@ struct EtsTable
    term name;
    bool is_named;
    int32_t owner_process_id;
-    size_t keypos;
+    size_t key_index;


good catch.

bettio · 2025-07-01T17:58:43Z

src/libAtomVM/nifs.c

@@ -3297,35 +3297,96 @@ static term nif_erlang_raise(Context *ctx, int argc, term argv[])
    return term_invalid_term();
 }

+static inline term get_option_key(term option, term *maybe_value)


it took me a moment to find the purpose of this function, so I suggest naming it extract_option_key_value().

Also, if you plan to make something generic (while keeping a clean and simple API) that can be used elsewhere as well, returning true as value for atoms might be a good choice (while using undefined when no property is found).

bettio · 2025-07-01T19:14:45Z

src/libAtomVM/nifs.c

+        VALIDATE_VALUE(key_atom, term_is_atom);
+
+        switch (key_atom) {
+            case NAMED_TABLE_ATOM: {


fun fact: on OTP it is not a proplist, so ets:new(foo, [{named_table, true}]). raises badarg, but ets:new(fiz, [named_table]) doesn't.
So I think this can be handled with a simple change:

switch (head) { case NAMED_TABLE_ATOM: ... default: VALIDATE_VALUE(head, term_is_tuple); term value; term key_atom = get_option_key(head, &value); VALIDATE_VALUE(key_atom, term_is_atom);

ets:new(foo, [{named_table, true}]). also, let's make sure in tests that fails as expected.

bettio · 2025-07-01T19:48:42Z

src/libAtomVM/ets.c

@@ -248,78 +243,105 @@ static bool true_pred(struct EtsTable *table, void *data)
    return true;
 }

-static void ets_delete_all_tables(struct Ets *ets, GlobalContext *global)
+static void delete_all_tables(struct Ets *ets, GlobalContext *global)


Sounds a reasonable point, I will note this on the idea for the updated coding style.

Signed-off-by: Jakub Gonet <jakub.gonet@swmansion.com>

ETS mixes position (1-based) with indexing (0-based). Tuple are 0-indexed so we should convert to it at earliest possible moment. This commit doesn't introduce any change in behavior. Signed-off-by: Jakub Gonet <jakub.gonet@swmansion.com>

Signed-off-by: Jakub Gonet <jakub.gonet@swmansion.com>

ETS supports setting different key position (default: first element in tuple) in ets:new/2. Checking if updated element shouldn't be placed as first element is wrong. Signed-off-by: Jakub Gonet <jakub.gonet@swmansion.com>

Additionally, unifies tuple arity checks (no change in behavior). Signed-off-by: Jakub Gonet <jakub.gonet@swmansion.com>

Signed-off-by: Jakub Gonet <jakub.gonet@swmansion.com>

jgonet commented Jun 17, 2025

View reviewed changes

bettio requested changes Jul 1, 2025

View reviewed changes

jgonet added 17 commits July 8, 2025 21:46

ets: add duplicate_bag option

f632d7b

Signed-off-by: Jakub Gonet <jakub.gonet@swmansion.com>

ets: no prefix for internal functions, add missing headers

3700a10

Signed-off-by: Jakub Gonet <jakub.gonet@swmansion.com>

term: improve docs

fbcf57f

Signed-off-by: Jakub Gonet <jakub.gonet@swmansion.com>

ets: use index instead of position (NIFs)

e87cc6a

ETS mixes position (1-based) with indexing (0-based). Tuple are 0-indexed so we should convert to it at earliest possible moment. This commit doesn't introduce any change in behavior. Signed-off-by: Jakub Gonet <jakub.gonet@swmansion.com>

ets: use 0-indexing for lookup/2

bf77d3b

Signed-off-by: Jakub Gonet <jakub.gonet@swmansion.com>

ets: use 0-indexing for update_counter/4

d892b24

Signed-off-by: Jakub Gonet <jakub.gonet@swmansion.com>

ets: fix update_counter position check

64f2bb2

ETS supports setting different key position (default: first element in tuple) in ets:new/2. Checking if updated element shouldn't be placed as first element is wrong. Signed-off-by: Jakub Gonet <jakub.gonet@swmansion.com>

ets: rename ets_table->keypos to key_index

d791640

Additionally, unifies tuple arity checks (no change in behavior). Signed-off-by: Jakub Gonet <jakub.gonet@swmansion.com>

ets: rename params

72ea5b2

Signed-off-by: Jakub Gonet <jakub.gonet@swmansion.com>

ets: rename entry to tuple in insert

c804e42

Signed-off-by: Jakub Gonet <jakub.gonet@swmansion.com>

ets: rename internal lookup function

daaabc2

Signed-off-by: Jakub Gonet <jakub.gonet@swmansion.com>

ets: use has_key_at helper everywhere

492beb1

Signed-off-by: Jakub Gonet <jakub.gonet@swmansion.com>

ets: support duplicate_bag in lookup/2

684d4b8

Signed-off-by: Jakub Gonet <jakub.gonet@swmansion.com>

ets: support duplicate_bag in insert/2 list

98acad0

Signed-off-by: Jakub Gonet <jakub.gonet@swmansion.com>

ets: remove prefix from internal functions

bca4c4f

Signed-off-by: Jakub Gonet <jakub.gonet@swmansion.com>

ets: add duplicate_bag error for update_counter/4

d6eee06

Signed-off-by: Jakub Gonet <jakub.gonet@swmansion.com>

ets: uncomment working tests

0e7d3f5

Signed-off-by: Jakub Gonet <jakub.gonet@swmansion.com>

jgonet force-pushed the jgonet/ets-2 branch from 42fd2a1 to 0e7d3f5 Compare July 8, 2025 19:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Support duplicate bag in insert and lookup #1718

Support duplicate bag in insert and lookup #1718

Uh oh!

jgonet commented Jun 17, 2025 •

edited

Loading

Uh oh!

jgonet Jun 17, 2025

Uh oh!

jgonet Jun 17, 2025

Uh oh!

jgonet Jun 17, 2025

Uh oh!

bettio Jul 1, 2025

Uh oh!

jgonet Jul 1, 2025

Uh oh!

jgonet Jun 17, 2025

Uh oh!

bettio Jul 1, 2025

Uh oh!

bettio left a comment

Uh oh!

bettio Jul 1, 2025

Uh oh!

bettio Jul 1, 2025

Uh oh!

bettio Jul 1, 2025

Uh oh!

bettio Jul 1, 2025

Uh oh!

Uh oh!

Support duplicate bag in insert and lookup #1718

Are you sure you want to change the base?

Support duplicate bag in insert and lookup #1718

Uh oh!

Conversation

jgonet commented Jun 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bettio left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jgonet commented Jun 17, 2025 •

edited

Loading