leerho commented on code in PR #676:
URL: https://github.com/apache/datasketches-java/pull/676#discussion_r2229703826
##########
src/main/java/org/apache/datasketches/count/CountMinSketch.java:
##########
@@ -406,31 +406,31 @@ public byte[] toByteArray() {
final int flagsByte = isEmpty() ? Flag.IS_EMPTY.mask() : 0;
wseg.set(JAVA_BYTE, offset++, (byte) flagsByte);
final int NULL_32 = 0;
- wseg.set(JAVA_INT_UNALIGNED_BIG_ENDIAN, offset, NULL_32);
+ wseg.set(JAVA_INT_UNALIGNED, offset, NULL_32);
offset += 4;
Review Comment:
This (and following) is a use-case where the new
datasketches.common.positional.PositionalSegment could be used. You might want
to look at it. I used the term "positional" instead of "buffer", because
"buffer" is a widely used generic term and doesn't convey what is actually
going on. The PS would eliminate all of the offset tracking that you had to do
by hand (with magic numbers).
I had already migrated theta and tuple, but when I got to bloomfilter, I
decided there was a real need for it -- so I paused and created it. It is
already used in bloomfilter, tdigest, kll, and req. I intend to go back and
use it in the rest of the library where it makes sense. And here it makes
perfect sense, but this suggestion is optional.
Nonetheless, if you decide not to use PS, I would recommend not using magic
numbers for the offset adjustments and use the build-in static constants such
as Integer.BYTES, Long.BYTES, etc. The code will become more obvious as to what
you are doing and more robust. :-)
--
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: dev-unsubscr...@datasketches.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@datasketches.apache.org
For additional commands, e-mail: dev-h...@datasketches.apache.org
