damccorm commented on code in PR #41060:
URL: https://github.com/apache/beam/pull/41060#discussion_r4531167695
##########
sdks/python/apache_beam/io/gcp/bigquery_tools.py:
##########
@@ -1562,9 +1570,15 @@ def insert_rows(
str(self.unique_row_id) if not insert_ids else insert_ids[i]
for i, _ in enumerate(rows)
]
+ _json_safe = (str, bool, type(None))
rows = [
- fast_json_loads(fast_json_dumps(r, default=default_encoder))
- for r in rows
+ r if (
Review Comment:
> So any row with a FLOAT, RECORD or REPEATED column gets slower, and so do
rows of short ints and strings. What row was the description's 5.52x measured
on, and was orjson installed? Its 116.9 ms for 30,000 rows is 3.9 µs per row on
master, and I only get near that with orjson for rows of roughly 1 kB strings.
If the target is string-heavy rows, is the slowdown on the other shapes
acceptable?
Yeah, the microbenchmark here is admittedly with a favorable happy path
comparison using a mix of strings/ints/bools. It is unsurprising that anything
off the happy path is paying a penalty since it is falling back to the same as
previous. I am surprised that pure int dicts did worse.
As to whether its worth it, my read is probably still yes. In the worst case
here, we are paying a fixed cost per row of `O(length of dict)`. In the best
case, we are saving cost of roughly `O(longest string length x num comparable
strings)`. I expect the former to mostly be a rounding error when
`_insert_all_rows` gets called, but the latter could more meaningfully impact
things.
With that said, I am comfortable cutting it if you are not comfortable
taking it. The other wins in the PR are certainly clearer.
--
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]
