Java8-11 changes consistent with 17 #576

leerho · 2024-06-19T22:06:57Z

This PR is targeting a DS-Java 6.1.0 release. The changes in this PR make the Java repo consistent with the just released Memory 3.0.0. This ds-java release is still targeted for Java 8 and 11 and will likely be the last release supporting Java 8 and 11. If all goes as planned, the next release will be DS-Java 7.0.0, which will be targeted at Java 17.

All of the following changes are internal and do not impact the DS-Java API.

DS-Java changes from Master to branch java8-11_changes_consistent_with_17:

common/Util.java

1 javadoc

theta/Sketch

Deprecation removed on getMaxCompactSketchBytes
bug fix: getCompactSketchMaxBytes(lgK)

theta/Sketches

Deprecation removed on getMaxCompactSketchBytes(numEntries), Javadoc improved.

filters/bloomfilter

all new code for 6.1.0.
changes are to make the calls to Memory compatible with Memory 3.0.0.

hll/DirectAuxHashMapTest

WritableHandle removed

hll/DirectCouponListTest

WritableHandle removed

kll/KllDoublesValidationTest

code comment change

quantiles/DebugUnionTest

WritableHandle removed

quantiles/DirectQuantilesMemoryRequestTest

WritableHandle removed

quantiles/DoublesSketchTest

WritableHandle removed

quantiles/PreambleUtilTest

WritableHandle removed

theta/CompactSketchTest

WritableHandle removed

theta/DirectQuickSelectSketchTest

WritableHandle removed

theta/HeapifyWrapSerVer1and2Test

All variable renaming

theta/SketchesTest

bug fix: getCompactSketchMaxBytes

theta/UnionImplTest

WritableHandle removed

theta/UpdateSketchTest

removed some code comments

pom.xml

updated Memory dependency to 3.0.0
Removed some comments
Reorganized the section
updated some of the plugin versions

Readme

Fixed the badges
Removed references to "-P strict"

leerho · 2024-06-20T15:46:25Z

This must wait until DS-Memory 3.0.0 is released.

src/test/java/org/apache/datasketches/quantiles/DirectQuantilesMemoryRequestTest.java

jmalkin

I didn't find that the variable name changes in HeapifyWrapSerVer1and2Test.java actually made things any clearer. Expanding the comments above each block would have been sufficient.

The use of wmem.close() seemed arbitrary at times? Maybe it was always tied to a try-with-resources block and I didn't see that in some cases?

But we have a few tests that don't actually seem to test anything, so we should fix those. Even if the previous version was similarly flawed.

jmalkin · 2024-08-09T04:49:09Z

src/test/java/org/apache/datasketches/theta/UnionImplTest.java

+
+    final Union union2 = SetOperation.builder().buildUnion(); //on-heap union
+    assertFalse(union2.isSameResource(wmem2));  //obviously not
+    wmem.close();


why do we not need to close wmem2?

Because it has already been closed. In fact, if you try to close wmem2, it will throw an error.

The purpose of this test is to check the move and resize behavior when a sketch is off-heap.

Note on line 193 we get sufficient bytes to hold the sketch, but on line 195 we under allocate space by half. This will force the sketch to request more memory.

The default MemoryRequestServer will allocate new memory on the heap, and the sketch itself will move the data from wmem2 to the new heap allocation, and then resize the internal hash table into the new memory. Then the sketch will request the MRS to close wmem2, which it does.

I will add some more comments to make this test a little clearer

jmalkin · 2024-08-12T18:40:01Z

src/test/java/org/apache/datasketches/quantiles/DirectQuantilesMemoryRequestTest.java

+
+    //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();


Why do we get lastMem but not assert anything about it?

The real objective of this test is to be an example for the user on how the MemoryRequestServer can be used to help with limited memory scenarios where the sketch needs to expand beyond what was allocated. This is why there are extensive code comments in this test method.

The actual test that the expansion of memory occurred (several times) and the test that the sketch continued successfully is on line 67. After that, the actual capacity of the last memory allocated is printed out just for the user. There is no reason to test it, we know that it was big enough.

No change is necessary.

I think in order to call it a test and pretend it means anything we should assert that the size is larger than the initial size.

