Allow same names for dimensions and time dimension groups in generated views#1010
Allow same names for dimensions and time dimension groups in generated views#1010
Conversation
akkomar
force-pushed
the
time_dimensions
branch
2 times, most recently
from
d972d4c to
e897c0d
Compare
| and not (name == "event" and dimension["type"] == "time") | ||
| # workaround for `mozdata.firefox_desktop.desktop_installs` | ||
| and not (name == "attribution_dltoken" and dimension["type"] == "time") |
There was a problem hiding this comment.
We no longer need these workarounds now.
sean-rose
left a comment
sean-rose
left a comment
There was a problem hiding this comment.
r+wc
I appreciate you being willing to pivot to this alternate approach after having already coded your original approach.
generator/views/lookml_utils.py
Outdated
| and dimension["name"] != "submission" | ||
| and not dimension["name"].endswith("end") | ||
| and not dimension["name"].endswith("start") |
There was a problem hiding this comment.
Same here.
It seems like it'd be good to refactor this repeated logic in _generate_dimensions and _generate_dimensions_from_query out into a separate function named something like _generate_dimensions_from_schema. The only catch is the exception messages for duplicate dimensions are different, and in _generate_dimensions it does helpfully include the table in the error message, but for that case you could probably wrap the _generate_dimensions_from_schema call in a try/catch and do something like
raise click.ClickException(f"Error generating dimensions for table {table!r}") from e2 participants
Alternative to #1007 that would fix mozilla/bigquery-etl#5903, implements suggestion from #1007 (comment).
This approach seems to be minimally invasive and generalized, without hardcoding table names, allowing to unblock mozilla/bigquery-etl#5903.