Methods to compute partition limits by leerho · Pull Request #546 · apache/datasketches-java

leerho · 2024-04-12T19:39:29Z

No description provided.

already documented in the body of the javadoc.

leerho · 2024-04-12T20:31:43Z

Class by Class description of the changes.

KllItemsSketch

Propagation of new split out methods defined by PartitioningFeature.
Propagation of not requiring the normRankErr.

Partitioner

Propagation of new split out methods defined by PartitioningFeature.

ItemsSketch

Propagation of new split out methods defined by PartitioningFeature.
Propagation of not requiring the normRankErr.

GenericPartitionBoundaries

Javadoc and code comment improvements
Removed super interface PartitionBoundaries and pulled its methods with javadoc here.

GenericSortedView

extend new interface SketchPartitionLimits

ItemsSketchSortedView

Propagation and implementation of new split out methods defined by PartitioningFeature.
Simplified error messages
Simplified error detection for numEquallySizedParts too large
I figured out a simpler way to validate the maximum number of partitions without requiring the sketch normRankErr.
Removed now unneeded 'normRankErr and PARTITIONING_ERROR_FACTOR

PartitionBoundaries

Removed. Its methods moved to its only child class, GenericPartitionBoundaries.

PartitioningFeature

Split getPartitionBoundaries(numEquallySizedParts) into two methods:
- getPartitionBoundariesFromNumParts(numPartitions) and
- getPartitionBoundariesFromPartSize(nominalPartSizeItems).
- This reflects how Druid actually was trying to use this method and ended up trying to simulate the second method incorrectly.
- Removed the @throws clause because it was duplicative of what is already in the Javadoc body. (Also, the CI/CD runner was throwing a class not found exception on SketchesArgumentException. I think it needed to be fully qualified. But I decided to remove these clauses anyway.)

QuantilesGenericAPI

extend new interface SketchPartitionLimits

SketchPartitionLimits

This new interface defines the methods required to implement the two new limit methods
- getMaxPartitions()
- getMinPartitionSizeItems()
  Which can actually be implemented here as defaults.

/TEST/...

ItemsSketchTest

Propagation of change in ItemsSketchSortedView constructor change.

CrossCheckQuantilesTest

Propagation of change in ItemsSketchSortedView constructor change.

PartitionBoundariesTest

Propagation of method name changes.
Added tests for new SketchPartitionLimits methods

jmalkin

I'm not going to block this, but I'm not sure if we fully test both success and failure of the new things? It's possible some of it is implicitly tested via existing code making that a non-issue. But if not, we should fix that before merging.

jmalkin · 2024-04-13T00:02:10Z

+public interface SketchPartitionLimits {
+
+  /**
+   * Gets the maximum number of partitions this sketch will support based on the configured size <i>K</i>


This doesn't seem to use the value of k anywhere? I guess it's implicit in getNumRetained()?

The comment was about the documentation saying it uses k when the method does not actually do so explicitly (k will impact the number retained outside of exact mode, though, so it's an implicit dependence).

I will fix.

jmalkin · 2024-04-13T00:16:17Z

-
-    if (numEquallySized > maxParts) {
+    final long partSizeItems = getMinPartitionSizeItems();
+    if (nominalPartitionSize < partSizeItems) {


i think this would read a little more clearly if we rename partSizeItems to minPartSizeItems

jmalkin · 2024-04-13T00:19:56Z

-          + "Retained Items / 2: " + (svLen / 2));
+          + " The requested number of partitions is too large for this sketch.");
    }
+    final long totalN = this.totalN;


totalN doesn't seem to be used for quite a while in case you want to define it closer to where it's actually used. Not a big deal either way.

I'll look at it.

Turns out is was already a class member. I didn't need this line.

jmalkin · 2024-04-13T01:32:25Z

+
+    println("Min Size Limit: " + sizeLimit);
+    @SuppressWarnings("unused")
+    GenericPartitionBoundaries<String> gpb = sk.getPartitionBoundariesFromPartSize(sizeLimit - 1);


do we ever test this method so that it succeeds rather than throwing?

I tested it manually, and it passes at sizeLimit and fails at sizeLimit - 1.
I didn't think it was necessary, but I could add a test that passes exactly at sizeLimit.
Same thing goes for the limit on num partitions.

- getMaxPartitions() - getMinPartSizeItems()

+    //this should pass
+    final int goodNumPartsRequest = sk.getMaxPartitions();
+    println("Good numPartsRequest " + goodNumPartsRequest);
+    GenericPartitionBoundaries<String> gpb = sk.getPartitionBoundariesFromNumParts(goodNumPartsRequest);


+    //this should pass
+    final long goodPartSizeRequest= sk.getMinPartitionSizeItems();
+    println("Good partSizeRequest " + goodPartSizeRequest);
+    GenericPartitionBoundaries<String> gpb = sk.getPartitionBoundariesFromPartSize(goodPartSizeRequest);


leerho added 3 commits April 11, 2024 21:37

temp commit

20166c7

Merge branch 'master' into methods_to_compute_partition_limits

71b9648

Added tests for new partition limits

30a813d

leerho requested a review from jmalkin April 12, 2024 19:39

github-advanced-security AI found potential problems Apr 12, 2024

View reviewed changes

Comment thread src/test/java/org/apache/datasketches/quantilescommon/PartitionBoundariesTest.java Fixed

Comment thread src/test/java/org/apache/datasketches/quantilescommon/PartitionBoundariesTest.java Fixed

Removed javadoc @throws clauses -- because they duplicate what is

5cc7887

already documented in the body of the javadoc.

jmalkin approved these changes Apr 13, 2024

View reviewed changes

Added two limit check methods:

de4ef79

- getMaxPartitions() - getMinPartSizeItems()

github-advanced-security AI found potential problems Apr 14, 2024

View reviewed changes

leerho merged commit 30fa37e into master Apr 14, 2024

leerho deleted the methods_to_compute_partition_limits branch April 14, 2024 05:41

Conversation

leerho commented Apr 12, 2024

Uh oh!

Uh oh!

Uh oh!

leerho commented Apr 12, 2024

Uh oh!

jmalkin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

leerho Apr 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Check notice

Check notice

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

leerho Apr 13, 2024 •

edited

Loading