Skip to content
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

Jump to bottom

Adds error checking for whether big query dataset exists #1319

Merged
merged 2 commits into from
May 16, 2023
Merged

Adds error checking for whether big query dataset exists #1319

merged 2 commits into from
May 16, 2023

Conversation

saurav-c
Copy link
Contributor

@saurav-c saurav-c commented May 15, 2023 •
edited
Loading

Describe your changes and why you are making these changes

This PR adds error checking to the BigQuery executor to see if the dataset exists before trying to save an artifact to it.

Related issue number (if any)

ENG 2947

Loom demo (if any)

Screenshot 2023-05-15 at 11 19 24 AM

Checklist before requesting a review

  • I have created a descriptive PR title. The PR title should complete the sentence "This PR...".
  • I have performed a self-review of my code.
  • I have included a small demo of the changes. For the UI, this would be a screenshot or a Loom video.
  • If this is a new feature, I have added unit tests and integration tests.
  • I have run the integration tests locally and they are passing.
  • I have run the linter script locally (See python3 scripts/run_linters.py -h for usage).
  • All features on the UI continue to work correctly.
  • Added one of the following CI labels:
    • run_integration_test: Runs integration tests
    • skip_integration_test: Skips integration tests (Should be used when changes are ONLY documentation/UI)

@saurav-c saurav-c requested review from cw75 and likawind May 15, 2023 15:19
@saurav-c saurav-c added the run_integration_test Triggers integration tests label May 15, 2023
vsreekanti
vsreekanti approved these changes May 16, 2023
View reviewed changes
Copy link
Contributor

@vsreekanti vsreekanti left a comment

Choose a reason for hiding this comment

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

LGTM. One small nit and one small clarification in line.

Might be worth adding a test? Can probably leave this as a backlog task.

