You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@datasketches.apache.org by "leerho (via GitHub)" <gi...@apache.org> on 2025/08/12 01:44:26 UTC
[PR] Cleanup phase 1 (datasketches-java)
leerho opened a new pull request, #683:
URL: https://github.com/apache/datasketches-java/pull/683
# Main
## common
### Family
* added static initializer check for machine native endianness
### Util
* added static initializer check for machine native endianness
### MemorySegmentRequest
* Added comments to empty blocks.
### BigEndianNativeOrderNotSupportedException
* Removed, replaced with more general SketechesNotSupportedException
### SketechesNotSupportedException
* Replaces BigEndianNativeOrderNotSupportedException
## hll
### AbstractCoupons
* Added comments to empty blocks.
### DirectCouponHashSet
* Simple Reformating, no code change
### DirectCouponList
* Set segment ReadOnly if given flag is ReadOnly.
### DirectHll4Array
* fixed a bug, the given readOnly parameter was not passed.
### DirectHll6Array
* fixed a bug, the given readOnly parameter was not passed.
### DirectHll8Array
* fixed a bug, the given readOnly parameter was not passed.
### DirectHllArray
* fixed a bug, the given readOnly parameter was not used.
### HllArray
* Added comments to empty blocks.
## hllmap
### Map
* Added comments to empty blocks.
* Added parentheses for clarity
## kll
### KllDoublesSketch
* Removed unnecessary parameter
### KllFloatsSketch
* Removed unnecessary parameter
### KllHeapDoublesSketch
* Added comments to empty blocks.
### KllHeapFloatsSketch
* Added comments to empty blocks.
### KllHeapLongsSketch
* Added comments to empty blocks.
### KllHelper
* remove unused SerDe
### KllItemsHelper
* Added comments to empty blocks.
### KllItemsSketch
* remove unused SerDe
### KllLongsSketch
* remove unused SerDe
### KllPreambleUtil
* remove unused \<T\> qualifier
## (classic) quantiles
### ClassicUtil
* Added method to compute growing base buffer size.
* package-private static method checkIsMemorySegmentCompact(MemorySegment) was returning true if readOnly flag was set or compact flag was set. Changed it to only return true if compact flag is set.
### CompactDoublesSketch
* Added comments to empty blocks.
### DirectCompactDoublesSketch
* Clarified comment
* Added 2nd parameter to accessor wrap() for clarity
### DirectDoublesSketchAccessor
* Changed idxOffset to byteOffset for more clarity
* Used MemorySegment.asSlice() where it made sense.
* Simple Reformating, no code change
### DirectUpdateDoublesSketch
* Refactoring due to removal of DirectUpdateDoublesSketchR
### DoublesBufferAccessor
* Added missing Javadoc
### DoublesMergeImpl
* Added 2nd parameter for clarity
### DoublesSketch
* Cleaned up javadoc
* Added MemorySegmentRequest as 2nd parameter to factory wrap()
* Changed method name for more clarity.
* Improved Javadocs
* Added 2nd parameter to accessor wrap() for clarity
### DoublesSketchAccessor
* Added and improved Javadocs
* Added more code comments
### DoublesSketchIterator
* Added 2nd parameter to accessor wrap() for clarity
### DoublesUnion
* Improved Javadoc
### DoublesUnionImpl
* Refactoring due to removal of DoublesUnionImplR
### DoublesUtil
* Changes due to internal API change
* Removed unnecessary \<T\> qualifier
* minor refactoring
### HeapCompactDoublesSketch
* Added 2nd parameter to accessor wrap() for clarity
### HeapDoublesSketchAccessor
* Minor variable name changes
### HeapUpdateDoublesSketch
* Added code comments
* updated growth method to refer to a common util method
### DirectUpdateDoublesSketchR
* Removed, not needed in the context of FFM
### DoublesUnionImplR
* Removed, not needed in the context of FFM
## req
### ReqCompactor
* Added comments to empty blocks.
## theta
### ConcurrentDirectQuickSelectSketch
* Added comments to empty blocks.
* Added "finals"
* Removed trailing spaces
### ConcurrentHeapQuickSelectSketch
* Added comments to empty blocks.
### ConcurrentHeapThetaBuffer
* Added comments to empty blocks.
* Changes due to removing DirectUpdateDoublesSketchR
### DirectQuickSelectSketch
* Added SuppressWarnings for dummy, but required, variable
## sampling
### EbppsItemsSketch
* Added comments to empty blocks.
## tuple/arrayofdoubles
### DirectArrayOfDoublesQuickSelectSketch
* Added "finals"
* Added SuppressWarnings for dummy, but required, variable
# Test
## (classic) quantiles
### DirectCompactDoublesSketchTest
* Add MemorySegmentRequest for modified API
### DirectUpdateDoublesSketchTest
* Refactoring due to removal of DirectUpdateDoublesSketchR
### DoublesSketchTest
* Added 2nd parameter to accessor wrap() for clarity
* Added 2nd parameter to factory wrap()
### DoublesUnionBuilderTest
* Changes due to internal API change
### DoublesUnionImplTest
* Changes due to internal API change
### DoublesUtilTest
* Changes due to internal API change
### HeapUpdateDoublesSketchTest
* Added 2nd parameter to accessor wrap() for clarity
* Changes due to internal API change
* updated diagnostic println methods
### QuantilesSketchCrossLanguageTest
* Changes due to internal API change
### ReadOnlyMemoryTest
* Changes due to internal API change
--
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-unsubscribe@datasketches.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@datasketches.apache.org
For additional commands, e-mail: dev-help@datasketches.apache.org
Re: [PR] Cleanup phase 1 (datasketches-java)
Posted by "leerho (via GitHub)" <gi...@apache.org>.
leerho commented on code in PR #683:
URL: https://github.com/apache/datasketches-java/pull/683#discussion_r2270560413
##########
src/main/java/org/apache/datasketches/quantiles/HeapUpdateDoublesSketch.java:
##########
@@ -373,6 +375,11 @@ MemorySegment getMemorySegment() {
return null;
}
Review Comment:
This Copilot message conflated two different methods together. It points to the "return null" of getMemorySegment(), then talks about the setReadOnly(). Nonetheless I added javadoc to setReadOnly(). The javadoc of getMemorySegment in the parent class is already sufficient.
--
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-unsubscribe@datasketches.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@datasketches.apache.org
For additional commands, e-mail: dev-help@datasketches.apache.org
Re: [PR] Cleanup phase 1 (datasketches-java)
Posted by "leerho (via GitHub)" <gi...@apache.org>.
leerho commented on code in PR #683:
URL: https://github.com/apache/datasketches-java/pull/683#discussion_r2270560413
##########
src/main/java/org/apache/datasketches/quantiles/HeapUpdateDoublesSketch.java:
##########
@@ -373,6 +375,11 @@ MemorySegment getMemorySegment() {
return null;
}
Review Comment:
This Copilot message conflated two different methods together. Nonetheless I added javadocs to both.
--
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-unsubscribe@datasketches.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@datasketches.apache.org
For additional commands, e-mail: dev-help@datasketches.apache.org
Re: [PR] Cleanup phase 1 (datasketches-java)
Posted by "leerho (via GitHub)" <gi...@apache.org>.
leerho commented on code in PR #683:
URL: https://github.com/apache/datasketches-java/pull/683#discussion_r2274770616
##########
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:
Copilot clearly doesn't understand math or logic, and the suggested change is nonsense!
The code is fine the way it is.
--
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-unsubscribe@datasketches.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@datasketches.apache.org
For additional commands, e-mail: dev-help@datasketches.apache.org
Re: [PR] Cleanup phase 1 (datasketches-java)
Posted by "leerho (via GitHub)" <gi...@apache.org>.
leerho commented on code in PR #683:
URL: https://github.com/apache/datasketches-java/pull/683#discussion_r2270539503
##########
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:
OK, it shortened it a few characters.
--
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-unsubscribe@datasketches.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@datasketches.apache.org
For additional commands, e-mail: dev-help@datasketches.apache.org
Re: [PR] Cleanup phase 1 (datasketches-java)
Posted by "leerho (via GitHub)" <gi...@apache.org>.
leerho commented on code in PR #683:
URL: https://github.com/apache/datasketches-java/pull/683#discussion_r2274752129
##########
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:
Reexamining this code I realized that the setReadOnly was not only confusing, it was unnecessary. So it has been 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: dev-unsubscribe@datasketches.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@datasketches.apache.org
For additional commands, e-mail: dev-help@datasketches.apache.org
Re: [PR] Cleanup phase 1 (datasketches-java)
Posted by "leerho (via GitHub)" <gi...@apache.org>.
leerho merged PR #683:
URL: https://github.com/apache/datasketches-java/pull/683
--
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-unsubscribe@datasketches.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@datasketches.apache.org
For additional commands, e-mail: dev-help@datasketches.apache.org
Re: [PR] Cleanup phase 1 (datasketches-java)
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
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-unsubscribe@datasketches.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@datasketches.apache.org
For additional commands, e-mail: dev-help@datasketches.apache.org