Serde incompatible with C++ TDigest #701

Activity

1. leerho commented on Dec 17, 2025

   

   leerho

   on Dec 17, 2025

   Member

   More actions

   Thank you for pointing this out. We are looking into it.
2. AlexanderSaydakov commented on Dec 17, 2025

   

   AlexanderSaydakov

   on Dec 17, 2025

   Contributor

   More actions

   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.
3. leerho commented on Dec 17, 2025

   

   leerho

   on Dec 17, 2025

   Member

   More actions

   @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?
4. AlexanderSaydakov commented on Dec 17, 2025

   

   AlexanderSaydakov

   on Dec 17, 2025

   Contributor

   More actions

   In the default mode they must be compatible.
   I suggest fixing this by implementing the same approach in Java.
5. tisonkun commented on Dec 17, 2025

   

   tisonkun

   on Dec 17, 2025

   Last edited by tisonkun

   MemberAuthor

   More actions

   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.
6. leerho commented on Dec 19, 2025

   

   leerho

   on Dec 19, 2025

   Member

   More actions

   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?
7. tisonkun commented on Dec 19, 2025

   

   tisonkun

   on Dec 19, 2025

   MemberAuthor

   More actions

   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);
   	}
   	}
   	}
   	}
8. tisonkun commented on Dec 19, 2025

   

   tisonkun

   on Dec 19, 2025

   MemberAuthor

   More actions

   Here is the patch: #704
9. leerho

   closed this as completedin #704on Dec 19, 2025