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 29 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 👍 / 👎.
84410ae to
92c08af
Compare
adf80bc to
e4e46ef
Compare
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 👍 / 👎.
e4e46ef to
5efe508
Compare
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 👍 / 👎.
8dad963 to
4815fce
Compare
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 👍 / 👎.
4815fce to
7e3fe2e
Compare
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 👍 / 👎.
4380274 to
8346ee9
Compare
1dfe950 to
0a99120
Compare
8346ee9 to
e7c09f7
Compare
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 👍 / 👎.
e7c09f7 to
ca47e68
Compare
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 👍 / 👎.
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 👍 / 👎.
ec74f18 to
e72acbd
Compare
ca47e68 to
d029bab
Compare
Parsing the tree as one project merges every 'view: +...' found under it, so a stale or alternate-model refinement -- e.g. one left in archive/ -- silently overrode a loaded view's fields or sql_table_name even though the LookML model only includes views/. Per-file parsing used to drop such refinements on the floor, so the whole-project parse introduced this exposure. Resolve each file's include: declarations (a leading / is project-root relative, otherwise relative to the declaring file; // cross-project includes have no local files) and skip refinements from files no include reaches. Scope ONLY the refinement merge: views still all parse, so a project whose includes do not enumerate every view keeps loading them, and a project declaring no includes (the common single-directory case) is unaffected.
Two gaps in the whole-project parse: - Two files under the tree defining the SAME view (an archived copy beside the live one) reached add_model and raised a duplicate-model error, failing the entire load for a repo Looker handles fine. Defer installing base views until every file is parsed, then resolve a same-name collision using the includes -- the included copy wins. Only a COLLISION consults the includes, so a view with no rival is kept regardless and an imperfectly-resolved include can never silently drop one. With no includes declared it is a deterministic first-wins rather than a crash. - LookML allows the .lkml suffix to be omitted (include: /views/*.view or /views/orders.view), which resolved to nothing -- skewing the included set so an included refinement looked un-included, or nothing was scoped and stale refinements merged. Try the suffixed form too.
…licate views Two consequences of loading a LookML tree as one project: - Every file's explores ran against the loaded views, so an archived or alternate model file's joins/segments silently attached to the LIVE model. (Per-file parsing had made all cross-file explores inert, so this path both fixed real explores and exposed stale ones.) Skip explores from files no include reaches. A file DECLARING includes is part of the project -- that is where explores live -- so the active model file is unaffected, and with no includes declared nothing is filtered. - Duplicate-view resolution auto-picked a winner even when BOTH copies were included, masking a real conflict and loading whichever path sorted first. Only auto-resolve when exactly one copy is included (the archived-copy case); when both are, or when there are no includes to distinguish them, install both and let add_model surface the conflict.
Two bugs in the include-based project scoping: - Duplicate views were judged pairwise as files streamed in, so two archived copies sorting before the live one left the first archived candidate queued as an unresolvable duplicate even after the included copy later won -- raising "already exists" for a project the scoping was meant to handle. Group candidates by view name and decide over all of them at once: exactly one included copy wins, otherwise every copy is installed so the conflict surfaces. - Includes were recorded from EVERY .lkml file, so a view file including a helper populated the included set even with no model file selecting the project. Later filtering then treated unrelated sibling refinements and explores as un-included and dropped them from a plain single-directory load. Only a .model.lkml file's includes activate scoping.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ae0d883c62
ℹ️ 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 dim.name.endswith("_" + tf): | ||
| base_name = dim.name[: -(len(tf) + 1)] | ||
| break | ||
| base_name_groups.setdefault((base_name, dim.sql), []).append(dim) |
There was a problem hiding this comment.
When two same-prefix native time dimensions both rely on default column SQL, dim.sql is None for both even though dim.sql_expr points at different columns. For example started and started_hour at hour grain are collapsed into one dimension_group, so the export emits only started_hour backed by ${TABLE}.started and drops/misroutes the original started_hour column on re-import. Use the effective SQL expression when grouping/choosing the group source, not only the explicit sql field.
Useful? React with 👍 / 👎.
Same-prefix time dimensions were grouped by the explicit `sql` field, which is None for any dim relying on its default column. Two such dims -- `started` and `started_hour` at hour grain -- therefore keyed identically and collapsed into one dimension_group, even though they read DIFFERENT columns: the export emitted a single `started_hour` field backed by the `started` column and dropped the other dimension entirely. Key on the effective expression (sql or name) so default-column dims are compared by the column they actually read. Dims that genuinely share a source still group into one dimension_group, and explicit-SQL collisions are unaffected.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 06b755dae3
ℹ️ 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".
| # ONLY a model file's includes activate scoping. A view file may include a helper, which | ||
| # says nothing about which files the project selects -- letting it populate the included | ||
| # set would make unrelated sibling refinements/explores look un-included and drop them. | ||
| if include_specs is not None and file_path.name.endswith(".model.lkml"): |
There was a problem hiding this comment.
When a model file includes /views/orders.view.lkml and that view includes a helper/refinement file, this guard never records the nested include because it only looks at .model.lkml files. included_paths is still non-empty from the model include, so the later refinement/explore filter treats the helper as un-included and skips its view: +orders or explore even though it is reachable through the selected view. That makes valid project refinements disappear in CLI directory loads; follow includes from files already selected by a model rather than ignoring every non-model include.
Useful? React with 👍 / 👎.
Restricting include recording to .model.lkml files kept a stray view file's helper include from switching scoping on, but it also stopped includes being FOLLOWED from a view the model selects: with model -> orders.view -> refine.view, the refinement was reachable through the project yet treated as un-included, so a valid refinement silently disappeared from CLI directory loads. Reachability from a model file is the actual rule, so record every file's includes and resolve them as a transitive closure seeded from the model files. A refinement reached through a selected view now applies; a directory no model selects still activates no scoping at all; and stale un-included refinements and archived duplicate views stay excluded as before.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 828a277026
ℹ️ 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".
| includes_by_file: dict[Path, list[str]] = {} | ||
| for _including_file, _pattern in include_specs: | ||
| includes_by_file.setdefault(_including_file.resolve(), []).append(_pattern) | ||
| _model_files = [f for f in includes_by_file if f.name.endswith(".model.lkml")] |
There was a problem hiding this comment.
When any model file declares include:, this only seeds included_paths from .model.lkml files that themselves had include specs, so a sibling self-contained model file with no includes is not considered included. Its views are still parsed by the first pass, but the explore pass later skips that file because included_paths is non-empty; for example, a valid two.model.lkml containing both view: local and explore: local { sql_always_where: ... } beside an include-based model silently loses the explore segment. Seed all model files (or otherwise exempt model files) rather than only keys present in includes_by_file.
Useful? React with 👍 / 👎.
Seeding the include closure only from model files that declared include: left a self-contained model unseeded, so an include-based sibling switched scoping on and silently dropped the self-contained file's explores. Seed every model file, and scope explores per model file instead: an explore resolves against the views its own file defines plus the views its include closure reaches, as Looker resolves a model's fields. A self-contained model sees its own views; an archived model that includes nothing and defines nothing sees none, so its joins still cannot leak onto the live model.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 11338d77e1
ℹ️ 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".
| # columns -- `started` and `started_hour` -- and keying on None would collapse | ||
| # them into one dimension_group, emitting a single field backed by the wrong | ||
| # column and losing the other dim entirely. | ||
| base_name_groups.setdefault((base_name, dim.sql_expr), []).append(dim) |
There was a problem hiding this comment.
When exporting a parsed LookML dimension_group that has multiple timeframes but omits sql, each generated Dimension has sql=None, so dim.sql_expr falls back to the generated field name (created_date, created_week, etc.). This key then treats those timeframes as different sources and exports later ones as collision standalone dimensions like sql: DATE_TRUNC('week', created_week), instead of round-tripping the original single dimension_group: created { timeframes: [date, week] }; use the shared implicit group source/base name for imported no-SQL groups rather than the generated field name.
Useful? React with 👍 / 👎.
Grouping time dimensions by their effective SQL expression fixed native
default-column dims reading different columns, but broke the opposite case:
an imported dimension_group that omits sql generates dims whose sql is None,
so the effective expression fell back to each generated field name. Every
timeframe then read as its own source and the group was split into standalone
dims backed by columns that never existed, such as
sql: DATE_TRUNC('week', created_week).
Key an imported no-sql group on the group itself: its timeframes share the
implicit column named after the base. The group round-trips unchanged, with
no invented sql, and native default-column dims still stay distinct.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 94237f811e
ℹ️ 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".
| rels = getattr(model, "relationships", None) | ||
| if rels: | ||
| model.relationships = [r for r in rels if r.name not in dropped] | ||
| if all_metrics is not None and dropped: |
There was a problem hiding this comment.
Fresh evidence beyond the earlier template-metric cases is that a later file can overwrite the skipped LookML template in all_models before this cleanup runs, leaving dropped empty. For example, a directory with base.view.lkml marked extension: required and a period_over_period measure pop, plus a Python/native file loaded afterward defining a real Model(name="base"), skips this guarded block and registers the _lookml_template_metric from the template; CLI compile(metrics=["pop"]) then fails with No models found for query. The provenance marker needs to be cleaned up even when the template model was overwritten or otherwise is no longer present in all_models.
Useful? React with 👍 / 👎.
| _file, _view_def, _model = winner | ||
| raw_view_defs[_name] = _view_def | ||
| view_source_files[_name] = str(_file) | ||
| graph.add_model(_model) |
There was a problem hiding this comment.
When include scoping is active, this still installs every unique base view even if no model include reaches its file, so an included view can inherit fields from an archived/unincluded parent. For example, orders.model.lkml including only /views/orders.view.lkml, with orders declaring extends: [base], will still merge archive/base.view.lkml into orders here because base has no duplicate; CLI then exposes fields Looker would not have in that model. Unique un-included views should not participate in inheritance for scoped project loads, or included children should leave those parents unresolved.
Useful? React with 👍 / 👎.
Two independent leaks from an un-included file into a scoped project load. An included view resolved extends against any unique view, including one no model include reaches, merging an archived parent's fields into the child. A unique view is still installed regardless -- an imperfectly-resolved include must never silently drop a view -- but being loaded is not being in scope, so an un-included parent is now treated as absent for inheritance. The child still loads, with its extends left unresolved. The template-metric cleanup was gated on a model having been dropped, so a later file overwriting the template's name skipped it: nothing was dropped, yet the template's auto-registered graph metric survived and broke compile with "No models found for query". The provenance marker is the proof on its own, since the parser only stamps templates, so the cleanup no longer keys off the drop set.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f70201ed57
ℹ️ 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".
| self._parse_explores_from_file( | ||
| lkml_file, graph, allowed_views={v for v, src in _view_files.items() if src in _scope} | ||
| ) |
There was a problem hiding this comment.
When include scoping is active, this only restricts which explores may run by checking the explore's base view; _parse_explore() still appends relationships to any joined view name that was loaded from the tree. Because unique un-included views are still installed, a model file that includes only orders.view.lkml but has join: customers can silently wire queries to an archived/unincluded customers.view.lkml that Looker would not have in that model's scope. The same per-model scope should be applied to join targets (including from: targets), not just the base explore view.
Useful? React with 👍 / 👎.
Scoping only an explore's base view left its joins unrestricted. Unique un-included views are still installed, so a model including just orders.view.lkml could wire join: customers to an archived customers.view.lkml and silently query the wrong table. Apply the same per-model scope to join targets, checking the from: target since that is the view a join alias actually reads. A join to a never-defined view is still kept: that dangling relationship is a real error validate must surface, and dropping it would hide the typo. The base-view check also moves into _parse_explore, which owns the from: fallback resolution -- checking a re-derived base name outside it could drift from the view the explore actually attaches to.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e5d402d02c
ℹ️ 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 included_paths and _resolved not in included_paths: | ||
| logger.debug("Ignoring LookML explores in %s: not reached by any include.", lkml_file) | ||
| continue | ||
| self._parse_explores_from_file(lkml_file, graph) |
There was a problem hiding this comment.
When an active model includes a separate .explore.lkml sidecar, this fall-through parses that included file with allowed_views=None; only .model.lkml files get a scoped view set above. In a project where orders.model.lkml includes orders.view.lkml and orders.explore.lkml, a join in the explore sidecar can still wire to any loaded unique but un-included view such as archive/customers.view.lkml, bypassing the include scoping and sending CLI queries to a stale table.
Useful? React with 👍 / 👎.
Explore scoping only applied to files named *.model.lkml, but explores routinely live in an included sidecar such as orders.explore.lkml. That file fell through to the unscoped branch, so a join in it could wire to any loaded unique view, including an archived one, and send queries to a stale table. Build the scope per file from the include closure instead: every file a model reaches inherits that model's view scope, whatever it is named, and a file reached by several models sees the union -- the closest single-graph answer to Looker's per-model resolution. This subsumes the model-file special case.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a9f281bcaa
ℹ️ 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 not _scoping_active: | ||
| return True | ||
| source = view_source_files.get(name) | ||
| return source is None or Path(source).resolve() in included_paths |
There was a problem hiding this comment.
When a LookML repo has multiple .model.lkml files, this checks the parent against the global union of every included path. If model A includes orders.view.lkml and model B includes base.view.lkml, an orders view with extends: [base] will inherit fields from base even though the model that selected orders cannot see that parent. That lets CLI loads expose fields from another model's include scope; the parent check needs to use the child/source file's own include closure rather than included_paths for the whole project.
Useful? React with 👍 / 👎.
The parent check used the project-wide union of included paths, so with two model files -- A including only orders.view.lkml, B including only base.view.lkml -- orders inherited from base anyway, exposing fields from a model that never selected it. Check the parent against the child's own file scope instead, reusing the per-file scope map built for explores so an extends and an explore in the same file resolve against exactly the same views. That map moves up to just after view installation, where view_source_files is complete.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8daaa88c4e
ℹ️ 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 included_paths and _ridx < len(refinement_source_files): | ||
| _src = refinement_source_files[_ridx].resolve() | ||
| if _src not in included_paths: |
There was a problem hiding this comment.
When a project has multiple .model.lkml files that include the same base view but only one model includes a view: +orders refinement, this check uses the global union of all included paths, so the refinement is merged into the single orders model for every CLI directory load. For example, a staging model that includes +orders { sql_table_name: staging_orders } can make the prod model that only included orders.view.lkml query the staging table/fields; refinements need to be applied per model include closure, or this ambiguity should be surfaced instead of using the global union.
Useful? React with 👍 / 👎.
A refinement was accepted whenever any model included it, so a staging model's
+orders { sql_table_name: staging_orders } silently repointed the view a prod
model uses without it.
Applying refinements per model include closure is not representable: Looker
resolves each model separately, while a graph holds one model per view name
and cannot hold both the refined and unrefined view. Refusing to load a valid
project, or dropping the refinement and mis-serving the model that did select
it, are both worse than the status quo. So the refinement is still applied and
the ambiguity is reported instead of resolved by a coin flip, naming the models
that use the view without it.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d1077f54aa
ℹ️ 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".
| lkml_files = [] | ||
| if source_path.is_dir(): | ||
| lkml_files = list(source_path.rglob("*.lkml")) | ||
| lkml_files = sorted(source_path.rglob("*.lkml")) |
There was a problem hiding this comment.
When a model includes multiple view: +orders refinement files in a deliberate order, this sorted tree walk applies them by pathname instead of by the model's include sequence. Looker refinements are order-sensitive (the include docs note that refinements leverage include order: https://docs.cloud.google.com/looker/docs/reference/param-model-include), so a model that includes z_ref and then a_ref should let a_ref win, but this code loads a_ref before z_ref if the filenames sort that way and can leave the CLI querying the wrong sql_table_name or field override. Build the refinement merge order from the include closure in include order, falling back to sorted order only when there is no model include scope.
Useful? React with 👍 / 👎.
Looker gives the last-included view: +orders precedence, but the tree walk is sorted by pathname for determinism, so a model including z_ref then a_ref applied a_ref first and let z_ref win -- pointing the view at the wrong table. Order the refinement merge by the models' include closure, walked depth-first in declaration order. A glob cannot express an order, so its matches stay sorted, and with no include scope the sorted order still stands: a plain directory load remains deterministic. A file is ordered after the files it includes, since include: brings that content in where it is written and includes sit at the top of a file. Looker documents that refinements follow include order but not how a nested include orders against its includer, so this mirrors plain file inclusion.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 400c9dd27f
ℹ️ 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 this default makes a tableless LookML view queryable and the view has dimension_group: created without sql, the generated dimensions still have sql=None, so compiling orders.created_date reads created_date instead of Looker's implicit base column created. That means this new default-table path can produce SQL against non-existent or wrong columns for otherwise valid LookML; set the generated dimensions' SQL to the group name when the group omits sql.
Useful? React with 👍 / 👎.
| if _scoping_active: | ||
| for _model_file in _model_files: | ||
| for _reached in _ordered_include_closure(_model_file): | ||
| _include_order.setdefault(_reached, len(_include_order)) |
There was a problem hiding this comment.
When two .model.lkml files include the same base view and refinement files in different orders, this global _include_order keeps the order from whichever model file is processed first. Looker applies refinements per model include order, so the other model can get the wrong sql_table_name or field overrides with no warning even though all files are included; detect conflicting per-model orders or avoid collapsing them into one global refinement order.
Useful? React with 👍 / 👎.
A dimension_group without sql reads Looker's implicit <group> column, but its
generated dimensions carried sql=None, so each fell back to its own field name
and compiled DATE_TRUNC('day', created_date) against a column that does not
exist. They now resolve to {model}.<group>.
An implicit group is marked at parse time rather than inferred from sql being
None, so export still tells it from an explicit sql: ${TABLE}.created and
re-emits the group without inventing one.
Also report models that disagree on refinement include order. Looker applies
refinements in each model's own order; one model per view name can hold only
one result, so the first model file's order silently won. Comparing each pair
of a view's refinement files means a model including merely more of them is
not mistaken for a conflict.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4888482f3c
ℹ️ 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".
| _closure = _include_closure(_model_file) | ||
| _allowed = {v for v, src in _view_files.items() if src in _closure} | ||
| for _reached in _closure: | ||
| _scope_by_file.setdefault(_reached, set()).update(_allowed) |
There was a problem hiding this comment.
When the same included .explore.lkml file is reachable from two model files, this union gives that file the combined view set rather than the including model's view set. For example, a shared sidecar included by an orders model that only includes orders and a separate customers model that only includes customers will allow explore: orders { join: customers ... } because customers is in the union, even though no single LookML model includes both views; CLI queries can then attach an out-of-scope/stale table. Fresh evidence is the current _scope_by_file union, which still loses the per-model scope needed by the join filter.
Useful? React with 👍 / 👎.
Giving a sidecar reachable from several models their combined view set let an explore pair a base view from one model with a join target only another model includes -- a pair no single LookML model can see -- wiring queries to an out-of-scope table. An explore resolves within ONE model, so narrow the candidates to the models that see its base view and check each join against those. A join only some of them include is kept for the models that do and the divergence reported, as one model per view name cannot hold both shapes. Extends keeps the union: it is checked from the child's file, and the child is in the scope of every model reaching that file, so "parent in the union" already means one model sees child and parent both. Updates a test that asserted the union behaviour for shared sidecars, which is the case this fixes.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 27c750c190
ℹ️ 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".
| base_sql = "{model}." + group_name | ||
| implicit_sql = True |
There was a problem hiding this comment.
When a no-sql dimension_group is referenced from another field, this fallback only updates the generated Dimension objects after resolved_dimension_sql has already been built. For example dimension_group: created { timeframes: [date] } plus measure: cd { type: count_distinct sql: ${created_date} ;; } still expands the measure to {model}.created_date, while LookML's implicit column is {model}.created; direct created_date queries now use the right column but measures/dimensions/segments that reference it query a non-existent generated field. Seed the lookup entries for implicit group timeframes before resolving field SQL.
Useful? React with 👍 / 👎.
Resolving a no-sql dimension_group's implicit column on the generated
dimensions alone was too late: the lookup that expands ${created_date} in
other fields is built first, so a measure or dimension over the timeframe
still queried the generated field name -- a column that does not exist --
while a direct query of the timeframe was correct. Seed the lookup for
implicit groups too.
Mark implicit groups from the DEFINITION rather than from an unset sql: the
seeded lookup now supplies their SQL, so inferring the mark would have stopped
marking them and made export invent a sql: on a group that never had one.
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.