MultiMap: don't sort values; use .push(..) and .swap_remove(..) instead

crates/table/src/btree_index.rs

@@ -130,16 +130,17 @@ impl TypedIndex {
     /// this will behave oddly; it may return an error,
     /// or may insert a nonsense value into the index.
     /// Note, however, that it will not invoke undefined behavior.
-    fn insert(&mut self, cols: &ColList, row_ref: RowRef<'_>) -> Result<bool, InvalidFieldError> {
+    fn insert(&mut self, cols: &ColList, row_ref: RowRef<'_>) -> Result<(), InvalidFieldError> {
         fn insert_at_type<T: Ord + ReadColumn>(
             this: &mut MultiMap<T, RowPointer>,
             cols: &ColList,
             row_ref: RowRef<'_>,
-        ) -> Result<bool, InvalidFieldError> {
+        ) -> Result<(), InvalidFieldError> {
             debug_assert!(cols.is_singleton());
             let col_pos = cols.head();
             let key = row_ref.read_col(col_pos).map_err(|_| col_pos)?;
-            Ok(this.insert(key, row_ref.pointer()))
+            this.insert(key, row_ref.pointer());
+            Ok(())
         }
         match self {
             TypedIndex::Bool(ref mut this) => insert_at_type(this, cols, row_ref),
@@ -158,7 +159,8 @@ impl TypedIndex {
 
             TypedIndex::AlgebraicValue(ref mut this) => {
                 let key = row_ref.project_not_empty(cols)?;
-                Ok(this.insert(key, row_ref.pointer()))
+                this.insert(key, row_ref.pointer());
+                Ok(())
             }
         }
     }
@@ -379,7 +381,7 @@ impl BTreeIndex {
     /// This index will extract the necessary values from `row` based on `self.cols`.
     ///
     /// Return false if `ptr` was already indexed prior to this call.
-    pub fn insert(&mut self, cols: &ColList, row_ref: RowRef<'_>) -> Result<bool, InvalidFieldError> {
+    pub fn insert(&mut self, cols: &ColList, row_ref: RowRef<'_>) -> Result<(), InvalidFieldError> {
         self.idx.insert(cols, row_ref)
     }
 
@@ -420,12 +422,11 @@ impl BTreeIndex {
         &mut self,
         cols: &ColList,
         rows: impl IntoIterator<Item = RowRef<'table>>,
-    ) -> Result<bool, InvalidFieldError> {
-        let mut all_inserted = true;
+    ) -> Result<(), InvalidFieldError> {
         for row_ref in rows {
-            all_inserted &= self.insert(cols, row_ref)?;
+            self.insert(cols, row_ref)?;
         }
-        Ok(all_inserted)
+        Ok(())
     }
 
     /// Deletes all entries from the index, leaving it empty.
@@ -517,14 +518,10 @@ mod test {
             prop_assert_eq!(index.idx.len(), 0);
             prop_assert_eq!(index.contains_any(&value), false);
 
-            prop_assert_eq!(index.insert(&cols, row_ref).unwrap(), true);
+            index.insert(&cols, row_ref).unwrap();
             prop_assert_eq!(index.idx.len(), 1);
             prop_assert_eq!(index.contains_any(&value), true);
 
-            // Try inserting again, it should fail.
-            prop_assert_eq!(index.insert(&cols, row_ref).unwrap(), false);
-            prop_assert_eq!(index.idx.len(), 1);
-
             prop_assert_eq!(index.delete(&cols, row_ref).unwrap(), true);
             prop_assert_eq!(index.idx.len(), 0);
             prop_assert_eq!(index.contains_any(&value), false);
@@ -548,7 +545,7 @@ mod test {
             );
 
             // Insert.
-            prop_assert_eq!(index.insert(&cols, row_ref).unwrap(), true);
+            index.insert(&cols, row_ref).unwrap();
 
             // Inserting again would be a problem.
             prop_assert_eq!(index.idx.len(), 1);
@@ -581,7 +578,7 @@ mod test {
                 let ptr = table.insert(&mut blob_store, &row).unwrap().1;
                 val_to_ptr.insert(x, ptr);
                 let row_ref = table.get_row_ref(&blob_store, ptr).unwrap();
-                prop_assert_eq!(index.insert(&cols, row_ref).unwrap(), true);
+                index.insert(&cols, row_ref).unwrap();
             }
 
             fn test_seek(index: &BTreeIndex, val_to_ptr: &HashMap<u64, RowPointer>, range: impl RangeBounds<AlgebraicValue>, expect: impl IntoIterator<Item = u64>) -> TestCaseResult {

crates/table/src/btree_index/multimap.rs

@@ -23,27 +23,19 @@ impl<K: Ord, V: Ord> MultiMap<K, V> {
 
     /// Inserts the relation `key -> val` to this multimap.
     ///
-    /// Returns false if `key -> val` was already in the map.
-    pub fn insert(&mut self, key: K, val: V) -> bool {
-        let vset = self.map.entry(key).or_default();
-        // Use binary search to maintain the sort order.
-        // This is used to determine in `O(log(vset.len()))` whether `val` was already present.
-        let Err(idx) = vset.binary_search(&val) else {
-            return false;
-        };
-        vset.insert(idx, val);
-        true
+    /// The map does not check whether `key -> val` was already in the map.
+    pub fn insert(&mut self, key: K, val: V) {
+        self.map.entry(key).or_default().push(val);
     }
 
     /// Deletes `key -> val` from this multimap.
     ///
     /// Returns whether `key -> val` was present.
     pub fn delete(&mut self, key: &K, val: &V) -> bool {
         if let Some(vset) = self.map.get_mut(key) {
-            // The `vset` is sorted so we can binary search.
-            if let Ok(idx) = vset.binary_search(val) {
-                // Maintain the sorted order. Don't use `swap_remove`!
-                vset.remove(idx);
+            // The `vset` is not sorted, so we have to do a linear scan first.
+            if let Some(idx) = vset.iter().position(|v| v == val) {
+                vset.swap_remove(idx);
                 return true;
             }
         }