Copilot commented on code in PR #684:
URL: https://github.com/apache/datasketches-java/pull/684#discussion_r2286796244
##########
.github/workflows/javadoc.yml:
##########
@@ -30,7 +33,7 @@ jobs:
run: mvn javadoc:javadoc
- name: Deploy JavaDoc
- uses: JamesIves/[email protected]
+ uses:
JamesIves/github-pages-deploy-action@881db5376404c5c8d621010bcbec0310b58d5e29
Review Comment:
Using a full commit SHA instead of a version tag for GitHub Action reduces
transparency and makes it harder to verify the action version. Consider using a
tagged version like @v4.6.8 or newer.
```suggestion
uses: JamesIves/[email protected]
```
##########
src/main/java/org/apache/datasketches/quantiles/DoublesSketch.java:
##########
@@ -151,29 +151,59 @@ public static DoublesSketch heapify(final MemorySegment
srcSeg) {
}
/**
- * Wrap this sketch around the given updatable MemorySegment image of a
DoublesSketch, compact or updatable.
+ * Wrap this sketch around the given MemorySegment image of a compact,
read-only DoublesSketch.
*
- * @param srcSeg the given MemorySegment image of a DoublesSketch that may
have data
- * @return a sketch that wraps the given srcSeg in read-only mode.
+ * @param srcSeg the given MemorySegment image of a compact, read-only
DoublesSketch.
+ * @return a compact, read-only sketch that wraps the given MemorySegment.
*/
public static DoublesSketch wrap(final MemorySegment srcSeg) {
+ if (!checkIsMemorySegmentCompact(srcSeg)) {
+ throw new SketchesArgumentException("MemorySegment sketch image must be
in compact form.");
Review Comment:
The wrap() method now throws an exception for non-compact segments, which is
a breaking API change. Consider providing a deprecation period or clearer
migration path for existing code that may pass updatable segments to this
method.
##########
src/main/java/org/apache/datasketches/quantiles/DoublesSketch.java:
##########
@@ -151,29 +151,59 @@ public static DoublesSketch heapify(final MemorySegment
srcSeg) {
}
/**
- * Wrap this sketch around the given updatable MemorySegment image of a
DoublesSketch, compact or updatable.
+ * Wrap this sketch around the given MemorySegment image of a compact,
read-only DoublesSketch.
*
- * @param srcSeg the given MemorySegment image of a DoublesSketch that may
have data
- * @return a sketch that wraps the given srcSeg in read-only mode.
+ * @param srcSeg the given MemorySegment image of a compact, read-only
DoublesSketch.
+ * @return a compact, read-only sketch that wraps the given MemorySegment.
*/
public static DoublesSketch wrap(final MemorySegment srcSeg) {
+ if (!checkIsMemorySegmentCompact(srcSeg)) {
+ throw new SketchesArgumentException("MemorySegment sketch image must be
in compact form.");
Review Comment:
The error message could be more helpful by suggesting the alternative method
to use: "MemorySegment sketch image must be in compact form. Use writableWrap()
for updatable sketches."
```suggestion
throw new SketchesArgumentException("MemorySegment sketch image must
be in compact form. Use writableWrap() for updatable sketches.");
```
##########
src/main/java/org/apache/datasketches/quantiles/DoublesSketch.java:
##########
@@ -151,29 +151,59 @@ public static DoublesSketch heapify(final MemorySegment
srcSeg) {
}
/**
- * Wrap this sketch around the given updatable MemorySegment image of a
DoublesSketch, compact or updatable.
+ * Wrap this sketch around the given MemorySegment image of a compact,
read-only DoublesSketch.
*
- * @param srcSeg the given MemorySegment image of a DoublesSketch that may
have data
- * @return a sketch that wraps the given srcSeg in read-only mode.
+ * @param srcSeg the given MemorySegment image of a compact, read-only
DoublesSketch.
+ * @return a compact, read-only sketch that wraps the given MemorySegment.
*/
public static DoublesSketch wrap(final MemorySegment srcSeg) {
+ if (!checkIsMemorySegmentCompact(srcSeg)) {
+ throw new SketchesArgumentException("MemorySegment sketch image must be
in compact form.");
+ }
+ return DirectCompactDoublesSketch.wrapInstance(srcSeg);
+ }
+
+ /**
+ * Wrap this sketch around the given MemorySegment image of an updatable
DoublesSketch.
+ *
+ * <p>The given MemorySegment must be writable and it must contain a
<i>UpdateDoublesSketch</i>.
+ * The sketch will be updated and managed totally within the MemorySegment.
If the given source
+ * MemorySegment is created off-heap, then all the management of the
sketch's internal data will be off-heap as well.</p>
+ *
+ * <p><b>NOTE:</b>If during updating of the sketch the sketch requires more
capacity than the given size of the MemorySegment, the sketch
+ * will request more capacity using the {@link MemorySegmentRequest
MemorySegmentRequest} interface. The default of this interface will
+ * return a new MemorySegment on the heap.</p>
+ *
+ * @param srcSeg the given MemorySegment image of an
<i>UpdateDoublesSketch</i>.
+ * @return an updatable sketch that wraps the given MemorySegment.
+ */
+ public static DoublesSketch writableWrap(final MemorySegment srcSeg) {
if (checkIsMemorySegmentCompact(srcSeg)) {
- return DirectCompactDoublesSketch.wrapInstance(srcSeg);
+ throw new SketchesArgumentException("MemorySegment sketch image must be
in updatable form.");
}
return DirectUpdateDoublesSketch.wrapInstance(srcSeg, null);
}
/**
- * Wrap this sketch around the given updatable MemorySegment image of a
DoublesSketch, compact or updatable.
+ * Wrap this sketch around the given MemorySegment image of an updatable
DoublesSketch.
+ *
+ * <p>The given MemorySegment must be writable and it must contain a
<i>UpdateDoublesSketch</i>.
+ * The sketch will be updated and managed totally within the MemorySegment.
If the given source
+ * MemorySegment is created off-heap, then all the management of the
sketch's internal data will be off-heap as well.</p>
+ *
+ * <p><b>NOTE:</b>If during updating of the sketch the sketch requires more
capacity than the given size of the MemorySegment, the sketch
+ * will request more capacity using the {@link MemorySegmentRequest
MemorySegmentRequest} interface. The default of this interface will
+ * return a new MemorySegment on the heap. It is up to the user to
optionally extend this interface if more flexible
+ * handling of requests for more capacity is required.</p>
*
- * @param srcSeg the given MemorySegment image of a DoublesSketch that may
have data.
+ * @param srcSeg the given MemorySegment image of a DoublesSketch.
* @param mSegReq the MemorySegmentRequest used if the given MemorySegment
needs to expand.
* Otherwise, it can be null and the default MemorySegmentRequest will be
used.
- * @return a sketch that wraps the given srcSeg in read-only mode.
+ * @return a sketch that wraps the given MemorySegment.
*/
- public static DoublesSketch wrap(final MemorySegment srcSeg, final
MemorySegmentRequest mSegReq) {
+ public static DoublesSketch writableWrap(final MemorySegment srcSeg, final
MemorySegmentRequest mSegReq) {
if (checkIsMemorySegmentCompact(srcSeg)) {
- return DirectCompactDoublesSketch.wrapInstance(srcSeg);
+ throw new SketchesArgumentException("MemorySegment sketch image must be
in updatable form.");
Review Comment:
The error message could be more helpful by suggesting the alternative method
to use: "MemorySegment sketch image must be in updatable form. Use wrap() for
compact sketches."
```suggestion
throw new SketchesArgumentException("MemorySegment sketch image must
be in updatable form. Use wrap() for compact sketches.");
```
##########
src/main/java/org/apache/datasketches/quantiles/DoublesSketch.java:
##########
@@ -151,29 +151,59 @@ public static DoublesSketch heapify(final MemorySegment
srcSeg) {
}
/**
- * Wrap this sketch around the given updatable MemorySegment image of a
DoublesSketch, compact or updatable.
+ * Wrap this sketch around the given MemorySegment image of a compact,
read-only DoublesSketch.
*
- * @param srcSeg the given MemorySegment image of a DoublesSketch that may
have data
- * @return a sketch that wraps the given srcSeg in read-only mode.
+ * @param srcSeg the given MemorySegment image of a compact, read-only
DoublesSketch.
+ * @return a compact, read-only sketch that wraps the given MemorySegment.
*/
public static DoublesSketch wrap(final MemorySegment srcSeg) {
+ if (!checkIsMemorySegmentCompact(srcSeg)) {
+ throw new SketchesArgumentException("MemorySegment sketch image must be
in compact form.");
+ }
+ return DirectCompactDoublesSketch.wrapInstance(srcSeg);
+ }
+
+ /**
+ * Wrap this sketch around the given MemorySegment image of an updatable
DoublesSketch.
+ *
+ * <p>The given MemorySegment must be writable and it must contain a
<i>UpdateDoublesSketch</i>.
+ * The sketch will be updated and managed totally within the MemorySegment.
If the given source
+ * MemorySegment is created off-heap, then all the management of the
sketch's internal data will be off-heap as well.</p>
+ *
+ * <p><b>NOTE:</b>If during updating of the sketch the sketch requires more
capacity than the given size of the MemorySegment, the sketch
+ * will request more capacity using the {@link MemorySegmentRequest
MemorySegmentRequest} interface. The default of this interface will
+ * return a new MemorySegment on the heap.</p>
+ *
+ * @param srcSeg the given MemorySegment image of an
<i>UpdateDoublesSketch</i>.
+ * @return an updatable sketch that wraps the given MemorySegment.
+ */
+ public static DoublesSketch writableWrap(final MemorySegment srcSeg) {
if (checkIsMemorySegmentCompact(srcSeg)) {
- return DirectCompactDoublesSketch.wrapInstance(srcSeg);
+ throw new SketchesArgumentException("MemorySegment sketch image must be
in updatable form.");
Review Comment:
The error message could be more helpful by suggesting the alternative method
to use: "MemorySegment sketch image must be in updatable form. Use wrap() for
compact sketches."
```suggestion
throw new SketchesArgumentException("MemorySegment sketch image must
be in updatable form. Use wrap() for compact sketches.");
```
--
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]
