From 11074d312888f250cc3fc746ec3ad0bd06a00039 Mon Sep 17 00:00:00 2001 From: Nipuna Fernando Date: Wed, 10 Jan 2024 12:36:57 +0530 Subject: [PATCH 1/6] Handle hash collision with put method --- .../internal/values/TableValueImpl.java | 35 ++++++++++++++-- .../langlib/test/LangLibTableTest.java | 14 ++++++- .../test/resources/test-src/tablelib_test.bal | 42 ++++++++++++++++++- 3 files changed, 84 insertions(+), 7 deletions(-) diff --git a/bvm/ballerina-runtime/src/main/java/io/ballerina/runtime/internal/values/TableValueImpl.java b/bvm/ballerina-runtime/src/main/java/io/ballerina/runtime/internal/values/TableValueImpl.java index badb37a26cf7..6040f1672c1c 100644 --- a/bvm/ballerina-runtime/src/main/java/io/ballerina/runtime/internal/values/TableValueImpl.java +++ b/bvm/ballerina-runtime/src/main/java/io/ballerina/runtime/internal/values/TableValueImpl.java @@ -571,12 +571,12 @@ public void addData(V data) { maxIntKey = ((Long) TypeChecker.anyToInt(key)).intValue(); } + Map.Entry entry = new AbstractMap.SimpleEntry(key, data); Long hash = TableUtils.hash(key, null); if (entries.containsKey(hash)) { updateIndexKeyMappings(hash, key, data); List> extEntries = entries.get(hash); - Map.Entry entry = new AbstractMap.SimpleEntry(key, data); extEntries.add(entry); List extValues = values.get(hash); extValues.add(data); @@ -585,7 +585,6 @@ public void addData(V data) { ArrayList newData = new ArrayList<>(); newData.add(data); - Map.Entry entry = new AbstractMap.SimpleEntry(key, data); putData(key, data, newData, entry, hash); } @@ -616,6 +615,10 @@ public V putData(K key, V data) { ErrorHelper.getErrorDetails(ErrorCodes.KEY_NOT_FOUND_IN_VALUE, key, data)); } + if (entries.containsKey(hash)) { + return handleHashCollisionInPut(key, data, entry, hash); + } + return putData(key, data, newData, entry, hash); } @@ -632,15 +635,39 @@ public V putData(V data) { MapValue dataMap = (MapValue) data; checkInherentTypeViolation(dataMap, tableType); K key = this.keyWrapper.wrapKey(dataMap); + Map.Entry entry = new AbstractMap.SimpleEntry<>(key, data); + Long hash = TableUtils.hash(key, null); + + if (entries.containsKey(hash)) { + return handleHashCollisionInPut(key, data, entry, hash); + } ArrayList newData = new ArrayList<>(); newData.add(data); - Map.Entry entry = new AbstractMap.SimpleEntry<>(key, data); - Long hash = TableUtils.hash(key, null); return putData((K) key, data, newData, entry, hash); } + private V handleHashCollisionInPut(K key, V data, Map.Entry entry, Long hash) { + updateIndexKeyMappings(hash, key, data); + List> entryList = entries.get(hash); + for (Map.Entry extEntry: entryList) { + if (TypeChecker.isEqual(key, extEntry.getKey())) { + entryList.remove(extEntry); + entryList.add(entry); + List valueList = values.get(hash); + valueList.remove(extEntry.getValue()); + valueList.add(data); + return data; + } + } + List> extEntries = entries.get(hash); + extEntries.add(entry); + List extValues = values.get(hash); + extValues.add(data); + return data; + } + public V remove(K key) { keyValues.remove(key); Long hash = TableUtils.hash(key, null); diff --git a/langlib/langlib-test/src/test/java/org/ballerinalang/langlib/test/LangLibTableTest.java b/langlib/langlib-test/src/test/java/org/ballerinalang/langlib/test/LangLibTableTest.java index c5451fc43175..3e02526bf782 100644 --- a/langlib/langlib-test/src/test/java/org/ballerinalang/langlib/test/LangLibTableTest.java +++ b/langlib/langlib-test/src/test/java/org/ballerinalang/langlib/test/LangLibTableTest.java @@ -123,8 +123,18 @@ public void testHashCollisionHandlingScenarios() { } @Test - public void testHashCollisionInQuery() { - BRunUtil.invoke(compileResult, "testHashCollisionInQuery"); + public void testHashCollisionInQueryWithAdd() { + BRunUtil.invoke(compileResult, "testHashCollisionInQueryWithAdd"); + } + + @Test + public void testHashCollisionInQueryWithPut() { + BRunUtil.invoke(compileResult, "testHashCollisionInQueryWithPut"); + } + + @Test + public void testHashCollisionInFilter() { + BRunUtil.invoke(compileResult, "testHashCollisionInFilter"); } @Test diff --git a/langlib/langlib-test/src/test/resources/test-src/tablelib_test.bal b/langlib/langlib-test/src/test/resources/test-src/tablelib_test.bal index 40394f1b6a5e..aaef8445f433 100644 --- a/langlib/langlib-test/src/test/resources/test-src/tablelib_test.bal +++ b/langlib/langlib-test/src/test/resources/test-src/tablelib_test.bal @@ -355,7 +355,7 @@ function testHashCollisionHandlingScenarios() { } -function testHashCollisionInQuery() { +function testHashCollisionInQueryWithAdd() { table key(k) tbl1 = table [{k: "10"}]; table tbl2 = table [{k: 0}]; @@ -379,6 +379,46 @@ function testHashCollisionInQuery() { assertEquals(tbl2, tbl4); } +function testHashCollisionInQueryWithPut() { + table key(k) tbl1 = table [{k: "10"}]; + table tbl2 = table [{k: 0}]; + + tbl1.put({k: 5}); + tbl1.put({k: ()}); + tbl1.put({k: -31}); + tbl1.put({k: 0}); + tbl1.put({k: 100.05}); + tbl1.put({k: 30}); + table tbl3 = + from var tid in tbl1 + where tid["k"] == 0 + select tid; + assertEquals(tbl2, tbl3); + + _ = tbl1.remove(()); + table tbl4 = + from var tid in tbl1 + where tid["k"] == 0 + select tid; + assertEquals(tbl2, tbl4); +} + +function testHashCollisionInFilter() { + table key(k) tbl1 = table [{k: "10"}]; + table key(k) tbl2 = table [{k: ()}, {k: 0}]; + + tbl1.put({k: 5}); + tbl1.put({k: ()}); + tbl1.put({k: -31}); + tbl1.put({k: 0}); + tbl1.put({k: 100.05}); + tbl1.put({k: 30}); + + table tbl3 = tbl1.filter( + function(record {readonly int|string|float? k;} tid) returns boolean + => tid["k"] == 0 || tid["k"] == ()); + assertEquals(tbl2, tbl3); +} public function testGetKeysOfHashCollidedKeys() { table key(k) tbl1 = table [ {k: 5}, {k: 0}, {k: ()}, {k: 2} From 1ebdc03aa6561e22da7f83713938bd75616f10be Mon Sep 17 00:00:00 2001 From: Nipuna Fernando Date: Wed, 17 Jan 2024 11:06:44 +0530 Subject: [PATCH 2/6] Address review suggestions --- .../internal/values/TableValueImpl.java | 6 ++-- .../test/resources/test-src/tablelib_test.bal | 28 ++++++++++++------- 2 files changed, 20 insertions(+), 14 deletions(-) diff --git a/bvm/ballerina-runtime/src/main/java/io/ballerina/runtime/internal/values/TableValueImpl.java b/bvm/ballerina-runtime/src/main/java/io/ballerina/runtime/internal/values/TableValueImpl.java index 6040f1672c1c..9c40db1ed944 100644 --- a/bvm/ballerina-runtime/src/main/java/io/ballerina/runtime/internal/values/TableValueImpl.java +++ b/bvm/ballerina-runtime/src/main/java/io/ballerina/runtime/internal/values/TableValueImpl.java @@ -661,10 +661,8 @@ private V handleHashCollisionInPut(K key, V data, Map.Entry entry, Long ha return data; } } - List> extEntries = entries.get(hash); - extEntries.add(entry); - List extValues = values.get(hash); - extValues.add(data); + entryList.add(entry); + values.get(hash).add(data); return data; } diff --git a/langlib/langlib-test/src/test/resources/test-src/tablelib_test.bal b/langlib/langlib-test/src/test/resources/test-src/tablelib_test.bal index aaef8445f433..a2bf5745942c 100644 --- a/langlib/langlib-test/src/test/resources/test-src/tablelib_test.bal +++ b/langlib/langlib-test/src/test/resources/test-src/tablelib_test.bal @@ -358,6 +358,8 @@ function testHashCollisionHandlingScenarios() { function testHashCollisionInQueryWithAdd() { table key(k) tbl1 = table [{k: "10"}]; table tbl2 = table [{k: 0}]; + table key(k) tbl3 = table [ + {k: "10"}, {k: 5}, {k: ()}, {k: -31}, {k: 0}, {k: 100.05}, {k: 30}]; tbl1.add({k: 5}); tbl1.add({k: ()}); @@ -365,23 +367,27 @@ function testHashCollisionInQueryWithAdd() { tbl1.add({k: 0}); tbl1.add({k: 100.05}); tbl1.add({k: 30}); - table tbl3 = + assertEquals(tbl3, tbl1); + + table tbl4 = from var tid in tbl1 where tid["k"] == 0 select tid; - assertEquals(tbl2, tbl3); + assertEquals(tbl2, tbl4); _ = tbl1.remove(()); - table tbl4 = + table tbl5 = from var tid in tbl1 where tid["k"] == 0 select tid; - assertEquals(tbl2, tbl4); + assertEquals(tbl2, tbl5); } function testHashCollisionInQueryWithPut() { table key(k) tbl1 = table [{k: "10"}]; table tbl2 = table [{k: 0}]; + table key(k) tbl3 = table [ + {k: "10"}, {k: 5}, {k: ()}, {k: -31}, {k: 0}, {k: 100.05}, {k: 30}]; tbl1.put({k: 5}); tbl1.put({k: ()}); @@ -389,18 +395,20 @@ function testHashCollisionInQueryWithPut() { tbl1.put({k: 0}); tbl1.put({k: 100.05}); tbl1.put({k: 30}); - table tbl3 = + assertEquals(tbl3, tbl1); + + table tbl4 = from var tid in tbl1 where tid["k"] == 0 select tid; - assertEquals(tbl2, tbl3); + assertEquals(tbl2, tbl4); _ = tbl1.remove(()); - table tbl4 = + table tbl5 = from var tid in tbl1 where tid["k"] == 0 select tid; - assertEquals(tbl2, tbl4); + assertEquals(tbl2, tbl5); } function testHashCollisionInFilter() { @@ -414,10 +422,10 @@ function testHashCollisionInFilter() { tbl1.put({k: 100.05}); tbl1.put({k: 30}); - table tbl3 = tbl1.filter( + table tbl4 = tbl1.filter( function(record {readonly int|string|float? k;} tid) returns boolean => tid["k"] == 0 || tid["k"] == ()); - assertEquals(tbl2, tbl3); + assertEquals(tbl2, tbl4); } public function testGetKeysOfHashCollidedKeys() { table key(k) tbl1 = table [ From 0a746ff5eb2b5b175a8f2855889eddb31749518e Mon Sep 17 00:00:00 2001 From: Nipuna Fernando Date: Wed, 31 Jan 2024 11:55:59 +0530 Subject: [PATCH 3/6] Address review suggestions --- .../internal/values/TableValueImpl.java | 6 +-- .../langlib/test/LangLibTableTest.java | 41 ++++++++----------- 2 files changed, 19 insertions(+), 28 deletions(-) diff --git a/bvm/ballerina-runtime/src/main/java/io/ballerina/runtime/internal/values/TableValueImpl.java b/bvm/ballerina-runtime/src/main/java/io/ballerina/runtime/internal/values/TableValueImpl.java index 9c40db1ed944..e07028ab6992 100644 --- a/bvm/ballerina-runtime/src/main/java/io/ballerina/runtime/internal/values/TableValueImpl.java +++ b/bvm/ballerina-runtime/src/main/java/io/ballerina/runtime/internal/values/TableValueImpl.java @@ -616,7 +616,7 @@ public V putData(K key, V data) { } if (entries.containsKey(hash)) { - return handleHashCollisionInPut(key, data, entry, hash); + return handleHashCollisionForPut(key, data, entry, hash); } return putData(key, data, newData, entry, hash); @@ -639,7 +639,7 @@ public V putData(V data) { Long hash = TableUtils.hash(key, null); if (entries.containsKey(hash)) { - return handleHashCollisionInPut(key, data, entry, hash); + return handleHashCollisionForPut(key, data, entry, hash); } ArrayList newData = new ArrayList<>(); @@ -648,7 +648,7 @@ public V putData(V data) { return putData((K) key, data, newData, entry, hash); } - private V handleHashCollisionInPut(K key, V data, Map.Entry entry, Long hash) { + private V handleHashCollisionForPut(K key, V data, Map.Entry entry, Long hash) { updateIndexKeyMappings(hash, key, data); List> entryList = entries.get(hash); for (Map.Entry extEntry: entryList) { diff --git a/langlib/langlib-test/src/test/java/org/ballerinalang/langlib/test/LangLibTableTest.java b/langlib/langlib-test/src/test/java/org/ballerinalang/langlib/test/LangLibTableTest.java index 3e02526bf782..89a71714b5d5 100644 --- a/langlib/langlib-test/src/test/java/org/ballerinalang/langlib/test/LangLibTableTest.java +++ b/langlib/langlib-test/src/test/java/org/ballerinalang/langlib/test/LangLibTableTest.java @@ -117,31 +117,6 @@ public void testGetValue() { BRunUtil.invoke(compileResult, "testGetValue"); } - @Test - public void testHashCollisionHandlingScenarios() { - BRunUtil.invoke(compileResult, "testHashCollisionHandlingScenarios"); - } - - @Test - public void testHashCollisionInQueryWithAdd() { - BRunUtil.invoke(compileResult, "testHashCollisionInQueryWithAdd"); - } - - @Test - public void testHashCollisionInQueryWithPut() { - BRunUtil.invoke(compileResult, "testHashCollisionInQueryWithPut"); - } - - @Test - public void testHashCollisionInFilter() { - BRunUtil.invoke(compileResult, "testHashCollisionInFilter"); - } - - @Test - public void testGetKeysOfHashCollidedKeys() { - BRunUtil.invoke(compileResult, "testGetKeysOfHashCollidedKeys"); - } - @Test public void testGetKeyList() { Object result = BRunUtil.invoke(compileResult, "testGetKeyList"); @@ -612,4 +587,20 @@ public void testTableIterationAfterPut() { BRunUtil.invoke(compileResult, "testTableIterationAfterPut3"); BRunUtil.invoke(compileResult, "testTableIterationAfterPut4"); } + + @Test(dataProvider = "functionsToTestHashCollisionInTable") + public void testHashCollisionInTable(String function) { + BRunUtil.invoke(compileResult, function); + } + + @DataProvider + public Object[] functionsToTestHashCollisionInTable() { + return new String[]{ + "testHashCollisionHandlingScenarios", + "testHashCollisionInQueryWithAdd", + "testHashCollisionInQueryWithPut", + "testHashCollisionInFilter", + "testGetKeysOfHashCollidedKeys" + }; + } } From feba581dec2c25610c56c8a006379287a8a0ab8a Mon Sep 17 00:00:00 2001 From: Nipuna Fernando Date: Thu, 7 Mar 2024 15:13:06 +0530 Subject: [PATCH 4/6] Refactor the code for better readability --- .../internal/values/TableValueImpl.java | 7 ++-- .../test/resources/test-src/tablelib_test.bal | 36 +++++++++++++------ 2 files changed, 29 insertions(+), 14 deletions(-) diff --git a/bvm/ballerina-runtime/src/main/java/io/ballerina/runtime/internal/values/TableValueImpl.java b/bvm/ballerina-runtime/src/main/java/io/ballerina/runtime/internal/values/TableValueImpl.java index e07028ab6992..30e680cd8d77 100644 --- a/bvm/ballerina-runtime/src/main/java/io/ballerina/runtime/internal/values/TableValueImpl.java +++ b/bvm/ballerina-runtime/src/main/java/io/ballerina/runtime/internal/values/TableValueImpl.java @@ -616,7 +616,7 @@ public V putData(K key, V data) { } if (entries.containsKey(hash)) { - return handleHashCollisionForPut(key, data, entry, hash); + return updateExistingEntry(key, data, entry, hash); } return putData(key, data, newData, entry, hash); @@ -639,7 +639,7 @@ public V putData(V data) { Long hash = TableUtils.hash(key, null); if (entries.containsKey(hash)) { - return handleHashCollisionForPut(key, data, entry, hash); + return updateExistingEntry(key, data, entry, hash); } ArrayList newData = new ArrayList<>(); @@ -648,10 +648,11 @@ public V putData(V data) { return putData((K) key, data, newData, entry, hash); } - private V handleHashCollisionForPut(K key, V data, Map.Entry entry, Long hash) { + private V updateExistingEntry(K key, V data, Map.Entry entry, Long hash) { updateIndexKeyMappings(hash, key, data); List> entryList = entries.get(hash); for (Map.Entry extEntry: entryList) { + // Handle hash-collided entries if (TypeChecker.isEqual(key, extEntry.getKey())) { entryList.remove(extEntry); entryList.add(entry); diff --git a/langlib/langlib-test/src/test/resources/test-src/tablelib_test.bal b/langlib/langlib-test/src/test/resources/test-src/tablelib_test.bal index a2bf5745942c..3ee983658b63 100644 --- a/langlib/langlib-test/src/test/resources/test-src/tablelib_test.bal +++ b/langlib/langlib-test/src/test/resources/test-src/tablelib_test.bal @@ -384,17 +384,19 @@ function testHashCollisionInQueryWithAdd() { } function testHashCollisionInQueryWithPut() { - table key(k) tbl1 = table [{k: "10"}]; - table tbl2 = table [{k: 0}]; - table key(k) tbl3 = table [ - {k: "10"}, {k: 5}, {k: ()}, {k: -31}, {k: 0}, {k: 100.05}, {k: 30}]; - - tbl1.put({k: 5}); - tbl1.put({k: ()}); - tbl1.put({k: -31}); - tbl1.put({k: 0}); - tbl1.put({k: 100.05}); - tbl1.put({k: 30}); + table key(k) tbl1 = table [{k: "10", v: 1}]; + table tbl2 = table [{k: 0, v: 2}]; + table key(k) tbl3 = table [ + {k: "10", v: 1}, {k: 5, v: 2}, {k: 0, v: 2}, {k: (), v: 3}, {k: -31, v: 4}, {k: 100.05, v: 1}, {k: 30, v: 2}]; + + tbl1.put({k: 5, v: 1}); + tbl1.put({k: (), v: 1}); + tbl1.put({k: -31, v: 4}); + tbl1.put({k: 0, v: 2}); + tbl1.put({k: 5, v: 2}); + tbl1.put({k: 100.05, v: 1}); + tbl1.put({k: 30, v: 2}); + tbl1.put({k: (), v: 3}); assertEquals(tbl3, tbl1); table tbl4 = @@ -433,6 +435,18 @@ public function testGetKeysOfHashCollidedKeys() { ]; assertEquals(tbl1.keys(), [5, 0, (), 2]); + + table key(k) tbl2 = table [ + {k: 5}, {k: 0}, {k: 2} + ]; + tbl2.add({k: ()}); + assertEquals(tbl2.keys(), [5, 0, 2, ()]); + + table key(k) tbl3 = table [ + {k: 5}, {k: 0}, {k: 2} + ]; + tbl3.put({k: ()}); + assertEquals(tbl3.keys(), [5, 0, 2, ()]); } function testGetKeyList() returns any[] { From 0feb7207bbd306bc98f3b3432b2d6913f820d465 Mon Sep 17 00:00:00 2001 From: Nipuna Fernando Date: Thu, 7 Mar 2024 15:25:08 +0530 Subject: [PATCH 5/6] Extract adding a new value to putNewData --- .../internal/values/TableValueImpl.java | 18 ++++++------------ 1 file changed, 6 insertions(+), 12 deletions(-) diff --git a/bvm/ballerina-runtime/src/main/java/io/ballerina/runtime/internal/values/TableValueImpl.java b/bvm/ballerina-runtime/src/main/java/io/ballerina/runtime/internal/values/TableValueImpl.java index 30e680cd8d77..522f17d39153 100644 --- a/bvm/ballerina-runtime/src/main/java/io/ballerina/runtime/internal/values/TableValueImpl.java +++ b/bvm/ballerina-runtime/src/main/java/io/ballerina/runtime/internal/values/TableValueImpl.java @@ -583,9 +583,7 @@ public void addData(V data) { return; } - ArrayList newData = new ArrayList<>(); - newData.add(data); - putData(key, data, newData, entry, hash); + putNewData(key, data, entry, hash); } public V getData(K key) { @@ -602,9 +600,6 @@ public V getData(K key) { } public V putData(K key, V data) { - ArrayList newData = new ArrayList<>(); - newData.add(data); - Map.Entry entry = new AbstractMap.SimpleEntry(key, data); Object actualKey = this.keyWrapper.wrapKey((MapValue) data); Long actualHash = TableUtils.hash(actualKey, null); @@ -619,10 +614,12 @@ public V putData(K key, V data) { return updateExistingEntry(key, data, entry, hash); } - return putData(key, data, newData, entry, hash); + return putNewData(key, data, entry, hash); } - private V putData(K key, V value, List data, Map.Entry entry, Long hash) { + private V putNewData(K key, V value, Map.Entry entry, Long hash) { + List data = new ArrayList<>(); + data.add(value); updateIndexKeyMappings(hash, key, value); List> entryList = new ArrayList<>(); entryList.add(entry); @@ -642,10 +639,7 @@ public V putData(V data) { return updateExistingEntry(key, data, entry, hash); } - ArrayList newData = new ArrayList<>(); - newData.add(data); - - return putData((K) key, data, newData, entry, hash); + return putNewData((K) key, data, entry, hash); } private V updateExistingEntry(K key, V data, Map.Entry entry, Long hash) { From 3672912e3413cdca23be02f0e2f8e93728fed21a Mon Sep 17 00:00:00 2001 From: Nipuna Fernando Date: Fri, 8 Mar 2024 16:42:14 +0530 Subject: [PATCH 6/6] Address review suggestions --- .../ballerina/runtime/internal/values/TableValueImpl.java | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/bvm/ballerina-runtime/src/main/java/io/ballerina/runtime/internal/values/TableValueImpl.java b/bvm/ballerina-runtime/src/main/java/io/ballerina/runtime/internal/values/TableValueImpl.java index 522f17d39153..796554b95492 100644 --- a/bvm/ballerina-runtime/src/main/java/io/ballerina/runtime/internal/values/TableValueImpl.java +++ b/bvm/ballerina-runtime/src/main/java/io/ballerina/runtime/internal/values/TableValueImpl.java @@ -613,7 +613,6 @@ public V putData(K key, V data) { if (entries.containsKey(hash)) { return updateExistingEntry(key, data, entry, hash); } - return putNewData(key, data, entry, hash); } @@ -638,22 +637,19 @@ public V putData(V data) { if (entries.containsKey(hash)) { return updateExistingEntry(key, data, entry, hash); } - return putNewData((K) key, data, entry, hash); } private V updateExistingEntry(K key, V data, Map.Entry entry, Long hash) { updateIndexKeyMappings(hash, key, data); + List valueList = values.get(hash); List> entryList = entries.get(hash); for (Map.Entry extEntry: entryList) { // Handle hash-collided entries if (TypeChecker.isEqual(key, extEntry.getKey())) { entryList.remove(extEntry); - entryList.add(entry); - List valueList = values.get(hash); valueList.remove(extEntry.getValue()); - valueList.add(data); - return data; + break; } } entryList.add(entry);