I wonder why Memory.wrap(..) and WritableMemory.wrap(..) don't handle this conversion instead of forcing all systems calling those methods?
also, this is kind of error prone in case we miss calling wrap via LittleEndianWrapper and call Memory.wrap(..) directly in some place.

I see some direct calls to Memory.wrap() still remain, like in ArrayOfDoublesSketch.deserializeFromByteArray, do all call sites need to go through the LittleEndianWrapper?

Also, would it be simpler to use public static Memory wrap(final ByteBuffer byteBuf, final ByteOrder byteOrder) here?

This is about wrapping a ByteBuffer. New version of the memory library handles the byte order of the incoming ByteBuffer correctly (before it was simplistic and forgiving). But Druid sometimes does not set the byte order correctly. So now it is necessary to force the byte order of sketches to be little endian. And it must be done externally to the memory library because it is really about Druid not setting it correctly.

would it be simpler to use public static Memory wrap(final ByteBuffer byteBuf, final ByteOrder byteOrder) here?

This is a good point. I will think about it.

I see some direct calls to Memory.wrap() still remain

They still remain where the input is not a ByteBuffer

so, are we saying that sketch objects are "always" serialized in little endian and when druid loads those bytes in the ByteBuffer, the order is big endian (jvm default) and that needs to be corrected since memory doesn't know that it is getting a sketch object and wouldn't be able to correctly handle the contents otherwise?

Please add comments to these methods on why the wrapping is necessary and why it's only needed for ByteByffer and not byte[]

we call union.update(..) in updateUnion(..) method defined later in the same file which is prone to failure without SynchronizedUnion.
Also, I wouldn't want to put synchronization on that method itself as it gets called in other situations where synchronization isn't necessary.
So, It probably still makes sense to keep SynchronizedUnion class around.

I don't see how synchronizing on Union objects would be avoided in those other situations when synchronization is not necessary.
Could you clarify what is error-prone? We obtain exclusive locks in aggregate() and get() in case they are called at the same time.

It probably still makes sense to keep SynchronizedUnion class around

Given the new API, it is impossible to make SynchronizedUnion a subclass of Union as it was before. But making it just a wrapper breaks a lot of other code, so it was way easier to get rid of it altogether.

you're right , Now I notice that updateUnion(..) is also called from synchronized block. nvmnd.

can you please explain this change? given that everything is backwards compatible why did this have to change?

This was a mistake in the test. The number of values in the union and incoming sketches must match, which was not enforced before. Correct code should not be affected.

ok