vbhanuchander-lang commented on code in PR #28996:
URL: https://github.com/apache/flink/pull/28996#discussion_r3924294076
##########
flink-python/pyflink/fn_execution/formats/avro.py:
##########
@@ -87,6 +87,13 @@ def read_bytes(self):
assert (nbytes >= 0), nbytes
return self.read(nbytes)
+ def read_decimal_from_bytes(self, precision, scale):
+ # avro's implementation sizes the payload with read_long, which is an
Avro zig-zag long
+ # upstream but a fixed 8-byte long here. On the JVM a bytes-backed
decimal is framed like
+ # any other bytes field, so the size is a 4-byte int.
+ size = self.read_int()
Review Comment:
Good catch — done in `6669023`, essentially as you wrote it. State written
by an earlier version is
exactly the case I had not considered.
I checked the discriminator holds in every case rather than just the common
one, because the whole
scheme rests on a zero length being impossible in the current framing. It
is: `write_decimal_bytes`
computes `bytes_req` from `bit_length() + 1`, so it always writes at least
one byte of unscaled
value — `Decimal("0")` encodes as a single `00`. A payload length of zero
therefore never occurs in
the current framing and can only be the historical 8-byte prefix, whose high
half is always zero for
any real payload.
Verified both directions across every payload width, 1 to 4 bytes (`0.00`,
`12.34`, `-12.34`, `1.28`,
`-1.28`, `999999.99`, `-999999.99`): each decodes correctly from the current
framing and from the
same payload re-framed the old way. Removing the compatibility branch makes
the old-framing cases
fail with `struct.error: unpack requires a buffer of 1 bytes`, so the new
tests do guard it rather
than passing either way.
Two notes on scope:
- The write path is unchanged — it still emits the JVM-compatible 4-byte
prefix, so this is
read-side tolerance only and nothing new is written in the old shape.
- I did not touch `skip_bytes`. It is shared by every bytes field, not just
decimals, and an empty
bytes field is legitimately zero-length there, so the same discriminator
would be wrong. avro 1.12
has no `skip_decimal_from_bytes` for me to override instead. That means a
*skipped* decimal in
old-framing data would still mis-read; if you would rather cover that too,
I think it needs a
decimal-aware skip in the datum reader and I am happy to add it.
`flake8` is clean at the project's 100-column setting.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
