kaivalnp commented on code in PR #16706:
URL: https://github.com/apache/lucene/pull/16706#discussion_r4124586419
##########
lucene/sandbox/src/test/org/apache/lucene/sandbox/codecs/dedup/TestDedupFlatVectorsFormat.java:
##########
@@ -211,8 +212,13 @@ public void testOffHeapSize() throws Exception {
DedupFlatVectorsReader dedupReader = getDedupReader(leafReader, "f");
FieldInfo fieldInfo = leafReader.getFieldInfos().fieldInfo("f");
+ // fieldOrdToGroupOrd is packed with the minimum bits required for the
largest group
+ // ordinal. There are 2 distinct vectors (group ords 0 and 1), so each
entry needs
+ // bitsRequired(1) bits rather than a full 32-bit int.
+ int bitsPerValue = DirectWriter.bitsRequired(1);
long expectedOffHeapSize =
- (docVectors.length * Integer.BYTES) // fieldOrdToGroupOrd mapping
+ DirectWriter.bytesRequired(
+ docVectors.length, bitsPerValue) // fieldOrdToGroupOrd
mapping
Review Comment:
Nice! Can you add a separate test with >2 unique vectors, just to enforce
that `bytesRequired` changes with `log2 maxGroupOrd`?
##########
lucene/sandbox/src/java/org/apache/lucene/sandbox/codecs/dedup/DedupUtil.java:
##########
@@ -120,9 +116,16 @@ static void writeFieldInfo(
ORD_TO_DOC_DIRECT_MONOTONIC_BLOCK_SHIFT, meta, vectorData,
vectorCount, maxDoc, docs);
// write fieldOrdToGroupOrd
+ //
+ // The group ordinals are typically far smaller than 2^32 (the whole point
of de-duplication is
+ // that distinct vectors are few relative to documents), so we pack each
ordinal using the
+ // minimum number of bits that can represent the largest group ordinal
referenced by this field
+ // (tracked by the caller as vectors are added). The chosen width is
persisted to the metadata
+ // so the reader can decode without assuming a fixed width.
Review Comment:
This comment is a nice explanation of why this change was made, but feels
overly verbose to persist in code.
Can we shorten it to something like "Use minimum bits per entry of
`fieldOrdToGroupOrd` mapping"?
##########
lucene/sandbox/src/java/org/apache/lucene/sandbox/codecs/dedup/DedupFlatVectorsFormat.java:
##########
@@ -81,6 +81,7 @@
*
org.apache.lucene.codecs.lucene95.OrdToDocDISIReaderConfiguration#writeStoredMeta}
* <li><b>[int64]</b> offset to this field's {@code fieldOrdToGroupOrd} map
in the .vdd file
* <li><b>[int64]</b> length of this field's {@code fieldOrdToGroupOrd} map,
in bytes
+ * <li><b>[int32]</b> bits per value used to pack this field's {@code
fieldOrdToGroupOrd} map
Review Comment:
Note: changing the on-disk format is okay to do now because the format
hasn't been "released", and would've been trickier later.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
