davecromberge commented on a change in pull request #324:
URL:
https://github.com/apache/incubator-datasketches-java/pull/324#discussion_r455378653
##########
File path: src/main/java/org/apache/datasketches/theta/CompactOperations.java
##########
@@ -229,10 +233,20 @@ static final Memory loadCompactMemory(
}
final byte famID = (byte) Family.COMPACT.getID();
+ //Caution: The following loads directly into Memory without creating a
heap byte[] first,
+ // which would act as a pre-clearing, initialization mechanism. So it is
important to make sure
+ // that all fields are initialized, even those that are not used by the
CompactSketch.
+ // Otherwise, uninitialized fields could be filled with off-heap garbage,
which could cause
+ // other problems downstream if those fields are not filtered out first.
+ // As written below, all fields are initialized avoiding an extra copy.
+
+ //The first 8 bytes (pre0)
insertPreLongs(dstMem, preLongs); //RF not used = 0
insertSerVer(dstMem, SER_VER);
insertFamilyID(dstMem, famID);
- //ignore lgNomLongs, lgArrLongs bytes for compact sketches
+ //The following initializes the lgNomLongs and lgArrLongs to 0.
+ //They are not used in CompactSketches.
+ dstMem.putShort(LG_NOM_LONGS_BYTE, (short)0);
Review comment:
Does clearing LG_NOM_LONGS_BYTE to zero have the effect of clearing both
lgNomLongs and lgArrLongs? From the preamble output, it appeared that they
were two separate fields, and I didn't see an explicit byte cleared for
lgArrLongs.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
