Apache
/
druid
mirror of https://github.com/apache/druid.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

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.
This commit is contained in:
Gian Merlino 2021-01-14 18:29:13 -08:00 committed by GitHub
parent 2bbf89db81
commit 7354953b1b
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
2 changed files with 123 additions and 73 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

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

@ -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". * 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. * 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) 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'"); Preconditions.checkState(this != scratch, "'scratch' must be a different instance from 'this'");
//noinspection ObjectEquality //noinspection ObjectEquality
Preconditions.checkState(other != scratch, "'scratch' must be a different instance from 'other'"); 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[] scratchSelection = scratch.getSelection();
final int[] otherSelection = other.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". * 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( Preconditions.checkState(
selection.length >= other.getSelectionSize(), selection.length >= other.getSelectionSize(),
"Capacity[%s] cannot fit other match's selectionSize[%s]", "Capacity[%s] cannot fit other match's selectionSize[%s]",
selection.length, selection.length,
other.getSelectionSize() other.getSelectionSize()
); );
System.arraycopy(other.getSelection(), 0, selection, 0, other.getSelectionSize()); System.arraycopy(other.getSelection(), 0, selection, 0, other.getSelectionSize());
selectionSize = other.getSelectionSize(); selectionSize = other.getSelectionSize();
assert isValid(null); assert isValid(null);
return this;
} }
@Override @Override

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

