GH-4847 ArrayBindingSetIterator should not advance on hasNext, also avoid LinkedHashSet althoughether

Signed-off-by: Jerven Bolleman <jerven.bolleman@sib.swiss>

Author: JervenBolleman

core/queryalgebra/evaluation/src/main/java/org/eclipse/rdf4j/query/algebra/evaluation/ArrayBindingSet.java

@@ -10,11 +10,13 @@
  *******************************************************************************/
 package org.eclipse.rdf4j.query.algebra.evaluation;
 
+import java.util.AbstractSet;
 import java.util.ArrayList;
 import java.util.Arrays;
+import java.util.Collection;
 import java.util.Collections;
+import java.util.ConcurrentModificationException;
 import java.util.Iterator;
-import java.util.LinkedHashSet;
 import java.util.List;
 import java.util.Set;
 import java.util.function.BiConsumer;
@@ -44,9 +46,6 @@ public class ArrayBindingSet extends AbstractBindingSet implements MutableBindin
 
 	private final String[] bindingNames;
 
-	// Creating a LinkedHashSet is expensive, so we should cache the binding names set
-	private Set<String> bindingNamesSetCache;
-
 	private final boolean[] whichBindingsHaveBeenSet;
 
 	private final Value[] values;
@@ -105,7 +104,6 @@ public BiConsumer<Value, ArrayBindingSet> getDirectSetBinding(String bindingName
 		return (v, a) -> {
 			a.values[index] = v;
 			a.whichBindingsHaveBeenSet[index] = true;
-			a.clearCache();
 		};
 	}
 
@@ -120,7 +118,6 @@ public BiConsumer<Value, ArrayBindingSet> getDirectAddBinding(String bindingName
 			assert !a.whichBindingsHaveBeenSet[index] : "variable already bound: " + bindingName;
 			a.values[index] = v;
 			a.whichBindingsHaveBeenSet[index] = true;
-			a.clearCache();
 		};
 
 	}
@@ -175,30 +172,21 @@ private int getIndex(String bindingName) {
 
 	@Override
 	public Set<String> getBindingNames() {
-		if (bindingNamesSetCache == null) {
-			int size = size();
-			if (size == 0) {
-				this.bindingNamesSetCache = Collections.emptySet();
-			} else if (size == 1) {
-				for (int i = 0; i < this.bindingNames.length; i++) {
-					if (whichBindingsHaveBeenSet[i]) {
-						this.bindingNamesSetCache = Collections.singleton(bindingNames[i]);
-						break;
-					}
+		final int size = ArrayBindingSet.this.size();
+		switch (size) {
+		case 0:
+			return Collections.emptySet();
+		case 1:
+			for (int i = 0; i < this.bindingNames.length; i++) {
+				if (whichBindingsHaveBeenSet[i]) {
+					return Collections.singleton(bindingNames[i]);
 				}
-				assert this.bindingNamesSetCache != null;
-			} else {
-				LinkedHashSet<String> bindingNamesSetCache = new LinkedHashSet<>(size * 2);
-				for (int i = 0; i < this.bindingNames.length; i++) {
-					if (whichBindingsHaveBeenSet[i]) {
-						bindingNamesSetCache.add(bindingNames[i]);
-					}
-				}
-				this.bindingNamesSetCache = Collections.unmodifiableSet(bindingNamesSetCache);
 			}
+			throw new ConcurrentModificationException(
+					"An bindingset has been modified during the getBindingNames call");
+		default:
+			return new MinimallyAllocatingSet(size);
 		}
-
-		return bindingNamesSetCache;
 	}
 
 	@Override
@@ -285,6 +273,53 @@ public List<String> getSortedBindingNames() {
 	/*------------------------------------*
 	 * Inner class ArrayBindingSetIterator *
 	 *------------------------------------*/
+	private static final class BindingToBindingNameIterator implements Iterator<String> {
+		private final Iterator<Binding> nested;
+
+		private BindingToBindingNameIterator(Iterator<Binding> nested) {
+			this.nested = nested;
+		}
+
+		@Override
+		public boolean hasNext() {
+			return nested.hasNext();
+		}
+
+		@Override
+		public String next() {
+			return nested.next().getName();
+		}
+	}
+
+	private final class MinimallyAllocatingSet extends AbstractSet<String> {
+
+		private final int size;
+
+		private MinimallyAllocatingSet(int size) {
+			this.size = size;
+		}
+
+		@Override
+		public int size() {
+			return size;
+		}
+
+		@Override
+		public Iterator<String> iterator() {
+			Iterator<Binding> nested = ArrayBindingSet.this.iterator();
+			return new BindingToBindingNameIterator(nested);
+		}
+
+		@Override
+		public boolean add(String e) {
+			throw new UnsupportedOperationException();
+		}
+
+		@Override
+		public boolean addAll(Collection<? extends String> c) {
+			throw new UnsupportedOperationException();
+		}
+	}
 
 	private class ArrayBindingSetIterator implements Iterator<Binding> {
 
@@ -295,8 +330,8 @@ public ArrayBindingSetIterator() {
 
 		@Override
 		public boolean hasNext() {
-			for (; index < values.length; index++) {
-				if (whichBindingsHaveBeenSet[index] && values[index] != null) {
+			for (int at = index; at < values.length; at++) {
+				if (whichBindingsHaveBeenSet[at] && values[at] != null) {
 					return true;
 				}
 			}
@@ -337,11 +372,9 @@ public void addBinding(Binding binding) {
 			return;
 		}
 
-		assert !this.whichBindingsHaveBeenSet[index];
+		assert !this.whichBindingsHaveBeenSet[index] : "variable already bound: " + binding.getName();
 		this.values[index] = binding.getValue();
 		this.whichBindingsHaveBeenSet[index] = true;
-		clearCache();
-
 	}
 
 	@Override
@@ -352,7 +385,6 @@ public void setBinding(Binding binding) {
 		}
 		this.values[index] = binding.getValue();
 		this.whichBindingsHaveBeenSet[index] = true;
-		clearCache();
 	}
 
 	@Override
@@ -364,7 +396,6 @@ public void setBinding(String name, Value value) {
 
 		this.values[index] = value;
 		this.whichBindingsHaveBeenSet[index] = value != null;
-		clearCache();
 	}
 
 	@Override
@@ -376,8 +407,4 @@ public boolean isEmpty() {
 		}
 		return true;
 	}
-
-	private void clearCache() {
-		bindingNamesSetCache = null;
-	}
 }