Copilot commented on code in PR #675:
URL: https://github.com/apache/datasketches-java/pull/675#discussion_r2243669278
##########
src/test/java/org/apache/datasketches/quantiles/ReadOnlyMemoryTest.java:
##########
@@ -184,92 +185,92 @@ public void wrapEmptyUpdateSketch() {
@Test
public void wrapEmptyCompactSketch() {
final UpdateDoublesSketch s1 = DoublesSketch.builder().build();
- final Memory mem = Memory.wrap(s1.compact().toByteArray());
- DoublesSketch s2 = DoublesSketch.wrap(mem); // compact, so this is ok
+ final MemorySegment seg =
MemorySegment.ofArray(s1.compact().toByteArray());
+ final DoublesSketch s2 = DoublesSketch.wrap(seg); // compact, so this is ok
Assert.assertTrue(s2.isEmpty());
}
@Test
public void heapifyUnionFromSparse() {
- UpdateDoublesSketch s1 = DoublesSketch.builder().build();
+ final UpdateDoublesSketch s1 = DoublesSketch.builder().build();
s1.update(1);
s1.update(2);
- Memory mem = Memory.wrap(s1.toByteArray(false));
- DoublesUnion u = DoublesUnion.heapify(mem);
+ final MemorySegment seg = MemorySegment.ofArray(s1.toByteArray(false));
+ final DoublesUnion u = DoublesUnion.heapify(seg);
u.update(3);
- DoublesSketch s2 = u.getResult();
+ final DoublesSketch s2 = u.getResult();
Assert.assertEquals(s2.getMinItem(), 1.0);
Assert.assertEquals(s2.getMaxItem(), 3.0);
}
@Test
public void heapifyUnionFromCompact() {
- UpdateDoublesSketch s1 = DoublesSketch.builder().build();
+ final UpdateDoublesSketch s1 = DoublesSketch.builder().build();
s1.update(1);
s1.update(2);
- Memory mem = Memory.wrap(s1.toByteArray(true));
- DoublesUnion u = DoublesUnion.heapify(mem);
+ final MemorySegment seg = MemorySegment.ofArray(s1.toByteArray(true));
+ final DoublesUnion u = DoublesUnion.heapify(seg);
u.update(3);
- DoublesSketch s2 = u.getResult();
+ final DoublesSketch s2 = u.getResult();
Assert.assertEquals(s2.getMinItem(), 1.0);
Assert.assertEquals(s2.getMaxItem(), 3.0);
}
@Test
public void wrapUnionFromSparse() {
- UpdateDoublesSketch s1 = DoublesSketch.builder().build();
+ final UpdateDoublesSketch s1 = DoublesSketch.builder().build();
s1.update(1);
s1.update(2);
- Memory mem = Memory.wrap(s1.toByteArray(false));
- DoublesUnion u = DoublesUnion.wrap(mem);
- DoublesSketch s2 = u.getResult();
+ final MemorySegment seg =
MemorySegment.ofArray(s1.toByteArray(false)).asReadOnly();
+ final DoublesUnion u = DoublesUnion.wrap(seg);
Review Comment:
The MemorySegment should be made read-only using `.asReadOnly()` to properly
test read-only behavior, as the original test was checking read-only buffer
behavior.
##########
src/test/java/org/apache/datasketches/quantiles/DoublesSketchTest.java:
##########
@@ -115,66 +114,68 @@ static void testSketchEquality(final DoublesSketch
sketch1,
@Test
public void checkIsSameResource() {
- int k = 16;
- WritableMemory mem = WritableMemory.writableWrap(new byte[(k*16) +24]);
- WritableMemory cmem = WritableMemory.writableWrap(new byte[8]);
- DirectUpdateDoublesSketch duds =
- (DirectUpdateDoublesSketch)
DoublesSketch.builder().setK(k).build(mem);
- assertTrue(duds.isSameResource(mem));
- DirectCompactDoublesSketch dcds = (DirectCompactDoublesSketch)
duds.compact(cmem);
- assertTrue(dcds.isSameResource(cmem));
+ final int k = 16;
+ final MemorySegment seg = MemorySegment.ofArray(new byte[(k*16) +24]);
+ final MemorySegment cseg = MemorySegment.ofArray(new byte[8]);
+ final DirectUpdateDoublesSketch duds =
+ (DirectUpdateDoublesSketch)
DoublesSketch.builder().setK(k).build(seg);
+ assertTrue(duds.isSameResource(seg));
+ final DirectCompactDoublesSketch dcds = (DirectCompactDoublesSketch)
duds.compact(cseg);
+ assertTrue(dcds.isSameResource(cseg));
- UpdateDoublesSketch uds = DoublesSketch.builder().setK(k).build();
- assertFalse(uds.isSameResource(mem));
+ final UpdateDoublesSketch uds = DoublesSketch.builder().setK(k).build();
+ assertFalse(uds.isSameResource(seg));
}
@Test
public void checkEmptyExceptions() {
- int k = 16;
- UpdateDoublesSketch uds = DoublesSketch.builder().setK(k).build();
- try { uds.getMaxItem(); fail(); } catch (IllegalArgumentException e) {}
- try { uds.getMinItem(); fail(); } catch (IllegalArgumentException e) {}
- try { uds.getRank(1.0); fail(); } catch (IllegalArgumentException e) {}
- try { uds.getPMF(new double[] { 0, 0.5, 1.0 }); fail(); } catch
(IllegalArgumentException e) {}
- try { uds.getCDF(new double[] { 0, 0.5, 1.0 }); fail(); } catch
(IllegalArgumentException e) {}
+ final int k = 16;
+ final UpdateDoublesSketch uds = DoublesSketch.builder().setK(k).build();
+ try { uds.getMaxItem(); fail(); } catch (final IllegalArgumentException e)
{}
+ try { uds.getMinItem(); fail(); } catch (final IllegalArgumentException e)
{}
+ try { uds.getRank(1.0); fail(); } catch (final IllegalArgumentException e)
{}
+ try { uds.getPMF(new double[] { 0, 0.5, 1.0 }); fail(); } catch (final
IllegalArgumentException e) {}
+ try { uds.getCDF(new double[] { 0, 0.5, 1.0 }); fail(); } catch (final
IllegalArgumentException e) {}
}
@Test
public void directSketchShouldMoveOntoHeapEventually() {
- WritableMemory wmem = WritableMemory.allocateDirect(1000,
Arena.ofConfined());
- WritableMemory wmem2 = wmem;
- UpdateDoublesSketch sketch = DoublesSketch.builder().build(wmem);
- Assert.assertTrue(sketch.isSameResource(wmem));
+ final Arena arena = Arena.ofConfined();
+ final MemorySegment wseg = arena.allocate(1000);
+ final UpdateDoublesSketch sketch = DoublesSketch.builder().build(wseg);
+ Assert.assertTrue(sketch.isSameResource(wseg));
for (int i = 0; i < 1000; i++) {
sketch.update(i);
}
- Assert.assertFalse(sketch.isSameResource(wmem));
- Assert.assertFalse(wmem2.isAlive());
+ Assert.assertFalse(sketch.isSameResource(wseg));
+ arena.close();
+ Assert.assertFalse(wseg.scope().isAlive());
}
@Test
public void directSketchShouldMoveOntoHeapEventually2() {
+ final Arena arena = Arena.ofConfined();
int i = 0;
- WritableMemory wmem = WritableMemory.allocateDirect(50,
Arena.ofConfined());
- WritableMemory wmem2 = wmem;
- UpdateDoublesSketch sketch = DoublesSketch.builder().build(wmem);
- Assert.assertTrue(sketch.isSameResource(wmem));
+ final MemorySegment wseg = arena.allocate(50);
+ final UpdateDoublesSketch sketch = DoublesSketch.builder().build(wseg);
+ Assert.assertTrue(sketch.isSameResource(wseg));
for (; i < 1000; i++) {
- if (sketch.isSameResource(wmem)) {
+ if (sketch.isSameResource(wseg)) {
sketch.update(i);
} else {
//println("MOVED OUT at i = " + i);
break;
}
}
- Assert.assertFalse(wmem2.isAlive());
+ arena.close();
+ Assert.assertFalse(wseg.scope().isAlive());
Review Comment:
Similar to the previous issue, the Arena is closed before the final
assertion that checks if the scope is alive, which may affect the test's
validity.
```suggestion
Assert.assertFalse(wseg.scope().isAlive());
arena.close();
```
##########
src/test/java/org/apache/datasketches/quantiles/DoublesSketchTest.java:
##########
@@ -115,66 +114,68 @@ static void testSketchEquality(final DoublesSketch
sketch1,
@Test
public void checkIsSameResource() {
- int k = 16;
- WritableMemory mem = WritableMemory.writableWrap(new byte[(k*16) +24]);
- WritableMemory cmem = WritableMemory.writableWrap(new byte[8]);
- DirectUpdateDoublesSketch duds =
- (DirectUpdateDoublesSketch)
DoublesSketch.builder().setK(k).build(mem);
- assertTrue(duds.isSameResource(mem));
- DirectCompactDoublesSketch dcds = (DirectCompactDoublesSketch)
duds.compact(cmem);
- assertTrue(dcds.isSameResource(cmem));
+ final int k = 16;
+ final MemorySegment seg = MemorySegment.ofArray(new byte[(k*16) +24]);
+ final MemorySegment cseg = MemorySegment.ofArray(new byte[8]);
+ final DirectUpdateDoublesSketch duds =
+ (DirectUpdateDoublesSketch)
DoublesSketch.builder().setK(k).build(seg);
+ assertTrue(duds.isSameResource(seg));
+ final DirectCompactDoublesSketch dcds = (DirectCompactDoublesSketch)
duds.compact(cseg);
+ assertTrue(dcds.isSameResource(cseg));
- UpdateDoublesSketch uds = DoublesSketch.builder().setK(k).build();
- assertFalse(uds.isSameResource(mem));
+ final UpdateDoublesSketch uds = DoublesSketch.builder().setK(k).build();
+ assertFalse(uds.isSameResource(seg));
}
@Test
public void checkEmptyExceptions() {
- int k = 16;
- UpdateDoublesSketch uds = DoublesSketch.builder().setK(k).build();
- try { uds.getMaxItem(); fail(); } catch (IllegalArgumentException e) {}
- try { uds.getMinItem(); fail(); } catch (IllegalArgumentException e) {}
- try { uds.getRank(1.0); fail(); } catch (IllegalArgumentException e) {}
- try { uds.getPMF(new double[] { 0, 0.5, 1.0 }); fail(); } catch
(IllegalArgumentException e) {}
- try { uds.getCDF(new double[] { 0, 0.5, 1.0 }); fail(); } catch
(IllegalArgumentException e) {}
+ final int k = 16;
+ final UpdateDoublesSketch uds = DoublesSketch.builder().setK(k).build();
+ try { uds.getMaxItem(); fail(); } catch (final IllegalArgumentException e)
{}
+ try { uds.getMinItem(); fail(); } catch (final IllegalArgumentException e)
{}
+ try { uds.getRank(1.0); fail(); } catch (final IllegalArgumentException e)
{}
+ try { uds.getPMF(new double[] { 0, 0.5, 1.0 }); fail(); } catch (final
IllegalArgumentException e) {}
+ try { uds.getCDF(new double[] { 0, 0.5, 1.0 }); fail(); } catch (final
IllegalArgumentException e) {}
}
@Test
public void directSketchShouldMoveOntoHeapEventually() {
- WritableMemory wmem = WritableMemory.allocateDirect(1000,
Arena.ofConfined());
- WritableMemory wmem2 = wmem;
- UpdateDoublesSketch sketch = DoublesSketch.builder().build(wmem);
- Assert.assertTrue(sketch.isSameResource(wmem));
+ final Arena arena = Arena.ofConfined();
+ final MemorySegment wseg = arena.allocate(1000);
+ final UpdateDoublesSketch sketch = DoublesSketch.builder().build(wseg);
+ Assert.assertTrue(sketch.isSameResource(wseg));
for (int i = 0; i < 1000; i++) {
sketch.update(i);
}
- Assert.assertFalse(sketch.isSameResource(wmem));
- Assert.assertFalse(wmem2.isAlive());
+ Assert.assertFalse(sketch.isSameResource(wseg));
+ arena.close();
+ Assert.assertFalse(wseg.scope().isAlive());
Review Comment:
The Arena is being closed after checking if the sketch has moved off-heap,
but the Arena should remain alive until after the assertion to ensure the test
accurately verifies the expected behavior.
```suggestion
Assert.assertFalse(wseg.scope().isAlive());
arena.close();
```
##########
src/test/java/org/apache/datasketches/quantiles/ReadOnlyMemoryTest.java:
##########
@@ -184,92 +185,92 @@ public void wrapEmptyUpdateSketch() {
@Test
public void wrapEmptyCompactSketch() {
final UpdateDoublesSketch s1 = DoublesSketch.builder().build();
- final Memory mem = Memory.wrap(s1.compact().toByteArray());
- DoublesSketch s2 = DoublesSketch.wrap(mem); // compact, so this is ok
+ final MemorySegment seg =
MemorySegment.ofArray(s1.compact().toByteArray());
+ final DoublesSketch s2 = DoublesSketch.wrap(seg); // compact, so this is ok
Assert.assertTrue(s2.isEmpty());
}
@Test
public void heapifyUnionFromSparse() {
- UpdateDoublesSketch s1 = DoublesSketch.builder().build();
+ final UpdateDoublesSketch s1 = DoublesSketch.builder().build();
s1.update(1);
s1.update(2);
- Memory mem = Memory.wrap(s1.toByteArray(false));
- DoublesUnion u = DoublesUnion.heapify(mem);
+ final MemorySegment seg = MemorySegment.ofArray(s1.toByteArray(false));
+ final DoublesUnion u = DoublesUnion.heapify(seg);
u.update(3);
- DoublesSketch s2 = u.getResult();
+ final DoublesSketch s2 = u.getResult();
Assert.assertEquals(s2.getMinItem(), 1.0);
Assert.assertEquals(s2.getMaxItem(), 3.0);
}
@Test
public void heapifyUnionFromCompact() {
- UpdateDoublesSketch s1 = DoublesSketch.builder().build();
+ final UpdateDoublesSketch s1 = DoublesSketch.builder().build();
s1.update(1);
s1.update(2);
- Memory mem = Memory.wrap(s1.toByteArray(true));
- DoublesUnion u = DoublesUnion.heapify(mem);
+ final MemorySegment seg = MemorySegment.ofArray(s1.toByteArray(true));
+ final DoublesUnion u = DoublesUnion.heapify(seg);
u.update(3);
- DoublesSketch s2 = u.getResult();
+ final DoublesSketch s2 = u.getResult();
Assert.assertEquals(s2.getMinItem(), 1.0);
Assert.assertEquals(s2.getMaxItem(), 3.0);
}
@Test
public void wrapUnionFromSparse() {
- UpdateDoublesSketch s1 = DoublesSketch.builder().build();
+ final UpdateDoublesSketch s1 = DoublesSketch.builder().build();
s1.update(1);
s1.update(2);
- Memory mem = Memory.wrap(s1.toByteArray(false));
- DoublesUnion u = DoublesUnion.wrap(mem);
- DoublesSketch s2 = u.getResult();
+ final MemorySegment seg =
MemorySegment.ofArray(s1.toByteArray(false)).asReadOnly();
+ final DoublesUnion u = DoublesUnion.wrap(seg);
+ final DoublesSketch s2 = u.getResult();
Assert.assertEquals(s2.getMinItem(), 1.0);
Assert.assertEquals(s2.getMaxItem(), 2.0);
// ensure update and reset methods fail
try {
u.update(3);
fail();
- } catch (SketchesReadOnlyException e) {
+ } catch (final IllegalArgumentException e) {
// expected
}
try {
u.union(s2);
fail();
- } catch (SketchesReadOnlyException e) {
+ } catch (final IllegalArgumentException e) {
// expected
}
try {
- u.union(mem);
+ u.union(seg);
fail();
- } catch (SketchesReadOnlyException e) {
+ } catch (final IllegalArgumentException e) {
// expected
}
try {
u.reset();
fail();
- } catch (SketchesReadOnlyException e) {
+ } catch (final IllegalArgumentException e) {
// expected
}
try {
u.getResultAndReset();
fail();
- } catch (SketchesReadOnlyException e) {
+ } catch (final AssertionError e) { //null
Review Comment:
The expected exception type has changed from SketchesReadOnlyException to
AssertionError, but the comment suggests this might be incorrect. This should
be verified as the correct expected exception for the FFM API.
```suggestion
} catch (final SketchesReadOnlyException e) { // Thrown when attempting
to reset a read-only union
```
--
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]
