Skip to content

Follow up fix for merging errors from batch BigQuery inserts (#1348). #1350

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Dor1s merged 2 commits into from
Jan 17, 2020
Merged

Follow up fix for merging errors from batch BigQuery inserts (#1348). #1350

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
050f268
Follow up fix for merging errors from batch BigQuery inserts (#1348).
Dor1s Jan 17, 2020
c3b9ba6
fix pylint false positive
Dor1s Jan 17, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 12 additions & 2 deletions src/python/google_cloud_utils/big_query.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -370,7 +370,7 @@ def insert(self, inserts):
A json explained here:
https://cloud.google.com/bigquery/docs/reference/rest/v2/tabledata/insertAll
"""
result = None
result = {}
for i in range(0, len(inserts), INSERT_BATCH_SIZE):
response = self._insert_batch(
inserts[i:min(len(inserts), i + INSERT_BATCH_SIZE)])
Expand All @@ -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.

@inferno-chromium inferno-chromium Jan 17, 2020

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why we don't capture result (non-errors) from other parts of query response. This seems weird. @oliverchang for review as well.

@Dor1s Dor1s Jan 17, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There isn't anything else useful as per the documentation: https://cloud.google.com/bigquery/docs/reference/rest/v2/tabledata/insertAll

@oliverchang oliverchang Jan 19, 2020

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed that there is nothing much else in the returned values.

result = response
else:
result['insertErrors'].extend(response['insertErrors'])
# If there are new errors from the current batch, append to the result.

@oliverchang oliverchang Jan 19, 2020

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@Dor1s Dor1s Jan 21, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks, done!

new_errors = response.get('insertErrors')
if not new_errors:
continue

# Apparently result may not have errors, be careful.
if result.get('insertErrors'):

@oliverchang oliverchang Jan 19, 2020

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: we can do this much more concisely like so:

result.setdefault('insertErrors', []).extend(new_errors)

@Dor1s Dor1s Jan 21, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

beautiful, done!

result['insertErrors'].extend(new_errors)
else:
result['insertErrors'] = new_errors

return result