aviramha/ormsgpack

BIGINT passthrough semantics

pejter opened this issue · 5 comments

The new BIGINT_PASSTHROUGH in 1.1.0 seems to be a bit confusing.
Originally I thought it uses passthrough for the ints ormsgpack simply rejected before but it turns out that it also passes through u64 which previously was efficiently converted to the uint64 msgpack format.
I think the library should use msgpack formats if possible and the passthrough should extend that functionality instead of changing it.
So I propose that passthrough would happen only if the int can't fit into either int64 or uint64, which means it can't be converted to a native msgpack type and needs to be encoded.

I understand your opinion. @TariqTNO any objection?

I have no objections on this. I didn't do this previously it was slightly more efficient to do the passthrough like this as it can be checked in the type determination already and kept the int conversion a bit more simple. But then again, I've got no objections to the suggested changes.

Great. @pejter do you mind making a PR?

AFAIK there is no way to get the information about the compatibility with u64 ahead of time like there is for i64 with PyLong_NumBits, so we'd need to

  1. attempt conversion
  2. check for errors
  3. if errors happened call the default handler

As @TariqTNO said this won't be very efficient, but calling into python isn't efficient to begin with so using native msgpack types might still be faster than calling the default function and serialising whatever that produces. So for the range of i64::MAX to u64::MAX we might still be faster then the current behaviour, which I would consider a win.
I'm also not a wizard in either rust or cpython, so I don't know how long it'll take me and if it'll be any good.

Turns out I'm dumb and misread the cpython source code. It was actually a trivial change.