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 1 commit 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
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c5a0536389
ℹ️ 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".
| # unsupported derived tables) BEFORE inference + registration, so FK inference never | ||
| # targets a model that won't be registered (which would leave a dangling relationship) | ||
| # and add_model never rejects a template that was never meant to be queried. | ||
| all_models = {name: model for name, model in all_models.items() if _is_registerable_model(model)} |
There was a problem hiding this comment.
When a LookML explore has already added a relationship to a model that this filter removes (for example an extension: required template or an unsupported derived-table view used as an explicit join target), the relationship remains on the surviving model while the target model is dropped. I checked the CLI validation path in validation_runner.validate_directory, which reports an error whenever rel.name is absent from layer.graph.models, so sidemantic validate can still fail with a dangling relationship after this intended skip; remove or suppress relationships whose targets are filtered here.
Useful? React with 👍 / 👎.
nicosuave
force-pushed
the
fix/lookml-export-aggregations
branch
from
b01a0e7 to
83a40fa
Compare
nicosuave
force-pushed
the
fix/lookml-default-view-table
branch
from
c5a0536 to
417f230
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 417f2305c1
ℹ️ 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.name not in prev_layer.graph.models and (model.table or model.sql): | ||
| prev_layer.add_model(model) |
There was a problem hiding this comment.
When parsing inside an active with SemanticLayer() context, this deferred registration skips tableless templates but registers the surviving models with their original relationships intact. In the same scenario the loader path now fixes—e.g. an orders explore joining an extension: required customer template—the customer model is skipped here while orders.relationships still contains customer, leaving the active layer with a dangling relationship that validation reports as missing; strip relationships to models this loop will skip before calling add_model().
Useful? React with 👍 / 👎.
nicosuave
force-pushed
the
fix/lookml-export-aggregations
branch
from
83a40fa to
d226311
Compare
nicosuave
force-pushed
the
fix/lookml-default-view-table
branch
from
417f230 to
f051bbf
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f051bbf593
ℹ️ 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".
| # relationships pointing at a skipped template (e.g. an explore join to it) so the | ||
| # active layer is not left with a dangling relationship that validation flags. | ||
| if prev_layer is not None: | ||
| skipped = {name for name, m in graph.models.items() if not (m.table or m.sql)} |
There was a problem hiding this comment.
When parse() runs with an active SemanticLayer, this marks every model without table/sql as skipped, but tableless models are not always intentional templates. For example, view: child { extends: [missing] ... } remains tableless with extends='missing' after _chain_resolvable leaves it unresolved; this loop then silently omits child from the layer instead of surfacing the validation error that the model has no source. Limit the skip to the abstract/unsupported-derived-table markers so broken LookML is still reported.
Useful? React with 👍 / 👎.
nicosuave
force-pushed
the
fix/lookml-export-aggregations
branch
from
d226311 to
8fee1ca
Compare
nicosuave
force-pushed
the
fix/lookml-default-view-table
branch
from
f051bbf to
d1a7eb4
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d1a7eb4ce1
ℹ️ 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_name not in abstract_views | ||
| and not _extends_chain_has(model_name, unsupported_dt_views) | ||
| and not (model.meta or {}).get("unsupported_derived_table") | ||
| and (model.dimensions or model.metrics or model.segments) |
There was a problem hiding this comment.
When a valid LookML view omits sql_table_name and currently has no parsed dimensions, measures, or segments (for example view: orders {} or a view that only contains adapter-ignored LookML fields), this guard leaves table unset even though Looker still defaults the table name to the view name. In the CLI load path the model is not marked abstract/unsupported, so load_from_directory() still registers it and raises the no-table ModelValidationError; the default should not depend on parsed fields for non-template views.
Useful? React with 👍 / 👎.
nicosuave
force-pushed
the
fix/lookml-export-aggregations
branch
from
8fee1ca to
8f9d121
Compare
nicosuave
force-pushed
the
fix/lookml-default-view-table
branch
from
d1a7eb4 to
e093d18
Compare
nicosuave
force-pushed
the
fix/lookml-export-aggregations
branch
from
8f9d121 to
2ec0bbe
Compare
nicosuave
force-pushed
the
fix/lookml-default-view-table
branch
from
e093d18 to
8110430
Compare
nicosuave
force-pushed
the
fix/lookml-export-aggregations
branch
from
2ec0bbe to
9c8ddd7
Compare
nicosuave
force-pushed
the
fix/lookml-default-view-table
branch
from
8110430 to
9202c25
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9202c250ff
ℹ️ 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".
| # unsupported derived tables) BEFORE inference + registration, so FK inference never | ||
| # targets a model that won't be registered (which would leave a dangling relationship) | ||
| # and add_model never rejects a template that was never meant to be queried. | ||
| all_models = _drop_non_registerable_models(all_models) |
There was a problem hiding this comment.
When this removes a LookML template, graph-level metrics already copied from the adapter remain in all_metrics: all_metrics.update(graph.metrics) runs before this point, and SemanticGraph.add_model() auto-registers model metrics with type time_comparison/conversion. For an extension: required view with a period_over_period measure and no surviving concrete child, CLI loading now drops the only model but still registers the graph metric, so sidemantic info/queries expose a metric whose base model is missing and compile(metrics=[...]) fails with no models found. Filter the metrics associated with dropped templates, or avoid carrying graph metrics from skipped LookML templates.
Useful? React with 👍 / 👎.
| meta = model.meta or {} | ||
| return not (meta.get("extension_required") or meta.get("unsupported_derived_table")) |
There was a problem hiding this comment.
This helper is called on the combined output from every adapter, but the skip decision uses unscoped arbitrary model.meta keys. A native Sidemantic or other-format model with no source and user metadata like extension_required: true is now removed before add_model() instead of producing the missing table/sql validation error, so CLI validation can silently pass while dropping user-defined models. Please also require the LookML source format, or a parser-owned marker, before treating these keys as non-queryable-template signals.
Useful? React with 👍 / 👎.
nicosuave
force-pushed
the
fix/lookml-export-aggregations
branch
from
9c8ddd7 to
8b92870
Compare
nicosuave
force-pushed
the
fix/lookml-default-view-table
branch
from
9202c25 to
33ee7f7
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 33ee7f716c
ℹ️ 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 = {m.name for model in kept.values() for m in (getattr(model, "metrics", None) or [])} | ||
| orphaned = {m.name for name in dropped for m in (getattr(all_models[name], "metrics", None) or [])} - surviving |
There was a problem hiding this comment.
Fresh evidence beyond the prior orphaned-metric issue is that this name-only survivor check keeps a dropped template's graph metric whenever any surviving model has a same-named ordinary measure. For example, a skipped extension: required view with a period_over_period measure pop and a real orders view with a simple measure: pop leaves all_metrics['pop'] pointing at the skipped template metric; after CLI loading, sidemantic info/compile(metrics=['pop']) exposes a graph metric whose base model was removed and fails with no models found. The filter needs to distinguish actual graph-level metrics contributed by surviving models from same-named model-local measures.
Useful? React with 👍 / 👎.
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 👍 / 👎.
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.
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 👍 / 👎.
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.