The test of success is on line 67. By definition, if line 67 passes, the memory had to grow larger than the initial size. If the memory capacity did not grow larger than the initial size, the sketch would run out of memory and the test would fail. In fact it would fail before it ever reached line 67. So putting an extra test that the memory in fact grew beyond the initial size is redundant and impotent because that extra test would either never be executed or it would always pass.

jmalkin · 2024-08-12T18:45:22Z

src/test/java/org/apache/datasketches/quantiles/DoublesSketchTest.java

    }
+    println(sketch.toString());
  }

  @Test
  public void directSketchShouldMoveOntoHeapEventually2() {


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).

Right. If wmem is still alive at the end then the test failed. I just added one line.

jmalkin · 2024-08-12T18:46:29Z

src/test/java/org/apache/datasketches/quantiles/DoublesSketchTest.java

      }
-    } catch (final Exception e) {
-      throw new RuntimeException(e);
    }
  }

  @Test
  public void checkEmptyDirect() {


There's again no actual test here?

Right. In the process of applying a test, I discovered a minor bug in DoublesByteArrayImp::convertToByteArray(...). This test was generating an empty sketch, which should result in a returned byte[] of only 8 bytes. And it was 8 bytes, but the preLongs value in byte 0 was 2 instead of 1. This is now fixed.

jmalkin · 2024-08-12T18:47:51Z

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) {}


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.

Right. This comment applies to two adjacent test methods as well. Fixed.

leerho · 2024-08-15T21:22:53Z

Thank you for your comments!
In the process of investigating the first comment (theta/UnionImplTest). I uncovered some other issues that need to be fixed, but I will submit those under different PRs ... and try to make them as small as possible :)

leerho added 5 commits June 19, 2024 14:49

Java8-11 changes consistent with 17

fc6c471

More updates consistent with 17

28f5024

Fixing small things.

756dd4b

fix getCompactSketchMaxBytes & javadocs

e61d7b7

Remove dependency on Apache Parent POM

51ce694

leerho added 5 commits June 27, 2024 17:41

Remove code comment.

61864f0

replace parent pom.

80631a2

Update Memory dependency to 3.0.0.

0e1d83d

Merge branch 'master' into java8-11_changes_consistent_with_17

8b71ffa

Update POM with Memory 3.0.0 dependency

999f89a

github-advanced-security bot found potential problems Aug 6, 2024

View reviewed changes

src/test/java/org/apache/datasketches/quantiles/DirectQuantilesMemoryRequestTest.java Dismissed Show dismissed Hide dismissed

Remove references to -P strict

93ebd40

leerho marked this pull request as ready for review August 6, 2024 21:52

leerho requested review from jmalkin and AlexanderSaydakov August 6, 2024 21:54

AlexanderSaydakov approved these changes Aug 12, 2024

View reviewed changes

jmalkin requested changes Aug 12, 2024

View reviewed changes

Changes requested from reviews of PR#576

8a5e05f

leerho merged commit 16ab3da into master Aug 15, 2024
4 checks passed

leerho deleted the java8-11_changes_consistent_with_17 branch August 15, 2024 22:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Java8-11 changes consistent with 17 #576

Java8-11 changes consistent with 17 #576

leerho commented Jun 19, 2024 •

edited

Loading

leerho commented Jun 20, 2024

jmalkin left a comment

jmalkin Aug 9, 2024

leerho Aug 15, 2024

jmalkin Aug 12, 2024

leerho Aug 15, 2024 •

edited

Loading

jmalkin Aug 15, 2024

leerho Aug 15, 2024

jmalkin Aug 12, 2024

leerho Aug 15, 2024

jmalkin Aug 12, 2024

leerho Aug 15, 2024

jmalkin Aug 12, 2024

leerho Aug 15, 2024

leerho commented Aug 15, 2024

Java8-11 changes consistent with 17 #576

Java8-11 changes consistent with 17 #576

Conversation

leerho commented Jun 19, 2024 • edited Loading

leerho commented Jun 20, 2024

jmalkin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leerho Aug 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leerho commented Aug 15, 2024

leerho commented Jun 19, 2024 •

edited

Loading

leerho Aug 15, 2024 •

edited

Loading