Fix bugs in derived_view_schemas SQL generator
#5592
Merged
Conversation
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 comment has been minimized.
This comment has been minimized.
BenWu
reviewed
May 16, 2024
| for path in dataset_path.iterdir() | ||
| if path.is_dir() | ||
| and (path / VIEW_FILE).exists() | ||
| and not (path / SCHEMA_FILE).exists() |
There was a problem hiding this comment.
I don't think view deploys update the schema from dry run if a schema.yaml exists, unlike table deploys. So this might cause view deployment to publish outdated schemas and fail. IMO we can update the schema if it already exists, but if not then any schemas that need to be updated should be included in this PR
There was a problem hiding this comment.
I think if there's an explicit schema.yaml file for a view it's reasonable to expect that to be kept up to date and be accurate (e.g. your code for View.view_schema seems to make that assumption).
There was a problem hiding this comment.
It's a safe assumption because those schemas get updated by this generator but that wouldn't be the case with this change. Running this generator locally, at least funnel_retention_clients and funnel_retention_week_4 have columns added so those would fail to publish here
bigquery-etl/bigquery_etl/view/__init__.py
Lines 371 to 376 in 23fa414
Those should be updated in this PR. I would still lean towards updating existing schemas since we can't always rely on schemas being up to date which is the case for tables
There was a problem hiding this comment.
OK:
- I've changed the
View.view_schemalogic to always try dry-running first, then augment with theschema.yamlif one exists. - I've changed the
derived_view_schemasSQL generator to useView.view_schemato get schemas, rather than getting the deployed view schema withSchema.for_table()(which I suspect was probably preventing the logic you implemented in Bug 1868244 Publish views if dry run schema differs from deployed #5057 from working properly). - I've changed the
derived_view_schemasSQL generator to not skip views that already haveschema.yamlfiles.
It now gets the view schemas from their latest `schema.yaml` file or dry-running their latest SQL. Getting the schema from the currently deployed view wasn't appropriate because it wouldn't reflect the latest view code.
This comment has been minimized.
This comment has been minimized.
…he latest schema.
This comment has been minimized.
This comment has been minimized.
BenWu
approved these changes
May 16, 2024
bigquery_etl/view/__init__.py
Outdated
| """Derive view schema from a dry run result and/or a schema file.""" | ||
| schema = None | ||
|
|
||
| # check schema based on dry run results |
There was a problem hiding this comment.
I think dry running everything makes sense to make sure everything is updated but it will probably add a few minutes to the publish views task. Views should usually be up to date since the generator already ran. I don't have any issues with this but worth checking later on to see how much time this adds.
There was a problem hiding this comment.
I reversed course on this and went back to having it return the schema from schema.yaml if one exits by default, because it turns out dry-runs for some views which select from very large tables can take well over a minute.
The derived_view_schemas SQL generator logic will still always try to dry-run views to get their up-to-date schema.
There was a problem hiding this comment.
I did manage to get the dry-runs on some of the views that select from large tables to run quicker by adding partition column filters, but I'm still going to leave this with the original behavior of using the schema from schema.yaml by default, with the derived_view_schemas SQL generator using custom logic to always try to dry-run the views.
sean-rose
force-pushed
the
fix/derived_view_schemas
branch
from
0450a63
to
b094dcd
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
sean-rose
commented
May 16, 2024
bigquery_etl/view/__init__.py
Outdated
Comment on lines
187
to
199
| # We have to remove `CREATE OR REPLACE VIEW ... AS` from the query to avoid | ||
| # view creation permission denied errors, and we have to apply a `WHERE FALSE` | ||
| # filter to avoid partition column filter missing errors. | ||
| query=dedent( | ||
| f""" | ||
| WITH view_query AS ( | ||
| {CREATE_VIEW_PATTERN.sub("", self.content)} | ||
| ) | ||
| SELECT * | ||
| FROM view_query | ||
| WHERE FALSE | ||
| """ | ||
| ), |
There was a problem hiding this comment.
I was pleasantly surprised that dry-running the full CREATE VIEW ... query returns the view's schema, nicely side-stepping the issue of potentially having to add query filters on any partition columns from the underlying tables. However, it turns out that only works if you actually have permission to create the view in the target dataset, which we DEs generally do but CI does not, so the previous dry-run-to-get-view-schema logic would have always failed in a CI context.
CI doesn't have permission to do the dry-runs directly.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
sean-rose
force-pushed
the
fix/derived_view_schemas
branch
from
44aa8b9
to
c25347b
Compare
…the latest schema. Dry-runs for some views which select from very large tables can take well over a minute.
sean-rose
force-pushed
the
fix/derived_view_schemas
branch
from
c25347b
to
6e4c94e
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
…ml` files if those files actually exist.
This comment has been minimized.
This comment has been minimized.
sean-rose
force-pushed
the
fix/derived_view_schemas
branch
from
eb6e1e6
to
7ef67d9
Compare
To speed up the dry-run queries.
sean-rose
force-pushed
the
fix/derived_view_schemas
branch
from
7ef67d9
to
40d59a5
Compare
This comment has been minimized.
This comment has been minimized.
…1` table. So the `derived_view_schemas` SQL generator can detect the partition column to use and successfully dry-run the view to determine its schema.
This comment has been minimized.
This comment has been minimized.
In case other SQL generators create derived views.
sean-rose
commented
May 17, 2024
bigquery_etl/cli/generate.py
Outdated
Comment on lines
105
to
110
| # Run `stable_views` after `glean_usage` because both update `dataset_metadata.yaml` files | ||
| # and we want the `stable_views` updates to take precedence. | ||
| case "glean_usage": | ||
| return (1, command.name) | ||
| case "stable_views": | ||
| return (2, command.name) |
There was a problem hiding this comment.
This is based on comments in #2594.
There was a problem hiding this comment.
@scholtzan the sql.diff shows this changing some dataset_metadata.yaml files. Since I based the implementation on your comments, can you please confirm that stable_views should still run after glean_usage, and not vice versa?
There was a problem hiding this comment.
I believe for _, cmd in reversed(generate.commands.items()): causes stable_views to be run before glean_usage. I'm not sure whether it makes a difference here though? I believe glean_app_ping_views might depended on the stable views at some point.
There was a problem hiding this comment.
OK, I've reversed the order in this PR so that stable_views runs before glean_usage, and now no dataset_metadata.yaml changes appear in the sql.diff.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
sean-rose
commented
May 20, 2024
| case _: | ||
| return (3, command.name) | ||
|
|
||
| for cmd in sorted(generate.commands.values(), key=generator_command_sort_key): |
There was a problem hiding this comment.
I'm hoping running the SQL generators in a deterministic order will also reduce the seemingly random unrelated diffs that sometimes get reported in sql.diff. Currently the SQL generators are run in the reverse order that Path.iterdir() returns the sql_generators/* subdirectories, and according to the docs that order is "arbitrary".
BenWu
approved these changes
May 22, 2024
| @@ -247,7 +247,7 @@ def _traverse( | |||
| f"for {prefix}.{field_path} are incompatible" | |||
| ) | |||
|
|
|||
| if dtype == "RECORD": | |||
| if dtype == "RECORD" and nodes[node_name]["type"] == "RECORD": | |||
There was a problem hiding this comment.
Did an issue with this pop up while you were testing? I'm ignore_incompatible_fields would need to be true
There was a problem hiding this comment.
Did an issue with this pop up while you were testing?
Yes, it was causing the derived_view_schemas generator to fail for a couple views that transform an underlying struct column into something else:
telemetry.clients_daily_joinedtelemetry.clients_daily_v6
I'm
ignore_incompatible_fieldswould need to be true
The derived_view_schemas generator calls Schema.merge() with attributes=["description"], which kind of has a similar effect since the field incompatibility logic is applied at the attribute level.
scholtzan
approved these changes
May 28, 2024
This comment has been minimized.
This comment has been minimized.
Integration report for "Have
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
The
derived_view_schemasSQL generator was running against all types of directories (not just view directories), and it was reading the view schemas from the currently deployed views (not based on the latest view code).Changes:
Limit
derived_view_schemasSQL generator to actual view directories.Fix the
derived_view_schemasSQL generator to get the view schemas by dry-running their latest SQL and/or from their latestschema.yamlfile. Getting the schema from the currently deployed view wasn't appropriate because it wouldn't reflect the latest view code.Rename
View.view_schematoView.schema.Change
Viewso its schema dry-runs use the cloud function (CI doesn't have permission to run dry-run queries directly).Apply partition column filters in view dry-run queries when possible for speed/efficiency.
Don't allow missing fields to prevent view schema enrichment.
Only copy column descriptions during view schema enrichment.
Only try enriching view schemas from their reference table
schema.yamlfiles if those files actually exist.Change
main_1pctview to select directly from themain_remainder_1pct_v1table, so thederived_view_schemasSQL generator can detect the partition column to use and successfully dry-run the view to determine its schema.Formalize the order
bqetl generate allruns the SQL generators in.Have
bqetl generate allrunderived_view_schemaslast, in case other SQL generators create derived views.Fix
Schema._traverse()to only recurse if both fields are records.Checklist for reviewer:
<username>:<branch>of the fork as parameter. The parameter will also show upin the logs of the
manual-trigger-required-for-forkCI task together with more detailed instructions.For modifications to schemas in restricted namespaces (see
CODEOWNERS):┆Issue is synchronized with this Jira Task