Follow up fix for merging errors from batch BigQuery inserts (#1348). #1350
Conversation
|
/gcbrun |
| if result.get('insertErrors'): | ||
| result['insertErrors'].extend(new_errors) | ||
| else: | ||
| result = response |
There was a problem hiding this comment.
Why do we overwrite the result here ?
There was a problem hiding this comment.
great question, linter complained when I tried to assign only that field:
| E:394,10: 'result' does not support item assignment (unsupported-assignment-operation)
There was a problem hiding this comment.
let me give a try to upgrading it
There was a problem hiding this comment.
doesn't seem like we can use the version that has it fixed (2.3.0: pylint-dev/pylint#2767), but I've used {} instead of None and now it's happy
| @@ -379,8 +379,18 @@ def insert(self, inserts): | |||
| time.sleep(1) | |||
|
|
|||
| if not result: | |||
| # Use result from the first batch, appending errors from the rest. | |||
There was a problem hiding this comment.
Why we don't capture result (non-errors) from other parts of query response. This seems weird. @oliverchang for review as well.
There was a problem hiding this comment.
There isn't anything else useful as per the documentation: https://cloud.google.com/bigquery/docs/reference/rest/v2/tabledata/insertAll
There was a problem hiding this comment.
Agreed that there is nothing much else in the returned values.
|
/gcbrun |
There was a problem hiding this comment.
Landing this for now to ((hopefully) unblock the cron job. If @oliverchang knows a better solution, I'll be happy to revert these two fixes next week.
| continue | ||
|
|
||
| # Apparently result may not have errors, be careful. | ||
| if result.get('insertErrors'): |
There was a problem hiding this comment.
nit: we can do this much more concisely like so:
result.setdefault('insertErrors', []).extend(new_errors)| result = response | ||
| else: | ||
| result['insertErrors'].extend(response['insertErrors']) | ||
| # If there are new errors from the current batch, append to the result. |
There was a problem hiding this comment.
I feel the else makes the flow of the logic a bit confusing here. We can probably just continue after line 383 and unindent the rest.
| @@ -379,8 +379,18 @@ def insert(self, inserts): | |||
| time.sleep(1) | |||
|
|
|||
| if not result: | |||
| # Use result from the first batch, appending errors from the rest. | |||
There was a problem hiding this comment.
Agreed that there is nothing much else in the returned values.
| continue | ||
|
|
||
| # Apparently result may not have errors, be careful. | ||
| if result.get('insertErrors'): |
| result = response | ||
| else: | ||
| result['insertErrors'].extend(response['insertErrors']) | ||
| # If there are new errors from the current batch, append to the result. |
Follow up! Looks like either documentation or my reading of it is not right :)