@ -19,109 +19,150 @@
package org.apache.druid.query.filter.vector; package org.apache.druid.query.filter.vector;
import org.apache.druid.java.util.common.StringUtils;
import org.junit.Assert; import org.junit.Assert;
import org.junit.Rule;
import org.junit.Test; import org.junit.Test;
import org.junit.rules.ExpectedException;
public class VectorMatchTest 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 @Test
public void testRemoveAll() public void testAddAllExhaustive()
{ {
assertMatchEquals( // Tests every combination of vectors up to length VECTOR_SIZE.
VectorMatch.allFalse(), final VectorMatch scratch = VectorMatch.wrap(new int[VECTOR_SIZE]);
copy(VectorMatch.allTrue(VECTOR_SIZE)).removeAll(VectorMatch.allTrue(VECTOR_SIZE))
);
assertMatchEquals( final int[] arrayOne = new int[VECTOR_SIZE];
VectorMatch.allTrue(VECTOR_SIZE), final int[] arrayTwo = new int[VECTOR_SIZE];
copy(VectorMatch.allTrue(VECTOR_SIZE)).removeAll(VectorMatch.allFalse()) final int[] arrayExpected = new int[VECTOR_SIZE];
);
assertMatchEquals( for (int bitsOne = 0; bitsOne < VECTOR_BITS; bitsOne++) {
createMatch(new int[]{3, 6, 7, 8, 10}), for (int bitsTwo = 0; bitsTwo < VECTOR_BITS; bitsTwo++) {
createMatch(new int[]{3, 5, 6, 7, 8, 10}).removeAll(createMatch(new int[]{4, 5, 9})) final int lenOne = populate(arrayOne, bitsOne);
); final int lenTwo = populate(arrayTwo, bitsTwo);
final int lenExpected = populate(arrayExpected, bitsOne | bitsTwo);
assertMatchEquals( final VectorMatch matchOne = VectorMatch.wrap(arrayOne).setSelectionSize(lenOne);
createMatch(new int[]{3, 6, 7, 8, 10}), final VectorMatch matchTwo = VectorMatch.wrap(arrayTwo).setSelectionSize(lenTwo);
createMatch(new int[]{3, 5, 6, 7, 8, 10}).removeAll(createMatch(new int[]{2, 5, 9})) final VectorMatch matchExpected = VectorMatch.wrap(arrayExpected).setSelectionSize(lenExpected);
);
assertMatchEquals( assertMatchEquals(
createMatch(new int[]{6, 7, 8, 10}), StringUtils.format("%s + %s", matchOne, matchTwo),
createMatch(new int[]{3, 5, 6, 7, 8, 10}).removeAll(createMatch(new int[]{3, 5, 9})) matchExpected,
); matchOne.addAll(matchTwo, scratch)
);
assertMatchEquals( }
createMatch(new int[]{6, 7, 8}), }
createMatch(new int[]{3, 5, 6, 7, 8, 10}).removeAll(createMatch(new int[]{3, 5, 10}))
);
} }
@Test @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);
assertMatchEquals( expectedException.expect(IllegalStateException.class);
VectorMatch.allTrue(VECTOR_SIZE), expectedException.expectMessage("'other' must be a different instance from 'this'");
copy(VectorMatch.allTrue(VECTOR_SIZE)).addAll(VectorMatch.allTrue(VECTOR_SIZE), scratch) match.addAll(match, VectorMatch.wrap(new int[2]));
); }
assertMatchEquals( @Test
VectorMatch.allTrue(VECTOR_SIZE), public void testRemoveAllExhaustive()
createMatch(new int[]{}).addAll(VectorMatch.allTrue(VECTOR_SIZE), scratch) {
); // Tests every combination of vectors up to length VECTOR_SIZE.
assertMatchEquals( final int[] arrayOne = new int[VECTOR_SIZE];
createMatch(new int[]{3, 4, 5, 6, 7, 8, 9, 10}), final int[] arrayTwo = new int[VECTOR_SIZE];
createMatch(new int[]{3, 5, 6, 7, 8, 10}).addAll(createMatch(new int[]{4, 5, 9}), scratch) final int[] arrayExpected = new int[VECTOR_SIZE];
);
assertMatchEquals( for (int bitsOne = 0; bitsOne < VECTOR_BITS; bitsOne++) {
createMatch(new int[]{3, 4, 5, 6, 7, 8, 10}), for (int bitsTwo = 0; bitsTwo < VECTOR_BITS; bitsTwo++) {
createMatch(new int[]{3, 5, 6, 7, 8}).addAll(createMatch(new int[]{4, 5, 10}), scratch) final int lenOne = populate(arrayOne, bitsOne);
); final int lenTwo = populate(arrayTwo, bitsTwo);
final int lenExpected = populate(arrayExpected, bitsOne & ~bitsTwo);
assertMatchEquals( final VectorMatch matchOne = VectorMatch.wrap(arrayOne).setSelectionSize(lenOne);
createMatch(new int[]{2, 3, 5, 6, 7, 8, 9, 10}), final VectorMatch matchTwo = VectorMatch.wrap(arrayTwo).setSelectionSize(lenTwo);
createMatch(new int[]{3, 5, 6, 7, 8, 10}).addAll(createMatch(new int[]{2, 5, 9}), scratch) final VectorMatch matchExpected = VectorMatch.wrap(arrayExpected).setSelectionSize(lenExpected);
);
assertMatchEquals( assertMatchEquals(
createMatch(new int[]{3, 5, 6, 7, 8, 9, 10}), StringUtils.format("%s - %s", matchOne, matchTwo),
createMatch(new int[]{3, 5, 6, 7, 8, 10}).addAll(createMatch(new int[]{3, 5, 9}), scratch) matchExpected,
); matchOne.removeAll(matchTwo)
);
}
}
}
assertMatchEquals( @Test
createMatch(new int[]{3, 5, 6, 7, 8, 10}), public void testRemoveAllOnSelf()
createMatch(new int[]{3, 5, 6, 7, 8, 10}).addAll(createMatch(new int[]{3, 5, 10}), scratch) {
); 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.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.) * 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(); int len = 0;
final int[] newSelection = new int[selection.length];
System.arraycopy(selection, 0, newSelection, 0, selection.length);
return VectorMatch.wrap(newSelection).setSelectionSize(match.getSelectionSize());
}
private static VectorMatch createMatch(final int[] selection) for (int bit = 0; bit < VECTOR_SIZE; bit++) {
{ final int mask = (1 << bit);
final VectorMatch match = VectorMatch.wrap(new int[VECTOR_SIZE]); if ((bits & mask) == mask) {
System.arraycopy(selection, 0, match.getSelection(), 0, selection.length); array[len++] = bit;
match.setSelectionSize(selection.length); }
return match; }
return len;
} }
} }