github-actions[bot] commented on code in PR #41060:
URL: https://github.com/apache/beam/pull/41060#discussion_r4498113559
##########
sdks/python/apache_beam/io/gcp/bigquery_tools.py:
##########
@@ -1562,7 +1570,11 @@ def insert_rows(
str(self.unique_row_id) if not insert_ids else insert_ids[i]
for i, _ in enumerate(rows)
]
+ _json_safe = (str, int, bool, type(None))
rows = [
+ r if (
+ type(r) is dict and
+ all(type(v) in _json_safe for v in r.values())) else
Review Comment:
**🟡 Medium**: When `orjson` is installed (the default for
`fast_json_dumps`), integers outside the 64-bit range invoke `default_encoder`
(or raise `TypeError`), whereas bypassing `fast_json_dumps` passes
arbitrary-precision Python `int` objects directly to
`gcp_bigquery.Client.insert_rows_json` (which uses standard `json.dumps` and
serializes >64-bit unquoted integers in the JSON payload). Additionally, if `r`
has non-`str` keys, `json.dumps` / `orjson.dumps` behaves differently than
passing `r` through unchanged. Consider verifying `type(k) is str` as well, or
ensuring keys are strings:
```python
all(type(k) is str and type(v) in _json_safe for k, v in r.items())
```
```suggestion
all(type(k) is str and type(v) in _json_safe for k, v in
r.items())) else
```
##########
sdks/python/apache_beam/io/gcp/bigquery_tools.py:
##########
@@ -1828,8 +1840,11 @@ def read(self, size=-1):
raise io.UnsupportedOperation("AvroRowWriter is not readable")
def tell(self):
- # Flush the fastavro Writer to the underlying stream, otherwise there isn't
- # a reliable way to determine how many bytes have been written.
+ # Include both bytes already flushed to the underlying stream and bytes
+ # currently buffered in the fastavro Writer, avoiding a costly block flush
+ # (and 16-byte sync marker) after every single row.
+ if hasattr(self._avro_writer, 'io'):
Review Comment:
**🟢 Low**: `fastavro.write.Writer` in both the Cython (`_write_py.pyx`) and
pure-Python (`_write_py.py`) implementations exposes `.io` (a `MemoryIO` or
`BytesIO` buffer). However, `AvroRowWriter.tell()` previously had no direct
unit test verifying that multiple writes stay buffered within a single block
rather than flushing after every `tell()` call. Consider adding a test in
`TestAvroRowWriter` that calls `tell()` between `write()` calls and verifies
`tell()` increases monotonically without forcing separate Avro blocks.
--
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]
