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

fix(ingest/bigquery): Fix BigQueryTableType enum accesses #7685

Merged
merged 3 commits into from
Mar 25, 2023
Merged

fix(ingest/bigquery): Fix BigQueryTableType enum accesses #7685

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8dcc9f2
fix(ingest/bigquery): Fix BigQueryTableType enum accesses
asikowitz Mar 24, 2023
952a299
update test
asikowitz Mar 24, 2023
c5a6f58
lint
asikowitz Mar 24, 2023
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
3 changes: 1 addition & 2 deletions metadata-ingestion/src/datahub/ingestion/source/bigquery_v2/bigquery_schema.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@
from collections import defaultdict
from dataclasses import dataclass, field
from datetime import datetime, timezone
from enum import Enum
from typing import Any, Dict, List, Optional, cast

from google.cloud import bigquery
Expand All @@ -19,7 +18,7 @@
logger: logging.Logger = logging.getLogger(__name__)


class BigqueryTableType(Enum):
class BigqueryTableType:
Copy link
Collaborator

Choose a reason for hiding this comment

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

we could still use StrEnum?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Saw this, don't think it'll work here unless I change the queries -- that's what's actually failing, where I directly embed BigQueryTableType.MATERIALIZED_VIEW etc. in query text. If I keep as an Enum, then I have to change to BigQueryTableType.MATERIALIZED_VIEW.value, but I didn't see any value in keeping it as an enum when it's basically just a string const holder.

In [2]: from enum import Enum

In [3]: class Mine(str, Enum):
   ...:     A = "ONE"
   ...:     B = "TWO"
   ...: 

In [4]: Mine.A
Out[4]: <Mine.A: 'ONE'>

In [5]: Mine.A == "ONE"
Out[5]: True

In [6]: str(Mine.A)
Out[6]: 'Mine.A'

In [7]: str(Mine.A.value)
Out[7]: 'ONE'

Copy link
Collaborator

Choose a reason for hiding this comment

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

ugh didn't realize that it overrode the __str__

yep let's leave it like this then

# See https://cloud.google.com/bigquery/docs/information-schema-tables#schema
BASE_TABLE = "BASE TABLE"
EXTERNAL = "EXTERNAL"
Expand Down
5 changes: 2 additions & 3 deletions metadata-ingestion/tests/unit/test_bigquery_source.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@
from datahub.ingestion.source.bigquery_v2.bigquery_schema import (
BigQueryDataDictionary,
BigqueryProject,
BigqueryTableType,
BigqueryView,
)
from datahub.ingestion.source.bigquery_v2.lineage import LineageEdge
Expand Down Expand Up @@ -464,7 +463,7 @@ def test_get_views_for_dataset(
last_altered=bigquery_view_1.last_altered,
comment=bigquery_view_1.comment,
view_definition=bigquery_view_1.view_definition,
table_type=BigqueryTableType.VIEW,
table_type="VIEW",
Copy link
Collaborator

@mayurinehate mayurinehate Mar 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Is this change needed even after removing Enum subclassing from BigqueryTableType class ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

this is test, then its okay.

)
)
row2 = create_row( # Materialized view, no last_altered
Expand All @@ -473,7 +472,7 @@ def test_get_views_for_dataset(
created=bigquery_view_2.created,
comment=bigquery_view_2.comment,
view_definition=bigquery_view_2.view_definition,
table_type=BigqueryTableType.MATERIALIZED_VIEW,
table_type="MATERIALIZED VIEW",
)
)
query_mock.return_value = [row1, row2]
Expand Down