-
Notifications
You must be signed in to change notification settings - Fork 14.5k
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.
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
chore(bigquery): Add extra logging for BigQuery exceptions so we can have better insight on exceptions #22024
Merged
+60
−1
Merged
Changes from all commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -244,3 +244,44 @@ def test_mask_encrypted_extra_when_empty() -> None: | |
from superset.db_engine_specs.bigquery import BigQueryEngineSpec | ||
|
||
assert BigQueryEngineSpec.mask_encrypted_extra(None) is None | ||
|
||
|
||
def test_parse_error_message() -> None: | ||
""" | ||
Test that we parse a received message and just extract the useful information. | ||
|
||
Example errors: | ||
bigquery error: 400 Table \"case_detail_all_suites\" must be qualified with a dataset (e.g. dataset.table). | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. make all the test work with this example There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Updated 👍 |
||
|
||
(job ID: ddf30b05-44e8-4fbf-aa29-40bfccaed886) | ||
-----Query Job SQL Follows----- | ||
| . | . | . |\n 1:select * from case_detail_all_suites\n 2:LIMIT 1001\n | . | . | . | | ||
""" | ||
from superset.db_engine_specs.bigquery import BigQueryEngineSpec | ||
|
||
message = 'bigquery error: 400 Table "case_detail_all_suites" must be qualified with a dataset (e.g. dataset.table).\n\n(job ID: ddf30b05-44e8-4fbf-aa29-40bfccaed886)\n\n -----Query Job SQL Follows----- \n\n | . | . | . |\n 1:select * from case_detail_all_suites\n 2:LIMIT 1001\n | . | . | . |' | ||
expected_result = '400 Table "case_detail_all_suites" must be qualified with a dataset (e.g. dataset.table).' | ||
assert ( | ||
str(BigQueryEngineSpec.parse_error_exception(Exception(message))) | ||
== expected_result | ||
) | ||
|
||
|
||
def test_parse_error_raises_exception() -> None: | ||
""" | ||
Test that we handle any exception we might get from calling the parse_error_exception method. | ||
|
||
Example errors: | ||
400 Syntax error: Expected "(" or keyword UNNEST but got "@" at [4:80] | ||
bigquery error: 400 Table \"case_detail_all_suites\" must be qualified with a dataset (e.g. dataset.table). | ||
""" | ||
from superset.db_engine_specs.bigquery import BigQueryEngineSpec | ||
|
||
message = 'bigquery error: 400 Table "case_detail_all_suites" must be qualified with a dataset (e.g. dataset.table).' | ||
message_2 = "6" | ||
expected_result = '400 Table "case_detail_all_suites" must be qualified with a dataset (e.g. dataset.table).' | ||
assert ( | ||
str(BigQueryEngineSpec.parse_error_exception(Exception(message))) | ||
== expected_result | ||
) | ||
assert str(BigQueryEngineSpec.parse_error_exception(Exception(message_2))) == "6" |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried to get this to raise an exception and wasn't able to. I believe as long as the return value is a string, you should be able to perform all of the operations and don't need to put it in a try catch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm if for some unknown reason the Exception has no
:
for example, accessing the [1] would cause a:IndexError: list index out of range
. Just adding the try/catch there in case anything like that happens with the message we get.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, ok I tried it with no new line. That makes sense.