src/python/aqueduct_executor/operators/connectors/data/bigquery.py
@@ -48,6 +49,22 @@ def load(self, params: load.RelationalParams, df: Any, artifact_type: ArtifactTy
if artifact_type != ArtifactType.TABLE:
raise Exception("The data being loaded must be of type table, found %s" % artifact_type)

parts = params.table.split(".")
Copy link
Contributor

Choose a reason for hiding this comment

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

Might be worth documenting why we're splitting on "." and what the expected format is? Just in case BigQuery updates its API in the future and this breaks.

src/python/aqueduct_executor/operators/connectors/data/bigquery.py
# Check if dataset actually exists
dataset_id = parts[0]
try:
self.client.get_dataset(dataset_id)
Copy link
Contributor

Choose a reason for hiding this comment

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

What does the get_dataset API do? Does it return any data or just metadata?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's just metadata. Its a reference to the dataset specified.

@saurav-c saurav-c added skip_integration_test Skips required integration test (only documentation/UI changes) and removed run_integration_test Triggers integration tests labels May 16, 2023
@saurav-c saurav-c force-pushed the eng-2947-add-error-check-to-big-query-operator branch from b2cdbd7 to 8c2b91c Compare May 16, 2023 16:13
@saurav-c
Copy link
Contributor Author

Backlog issue for testing created here: https://linear.app/aqueducthq/issue/ENG-2983/add-test-case-for-accessing-bigquery-dataset-that-does-not-exist

@saurav-c saurav-c merged commit c335fd5 into main May 16, 2023
@saurav-c saurav-c deleted the eng-2947-add-error-check-to-big-query-operator branch May 16, 2023 16:15
likawind added a commit that referenced this pull request May 17, 2023
@likawind
Squashed commit of the following:
4196cf7 
commit e1fd9d7
Merge: 370665e 2fad420
Author: Wei Chen <[email protected]>
Date:   Wed May 17 14:19:11 2023 -0700

    Merge branch 'eng-2699-m1-refactor-wf-detail-pages-to-use-new' into eng-2699-m1-refactor-wf-detail-pages-to-use-new-2

commit 2fad420
Merge: b4485f4 676e81a
Author: Wei Chen <[email protected]>
Date:   Wed May 17 14:06:26 2023 -0700

    Merge branch 'eng-2699-m1-refactor-wf-detail-pages-to-use-new' of https://github.com/aqueducthq/aqueduct into eng-2699-m1-refactor-wf-detail-pages-to-use-new

commit b4485f4
Author: Wei Chen <[email protected]>
Date:   Mon Apr 24 15:43:51 2023 -0700

    rebase

commit d3a2669
Merge: e60ac09 18011cb
Author: Wei Chen <[email protected]>
Date:   Wed May 17 14:04:32 2023 -0700

    Merge branch 'eng-2712-m1-migrate-wf-edit-trigger-endpoints-to' into eng-2943-investigate-result-content-route-tests

commit 18011cb
Merge: ec515cf 8cb356c
Author: Wei Chen <[email protected]>
Date:   Wed May 17 14:02:47 2023 -0700

    Merge branch 'eng-2847-m1-implement-wfiddags-endpoint-with' into eng-2712-m1-migrate-wf-edit-trigger-endpoints-to

commit 8cb356c
Merge: ec94379 b3946a4
Author: Wei Chen <[email protected]>
Date:   Wed May 17 14:02:23 2023 -0700

    Merge branch 'main' of https://github.com/aqueducthq/aqueduct into eng-2847-m1-implement-wfiddags-endpoint-with

commit 676e81a
Author: Wei Chen <[email protected]>
Date:   Mon Apr 24 15:43:51 2023 -0700

    rebase

commit ec94379
Author: Wei Chen <[email protected]>
Date:   Wed May 17 13:28:38 2023 -0700

    comments

commit b3946a4
Author: kenxu95 <[email protected]>
Date:   Wed May 17 10:19:11 2023 -0700

    Move any overflow rows on the details header fields into an additional column (#1322)

commit e60ac09
Author: Wei Chen <[email protected]>
Date:   Tue May 16 20:04:49 2023 -0700

    fix

commit ec515cf
Merge: 1d0482a f81c065
Author: Wei Chen <[email protected]>
Date:   Tue May 16 17:37:04 2023 -0700

    Merge branch 'eng-2847-m1-implement-wfiddags-endpoint-with' into eng-2712-m1-migrate-wf-edit-trigger-endpoints-to

commit f81c065
Author: Wei Chen <[email protected]>
Date:   Tue May 16 17:34:45 2023 -0700

    lint

commit 1d0482a
Author: Wei Chen <[email protected]>
Date:   Tue May 16 17:29:44 2023 -0700

    works

commit 5ad5130
Author: Andre Giron <[email protected]>
Date:   Tue May 16 16:34:41 2023 -0700

    ENG-2979: Fixes bigquery and gcs dialogs that were crashing after file uploads. (#1326)

    Co-authored-by: Andre Giron <[email protected]>

commit 03aa76e
Author: Andre Giron <[email protected]>
Date:   Tue May 16 16:33:35 2023 -0700

    ENG-2979 S3 Dialog Validation Fixes (#1328)

    Co-authored-by: Andre Giron <[email protected]>

commit f63c5f3
Author: kenxu95 <[email protected]>
Date:   Tue May 16 16:24:14 2023 -0700

    Fix Snowflake save missing schema bug (#1331)

commit a458ce6
Author: Wei Chen <[email protected]>
Date:   Tue May 16 15:27:52 2023 -0700

    works

commit f529c5f
Author: Saurav Chhatrapati <[email protected]>
Date:   Tue May 16 13:36:12 2023 -0400

    Fixes cloudpickle serialization for Python 3.7 (#1324)

commit c335fd5
Author: Saurav Chhatrapati <[email protected]>
Date:   Tue May 16 12:15:48 2023 -0400

    Adds error checking for whether big query dataset exists (#1319)

commit d3521a4
Author: kenxu95 <[email protected]>
Date:   Mon May 15 20:58:16 2023 -0700

    Add ability to parameterize the save operator (#1320)

commit 942dec1
Author: kenxu95 <[email protected]>
Date:   Mon May 15 15:36:21 2023 -0700

    [UI] Merge the Conda resource into the Aqueduct Server  (#1311)

commit ccf4e17
Author: Wei Chen <[email protected]>
Date:   Mon May 15 14:57:19 2023 -0700

    [2/n] Adds gh actions to publish test pypi packages (#1262)

commit fb67044
Author: kenxu95 <[email protected]>
Date:   Mon May 15 12:04:48 2023 -0700

    Fix bug where duplicate fields showing in resource details dropdown (#1321)

commit cfeeb2f
Author: kenxu95 <[email protected]>
Date:   Mon May 15 11:45:31 2023 -0700

    Fix generated docstrings when viewing resource methods in Jupyter (#1315)

commit b544252
Author: eunice-chan <[email protected]>
Date:   Fri May 12 21:26:55 2023 -0500

    Eng 2884 test node routes (#1299)

    Co-authored-by: Eunice Chan <[email protected]>
    Co-authored-by: Wei Chen <[email protected]>

commit a8abb16
Author: Andre Giron <[email protected]>
Date:   Fri May 12 16:58:59 2023 -0700

    Fixes broken storybook build (#1317)

    Co-authored-by: Andre Giron <[email protected]>

commit d6ed5ca
Author: kenxu95 <[email protected]>
Date:   Fri May 12 10:56:42 2023 -0700

    Update the workflow "No Run" logo (#1312)

commit c9fec70
Author: Chenggang Wu <[email protected]>
Date:   Fri May 12 09:30:54 2023 -0700

    README typo fix (#1313)

commit b686014
Author: Andre Giron <[email protected]>
Date:   Thu May 11 12:49:53 2023 -0700

    ENG-2767 react hook form submission (#1232)

    Co-authored-by: Andre Giron <[email protected]>
    Co-authored-by: Chenggang Wu <[email protected]>
    Co-authored-by: kenxu95 <[email protected]>

commit 86269ec
Author: kenxu95 <[email protected]>
Date:   Thu May 11 12:09:42 2023 -0700

    Add appropriate workflow count to our connected notification cards and details page (#1302)

commit 7c75ad6
Author: kenxu95 <[email protected]>
Date:   Thu May 11 10:45:52 2023 -0700

    Update the artifact storage card presentation to be consistent with the other data integrations (#1297)

commit 4f9c227
Author: Andre Giron <[email protected]>
Date:   Wed May 10 16:55:23 2023 -0700

    ENG_2960: Fix broken documentation link for Aqueduct demo resource. (#1306)

    Co-authored-by: Andre Giron <[email protected]>

commit 6341917
Author: Andre Giron <[email protected]>
Date:   Wed May 10 16:28:23 2023 -0700

    Release version 0.3.2 (#1307)

    Co-authored-by: Vikram Sreekanti <[email protected]>
    Co-authored-by: Andre Giron <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vsreekanti vsreekanti vsreekanti approved these changes

@cw75 cw75 Awaiting requested review from cw75

@likawind likawind Awaiting requested review from likawind

Assignees
No one assigned
Labels
skip_integration_test Skips required integration test (only documentation/UI changes)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@saurav-c @vsreekanti