Add support for new native types #26
Conversation
There was a problem hiding this comment.
Need to settle on and add a style file
| /** | ||
| * Serialize an Instant as milliseconds since Unix epoch (UTC). | ||
| */ | ||
| public static void serializeTimestamp(Instant value, ImprintBuffer buffer) { |
There was a problem hiding this comment.
@agavra I think we had mentioned that timestamp should be milliseconds and that would be milliseconds since midnight, but Avro does milliseconds since Unix epoch and I think that's what we wanted to do here too which means this would need to be 8 bytes after all. If that's the case then I can go back to the main readme and update it again and clarify - just wanted to double check this again to make sure I wasn't getting myself too confused
There was a problem hiding this comment.
I believe we said that timestamp would be 8 bytes millis since midnight but "time" (which is time of day) would be millis since midnight.
EDIT: I just looked at the README and didn't notice that we changed both to 4 bytes. Good catch, "timestamp" should definitely be 8 bytes. I'll change that in the core README as well.
| long leastSignificantBits = buffer.getLong(); | ||
| return new UUID(mostSignificantBits, leastSignificantBits); | ||
| case DECIMAL: | ||
| int scale = VarInt.decode(buffer).getValue(); |
There was a problem hiding this comment.
should we check buffer.remaining here?
| if (code == TypeCode.DECIMAL.getCode()) { | ||
| var decimal = (BigDecimal) value; | ||
| byte[] unscaledBytes = decimal.unscaledValue().toByteArray(); | ||
| return 2 + 2 + unscaledBytes.length; |
There was a problem hiding this comment.
should we get the exact size of the varint? should probably add test for size computations
There was a problem hiding this comment.
Yeah we can do that, I had been trying to aggressively pare down the estimation cost but I don't think the varint calculation really adds that much anyways here
Add support for types described here - #25