VectorMatch: Disallow "copyFrom", "addAll" on self; improve tests. (#10755)

No existing code relies on being able to call these methods in this way.

The new tests exhaustively test all vectors up to size 7, and also test
behavior the run-on-self behavior that has been adjusted by this patch.

processing/src/main/java/org/apache/druid/query/filter/vector/VectorMatch.java

@@ -169,6 +169,8 @@ public class VectorMatch implements ReadableVectorMatch
   /**
    * Adds all rows from "other" to this object, using "scratch" as scratch space if needed. Does not modify "other".
    * Returns a reference to this object.
+   *
+   * "other" and "scratch" cannot be the same instance as each other, or as this object.
    */
   public VectorMatch addAll(final ReadableVectorMatch other, final VectorMatch scratch)
   {
@@ -176,6 +178,8 @@ public class VectorMatch implements ReadableVectorMatch
     Preconditions.checkState(this != scratch, "'scratch' must be a different instance from 'this'");
     //noinspection ObjectEquality
     Preconditions.checkState(other != scratch, "'scratch' must be a different instance from 'other'");
+    //noinspection ObjectEquality
+    Preconditions.checkState(this != other, "'other' must be a different instance from 'this'");
 
     final int[] scratchSelection = scratch.getSelection();
     final int[] otherSelection = other.getSelection();
@@ -208,19 +212,24 @@ public class VectorMatch implements ReadableVectorMatch
 
   /**
    * Copies "other" into this object, and returns a reference to this object. Does not modify "other".
+   *
+   * "other" cannot be the same instance as this object.
    */
-  public VectorMatch copyFrom(final ReadableVectorMatch other)
+  public void copyFrom(final ReadableVectorMatch other)
   {
+    //noinspection ObjectEquality
+    Preconditions.checkState(this != other, "'other' must be a different instance from 'this'");
+
     Preconditions.checkState(
         selection.length >= other.getSelectionSize(),
         "Capacity[%s] cannot fit other match's selectionSize[%s]",
         selection.length,
         other.getSelectionSize()
     );
+
     System.arraycopy(other.getSelection(), 0, selection, 0, other.getSelectionSize());
     selectionSize = other.getSelectionSize();
     assert isValid(null);
-    return this;
   }
 
   @Override

processing/src/test/java/org/apache/druid/query/filter/vector/VectorMatchTest.java

@@ -19,109 +19,150 @@
 
 package org.apache.druid.query.filter.vector;
 
+import org.apache.druid.java.util.common.StringUtils;
 import org.junit.Assert;
+import org.junit.Rule;
 import org.junit.Test;
+import org.junit.rules.ExpectedException;
 
 public class VectorMatchTest
 {
-  private static final int VECTOR_SIZE = 10;
+  private static final int VECTOR_SIZE = 7;
+  private static final int VECTOR_BITS = 1 << VECTOR_SIZE;
+
+  @Rule
+  public ExpectedException expectedException = ExpectedException.none();
 
   @Test
-  public void testRemoveAll()
+  public void testAddAllExhaustive()
   {
-    assertMatchEquals(
-        VectorMatch.allFalse(),
-        copy(VectorMatch.allTrue(VECTOR_SIZE)).removeAll(VectorMatch.allTrue(VECTOR_SIZE))
-    );
-
-    assertMatchEquals(
-        VectorMatch.allTrue(VECTOR_SIZE),
-        copy(VectorMatch.allTrue(VECTOR_SIZE)).removeAll(VectorMatch.allFalse())
-    );
-
-    assertMatchEquals(
-        createMatch(new int[]{3, 6, 7, 8, 10}),
-        createMatch(new int[]{3, 5, 6, 7, 8, 10}).removeAll(createMatch(new int[]{4, 5, 9}))
-    );
-
-    assertMatchEquals(
-        createMatch(new int[]{3, 6, 7, 8, 10}),
-        createMatch(new int[]{3, 5, 6, 7, 8, 10}).removeAll(createMatch(new int[]{2, 5, 9}))
-    );
-
-    assertMatchEquals(
-        createMatch(new int[]{6, 7, 8, 10}),
-        createMatch(new int[]{3, 5, 6, 7, 8, 10}).removeAll(createMatch(new int[]{3, 5, 9}))
-    );
-
-    assertMatchEquals(
-        createMatch(new int[]{6, 7, 8}),
-        createMatch(new int[]{3, 5, 6, 7, 8, 10}).removeAll(createMatch(new int[]{3, 5, 10}))
-    );
+    // Tests every combination of vectors up to length VECTOR_SIZE.
+    final VectorMatch scratch = VectorMatch.wrap(new int[VECTOR_SIZE]);
+
+    final int[] arrayOne = new int[VECTOR_SIZE];
+    final int[] arrayTwo = new int[VECTOR_SIZE];
+    final int[] arrayExpected = new int[VECTOR_SIZE];
+
+    for (int bitsOne = 0; bitsOne < VECTOR_BITS; bitsOne++) {
+      for (int bitsTwo = 0; bitsTwo < VECTOR_BITS; bitsTwo++) {
+        final int lenOne = populate(arrayOne, bitsOne);
+        final int lenTwo = populate(arrayTwo, bitsTwo);
+        final int lenExpected = populate(arrayExpected, bitsOne | bitsTwo);
+
+        final VectorMatch matchOne = VectorMatch.wrap(arrayOne).setSelectionSize(lenOne);
+        final VectorMatch matchTwo = VectorMatch.wrap(arrayTwo).setSelectionSize(lenTwo);
+        final VectorMatch matchExpected = VectorMatch.wrap(arrayExpected).setSelectionSize(lenExpected);
+
+        assertMatchEquals(
+            StringUtils.format("%s + %s", matchOne, matchTwo),
+            matchExpected,
+            matchOne.addAll(matchTwo, scratch)
+        );
+      }
+    }
   }
 
   @Test
-  public void testAddAll()
+  public void testAddAllOnSelf()
   {
-    final VectorMatch scratch = VectorMatch.wrap(new int[VECTOR_SIZE]);
+    final VectorMatch match = VectorMatch.wrap(new int[]{0, 1}).setSelectionSize(2);
+
+    expectedException.expect(IllegalStateException.class);
+    expectedException.expectMessage("'other' must be a different instance from 'this'");
+    match.addAll(match, VectorMatch.wrap(new int[2]));
+  }
+
+  @Test
+  public void testRemoveAllExhaustive()
+  {
+    // Tests every combination of vectors up to length VECTOR_SIZE.
+
+    final int[] arrayOne = new int[VECTOR_SIZE];
+    final int[] arrayTwo = new int[VECTOR_SIZE];
+    final int[] arrayExpected = new int[VECTOR_SIZE];
+
+    for (int bitsOne = 0; bitsOne < VECTOR_BITS; bitsOne++) {
+      for (int bitsTwo = 0; bitsTwo < VECTOR_BITS; bitsTwo++) {
+        final int lenOne = populate(arrayOne, bitsOne);
+        final int lenTwo = populate(arrayTwo, bitsTwo);
+        final int lenExpected = populate(arrayExpected, bitsOne & ~bitsTwo);
+
+        final VectorMatch matchOne = VectorMatch.wrap(arrayOne).setSelectionSize(lenOne);
+        final VectorMatch matchTwo = VectorMatch.wrap(arrayTwo).setSelectionSize(lenTwo);
+        final VectorMatch matchExpected = VectorMatch.wrap(arrayExpected).setSelectionSize(lenExpected);
+
+        assertMatchEquals(
+            StringUtils.format("%s - %s", matchOne, matchTwo),
+            matchExpected,
+            matchOne.removeAll(matchTwo)
+        );
+      }
+    }
+  }
+
+  @Test
+  public void testRemoveAllOnSelf()
+  {
+    final VectorMatch match = VectorMatch.wrap(new int[]{0, 1}).setSelectionSize(2);
 
-    assertMatchEquals(
-        VectorMatch.allTrue(VECTOR_SIZE),
-        copy(VectorMatch.allTrue(VECTOR_SIZE)).addAll(VectorMatch.allTrue(VECTOR_SIZE), scratch)
-    );
-
-    assertMatchEquals(
-        VectorMatch.allTrue(VECTOR_SIZE),
-        createMatch(new int[]{}).addAll(VectorMatch.allTrue(VECTOR_SIZE), scratch)
-    );
-
-    assertMatchEquals(
-        createMatch(new int[]{3, 4, 5, 6, 7, 8, 9, 10}),
-        createMatch(new int[]{3, 5, 6, 7, 8, 10}).addAll(createMatch(new int[]{4, 5, 9}), scratch)
-    );
-
-    assertMatchEquals(
-        createMatch(new int[]{3, 4, 5, 6, 7, 8, 10}),
-        createMatch(new int[]{3, 5, 6, 7, 8}).addAll(createMatch(new int[]{4, 5, 10}), scratch)
-    );
-
-    assertMatchEquals(
-        createMatch(new int[]{2, 3, 5, 6, 7, 8, 9, 10}),
-        createMatch(new int[]{3, 5, 6, 7, 8, 10}).addAll(createMatch(new int[]{2, 5, 9}), scratch)
-    );
-
-    assertMatchEquals(
-        createMatch(new int[]{3, 5, 6, 7, 8, 9, 10}),
-        createMatch(new int[]{3, 5, 6, 7, 8, 10}).addAll(createMatch(new int[]{3, 5, 9}), scratch)
-    );
-
-    assertMatchEquals(
-        createMatch(new int[]{3, 5, 6, 7, 8, 10}),
-        createMatch(new int[]{3, 5, 6, 7, 8, 10}).addAll(createMatch(new int[]{3, 5, 10}), scratch)
-    );
+    expectedException.expect(IllegalStateException.class);
+    expectedException.expectMessage("'other' must be a different instance from 'this'");
+    match.removeAll(match);
+  }
+
+  @Test
+  public void testCopyFromExhaustive()
+  {
+    // Tests every vector up to length VECTOR_SIZE.
+
+    final VectorMatch target = VectorMatch.wrap(new int[VECTOR_SIZE]);
+
+    final int[] array = new int[VECTOR_SIZE];
+    final int[] arrayTwo = new int[VECTOR_SIZE];
+
+    for (int bits = 0; bits < VECTOR_BITS; bits++) {
+      final int len = populate(array, bits);
+      populate(arrayTwo, bits);
+
+      final VectorMatch match = VectorMatch.wrap(array).setSelectionSize(len);
+      target.copyFrom(match);
+
+      // Sanity check: array shouldn't have been modified
+      Assert.assertArrayEquals(array, arrayTwo);
+
+      assertMatchEquals(match.toString(), match, target);
+    }
+  }
+
+  @Test
+  public void testCopyFromOnSelf()
+  {
+    final VectorMatch match = VectorMatch.wrap(new int[]{0, 1}).setSelectionSize(2);
+
+    expectedException.expect(IllegalStateException.class);
+    expectedException.expectMessage("'other' must be a different instance from 'this'");
+    match.copyFrom(match);
   }
 
   /**
    * Useful because VectorMatch equality is based on identity, not value. (Since they are mutable.)
    */
-  private static void assertMatchEquals(ReadableVectorMatch expected, ReadableVectorMatch actual)
+  private static void assertMatchEquals(String message, ReadableVectorMatch expected, ReadableVectorMatch actual)
   {
-    Assert.assertEquals(expected.toString(), actual.toString());
+    Assert.assertEquals(message, expected.toString(), actual.toString());
   }
 
-  private static VectorMatch copy(final ReadableVectorMatch match)
+  private static int populate(final int[] array, final int bits)
   {
-    final int[] selection = match.getSelection();
-    final int[] newSelection = new int[selection.length];
-    System.arraycopy(selection, 0, newSelection, 0, selection.length);
-    return VectorMatch.wrap(newSelection).setSelectionSize(match.getSelectionSize());
-  }
+    int len = 0;
 
-  private static VectorMatch createMatch(final int[] selection)
-  {
-    final VectorMatch match = VectorMatch.wrap(new int[VECTOR_SIZE]);
-    System.arraycopy(selection, 0, match.getSelection(), 0, selection.length);
-    match.setSelectionSize(selection.length);
-    return match;
+    for (int bit = 0; bit < VECTOR_SIZE; bit++) {
+      final int mask = (1 << bit);
+      if ((bits & mask) == mask) {
+        array[len++] = bit;
+      }
+    }
+
+    return len;
   }
 }