Serde incompatible with C++ TDigest

[tisonkun]

In Java's TDigestDouble, this field is always ignored as unused.

datasketches-java/src/main/java/org/apache/datasketches/tdigest/TDigestDouble.java

Line 429 in 37edd8c

posSeg.getInt(); // unused

However, C++'s impl can use it for denote the size of buffer vector.

https://github.com/apache/datasketches-cpp/blob/7bb979d3ef8929e235bcd22d67579e1f695f6ecd/tdigest/include/tdigest_impl.hpp#L321-L326

Also the Java impl always assumes there is no buffer serialized. This can lead to serde incompatible for any C++ TDigest has buffered data serialized with with_buffer, like:

https://github.com/apache/datasketches-cpp/blob/7bb979d3ef8929e235bcd22d67579e1f695f6ecd/tdigest/test/tdigest_test.cpp#L222-L235

[leerho]

Thank you for pointing this out. We are looking into it.

[AlexanderSaydakov]

Apparently, at some point we implemented serializing the buffer in C++, but did not do that in Java.
The default is to compress first, which is an undesirable side effect of serialization, so that we don't have to serialize the buffer (which takes more space). I don't remember why did we decide to provide this option to avoid compression and serializing the buffer.

[leerho]

@AlexanderSaydakov,
As @tisonkun points out, this leads to an immediate incompatibility between C++ and Java serialized TDigest sketches.
How do you suggest we fix this?

[AlexanderSaydakov]

In the default mode they must be compatible.
I suggest fixing this by implementing the same approach in Java.

[tisonkun]

Maybe keep it compatible only with the deserializing part? (As I did in apache/datasketches-rust#23).

I don't remember why did we decide to provide this option to avoid compression and serializing the buffer.

Yes, IMO always compress before serializing in this case helps.

[leerho]

Can we agree that the path for Rust is to compress before serializing, which means follow the Java version for this behavior.

If so, can we close?

[tisonkun]

Can we agree that the path for Rust is to compress before serializing, which means follow the Java version for this behavior.

This is correct.

If so, can we close?

The issue here is that C++ can serialize a TDigest sketch with_buffer. So Java's deserialization (heapify) should not assume no buffer in the bytes, which causes the incompatibility. I can submit a patch to demonstrate it and fix it.

There are even prepared test data where we test only those that compress before serializing, not the tdigest_double_buf_n{scale}_cpp.sk ones.

datasketches-java/src/test/java/org/apache/datasketches/tdigest/TDigestCrossLanguageTest.java

Lines 37 to 79 in 37edd8c

	@Test(groups = {CHECK_CPP_FILES})
	public void deserializeFromCppDouble() throws IOException {
	final int[] nArr = {0, 1, 10, 100, 1000, 10_000, 100_000, 1_000_000};
	for (final int n: nArr) {
	final byte[] bytes = Files.readAllBytes(cppPath.resolve("tdigest_double_n" + n + "_cpp.sk"));
	final TDigestDouble td = TDigestDouble.heapify(MemorySegment.ofArray(bytes));
	assertTrue(n == 0 ? td.isEmpty() : !td.isEmpty());
	assertEquals(td.getTotalWeight(), n);
	if (n > 0) {
	assertEquals(td.getMinValue(), 1);
	assertEquals(td.getMaxValue(), n);
	assertEquals(td.getRank(0), 0);
	assertEquals(td.getRank(n + 1), 1);
	if (n == 1) {
	assertEquals(td.getRank(n), 0.5);
	} else {
	assertEquals(td.getRank(n / 2), 0.5, 0.05);
	}
	}
	}
	}
	@Test(groups = {CHECK_CPP_FILES})
	public void deserializeFromCppFloat() throws IOException {
	final int[] nArr = {0, 1, 10, 100, 1000, 10_000, 100_000, 1_000_000};
	for (final int n: nArr) {
	final byte[] bytes = Files.readAllBytes(cppPath.resolve("tdigest_float_n" + n + "_cpp.sk"));
	final TDigestDouble td = TDigestDouble.heapify(MemorySegment.ofArray(bytes), true);
	assertTrue(n == 0 ? td.isEmpty() : !td.isEmpty());
	assertEquals(td.getTotalWeight(), n);
	if (n > 0) {
	assertEquals(td.getMinValue(), 1);
	assertEquals(td.getMaxValue(), n);
	assertEquals(td.getRank(0), 0);
	assertEquals(td.getRank(n + 1), 1);
	if (n == 1) {
	assertEquals(td.getRank(n), 0.5);
	} else {
	assertEquals(td.getRank(n / 2), 0.5, 0.05);
	}
	}
	}
	}

[tisonkun]

Here is the patch: #704