-
Notifications
You must be signed in to change notification settings - Fork 216
Cleanup phase2 #684
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Cleanup phase2 #684
Conversation
Add some missing methods.
Added storage layout documentation for the compact compressed sketch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR implements cleanup changes for the phase2 effort, upgrading Java version to 24 and improving code quality across the DataSketches library. The changes include workflow fixes, API improvements, and comprehensive code cleanup.
Key changes:
- Upgraded all GitHub Actions workflows to use Java 24 and re-enabled push triggers
- Refactored DoublesSketch API to clearly separate compact (read-only) from updatable sketches
- Fixed binary compatibility with C++ by setting P field to zero for compact sketches
- Enhanced documentation including new compressed layout documentation and improved Javadocs
Reviewed Changes
Copilot reviewed 22 out of 22 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| .github/workflows/*.yml | Updated Java version to 24 and re-enabled push triggers |
| pom.xml | Fixed maven-javadoc-plugin configuration |
| src/main/java/org/apache/datasketches/quantiles/DoublesSketch.java | Refactored API to separate wrap() for compact sketches and writableWrap() for updatable sketches |
| src/main/java/org/apache/datasketches/quantiles/UpdateDoublesSketch.java | Enhanced Javadoc documentation for MemorySegmentRequest handling |
| src/main/java/org/apache/datasketches/quantiles/DoublesUnionImpl.java | Added logic to handle both compact and updatable segments |
| src/main/java/org/apache/datasketches/theta/CompactOperations.java | Changed P field to 0.0 for C++ compatibility |
| src/main/java/org/apache/datasketches/theta/PreambleUtil.java | Added comprehensive documentation for compressed layout |
| test files | Updated method calls to use new API (wrap vs writableWrap) |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
NOTES
The main repo is being targeted for the upcoming Java 25 LTS release, which is due out next month (Sep). Currently, however, this code is only being compiled under Java 24 in order to get work done. I do not anticipate any changes being required in the move to Java 25.
The big change from the previous release (8.0.0 using Java 21) is the migration of all code that depended on datasketches-memory to FFM (Panama). This was completed with previous PRs. However, the change to FFM has created the opportunity to refactor/remove some classes that primarily existed for DS-memory, and to carefully go through the entire code base reviewing javadocs, code comments, and improving overall code completeness and quality.
So I'm using the time between now and when Java 25 is available to do just that. If anyone one else wants to help out with this effort, you are more than welcome :)
Summary of changes
File by File change comments
ROOT DIRECTORY
.GITHUB/WORKFLOWS
auto-jdk-matrix.yml
auto-os-jdk-matrix.yml
check_cpp_files.yml
codeql-analysis.yml
javadoc.yml
pom.xml
MAIN TREE
COMMON
Util.java
HLL
package-info.java
(classic) QUANTILES
DirectCompactDoublesSketch.java
DirectUpdateDoublesSketch.java
DoublesSketch.java
DoublesUnionImpl.java
UpdateDoublesSketch.java
THETA
CompactOperations
PreambleUtil
TEST TREE
(classic) QUANTILES
DirectCompactDoublesSketchTest.java
DirectUpdateDoublesSketchTest.java
DoublesMiscTest.java
DoublesSketchTest.java
DoublesUnionImplTest.java
DoublesUtilTest.java
QuantilesSketchCrossLanguageTest.java