-
Notifications
You must be signed in to change notification settings - Fork 3k
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
fix(ingest/bigquery): Pass whether view is materialized; pass last_altered correctly #7660
fix(ingest/bigquery): Pass whether view is materialized; pass last_altered correctly #7660
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
metadata-ingestion/src/datahub/ingestion/source/bigquery_v2/bigquery_schema.py
Show resolved
Hide resolved
comment=table.comment, | ||
view_definition=table.view_definition, | ||
materialized=table.table_type == "MATERIALIZED VIEW", |
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.
maybe we can add a bigquery_constants.py file?
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.
+1
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.
lgtm
@@ -18,6 +20,16 @@ | |||
logger: logging.Logger = logging.getLogger(__name__) | |||
|
|||
|
|||
class BigqueryTableType(Enum): |
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.
nice!
@@ -18,6 +20,16 @@ | |||
logger: logging.Logger = logging.getLogger(__name__) | |||
|
|||
|
|||
class BigqueryTableType(Enum): | |||
# See https://cloud.google.com/bigquery/docs/information-schema-tables#schema |
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.
<3 thank you for linking documentation. more of this pls!
@@ -173,20 +186,20 @@ class BigqueryQuery: | |||
sum(case when storage_tier = 'LONG_TERM' then total_billable_bytes else 0 end) as long_term_billable_bytes, | |||
sum(case when storage_tier = 'ACTIVE' then total_billable_bytes else 0 end) as active_billable_bytes, | |||
from | |||
`{project_id}`.`{dataset_name}`.INFORMATION_SCHEMA.PARTITIONS | |||
`{{project_id}}`.`{{dataset_name}}`.INFORMATION_SCHEMA.PARTITIONS |
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.
these changes scare me - do we need to make them?
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.
those are because it's using an f-string (template string), so we need to escape the {} characters when not doing var substitution
we then do another string format later which substitutes those
@patch( | ||
"datahub.ingestion.source.bigquery_v2.bigquery_schema.BigQueryDataDictionary.get_query_result" | ||
) | ||
@patch("google.cloud.bigquery.client.Client") |
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.
nice!
group by | ||
table_name) as p on | ||
t.table_name = p.table_name | ||
WHERE | ||
table_type in ('BASE TABLE', 'EXTERNAL') | ||
{table_filter} | ||
table_type in ({BigqueryTableType.BASE_TABLE}, {BigqueryTableType.EXTERNAL}) |
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.
confused - why not {{BigQueryTablType.BASE_TABLE}} -- two brackets instead of one?
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 believe this change caused regression in final query due to missing quotes around constants.
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.
LGTM. Nice work addressing this!
Checklist