jmalkin commented on code in PR #576:
URL: https://github.com/apache/datasketches-java/pull/576#discussion_r1710735794
##########
src/test/java/org/apache/datasketches/theta/UnionImplTest.java:
##########
@@ -192,24 +191,20 @@ public void checkMoveAndResize() {
final int k = 1 << 12;
final int u = 2 * k;
final int bytes = Sketches.getMaxUpdateSketchBytes(k);
- try (WritableHandle wh = WritableMemory.allocateDirect(bytes/2);
- WritableHandle wh2 = WritableMemory.allocateDirect(bytes/2) ) {
- final WritableMemory wmem = wh.getWritable();
- final UpdateSketch sketch =
Sketches.updateSketchBuilder().setNominalEntries(k).build(wmem);
- assertTrue(sketch.isSameResource(wmem));
-
- final WritableMemory wmem2 = wh2.getWritable();
- final Union union = SetOperation.builder().buildUnion(wmem2);
- assertTrue(union.isSameResource(wmem2));
-
- for (int i = 0; i < u; i++) { union.update(i); }
- assertFalse(union.isSameResource(wmem));
-
- final Union union2 = SetOperation.builder().buildUnion(); //on-heap union
- assertFalse(union2.isSameResource(wmem2)); //obviously not
- } catch (final Exception e) {
- throw new RuntimeException(e);
- }
+ WritableMemory wmem = WritableMemory.allocateDirect(bytes / 2);
+ WritableMemory wmem2 = WritableMemory.allocateDirect(bytes / 2);
+ final UpdateSketch sketch =
Sketches.updateSketchBuilder().setNominalEntries(k).build(wmem);
+ assertTrue(sketch.isSameResource(wmem));
+
+ final Union union = SetOperation.builder().buildUnion(wmem2);
+ assertTrue(union.isSameResource(wmem2));
+
+ for (int i = 0; i < u; i++) { union.update(i); }
+ assertFalse(union.isSameResource(wmem));
+
+ final Union union2 = SetOperation.builder().buildUnion(); //on-heap union
+ assertFalse(union2.isSameResource(wmem2)); //obviously not
+ wmem.close();
Review Comment:
why do we not need to close `wmem2`?
##########
src/test/java/org/apache/datasketches/quantiles/DirectQuantilesMemoryRequestTest.java:
##########
@@ -47,35 +46,30 @@ public void checkLimitedMemoryScenarios() { //Requesting
application
final int initBytes = ((2 * k) + 4) << 3; //just the BB
//########## Owning Implementation
- // This part would actually be part of the Memory owning implemention so
it is faked here
- try (WritableHandle wdh = WritableMemory.allocateDirect(initBytes,
- ByteOrder.nativeOrder(), new DefaultMemoryRequestServer())) {
- final WritableMemory wmem = wdh.getWritable();
- println("Initial mem size: " + wmem.getCapacity());
-
- //########## Receiving Application
- // The receiving application has been given wmem to use for a sketch,
- // but alas, it is not ultimately large enough.
- final UpdateDoublesSketch usk1 =
DoublesSketch.builder().setK(k).build(wmem);
- assertTrue(usk1.isEmpty());
-
- //Load the sketch
- for (int i = 0; i < u; i++) {
- // The sketch uses The MemoryRequest, acquired from wmem, to acquire
more memory as
- // needed, and requests via the MemoryRequest to free the old
allocations.
- usk1.update(i);
- }
- final double result = usk1.getQuantile(0.5);
- println("Result: " + result);
- assertEquals(result, u / 2.0, 0.05 * u); //Success
-
- //########## Owning Implementation
- //The actual Memory has been re-allocated several times,
- // so the above wmem reference is invalid.
- println("\nFinal mem size: " + wmem.getCapacity());
- } catch (Exception e) {
- throw new RuntimeException(e);
+ // This part would actually be part of the Memory owning implementation so
it is faked here
+ WritableMemory wmem = WritableMemory.allocateDirect(initBytes,
ByteOrder.nativeOrder(), new DefaultMemoryRequestServer());
+ println("Initial mem size: " + wmem.getCapacity());
+
+ //########## Receiving Application
+ // The receiving application has been given wmem to use for a sketch,
+ // but alas, it is not ultimately large enough.
+ final UpdateDoublesSketch usk1 =
DoublesSketch.builder().setK(k).build(wmem);
+ assertTrue(usk1.isEmpty());
+
+ //Load the sketch
+ for (int i = 0; i < u; i++) {
+ // The sketch uses The MemoryRequest, acquired from wmem, to acquire
more memory as
+ // needed, and requests via the MemoryRequest to free the old
allocations.
+ usk1.update(i);
}
+ final double result = usk1.getQuantile(0.5);
+ println("Result: " + result);
+ assertEquals(result, u / 2.0, 0.05 * u); //Success
+
+ //The actual Memory has been re-allocated several times,
+ // so the the wmem reference is invalid. Use the sketch to get the last
memory reference.
+ WritableMemory lastMem = usk1.getMemory();
Review Comment:
Why do we get `lastMem` but not assert anything about it?
##########
src/test/java/org/apache/datasketches/quantiles/DoublesSketchTest.java:
##########
@@ -140,50 +139,37 @@ public void checkEmptyExceptions() {
@Test
public void directSketchShouldMoveOntoHeapEventually() {
- try (WritableHandle wdh = WritableMemory.allocateDirect(1000,
- ByteOrder.nativeOrder(), new DefaultMemoryRequestServer())) {
- WritableMemory mem = wdh.getWritable();
- UpdateDoublesSketch sketch = DoublesSketch.builder().build(mem);
- Assert.assertTrue(sketch.isSameResource(mem));
- for (int i = 0; i < 1000; i++) {
- sketch.update(i);
- }
- Assert.assertFalse(sketch.isSameResource(mem));
- } catch (final Exception e) {
- throw new RuntimeException(e);
+ WritableMemory wmem = WritableMemory.allocateDirect(1000,
ByteOrder.nativeOrder(), new DefaultMemoryRequestServer());
+ UpdateDoublesSketch sketch = DoublesSketch.builder().build(wmem);
+ Assert.assertTrue(sketch.isSameResource(wmem));
+ for (int i = 0; i < 1000; i++) {
+ sketch.update(i);
}
+ println(sketch.toString());
}
@Test
public void directSketchShouldMoveOntoHeapEventually2() {
Review Comment:
This test doesn't actually test anything. There's no possible failure.
Rather than `break` it should set some sort of success flag and at the end
of the test the success flag should be true. If it is not, the sketch did not
actually move on heap. But right now we don't actually check that (and didn't
before).
##########
src/test/java/org/apache/datasketches/quantiles/DoublesSketchTest.java:
##########
@@ -140,50 +139,37 @@ public void checkEmptyExceptions() {
@Test
public void directSketchShouldMoveOntoHeapEventually() {
- try (WritableHandle wdh = WritableMemory.allocateDirect(1000,
- ByteOrder.nativeOrder(), new DefaultMemoryRequestServer())) {
- WritableMemory mem = wdh.getWritable();
- UpdateDoublesSketch sketch = DoublesSketch.builder().build(mem);
- Assert.assertTrue(sketch.isSameResource(mem));
- for (int i = 0; i < 1000; i++) {
- sketch.update(i);
- }
- Assert.assertFalse(sketch.isSameResource(mem));
- } catch (final Exception e) {
- throw new RuntimeException(e);
+ WritableMemory wmem = WritableMemory.allocateDirect(1000,
ByteOrder.nativeOrder(), new DefaultMemoryRequestServer());
+ UpdateDoublesSketch sketch = DoublesSketch.builder().build(wmem);
+ Assert.assertTrue(sketch.isSameResource(wmem));
+ for (int i = 0; i < 1000; i++) {
+ sketch.update(i);
}
+ println(sketch.toString());
}
@Test
public void directSketchShouldMoveOntoHeapEventually2() {
int i = 0;
- try (WritableHandle wdh =
- WritableMemory.allocateDirect(50, ByteOrder.LITTLE_ENDIAN, new
DefaultMemoryRequestServer())) {
- WritableMemory mem = wdh.getWritable();
- UpdateDoublesSketch sketch = DoublesSketch.builder().build(mem);
- Assert.assertTrue(sketch.isSameResource(mem));
- for (; i < 1000; i++) {
- if (sketch.isSameResource(mem)) {
- sketch.update(i);
- } else {
- //println("MOVED OUT at i = " + i);
- break;
- }
+ WritableMemory wmem = WritableMemory.allocateDirect(50,
ByteOrder.LITTLE_ENDIAN, new DefaultMemoryRequestServer());
+ UpdateDoublesSketch sketch = DoublesSketch.builder().build(wmem);
+ Assert.assertTrue(sketch.isSameResource(wmem));
+ for (; i < 1000; i++) {
+ if (wmem.isAlive()) {
+ sketch.update(i);
+ } else {
+ println("Sketch Move to Heap at i = " + i);
+ break;
}
- } catch (final Exception e) {
- throw new RuntimeException(e);
}
}
@Test
public void checkEmptyDirect() {
Review Comment:
There's again no actual test here?
##########
src/test/java/org/apache/datasketches/theta/DirectQuickSelectSketchTest.java:
##########
@@ -97,8 +88,7 @@ public void checkConstructorKtooSmall() {
@Test//(expectedExceptions = SketchesArgumentException.class)
public void checkConstructorMemTooSmall() {
int k = 16;
- try (WritableHandle h = makeNativeMemory(k/2)) {
- WritableMemory mem = h.getWritable();
+ try (WritableMemory mem = makeNativeMemory(k/2)) {
UpdateSketch.builder().setNominalEntries(k).build(mem);
} catch (final Exception e) {
if (e instanceof SketchesArgumentException) {}
Review Comment:
Why not use the expected exceptions annotation here? It's fine if there's a
reason, but that should be documented and the comment in the test annotation
removed.
--
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]
