Skip to content

fix: respect negative values on deserialization in REQ sketch #734

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.

Sign up for GitHub

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

Open
proost wants to merge 6 commits into
base: main
Choose a base branch
from
Open

fix: respect negative values on deserialization in REQ sketch #734

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
e532b7f
fix: respect negative values on deserialization in REQ sketch
proost Apr 25, 2026
6a514db
test: add cases from C++ deserialize
proost Apr 25, 2026
8535219
test: remove c++ compat test cases
proost Apr 25, 2026
d67731a
fix: init with NaN
proost May 13, 2026
be4646f
fix: init with Inf
proost May 15, 2026
15354ff
fix: remove guard
proost May 15, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/main/java/org/apache/datasketches/req/ReqSerDe.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -205,7 +205,7 @@ static final Compactor extractCompactor(final PositionalSegment posSeg, final bo
final float[] arr = new float[count];
posSeg.getFloatArray(arr, 0, count);
float minItem = Float.MAX_VALUE;
float maxItem = Float.MIN_VALUE;
float maxItem = -Float.MAX_VALUE;
Copy link
Copy Markdown
Member

@leerho leerho May 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What you found here is obviously incorrect, but neither is your solution. Internally, for the Classic Quantiles, KLL and REQ, the min and max values are initialized to NaN. and the first incoming value that is not NaN, sets both the min and max values. This allows input values of +/- infinity, which your solution would not recognize. I would argue that you should use a similar solution here, for consistency if nothing else.

I would also suggest you create a test that uses +/- Infinities, just to confirm that it works.

Copy link
Copy Markdown
Member Author

@proost proost May 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

d67731a

Oh, I was wrong. I changed like other sketches or Update method.

And also add Infinity cases

for (int i = 0; i < count; i++) {
minItem = min(minItem, arr[i]);
maxItem = max(maxItem, arr[i]);
Expand Down
43 changes: 43 additions & 0 deletions src/test/java/org/apache/datasketches/req/ReqCompactorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,4 +101,47 @@ private static void checkSerDeImpl(final int k, final boolean hra) {
assertEquals(fbuf2.getDelta(), delta);
assertTrue(fbuf.isEqualTo(fbuf2));
}

@Test
public void checkSerDeWithNegativeValues() {
checkSerDeNegativeImpl(12, false);
checkSerDeNegativeImpl(12, true);
}

private static void checkSerDeNegativeImpl(final int k, final boolean hra) {
final ReqCompactor c1 = new ReqCompactor((byte)0, hra, k, null);
final int nomCap = 2 * 3 * k;
final FloatBuffer fbuf = c1.getBuffer();

for (int i = 1; i <= nomCap; i++) {
fbuf.append(-i); //all negative values
}
final byte[] c1ser = c1.toByteArray();
final PositionalSegment posSeg = PositionalSegment.wrap(MemorySegment.ofArray(c1ser));
final Compactor compactor = ReqSerDe.extractCompactor(posSeg, fbuf.isSorted(), hra);
assertEquals(compactor.minItem, -nomCap);
assertEquals(compactor.maxItem, -1f);
}

@Test
public void checkSerDeWithMixedValues() {
checkSerDeMixedImpl(12, false);
checkSerDeMixedImpl(12, true);
}

private static void checkSerDeMixedImpl(final int k, final boolean hra) {
final ReqCompactor c1 = new ReqCompactor((byte)0, hra, k, null);
final int nomCap = 2 * 3 * k;
final int half = nomCap / 2;
final FloatBuffer fbuf = c1.getBuffer();

for (int i = 0; i < nomCap; i++) {
fbuf.append(i - half); // range: -half to half-1
}
final byte[] c1ser = c1.toByteArray();
final PositionalSegment posSeg = PositionalSegment.wrap(MemorySegment.ofArray(c1ser));
final Compactor compactor = ReqSerDe.extractCompactor(posSeg, fbuf.isSorted(), hra);
assertEquals(compactor.minItem, (float) -half);
assertEquals(compactor.maxItem, (float) (half - 1));
}
}
25 changes: 24 additions & 1 deletion src/test/java/org/apache/datasketches/req/ReqSketchCrossLanguageTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,30 @@ public void generateBinariesForCompatibilityTesting() throws IOException {
}
}

@Test(groups = {GENERATE_JAVA_FILES})
public void generateNegativeBinariesForCompatibilityTesting() throws IOException {
final int[] nArr = {1, 10};
for (final int n: nArr) {
final ReqSketch sk = ReqSketch.builder().build();
for (int i = 1; i <= n; i++) {
sk.update(-i);
}
putBytesToJavaPath("req_float_negative_n" + n + "_java.sk", sk.toByteArray());
}
}

@Test(groups = {GENERATE_JAVA_FILES})
public void generateMixedBinariesForCompatibilityTesting() throws IOException {
final int[] nArr = {1, 10};
for (final int n: nArr) {
final ReqSketch sk = ReqSketch.builder().build();
for (int i = -n; i <= n; i++) {
sk.update(i);
}
putBytesToJavaPath("req_float_mixed_n" + n + "_java.sk", sk.toByteArray());
}
}

@Test(groups = {CHECK_CPP_FILES})
public void deserializeFromCpp() throws IOException {
final int[] nArr = {0, 1, 10, 100, 1000, 10000, 100000, 1000000};
Expand All @@ -74,5 +98,4 @@ public void deserializeFromCpp() throws IOException {
}
}
}

}
Loading