Copilot commented on code in PR #683:
URL: https://github.com/apache/datasketches-java/pull/683#discussion_r2268382734
##########
src/main/java/org/apache/datasketches/quantiles/ClassicUtil.java:
##########
@@ -98,6 +98,21 @@ public static int getKFromEpsilon(final double epsilon,
final boolean pmf) {
return max(MIN_K, min(MAX_K, k));
}
+ /**
+ * Computes the new size of a growing base buffer.
+ * It doubles whatever the current size is until it reaches 2 * k items.
+ * This assumes all items are the same size and the combinedBuffer has no
levels above the base buffer.
+ * @param k the given k of the sketch
+ * @param curCombinedBufCap the current capacity of the combined buffer in
items.
+ * @return the new size of the base buffer in items.
+ */
+ static int computeGrowingBaseBufferCap(final int k, final int
curCombinedBufCap) {
+ if (curCombinedBufCap < (2 * k)) {
+ return 2 * Math.max(Math.min(k, curCombinedBufCap), MIN_K);
+ }
Review Comment:
[nitpick] The logic in `computeGrowingBaseBufferCap` could be simplified.
The `if` condition check is redundant since the `else` case returns `2 * k`
anyway. Consider refactoring for clarity.
```suggestion
```
##########
src/main/java/org/apache/datasketches/quantiles/DoublesUtil.java:
##########
@@ -137,6 +137,7 @@ private static String getSummary(final DoublesSketch sk) {
sb.append(LS).append("### Classic Quantiles
").append(thisSimpleName).append(" SUMMARY: ").append(LS);
sb.append(" Empty :
").append(sk.isEmpty()).append(LS);
sb.append(" Segment, Capacity bytes :
").append(sk.hasMemorySegment()).append(", ").append(segCap).append(LS);
+ sb.append(" Segment, ReadOnly :
").append(sk.hasMemorySegment() ? sk.getMemorySegment().isReadOnly() :
false).append(LS);
Review Comment:
[nitpick] The ternary operator can be simplified since
`sk.hasMemorySegment()` already guards the null check. Consider extracting this
to a variable for clarity: `final boolean isReadOnly = sk.hasMemorySegment() &&
sk.getMemorySegment().isReadOnly();`
##########
src/main/java/org/apache/datasketches/quantiles/HeapUpdateDoublesSketch.java:
##########
@@ -373,6 +375,11 @@ MemorySegment getMemorySegment() {
return null;
}
Review Comment:
The new `setReadOnly()` method lacks proper JavaDoc documentation. Consider
adding documentation explaining the purpose and behavior of this method.
```suggestion
/**
* This method is not supported for heap-based sketches. To obtain a
read-only version,
* convert this sketch to a CompactSketch instead.
*
* @throws SketchesNotSupportedException always thrown to indicate this
operation is not supported
*/
```
##########
src/main/java/org/apache/datasketches/quantiles/DoublesSketch.java:
##########
@@ -563,6 +564,11 @@ UpdateDoublesSketch downSampleInternal(final DoublesSketch
srcSketch, final int
*/
abstract MemorySegment getMemorySegment();
+ /**
+ * Sets the internal MemorySegment to Read Only.
Review Comment:
The JavaDoc for `setReadOnly()` is minimal. Consider adding more details
about when this method should be called and its effects on the sketch's
behavior.
```suggestion
* Sets the internal {@link MemorySegment} used by this sketch to
read-only mode.
* <p>
* <b>When to call:</b> This method should be called when you want to
prevent any further
* modifications to the sketch, for example after building the sketch and
before sharing it
* across threads or exposing it to untrusted code.
* </p>
* <p>
* <b>Effects:</b> After calling this method, any attempt to modify the
sketch (such as updating,
* merging, or resetting) will result in an exception (typically a {@link
java.lang.UnsupportedOperationException}
* or similar), depending on the implementation. This ensures the
integrity of the sketch's data.
* </p>
* <p>
* <b>Notes:</b> This method only has an effect if the sketch is backed by
a writable {@link MemorySegment}.
* If the sketch is not backed by a {@link MemorySegment}, or is already
read-only, this method may have no effect.
* </p>
```
--
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: dev-unsubscr...@datasketches.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@datasketches.apache.org
For additional commands, e-mail: dev-h...@datasketches.apache.org
