You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@datasketches.apache.org by "leerho (via GitHub)" <gi...@apache.org> on 2025/07/07 22:03:25 UTC
[PR] Ffm phase3 (datasketches-java)
leerho opened a new pull request, #670:
URL: https://github.com/apache/datasketches-java/pull/670
(no comment)
--
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: commits-unsubscribe@datasketches.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
For additional commands, e-mail: commits-help@datasketches.apache.org
Re: [PR] FFM phase3 (datasketches-java)
Posted by "leerho (via GitHub)" <gi...@apache.org>.
leerho commented on code in PR #670:
URL: https://github.com/apache/datasketches-java/pull/670#discussion_r2199268293
##########
src/main/java/org/apache/datasketches/filters/bloomfilter/DirectBitArray.java:
##########
@@ -50,19 +57,19 @@ static DirectBitArray initialize(final long numBits, final WritableMemory wmem)
final int arrayLength = (int) Math.ceil(numBits / 64.0); // we know it'll fit in an int based on above checks
final long requiredBytes = (2L + arrayLength) * Long.BYTES;
- if (wmem.getCapacity() < requiredBytes) {
- throw new SketchesArgumentException("Provided WritableMemory too small for requested array length. "
- + "Requited: " + requiredBytes + ", provided capcity: " + wmem.getCapacity());
+ if (wseg.byteSize() < requiredBytes) {
+ throw new SketchesArgumentException("Provided MemorySegment too small for requested array length. "
+ + "Requited: " + requiredBytes + ", provided capcity: " + wseg.byteSize());
Review Comment:
Thank you for your excellent summary!
--
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: commits-unsubscribe@datasketches.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
For additional commands, e-mail: commits-help@datasketches.apache.org
Re: [PR] FFM phase3 (datasketches-java)
Posted by "leerho (via GitHub)" <gi...@apache.org>.
leerho commented on code in PR #670:
URL: https://github.com/apache/datasketches-java/pull/670#discussion_r2198904045
##########
src/main/java/org/apache/datasketches/filters/bloomfilter/BloomFilter.java:
##########
@@ -779,21 +779,24 @@ public byte[] toByteArray() {
final byte[] bytes = new byte[(int) sizeBytes];
- if (wmem_ == null) {
- final WritableBuffer wbuf = WritableMemory.writableWrap(bytes).asWritableBuffer();
+ if (wseg_ == null) {
+ final MemorySegment seg = MemorySegment.ofArray(bytes);
+ final PositionalSegment posSeg = PositionalSegment.wrap(seg);
final int numPreLongs = isEmpty() ? Family.BLOOMFILTER.getMinPreLongs() : Family.BLOOMFILTER.getMaxPreLongs();
- wbuf.putByte((byte) numPreLongs);
- wbuf.putByte((byte) SER_VER);
- wbuf.putByte((byte) Family.BLOOMFILTER.getID());
- wbuf.putByte((byte) (bitArray_.isEmpty() ? EMPTY_FLAG_MASK : 0));
- wbuf.putShort(numHashes_);
- wbuf.putShort((short) 0); // unused
- wbuf.putLong(seed_);
-
- ((HeapBitArray) bitArray_).writeToBuffer(wbuf);
+ posSeg.setByte((byte) numPreLongs);
+ posSeg.setByte((byte) SER_VER);
+ posSeg.setByte((byte) Family.BLOOMFILTER.getID());
+ posSeg.setByte((byte) (bitArray_.isEmpty() ? EMPTY_FLAG_MASK : 0));
+ posSeg.setShort(numHashes_);
+ posSeg.setShort((short) 0); // unused
+ posSeg.setLong(seed_);
+
+ ((HeapBitArray) bitArray_).writeToSegmentAsStream(posSeg); //option: posSeg.asSlice()
+ System.out.println("HERE");
Review Comment:
I will.
--
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: commits-unsubscribe@datasketches.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
For additional commands, e-mail: commits-help@datasketches.apache.org
Re: [PR] FFM phase3 (datasketches-java)
Posted by "leerho (via GitHub)" <gi...@apache.org>.
leerho merged PR #670:
URL: https://github.com/apache/datasketches-java/pull/670
--
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: commits-unsubscribe@datasketches.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
For additional commands, e-mail: commits-help@datasketches.apache.org
Re: [PR] FFM phase3 (datasketches-java)
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #670:
URL: https://github.com/apache/datasketches-java/pull/670#discussion_r2198585082
##########
src/main/java/org/apache/datasketches/filters/bloomfilter/DirectBitArray.java:
##########
@@ -50,19 +57,19 @@ static DirectBitArray initialize(final long numBits, final WritableMemory wmem)
final int arrayLength = (int) Math.ceil(numBits / 64.0); // we know it'll fit in an int based on above checks
final long requiredBytes = (2L + arrayLength) * Long.BYTES;
- if (wmem.getCapacity() < requiredBytes) {
- throw new SketchesArgumentException("Provided WritableMemory too small for requested array length. "
- + "Requited: " + requiredBytes + ", provided capcity: " + wmem.getCapacity());
+ if (wseg.byteSize() < requiredBytes) {
+ throw new SketchesArgumentException("Provided MemorySegment too small for requested array length. "
+ + "Requited: " + requiredBytes + ", provided capcity: " + wseg.byteSize());
Review Comment:
Fix the typos in the exception message: change "Requited" to "Required" and "capcity" to "capacity".
```suggestion
+ "Required: " + requiredBytes + ", provided capacity: " + wseg.byteSize());
```
--
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: commits-unsubscribe@datasketches.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
For additional commands, e-mail: commits-help@datasketches.apache.org
Re: [PR] FFM phase3 (datasketches-java)
Posted by "freakyzoidberg (via GitHub)" <gi...@apache.org>.
freakyzoidberg commented on PR #670:
URL: https://github.com/apache/datasketches-java/pull/670#issuecomment-3058815211
Aside from the stray debug LGTM.
As discussed on Slack - their may be some win to get performance or safety wise following those practice
but could definitely be evaluated later
Thanks !
``` Resource Management
// Recommended pattern for Arena usage
try (Arena arena = Arena.ofConfined()) {
MemorySegment segment = arena.allocate(size);
// Use segment
} // Automatic cleanup
Error Handling Standardization
// Standardized validation pattern
private static void validateSegmentBounds(MemorySegment seg, long offset, long length) {
if (offset < 0 || length < 0 || offset + length > seg.byteSize()) {
throw new SketchesArgumentException("Segment access out of bounds");
}
}
```
--
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: commits-unsubscribe@datasketches.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
For additional commands, e-mail: commits-help@datasketches.apache.org
Re: [PR] FFM phase3 (datasketches-java)
Posted by "freakyzoidberg (via GitHub)" <gi...@apache.org>.
freakyzoidberg commented on code in PR #670:
URL: https://github.com/apache/datasketches-java/pull/670#discussion_r2198568626
##########
src/main/java/org/apache/datasketches/filters/bloomfilter/BloomFilter.java:
##########
@@ -779,21 +779,24 @@ public byte[] toByteArray() {
final byte[] bytes = new byte[(int) sizeBytes];
- if (wmem_ == null) {
- final WritableBuffer wbuf = WritableMemory.writableWrap(bytes).asWritableBuffer();
+ if (wseg_ == null) {
+ final MemorySegment seg = MemorySegment.ofArray(bytes);
+ final PositionalSegment posSeg = PositionalSegment.wrap(seg);
final int numPreLongs = isEmpty() ? Family.BLOOMFILTER.getMinPreLongs() : Family.BLOOMFILTER.getMaxPreLongs();
- wbuf.putByte((byte) numPreLongs);
- wbuf.putByte((byte) SER_VER);
- wbuf.putByte((byte) Family.BLOOMFILTER.getID());
- wbuf.putByte((byte) (bitArray_.isEmpty() ? EMPTY_FLAG_MASK : 0));
- wbuf.putShort(numHashes_);
- wbuf.putShort((short) 0); // unused
- wbuf.putLong(seed_);
-
- ((HeapBitArray) bitArray_).writeToBuffer(wbuf);
+ posSeg.setByte((byte) numPreLongs);
+ posSeg.setByte((byte) SER_VER);
+ posSeg.setByte((byte) Family.BLOOMFILTER.getID());
+ posSeg.setByte((byte) (bitArray_.isEmpty() ? EMPTY_FLAG_MASK : 0));
+ posSeg.setShort(numHashes_);
+ posSeg.setShort((short) 0); // unused
+ posSeg.setLong(seed_);
+
+ ((HeapBitArray) bitArray_).writeToSegmentAsStream(posSeg); //option: posSeg.asSlice()
+ System.out.println("HERE");
Review Comment:
Should remove this
--
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: commits-unsubscribe@datasketches.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
For additional commands, e-mail: commits-help@datasketches.apache.org
Re: [PR] FFM phase3 (datasketches-java)
Posted by "leerho (via GitHub)" <gi...@apache.org>.
leerho commented on PR #670:
URL: https://github.com/apache/datasketches-java/pull/670#issuecomment-3059465842
@freakyzoidberg was kind enough to submit this PR to Claude by Anthropic IA. Here is the detailed output with my responses. A few good catches, and a lot warnings that were out of context or not applicable.
FROM Claude by Anthropic:
CRITICAL
1. SHOW-STOPPER: Debug Code in Production
Location: BloomFilter.java:796, System.out.println("HERE");
Response: This was an "oops" on my part! Will fix.
2. SHOW-STOPPER: Inverted Validation Logic
Location: BloomFilter.java:168-171
checkArgument((preLongs < Family.BLOOMFILTER.getMinPreLongs()) || (preLongs > Family.BLOOMFILTER.getMaxPreLongs()),
"Possible corruption: Incorrect number of preamble bytes specified in header");
Impact: Valid operations will fail with incorrect error messages
Action: Fix logical conditions - these should throw when conditions are false, not true
Response: The AI tool was reading too much into a comment. The comment was inverted, but the code is correct.
I corrected the comment.
3. API Compatibility Assessment: COMPLETELY BROKEN
- Java 8 → Java 21+ requirement with --enable-preview flag
- All Memory/WritableMemory APIs → MemorySegment APIs
- 100% binary incompatibility - requires complete recompilation
- Source compatibility broken - requires code changes
- All downstream projects (Pig, Hive adapters) need major updates
Recommendation: This must be version 8.x, not a minor/patch release
Response: Yes, of course. Completely broken because this PR is only phase 3 out of many phases. I hadn't gotten around to updating the POM version yet. I will change it to 9.0.0-SNAPSHOT
HIGH SEVERITY
1. Missing bounds validation in MemorySegment operations.
Response: Unnecessary. MemorySegment is completely self contained and safe with its own bounds checking.
The only exception is if one uses any of MemorySegment's restricted methods -- which we do not.
2. Resource leaks - no Arena scope management throughout codebase.
Response: Arena scope management is not necessary.
a. All MemorySegments created internal to the codebase are on-heap, where Arena does not apply.
b. Off-Heap MemorySegments passed into the codebase by the user must be managed by the user's Arenas.
There is no way a library like this should attempt to manage user's Arenas, as that would be dangerous.
3. Integer overflow risks
Response: Yes this is true, although the Bloom Filter would have to be extreemly large for this to occur. Nonetheless, we need to do more checks on the builder constructors so that this cannot happen. Jon & I will work on this.
4. Unaligned memory access without comprehensive bounds checking.
Response: Unnecessary. MemorySegment is completely self contained with its own bounds checking.
PERFORMANCE REGRESSIONS
1. Excessive allocations in BloomFilter double/string updates (20-30% performance loss)
**Response**: double/string updates is a customer requirement.
2. Inefficient bit counting - O(n) scan in DirectBitArray after writes
_Response_: This is referring to "intersect", "union" and "invert", which are, by definition, expensive operations.
3. Suboptimal memory access patterns in hash operations
**Response**: Not sure what this is referring to.
4. Memory fragmentation risks in large object serialization
**Response**: The only serializations are of primitive arrays, which are not fragmented by the JVM. Object arrays, however, can be.
MEDIUM SEVERITY ISSUES
1. Inconsistent FFM migration - mixed Memory/MemorySegment imports across 561 files
**Response**: Yep, this migration is being done in phases, because to review the entire library migration all in one chunk is impractical.
2. Inconsistent naming conventions - seg, wseg, srcSeg, dstSeg used inconsistently
**Response**: Because they have different meanings, based on context.
3. Complex inheritance hierarchies becoming unwieldy
**Response**: Nuts. Only 3 classes have any inheritance and it is only with 2 levels of inheritance.
4. Missing documentation for FFM-specific considerations
**Response**: Perhaps. This will be improved as we go along.
ERROR HANDLING
1. Inconsistent exception types - mix of SketchesArgumentException, IllegalArgumentException
**Response**: Yep, and largely historical, Will try to fix as we go along.
2. Missing edge case validation for negative indices, empty segments
**Response**: Indices into MemorySegments will be caught by the MemorySegments.
Index access errors for Java arrays are caught by Java.
So I'm not sure what is being referred to here.
3. Poor error messages without sufficient context
**Response**: This is a work in progress.
--
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: commits-unsubscribe@datasketches.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
For additional commands, e-mail: commits-help@datasketches.apache.org