perf(bigquery): borrow schemas during REST row conversion - #6503
Conversation
Row conversion cloned table field schemas and allocated temporary schema wrappers for every cell, even though conversion only reads schema metadata. Pass borrowed field slices through the recursive conversion instead, which removes a deep `TableFieldSchema` clone, an `Arc`, and a `TableSchema` allocation per cell for scalar, nested and repeated values. Also reserve the cell vector up front, now that the column count is known before the loop runs. Signed-off-by: Fredrik Fornwall <fredrik@fornwall.net>
product-auto-label
Bot
added
the
api: bigquery
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the BigQuery row conversion logic to optimize allocations and simplify the code by passing TableFieldSchema directly to conversion functions instead of reconstructing intermediate Schema objects. The review feedback suggests a further optimization in convert_repeated to pre-allocate the values list using ListValue::with_capacity since the input size is known beforehand.
Signed-off-by: Fredrik Fornwall <fredrik@fornwall.net>
`convert_row` and `convert_repeated` own the cell vector they receive, so they can map it in place with `Value::take()` instead of building a second vector alongside it. This drops one `Vec` allocation per row, per repeated field and per nested record, and subsumes the capacity reservation added earlier. Signed-off-by: Fredrik Fornwall <fredrik@fornwall.net>
alvarowolfx
left a comment
alvarowolfx
left a comment
There was a problem hiding this comment.
Thanks for the improvements @fornwall
We usually try to avoid mut, so I added some minor style suggestions. But I can do it later since I'm doing some work semi-related to this.
|
/gcbrun |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6503 +/- ##
==========================================
- Coverage 96.39% 96.39% -0.01%
==========================================
Files 301 301
Lines 84762 84752 -10
==========================================
- Hits 81710 81699 -11
- Misses 3052 3053 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Cover `convert_value`'s fallback arm, which is only reachable when a cell value is neither null, string, object nor array. Signed-off-by: Fredrik Fornwall <fredrik@fornwall.net>
Just pushed a test hitting that. |
|
/gcbrun |
3 participants
Row conversion cloned table field schemas and allocated temporary schema wrappers for every cell, even though conversion only reads schema metadata.
Pass borrowed field slices through the recursive conversion instead, which removes a deep
TableFieldSchemaclone, anArc, and aTableSchemaallocation per cell for scalar, nested and repeated values.Also reserve the cell vector up front, now that the column count is known before the loop runs.