EstimateSketchUDF isn't processing BINARY fields correctly

[apache]

EstimateSketchUDF isn't processing BINARY fields correctly

[leerho]

@koke
@jmalkin
DataSketches-hive 1.1.0-incubating has been Released!

Please goto Downloads

[jmalkin]

This is a bug in lots of places, as noted in the PR. I'll start working to fix them all (that might take a bit but I'll try to get it done soon).

Wrapping the larger buffer will be harmless in most cases. The specific issue here is that there was an older version of the specialized SingleItemSketch that didn't include a flag indicating that it followed the slightly different single item format. To handle that problem, the only workaround that could be identified was to use the buffer length. In general, the images shouldn't need to rely on buffer size (other than ensuring it's not too small) when being read.

And checks against size for the empty sketch scenario should be a performance optimization, but work properly if parsing an actual empty sketch later. But we should fix this properly anyway.

[jmalkin]

My PR replaces all calls to getBytes() in the repo (aside from in the new code to wrap BytesWritable in a Memory). Had to copy bytes in a few places, but I was mostly able to just wrap using getLength() bytes. Just need it reviewed now.

[jmalkin]

@leerho Please revert that change and apply the PR that completely addresses the issue.

[leerho]

Done.

On Mon, Jun 1, 2020 at 8:52 PM Jon Malkin ***@***.***> wrote: @leerho <https://github.com/leerho> Please revert that change and apply the PR that completely addresses the issue. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#50 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADCXRQU2TY3H5XZGJON4MKLRURZO7ANCNFSM4NN5LCNQ> .

-- From my cell phone.

[jmalkin]

Thanks again @koke for finding this. We did add your unit test, while fixing the underlying issue everywhere in the repo so that future changes in other sketches won't risk triggering this problem again.

[koke]

Is there a plan to do a new release with this? I just hit a new issue with the IntersectSketchUDF and had to replicate the workaround for that one