This is an automated email from the ASF dual-hosted git repository.
gian pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/druid.git
The following commit(s) were added to refs/heads/master by this push:
new 7354953 VectorMatch: Disallow "copyFrom", "addAll" on self; improve
tests. (#10755)
7354953 is described below
commit 7354953b1bff619993b7887a15f10a6f52341d8e
Author: Gian Merlino <gianmerl...@gmail.com>
AuthorDate: Thu Jan 14 18:29:13 2021 -0800
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.
---
.../druid/query/filter/vector/VectorMatch.java | 13 +-
.../druid/query/filter/vector/VectorMatchTest.java | 203 +++++++++++++--------
2 files changed, 133 insertions(+), 83 deletions(-)
diff --git
a/processing/src/main/java/org/apache/druid/query/filter/vector/VectorMatch.java
b/processing/src/main/java/org/apache/druid/query/filter/vector/VectorMatch.java
index 54a6947..5c7e671 100644
---
a/processing/src/main/java/org/apache/druid/query/filter/vector/VectorMatch.java
+++
b/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
diff --git
a/processing/src/test/java/org/apache/druid/query/filter/vector/VectorMatchTest.java
b/processing/src/test/java/org/apache/druid/query/filter/vector/VectorMatchTest.java
index 5a67545..2c3991b 100644
---
a/processing/src/test/java/org/apache/druid/query/filter/vector/VectorMatchTest.java
+++
b/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;
}
}
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@druid.apache.org
For additional commands, e-mail: commits-h...@druid.apache.org
