Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

LUCENE-8868: New storing strategy for BKD tree leaves with low cardinality #730

Merged
merged 21 commits into from
Jun 26, 2019
Merged

LUCENE-8868: New storing strategy for BKD tree leaves with low cardinality #730

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
47df1d4
Add new storage strategy when cardinality if points is low on a BKD tree
iverase Jun 19, 2019
1f8738e
Compute correctly when to perform the optimisation
iverase Jun 19, 2019
69f4d4e
Change computation of low cardinality to consider the size of cardina…
iverase Jun 20, 2019
033564b
Fail if commpressed dim is -2 and we are on an earlier version
iverase Jun 20, 2019
15e8958
fix the formula to decide to use the new storage strategy. It takes
iverase Jun 21, 2019
f39370c
compute exactlay the cost of runLen compression
iverase Jun 23, 2019
80ee25d
Add TODO
iverase Jun 23, 2019
f904dfb
compute cardinality after sorting leaf and avoid copying of arrays
iverase Jun 23, 2019
918d399
clean up
iverase Jun 23, 2019
2f39af3
clean up
iverase Jun 24, 2019
6eaf5a1
leverage the commonPrefixLength info for mismatch
iverase Jun 24, 2019
13f1f51
clean up
iverase Jun 24, 2019
b9c271a
Merge branch 'master' into storageLowCardinality
iverase Jun 24, 2019
c8103c7
Address review comments, mostly formatting
iverase Jun 24, 2019
5054ff4
formatting
iverase Jun 24, 2019
885ea93
formatting
iverase Jun 24, 2019
a1471fe
formatting
iverase Jun 24, 2019
9f453d7
Add test that triggers low cardinality leaves
iverase Jun 25, 2019
e6aa3e6
Merge branch 'master' into storageLowCardinality
iverase Jun 26, 2019
30f36ce
formatting
iverase Jun 26, 2019
cc08c5f
Add entry in Changes.txt
iverase Jun 26, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
82 changes: 77 additions & 5 deletions lucene/core/src/java/org/apache/lucene/util/bkd/BKDReader.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -441,8 +441,15 @@ int readDocIDs(IndexInput in, long blockFP, int[] docIDs) throws IOException {

void visitDocValues(int[] commonPrefixLengths, byte[] scratchDataPackedValue, byte[] scratchMinIndexPackedValue, byte[] scratchMaxIndexPackedValue,
IndexInput in, int[] docIDs, int count, IntersectVisitor visitor) throws IOException {
if (version >= BKDWriter.VERSION_LOW_CARDINALITY_LEAVES) {
visitDocValuesWithCardinality(commonPrefixLengths, scratchDataPackedValue, scratchMinIndexPackedValue, scratchMaxIndexPackedValue, in, docIDs, count, visitor);
} else {
visitDocValuesNoCardinality(commonPrefixLengths, scratchDataPackedValue, scratchMinIndexPackedValue, scratchMaxIndexPackedValue, in, docIDs, count, visitor);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could we always call visitDocValuesWithCardinality? It seems to include the version check already?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually we cannot because in the previous version of the BKD tree the compressed dim byte was written after the leaf bounds. This is the reason I have to fork the version here.

}


void visitDocValuesNoCardinality(int[] commonPrefixLengths, byte[] scratchDataPackedValue, byte[] scratchMinIndexPackedValue, byte[] scratchMaxIndexPackedValue,
IndexInput in, int[] docIDs, int count, IntersectVisitor visitor) throws IOException {
readCommonPrefixes(commonPrefixLengths, scratchDataPackedValue, in);

if (numIndexDims != 1 && version >= BKDWriter.VERSION_LEAF_STORES_BOUNDS) {
Expand Down Expand Up @@ -480,12 +487,62 @@ void visitDocValues(int[] commonPrefixLengths, byte[] scratchDataPackedValue, by
int compressedDim = readCompressedDim(in);

if (compressedDim == -1) {
visitRawDocValues(commonPrefixLengths, scratchDataPackedValue, in, docIDs, count, visitor);
visitUniqueRawDocValues(scratchDataPackedValue, docIDs, count, visitor);
} else {
visitCompressedDocValues(commonPrefixLengths, scratchDataPackedValue, in, docIDs, count, visitor, compressedDim);
}
}

void visitDocValuesWithCardinality(int[] commonPrefixLengths, byte[] scratchDataPackedValue, byte[] scratchMinIndexPackedValue, byte[] scratchMaxIndexPackedValue,
IndexInput in, int[] docIDs, int count, IntersectVisitor visitor) throws IOException {

readCommonPrefixes(commonPrefixLengths, scratchDataPackedValue, in);
int compressedDim = readCompressedDim(in);
if (compressedDim == -1) {
//all values are the same
visitor.grow(count);
visitUniqueRawDocValues(scratchDataPackedValue, docIDs, count, visitor);
} else {
if (numIndexDims != 1 && version >= BKDWriter.VERSION_LEAF_STORES_BOUNDS) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the version check shouldn't be necessary since this method is only called when version gte VERSION_LOW_CARDINALITY_LEAVES?

byte[] minPackedValue = scratchMinIndexPackedValue;
System.arraycopy(scratchDataPackedValue, 0, minPackedValue, 0, packedIndexBytesLength);
byte[] maxPackedValue = scratchMaxIndexPackedValue;
//Copy common prefixes before reading adjusted
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you leave a space between the slashes and the text like we usually do in the rest of the codebase?

// box
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

move end of comment to the previous line?

System.arraycopy(minPackedValue, 0, maxPackedValue, 0, packedIndexBytesLength);
readMinMax(commonPrefixLengths, minPackedValue, maxPackedValue, in);

// The index gives us range of values for each dimension, but the actual range of values
// might be much more narrow than what the index told us, so we double check the relation
// here, which is cheap yet might help figure out that the block either entirely matches
// or does not match at all. This is especially more likely in the case that there are
// multiple dimensions that have correlation, ie. splitting on one dimension also
// significantly changes the range of values in another dimension.
Relation r = visitor.compare(minPackedValue, maxPackedValue);
if (r == Relation.CELL_OUTSIDE_QUERY) {
return;
}
visitor.grow(count);

if (r == Relation.CELL_INSIDE_QUERY) {
for (int i = 0; i < count; ++i) {
visitor.visit(docIDs[i]);
}
return;
}
} else {
visitor.grow(count);
}
if (compressedDim == -2) {
//low cardinality values
visitSparseRawDocValues(commonPrefixLengths, scratchDataPackedValue, in, docIDs, count, visitor);
} else {
//high cardinality
visitCompressedDocValues(commonPrefixLengths, scratchDataPackedValue, in, docIDs, count, visitor, compressedDim);
}
}
}

private void readMinMax(int[] commonPrefixLengths, byte[] minPackedValue, byte[] maxPackedValue, IndexInput in) throws IOException {
for (int dim = 0; dim < numIndexDims; dim++) {
int prefix = commonPrefixLengths[dim];
Expand All @@ -495,12 +552,27 @@ private void readMinMax(int[] commonPrefixLengths, byte[] minPackedValue, byte[]
}

// Just read suffixes for every dimension
private void visitRawDocValues(int[] commonPrefixLengths, byte[] scratchPackedValue, IndexInput in, int[] docIDs, int count, IntersectVisitor visitor) throws IOException {
for (int i = 0; i < count; ++i) {
private void visitSparseRawDocValues(int[] commonPrefixLengths, byte[] scratchPackedValue, IndexInput in, int[] docIDs, int count, IntersectVisitor visitor) throws IOException {
int i;
for (i = 0; i < count;) {
int length = in.readVInt();
for(int dim=0;dim<numDataDims;dim++) {
int prefix = commonPrefixLengths[dim];
in.readBytes(scratchPackedValue, dim*bytesPerDim + prefix, bytesPerDim - prefix);
}
for (int j = i; j < i + length; j++) {
visitor.visit(docIDs[j], scratchPackedValue);
}
i+= length;
}
if (i != count) {
throw new CorruptIndexException("Sub blocks do not add up to the expected count: " + count + " != " + i, in);
}
}

// Just read suffixes for every dimension
private void visitUniqueRawDocValues(byte[] scratchPackedValue, int[] docIDs, int count, IntersectVisitor visitor) throws IOException {
for (int i = 0; i < count; i++) {
visitor.visit(docIDs[i], scratchPackedValue);
}
}
Expand Down Expand Up @@ -530,7 +602,7 @@ private void visitCompressedDocValues(int[] commonPrefixLengths, byte[] scratchP

private int readCompressedDim(IndexInput in) throws IOException {
int compressedDim = in.readByte();
if (compressedDim < -1 || compressedDim >= numDataDims) {
if (compressedDim < -2 || compressedDim >= numDataDims) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe fail if compressedDim is -2 and version is not gte VERSION_LOW_CARDINALITY_LEAVES?

throw new CorruptIndexException("Got compressedDim="+compressedDim, in);
}
return compressedDim;
Expand Down
72 changes: 59 additions & 13 deletions lucene/core/src/java/org/apache/lucene/util/bkd/BKDWriter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,8 @@ public class BKDWriter implements Closeable {
//public static final int VERSION_CURRENT = VERSION_START;
public static final int VERSION_LEAF_STORES_BOUNDS = 5;
public static final int VERSION_SELECTIVE_INDEXING = 6;
public static final int VERSION_CURRENT = VERSION_SELECTIVE_INDEXING;
public static final int VERSION_LOW_CARDINALITY_LEAVES= 7;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a space before the equal sign? There are a couple other places where spaces are missing in this PR.

public static final int VERSION_CURRENT = VERSION_LOW_CARDINALITY_LEAVES;

/** How many bytes each docs takes in the fixed-width offline format */
private final int bytesPerDoc;
Expand Down Expand Up @@ -516,6 +517,7 @@ private class OneDimensionBKDWriter {
final int[] leafDocs = new int[maxPointsInLeafNode];
private long valueCount;
private int leafCount;
private int leafCardinality;

OneDimensionBKDWriter(IndexOutput out) {
if (numIndexDims != 1) {
Expand Down Expand Up @@ -546,6 +548,9 @@ void add(byte[] packedValue, int docID) throws IOException {
assert valueInOrder(valueCount + leafCount,
0, lastPackedValue, packedValue, 0, docID, lastDocID);

if (leafCount == 0 || Arrays.mismatch(leafValues, (leafCount - 1) * bytesPerDim, leafCount * bytesPerDim, packedValue, 0, bytesPerDim) != -1) {
leafCardinality++;
}
System.arraycopy(packedValue, 0, leafValues, leafCount * packedBytesLength, packedBytesLength);
leafDocs[leafCount] = docID;
docsSeen.set(docID);
Expand All @@ -558,7 +563,8 @@ assert valueInOrder(valueCount + leafCount,
if (leafCount == maxPointsInLeafNode) {
// We write a block once we hit exactly the max count ... this is different from
// when we write N > 1 dimensional points where we write between max/2 and max per leaf block
writeLeafBlock();
writeLeafBlock(leafCardinality);
leafCardinality = 0;
leafCount = 0;
}

Expand All @@ -567,7 +573,8 @@ assert valueInOrder(valueCount + leafCount,

public long finish() throws IOException {
if (leafCount > 0) {
writeLeafBlock();
writeLeafBlock(leafCardinality);
leafCardinality = 0;
leafCount = 0;
}

Expand All @@ -593,7 +600,7 @@ public long finish() throws IOException {
return indexFP;
}

private void writeLeafBlock() throws IOException {
private void writeLeafBlock(int leafCardinality) throws IOException {
assert leafCount != 0;
if (valueCount == 0) {
System.arraycopy(leafValues, 0, minPackedValue, 0, packedIndexBytesLength);
Expand All @@ -613,7 +620,7 @@ private void writeLeafBlock() throws IOException {
int offset = (leafCount - 1) * packedBytesLength;
int prefix = Arrays.mismatch(leafValues, 0, bytesPerDim, leafValues, offset, offset + bytesPerDim);
if (prefix == -1) {
prefix = bytesPerDim;
prefix = bytesPerDim;
}

commonPrefixLengths[0] = prefix;
Expand All @@ -635,7 +642,7 @@ public BytesRef apply(int i) {
assert valuesInOrderAndBounds(leafCount, 0, ArrayUtil.copyOfSubArray(leafValues, 0, packedBytesLength),
ArrayUtil.copyOfSubArray(leafValues, (leafCount - 1) * packedBytesLength, leafCount * packedBytesLength),
packedValues, leafDocs, 0);
writeLeafBlockPackedValues(scratchOut, commonPrefixLengths, leafCount, 0, packedValues);
writeLeafBlockPackedValues(scratchOut, commonPrefixLengths, leafCount, 0, packedValues, leafCardinality);
scratchOut.copyTo(out);
scratchOut.reset();
}
Expand Down Expand Up @@ -1028,17 +1035,43 @@ private void writeLeafBlockDocs(DataOutput out, int[] docIDs, int start, int cou
DocIdsWriter.writeDocIds(docIDs, start, count, out);
}

private void writeLeafBlockPackedValues(DataOutput out, int[] commonPrefixLengths, int count, int sortedDim, IntFunction<BytesRef> packedValues) throws IOException {
private void writeLeafBlockPackedValues(DataOutput out, int[] commonPrefixLengths, int count, int sortedDim, IntFunction<BytesRef> packedValues, int leafCardinality) throws IOException {
int prefixLenSum = Arrays.stream(commonPrefixLengths).sum();
if (prefixLenSum == packedBytesLength) {
// all values in this block are equal
out.writeByte((byte) -1);
} else {
} else if (leafCardinality * (packedBytesLength - prefixLenSum + 2) <= count * (packedBytesLength - prefixLenSum)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Am I reading it right that you are counting 2 for the vint? I think you could make it 1 instead, the reasoning being that if you vints are 2 bytes on average, then it means than your runs are very long (vint start using 2 bytes when they are greater than 127) and so the sparse encoding is an obvious win.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I was too conservative .

//estimate if storing the values with cardinality is cheaper than storing all values
out.writeByte((byte) -2);
if (numIndexDims != 1) {
writeActualBounds(out, commonPrefixLengths, count, packedValues);
}
BytesRef value = packedValues.apply(0);
System.arraycopy(value.bytes, value.offset, scratch1, 0, packedBytesLength);
int cardinality = 1;
for (int i = 1; i < count; i++) {
value = packedValues.apply(i);
if (Arrays.mismatch(value.bytes, value.offset, value.offset + value.length, scratch1, 0, packedBytesLength) != -1) {
out.writeVInt(cardinality);
for(int j = 0; j < numDataDims; j++) {
out.writeBytes(scratch1, j * bytesPerDim + commonPrefixLengths[j], bytesPerDim - commonPrefixLengths[j]);
}
System.arraycopy(value.bytes, value.offset, scratch1, 0, packedBytesLength);
cardinality = 1;
} else {
cardinality++;
}
}
out.writeVInt(cardinality);
for(int i = 0; i < numDataDims; i++) {
out.writeBytes(scratch1, i * bytesPerDim + commonPrefixLengths[i], bytesPerDim - commonPrefixLengths[i]);
}
} else {
assert commonPrefixLengths[sortedDim] < bytesPerDim;
out.writeByte((byte) sortedDim);
if (numIndexDims != 1) {
writeActualBounds(out, commonPrefixLengths, count, packedValues);
}
int compressedByteOffset = sortedDim * bytesPerDim + commonPrefixLengths[sortedDim];
commonPrefixLengths[sortedDim]++;
for (int i = 0; i < count; ) {
Expand Down Expand Up @@ -1246,11 +1279,17 @@ private void build(int nodeID, int leafNodeOffset,
final int count = to - from;
assert count <= maxPointsInLeafNode;

// Compute common prefixes
// Compute common prefixes and cardinality
Arrays.fill(commonPrefixLengths, bytesPerDim);
reader.getValue(from, scratchBytesRef1);
int leafCardinality = 1;
System.arraycopy(scratchBytesRef1.bytes, scratchBytesRef1.offset, scratch1, 0, packedBytesLength);
for (int i = from + 1; i < to; ++i) {
reader.getValue(i, scratchBytesRef2);
if (Arrays.mismatch(scratch1, 0, packedBytesLength, scratchBytesRef2.bytes, scratchBytesRef2.offset, scratchBytesRef2.offset + packedBytesLength) != -1) {
leafCardinality++;
System.arraycopy(scratchBytesRef2.bytes, scratchBytesRef2.offset, scratch1, 0, packedBytesLength);
}
for (int dim=0;dim<numDataDims;dim++) {
final int offset = dim * bytesPerDim;
int dimensionPrefixLength = commonPrefixLengths[dim];
Expand Down Expand Up @@ -1323,7 +1362,7 @@ public BytesRef apply(int i) {
};
assert valuesInOrderAndBounds(count, sortedDim, minPackedValue, maxPackedValue, packedValues,
docIDs, 0);
writeLeafBlockPackedValues(scratchOut, commonPrefixLengths, count, sortedDim, packedValues);
writeLeafBlockPackedValues(scratchOut, commonPrefixLengths, count, sortedDim, packedValues, leafCardinality);
scratchOut.copyTo(out);
scratchOut.reset();
} else {
Expand Down Expand Up @@ -1395,7 +1434,7 @@ private void build(int nodeID, int leafNodeOffset,
int from = Math.toIntExact(points.start);
int to = Math.toIntExact(points.start + points.count);
//we store common prefix on scratch1
computeCommonPrefixLength(heapSource, scratch1, from, to);
int leafCardinality = computeCommonPrefixLength(heapSource, scratch1, from, to);

int sortedDim = 0;
int sortedDimCardinality = Integer.MAX_VALUE;
Expand Down Expand Up @@ -1459,7 +1498,7 @@ public BytesRef apply(int i) {
};
assert valuesInOrderAndBounds(count, sortedDim, minPackedValue, maxPackedValue, packedValues,
heapSource.docIDs, from);
writeLeafBlockPackedValues(out, commonPrefixLengths, count, sortedDim, packedValues);
writeLeafBlockPackedValues(out, commonPrefixLengths, count, sortedDim, packedValues, leafCardinality);

} else {
// Inner node: partition/recurse
Expand Down Expand Up @@ -1516,16 +1555,22 @@ assert valuesInOrderAndBounds(count, sortedDim, minPackedValue, maxPackedValue,
}
}

private void computeCommonPrefixLength(HeapPointWriter heapPointWriter, byte[] commonPrefix, int from, int to) {
private int computeCommonPrefixLength(HeapPointWriter heapPointWriter, byte[] commonPrefix, int from, int to) {
Arrays.fill(commonPrefixLengths, bytesPerDim);
PointValue value = heapPointWriter.getPackedValueSlice(from);
BytesRef packedValue = value.packedValue();
for (int dim = 0; dim < numDataDims; dim++) {
System.arraycopy(packedValue.bytes, packedValue.offset + dim * bytesPerDim, commonPrefix, dim * bytesPerDim, bytesPerDim);
}
System.arraycopy(packedValue.bytes, packedValue.offset, scratch2, 0, packedBytesLength);
int leafCardinality = 1;
for (int i = from + 1; i < to; i++) {
value = heapPointWriter.getPackedValueSlice(i);
packedValue = value.packedValue();
if (Arrays.mismatch(scratch2, 0, packedBytesLength, packedValue.bytes, packedValue.offset, packedValue.offset + packedBytesLength) != -1) {
leafCardinality++;
System.arraycopy(packedValue.bytes, packedValue.offset, scratch2, 0, packedBytesLength);
}
for (int dim = 0; dim < numDataDims; dim++) {
if (commonPrefixLengths[dim] != 0) {
int j = Arrays.mismatch(commonPrefix, dim * bytesPerDim, dim * bytesPerDim + commonPrefixLengths[dim], packedValue.bytes, packedValue.offset + dim * bytesPerDim, packedValue.offset + dim * bytesPerDim + commonPrefixLengths[dim]);
Expand All @@ -1535,6 +1580,7 @@ private void computeCommonPrefixLength(HeapPointWriter heapPointWriter, byte[] c
}
}
}
return leafCardinality;
}

// only called from assert
Expand Down