Skip to content
New issue

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

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

Already on GitHub? # to your account

fix(bigquery): Properly display errors for BigQuery DBs #22349

Merged
merged 1 commit into from
Dec 7, 2022

Conversation

Antonio-RiveroMartnez
Copy link
Member

SUMMARY

This PR is fixing the wrong error messages we are seeing with BigQuery after we introduced these changes: #22024 .

Initially we were trying to get rid of bigquery error: section in the message, so we decided to split the exception message. However, the engine is NOT adding such section into the message, instead it's just returning: 400 Syntax error: ... for example, and it's us the ones adding that in this method: extract_error_message from the BaseEngineSpec class.

So, these proposed changes are not splitting the message, just getting the first line (if multiple), and we keep the bigquery error: section in it. We could also override the extract_error_message in BigQueryEngineSpec to remove that section, however, I'd like to be consistent with all our other engines.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Before:
Screen Shot 2022-12-06 at 19 24 19

After:
Screen Shot 2022-12-06 at 19 22 03

TESTING INSTRUCTIONS

  1. Open SQL Lab and force a Syntax error when querying a BigQuery DB
  2. Error message should display properly

ADDITIONAL INFORMATION

  • Has associated issue:
  • Required feature flags:
  • Changes UI
  • Includes DB Migration (follow approval process in SIP-59)
    • Migration is atomic, supports rollback & is backwards-compatible
    • Confirm DB migration upgrade and downgrade tested
    • Runtime estimates and downtime expectations provided
  • Introduces new feature or API
  • Removes existing feature or API

- Fix the way we truncate exception messages
- Update our tests
@codecov
Copy link

codecov bot commented Dec 6, 2022

Codecov Report

Merging #22349 (fbfe33e) into master (d2b76a8) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master   #22349      +/-   ##
==========================================
- Coverage   66.85%   66.85%   -0.01%     
==========================================
  Files        1847     1847              
  Lines       70560    70560              
  Branches     7737     7737              
==========================================
- Hits        47173    47171       -2     
- Misses      21380    21382       +2     
  Partials     2007     2007              
Flag Coverage Δ
hive 52.53% <0.00%> (ø)
mysql 77.95% <0.00%> (ø)
postgres 78.02% <0.00%> (ø)
presto 52.42% <0.00%> (ø)
python 81.23% <100.00%> (-0.01%) ⬇️
sqlite 76.48% <0.00%> (ø)
unit 50.91% <100.00%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
superset/db_engine_specs/bigquery.py 81.68% <100.00%> (-1.00%) ⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@hughhhh hughhhh merged commit 60a617e into apache:master Dec 7, 2022
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 2.1.0 and removed 🚢 2.1.3 labels Mar 13, 2024
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels size/S 🚢 2.1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants