Default LookML view table to the view name when unspecified#244
Default LookML view table to the view name when unspecified#244nicosuave wants to merge 5 commits into
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 84410ae8a8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # via extends or are abstract (extension: required). | ||
| if ( | ||
| table is None | ||
| and sql is None |
There was a problem hiding this comment.
For views that do have a derived_table block but not a sql or explore_source variant the adapter understands (for example derived_table: { sql_create: ... } or create_process), sql stays None, so this fallback now assigns table = name. That imports a derived table as a physical table named after the view and generated queries target the wrong relation instead of preserving or flagging the unsupported derived table; the fallback should also require derived_table is None.
Useful? React with 👍 / 👎.
nicosuave
force-pushed
the
fix/lookml-default-view-table
branch
from
84410ae to
92c08af
Compare
nicosuave
force-pushed
the
fix/lookml-export-aggregations
branch
from
adf80bc to
e4e46ef
Compare
nicosuave
force-pushed
the
fix/lookml-default-view-table
branch
from
92c08af to
d2cb276
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d2cb276882
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| table is None | ||
| and sql is None | ||
| and not view_def.get("derived_table") | ||
| and not extends |
There was a problem hiding this comment.
When a view extends a parent that also omitted sql_table_name, this guard prevents the child from ever getting its own default while the parent has already been assigned table = name; for view: base { ... } plus view: child { extends: [base] ... }, Sidemantic now compiles child against base instead of child. Looker applies defaults only after the merged view is interpreted, so an extended view with no explicit sql_table_name assumes the extending view name (see https://docs.cloud.google.com/looker/docs/reusing-code-with-extends#implementation_details_for_extends); this needs to distinguish explicit parent tables from synthetic defaults or apply the default after inheritance.
Useful? React with 👍 / 👎.
nicosuave
force-pushed
the
fix/lookml-export-aggregations
branch
from
e4e46ef to
5efe508
Compare
nicosuave
force-pushed
the
fix/lookml-default-view-table
branch
from
d2cb276 to
5efe508
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5af0072a7d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| and not name.startswith("+") | ||
| and view_def.get("extension") != "required" | ||
| and (dimensions or measures) |
There was a problem hiding this comment.
When the base view is fieldless and a +view refinement supplies the dimensions/measures, neither parsed model gets the implicit table: the base fails the (dimensions or measures) guard while the refinement is excluded by name.startswith("+"). After merge_model the final view has queryable fields but still no table, so adding or compiling that valid LookML pattern (view: orders {} plus view: +orders { dimension: id ... }) still fails validation even though Looker would default it to orders; the default needs to run on the merged view or otherwise account for fields added by refinements.
Useful? React with 👍 / 👎.
nicosuave
force-pushed
the
fix/lookml-export-aggregations
branch
from
8dad963 to
4815fce
Compare
nicosuave
force-pushed
the
fix/lookml-default-view-table
branch
from
5af0072 to
50ef09e
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 50ef09e59d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| model.table is None | ||
| and model.sql is None | ||
| and not model.extends | ||
| and not meta.get("extension_required") |
There was a problem hiding this comment.
When a tableless view extends an extension: required base, resolve_model_inheritance() copies the parent's meta onto the child, so this check treats the concrete child as abstract and never assigns the implicit table. A valid pattern like view: base { extension: required ... } plus view: orders { extends: [base] ... } still imports orders with table=None, causing SemanticLayer.add_model()/queries to fail even though the table should default to orders; the skip needs to apply only to the original extension-required view, not inherited metadata.
Useful? React with 👍 / 👎.
| and model.sql is None | ||
| and not model.extends | ||
| and not meta.get("extension_required") | ||
| and not meta.get("unsupported_derived_table") |
There was a problem hiding this comment.
The direct unsupported-derived-table case is now marked, but this pass runs after refinements are merged, and merge_model() replaces the parent's meta when a refinement has any metadata such as label, hidden, or tags. In that scenario the base view's unsupported_derived_table marker is lost, so a view with an unsupported derived_table can still be defaulted to a physical table named after the view instead of remaining unsupported; the marker needs to survive refinement merges or be tracked outside ordinary metadata.
Useful? React with 👍 / 👎.
nicosuave
force-pushed
the
fix/lookml-export-aggregations
branch
from
4815fce to
7e3fe2e
Compare
nicosuave
force-pushed
the
fix/lookml-default-view-table
branch
from
50ef09e to
1dfe950
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1dfe950fda
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| and model.sql is None | ||
| and not model.extends | ||
| and model_name not in abstract_views | ||
| and model_name not in unsupported_dt_views |
There was a problem hiding this comment.
When a view extends a parent whose derived_table is unsupported and does not override the source, resolve_model_inheritance() copies the parent's unsupported_derived_table marker onto the child, but this check only skips names captured before inheritance. In that scenario the child has table is None, sql is None, no remaining extends, and inherited fields, so it is defaulted to a physical table named after the child even though it should remain unsupported like its parent; generated queries will target the wrong relation.
Useful? React with 👍 / 👎.
nicosuave
force-pushed
the
fix/lookml-export-aggregations
branch
2 times, most recently
from
4380274 to
8346ee9
Compare
nicosuave
force-pushed
the
fix/lookml-default-view-table
branch
from
1dfe950 to
0a99120
Compare
nicosuave
force-pushed
the
fix/lookml-export-aggregations
branch
from
8346ee9 to
e7c09f7
Compare
nicosuave
force-pushed
the
fix/lookml-default-view-table
branch
from
0a99120 to
276e8e2
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 276e8e29e6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| model.table is None | ||
| and model.sql is None | ||
| and not model.extends | ||
| and model_name not in abstract_views |
There was a problem hiding this comment.
When a +view refinement is what adds extension: required, abstract_views was captured before refinements, and this guard never re-checks extension_required on the merged base model. A model like view: base { dimension: id ... } with view: +base { extension: required } therefore gets table='base' even though the merged view is abstract; track refinement-added abstractness for the refined view while still allowing concrete children to default.
Useful? React with 👍 / 👎.
| and not model.extends | ||
| and model_name not in abstract_views | ||
| and model_name not in unsupported_dt_views | ||
| and not (model.meta or {}).get("unsupported_derived_table") |
There was a problem hiding this comment.
When a child extends an unsupported derived table and also has its own model metadata (label, hidden, or tags) without overriding the source, merge_model() replaces the inherited meta, so this post-merge check no longer sees unsupported_derived_table and the child is not in unsupported_dt_views. For example view: base { derived_table: { sql_trigger_value: ... } ... } plus view: child { extends: [base] label: "Child" } is defaulted to table='child', causing generated queries to hit a fabricated physical table instead of leaving the unsupported derived table unqueryable.
Useful? React with 👍 / 👎.
nicosuave
force-pushed
the
fix/lookml-export-aggregations
branch
from
e7c09f7 to
ca47e68
Compare
nicosuave
force-pushed
the
fix/lookml-default-view-table
branch
from
276e8e2 to
0966fe7
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0966fe71be
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # child that only INHERITS abstractness is not treated as abstract. Record | ||
| # extends parents too, so descendants of an unsupported derived table can be | ||
| # detected after resolution clears `extends`. | ||
| abstract_views = abstract_pre | {n for n, m in graph.models.items() if (m.meta or {}).get("extension_required")} |
There was a problem hiding this comment.
The remaining gap is when one +view refinement adds extension: required and a later refinement only sets metadata such as label: merge_model() replaces meta, and this set is computed only from the pre-refinement models plus the final post-merge state, so the transient extension_required marker is missed. That leaves the refined base eligible for the defaulting block and assigns table = "base", making an abstract LookML view queryable; record the flag during each refinement merge or merge these meta flags instead.
Useful? React with 👍 / 👎.
nicosuave
force-pushed
the
fix/lookml-default-view-table
branch
from
0966fe7 to
ec74f18
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ec74f18f38
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| and not (model.meta or {}).get("unsupported_derived_table") | ||
| and (model.dimensions or model.metrics) | ||
| ): | ||
| model.table = model_name |
There was a problem hiding this comment.
When LookMLAdapter.parse() is called after constructing a default SemanticLayer() or inside with SemanticLayer(), the current-layer auto-registration path runs from Model.__init__ during _parse_view, before this post-parse assignment executes. For a valid tableless LookML view such as view: just_fields { ... }, layer.add_model validates immediately and raises ModelValidationError because table is still None, so this new default does not work in the default Python API ordering; the table needs to be set before model construction or auto-registration suppressed while parsing.
Useful? React with 👍 / 👎.
nicosuave
force-pushed
the
fix/lookml-default-view-table
branch
from
ec74f18 to
e72acbd
Compare
nicosuave
force-pushed
the
fix/lookml-export-aggregations
branch
from
ca47e68 to
d029bab
Compare
nicosuave
force-pushed
the
fix/lookml-export-aggregations
branch
from
8b92870 to
39f0919
Compare
nicosuave
force-pushed
the
fix/lookml-default-view-table
branch
from
33ee7f7 to
64fefb4
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 64fefb4aa6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| surviving = {n for model in kept.values() for n in _graph_metric_names(model)} | ||
| orphaned = {n for name in dropped for n in _graph_metric_names(all_models[name])} - surviving | ||
| for mn in orphaned: | ||
| all_metrics.pop(mn, None) |
There was a problem hiding this comment.
When a dropped LookML template has a time_comparison/conversion measure with the same name as a valid graph-level metric from another file, all_metrics may currently contain that surviving metric after the directory-wide all_metrics.update(...) calls. This name-only removal still pops it because surviving is computed only from model-local special metrics, so CLI loads silently lose the valid standalone metric; the filter needs to verify the metric being removed was contributed by the dropped model rather than matching only by name.
Useful? React with 👍 / 👎.
nicosuave
force-pushed
the
fix/lookml-export-aggregations
branch
from
39f0919 to
bf0b444
Compare
nicosuave
force-pushed
the
fix/lookml-default-view-table
branch
from
64fefb4 to
c57bdda
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c57bddabf3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if m.type in ("time_comparison", "conversion") and all_metrics.get(m.name) is m: | ||
| all_metrics.pop(m.name, None) |
There was a problem hiding this comment.
Fresh evidence beyond the prior orphaned-metric comments is that a label-only +view refinement (or any merge that reconstructs the model) creates new Metric objects for the skipped template, while all_metrics still holds the original object registered by graph.add_model() before refinement. Because this check only pops when the object identity still matches, a skipped extension: required view with a period_over_period measure and a later refinement leaves layer.graph.metrics['pop'] behind with no models loaded, so sidemantic info/compile(metrics=['pop']) exposes an orphan and fails with “No models found for query.”
Useful? React with 👍 / 👎.
nicosuave
force-pushed
the
fix/lookml-export-aggregations
branch
from
bf0b444 to
6416688
Compare
nicosuave
force-pushed
the
fix/lookml-default-view-table
branch
from
c57bdda to
589cca7
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 589cca7e79
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| elif ref in _surviving_measures: # unqualified ref a surviving model provides | ||
| return True |
There was a problem hiding this comment.
Fresh evidence beyond the earlier same-name metric comments is that this unqualified-base check keeps a graph metric from a skipped LookML template whenever any surviving model has a same-named base measure. In the CLI load path, an extension: required base view with total and pop based_on: total, plus a real orders.total, leaves pop in layer.graph.metrics even though no loaded model defines pop; compiling it then fails with the unresolved base (Measure total not found). The filter needs to prove the graph metric itself came from a surviving model or standalone metric source, not just that its unqualified base name exists somewhere.
Useful? React with 👍 / 👎.
nicosuave
force-pushed
the
fix/lookml-export-aggregations
branch
from
6416688 to
65a206c
Compare
nicosuave
force-pushed
the
fix/lookml-default-view-table
branch
from
589cca7 to
05a5e00
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 05a5e00331
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if ( | ||
| m is not None | ||
| and getattr(m, "type", None) in ("time_comparison", "conversion") | ||
| and not _resolves_to_surviving(m) | ||
| ): |
There was a problem hiding this comment.
When a dropped LookML template has a same-named time_comparison/conversion measure, this still removes a valid standalone graph metric from another file if that metric uses an unqualified base reference such as base_metric: total. The fresh evidence is that the new _resolves_to_surviving only treats qualified refs (orders.total) as survivors, but SQLGenerator._generate_with_window_functions.resolve_metric_ref also resolves unqualified refs by scanning model metrics when they are unambiguous; in a CLI directory load where the standalone metric is parsed after the template, all_metrics['pop'] can point to that valid standalone metric and line 87 pops it anyway.
Useful? React with 👍 / 👎.
nicosuave
force-pushed
the
fix/lookml-export-aggregations
branch
from
65a206c to
c2b8658
Compare
nicosuave
force-pushed
the
fix/lookml-default-view-table
branch
from
ff06f9c to
4a58268
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 569c6ab573
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| and not _extends_chain_has(model_name, unsupported_dt_views) | ||
| and not (model.meta or {}).get("unsupported_derived_table") | ||
| ): | ||
| model.table = model_name |
There was a problem hiding this comment.
When a parsed LookML graph is exported and then re-imported, _export_view does not write extension: required from model.meta; an abstract tableless base is therefore emitted as an ordinary tableless view. On re-import this new fallback assigns table = "base", making the template query a fabricated physical table instead of remaining non-queryable, which affects LookML round-trip/migrator workflows for projects with abstract base views. Preserve the extension marker on export or otherwise prevent exported templates from hitting this default.
Useful? React with 👍 / 👎.
nicosuave
force-pushed
the
fix/lookml-export-aggregations
branch
from
c2b8658 to
07c12a9
Compare
nicosuave
force-pushed
the
fix/lookml-default-view-table
branch
from
569c6ab to
be7df42
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: be7df4218e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if ( | ||
| tf not in self._INEXACT_NAME_TF | ||
| and dim.name.endswith("_" + tf) | ||
| and self._TIME_GRANULARITY_TIMEFRAMES[tf] == dim.granularity | ||
| ): |
There was a problem hiding this comment.
*
When exporting existing/native models that already have an hour-grain field named created_time (including graphs produced by earlier parser versions, where LookML time imported as hour), this suffix check no longer matches _time because time now maps to second. _group_timeframes() falls back to hour, writes timeframes: [hour], and parsing the exported LookML returns created_hour instead of created_time, breaking references to the original field; use a standalone hour-truncated dimension or another preservation path when the existing suffix would be renamed.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 56850cf663
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # Re-emit `extension: required` for an abstract (tableless) base so a round-trip keeps | ||
| # it non-queryable; otherwise it re-imports as an ordinary tableless view and the | ||
| # implicit-table default assigns it a physical table named after the view. | ||
| if (model.meta or {}).get("extension_required"): | ||
| view["extension"] = "required" |
There was a problem hiding this comment.
When a parsed LookML view was kept tableless because its derived_table is unsupported, _export_view only preserves extension: required templates and emits no derived-table marker for model.meta['unsupported_derived_table']. Round-tripping a view such as derived_table: { sql_trigger_value: ... } therefore exports it as an ordinary tableless view; on re-import the new implicit-table default assigns table = <view name>, so migrator/export workflows start querying a fabricated physical table instead of leaving the derived table unsupported.
Useful? React with 👍 / 👎.
nicosuave
force-pushed
the
fix/lookml-default-view-table
branch
from
56850cf to
787a47b
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 787a47b040
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (model.meta or {}).get("extension_required"): | ||
| view["extension"] = "required" |
There was a problem hiding this comment.
When exporting a graph parsed from view: base { extension: required ... } plus a concrete view: orders { extends: [base] ... }, inheritance leaves orders.meta['extension_required'] copied from the base even though the parser correctly treats only base as abstract. This check then emits extension: required for the concrete child as well, so round-tripped LookML makes the usable child view abstract/non-queryable; only the original tableless/template view should re-emit this marker.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8eb5d20323
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ordered_groups = sorted( | ||
| base_name_groups.items(), | ||
| key=lambda kv: (kv[0][0], 1 if self._DATE_TRUNC_RE.match(kv[0][1] or "") else 0, kv[0][1] or ""), |
There was a problem hiding this comment.
When a suffixless time dimension and an existing <base>_<timeframe> dimension share a base name but use different SQL, this ordering lets the suffixless source win the dimension_group whenever its SQL sorts first. The group then generates the existing suffixed field name for the suffixless source, while _export_collision_time_dim() renames the original suffixed dimension to e.g. started_2_hour, so round-tripped queries for started_hour hit the wrong column. Prefer a group whose generated field name already matches an existing suffixed dimension before falling back to SQL order.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 93b5cce4f5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # explicit bucket/subsecond forms, handled above, do) -- recording it | ||
| # would make the next export WIDEN a 1-minute dim into a 15-min bucket. | ||
| # Recover as a plain grain time dim (no inexact meta) instead. | ||
| if matched_tf not in self._INEXACT_NAME_TF: |
There was a problem hiding this comment.
When a colliding time_of_day field is exported as a standalone dimension, _export_collision_time_dim() preserves the name as *_time_of_day but emits an hour DATE_TRUNC. On re-import, matched_tf becomes time_of_day, yet this guard refuses to store recovered_timeframe because time_of_day is in _INEXACT_NAME_TF; the field then loses its LookML timeframe metadata, and the next export falls back to the hour timeframe and renames it to *_hour. This breaks repeated LookML round-trips for same-prefix time fields backed by different sources.
Useful? React with 👍 / 👎.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
A LookML view with no sql_table_name and no derived_table implicitly uses a table named after the view (Looker's default behavior), but the adapter left table unset, so importing such a view crashed layer validation with "Model must have a table". Default table to the view name in that case, while leaving refinements, extends-based views, and abstract (extension: required) views untouched.
Looker's 'time' timeframe keeps full timestamp precision (to the second), but the adapter mapped it to hour granularity, silently collapsing all sub-hour rows. Map 'time' to second granularity, and add second/minute to the export granularity->timeframe map so they round-trip instead of degrading to 'date'. Update the four tests that asserted the old hour grain.
Re-emit 'extension: required' on export so an abstract base round-trips as tableless instead of being defaulted to a physical table on re-import. Drop orphaned graph metrics from skipped LookML templates by a parser-owned provenance marker rather than a base-ref heuristic, so a same-named standalone metric from another file (with a qualified OR unqualified base) is never dropped, while a template's metric is dropped even after a refinement reconstructs the object.
When a suffixless time dimension (e.g. started) collides with an existing suffixed sibling (started_hour) on a different SQL source, the dimension_group winner was chosen purely by SQL sort order. A suffixless winner generates started_hour from ITS OWN source while the real started_hour is pushed to a standalone and renamed (started_2_hour) -- so a round-tripped query for started_hour silently reads the wrong column. Deprioritize a suffixless representative when picking the dimension_group slot so the suffixed sibling wins and its generated field maps to its own source. Safe against the earlier block-name conflict because the displaced suffixless dim disambiguates into its stem (started_2_hour), never bare started.
A collision-exported time_of_day field is a plain hour DATE_TRUNC named *_time_of_day. The import recovery guard grouped time_of_day with the exact-form inexact timeframes (minute15/30 buckets, sub-second) and refused to store its recovered timeframe, so the field lost its LookML timeframe metadata. A later export without a colliding sibling then fell back to the hour timeframe and renamed the field to *_hour, breaking repeated round-trips. Unlike the bucket/sub-second forms -- whose exact semantics live in a special SQL form a plain DATE_TRUNC has already lost -- time_of_day has no finer form: the plain hour DATE_TRUNC IS its canonical export, so it is recoverable by name. Split out _EXACT_FORM_TF (buckets + sub-second) for the recovery guard, leaving _INEXACT_NAME_TF (which still includes time_of_day) for the export-side native name-derivation skip.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
1 participant
Summary
Part of the LookML adapter correctness series. Stacked on #243 (base =
fix/lookml-export-aggregations).In Looker, a view with no
sql_table_nameand noderived_tableimplicitly queries a table named after the view. The adapter lefttableunset, so importing such a (perfectly valid) view crashed withModelValidationError: Model must have a table.Changes
tableto the view name when there is nosql_table_name, noderived_table, noextends, it is not a refinement (+view), and notextension: required.These exclusions preserve correct behavior for abstract/extended/refinement views, which legitimately have no table of their own.