Conversation
Wire up previously unsupported LookML features to existing core model capabilities: view extends/inheritance resolution, refinement merging, label/value_format/drill_fields on dimensions and measures, hidden/ group_label/tags via meta dict, explore from:/description/label/ sql_always_where/always_filter, join type (left_outer/inner/full_outer/ cross), percentile and list measure types.
![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: ecfb773bfb
ℹ️ 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 base_name in graph.models: | ||
| # Create a copy with the base name for merging | ||
| refinement_for_merge = refinement.model_copy(update={"name": base_name}) | ||
| merged = merge_model(refinement_for_merge, graph.models[base_name]) |
There was a problem hiding this comment.
Merging refinements via merge_model(refinement_for_merge, graph.models[base_name]) can clear required base properties, because refinement models are constructed with scalar fields like table, sql, description, and primary_key set (often to None/default) and merge_model treats those as explicit child overrides. In a common view: +base_view { ... } refinement that only adds fields, this can erase the base table or reset PK, producing unresolved models at query time.
Useful? React with 👍 / 👎.
sidemantic/adapters/lookml.py
Outdated
| # Parse joins | ||
| for join_def in explore_def.get("joins") or []: | ||
| relationship = self._parse_join(join_def, explore_name) | ||
| relationship = self._parse_join(join_def, base_model_name) |
There was a problem hiding this comment.
Passing base_model_name into _parse_join breaks aliased explores where sql_on references the explore name (e.g. explore: orders { from: fact_orders ... ${orders.customer_id} ... }). _parse_join checks for the provided base identifier in sql_on and drops the join when it does not match, so valid joins are silently omitted and relationship paths become incomplete.
Useful? React with 👍 / 👎.
sidemantic/adapters/lookml.py
Outdated
| for filter_dict in item: | ||
| if isinstance(filter_dict, dict): | ||
| for field, value in filter_dict.items(): | ||
| filter_sql = self._convert_lookml_filter_to_sql(field, str(value)) |
There was a problem hiding this comment.
Explore always_filter entries commonly use qualified fields like fact_orders.created_date; passing that directly to _convert_lookml_filter_to_sql generates expressions such as {model}.fact_orders.created_date ..., which is not a valid column reference in generated SQL. This makes the generated always-filter segments unusable for filtering.
Useful? React with 👍 / 👎.
1. Refinement scalar preservation: build Model kwargs conditionally so
unset scalars (table, description, primary_key) stay out of
model_fields_set and don't overwrite the base view during merge.
2. Explore alias in sql_on: when an explore uses from: aliasing,
_parse_join now accepts both the view name and the explore alias
when matching references in sql_on, preventing silent join drops.
3. Always filter view qualifier: strip view.field qualifiers (e.g.
fact_orders.created_date -> created_date) before generating SQL,
avoiding double-qualified {model}.view.col expressions.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cec7cd946d
ℹ️ 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_model.segments.append( | ||
| Segment( | ||
| name=segment_name, | ||
| sql=sql_always_where, |
There was a problem hiding this comment.
The explore-level sql_always_where string is stored verbatim as a segment SQL expression, so common LookML references like ${fact_orders.status} are never converted to {model}.status. When that segment is later resolved by the SQL generator, the unreplaced ${...} syntax produces malformed SQL in the WHERE clause instead of a valid column predicate. This makes sql_always_where-derived segments unusable in normal LookML explores that reference fields via ${view.field}.
Useful? React with 👍 / 👎.
sidemantic/adapters/lookml.py
Outdated
| except ValueError: | ||
| # If inheritance resolution fails (missing parent, circular), | ||
| # continue with unresolved models rather than crashing | ||
| pass |
There was a problem hiding this comment.
Inheritance resolution is wrapped in a single try/except over the whole model set, and any ValueError (for one missing/circular parent) causes the adapter to skip resolution for every model. In mixed projects, a single broken extends silently prevents valid parent-child chains from being merged, so models that should inherit dimensions/metrics remain unresolved with no signal to the caller.
Useful? React with 👍 / 👎.
| type="derived", | ||
| sql=percentile_sql, | ||
| description=measure_def.get("description"), |
There was a problem hiding this comment.
The percentile branch always constructs a Metric(type="derived") even when no sql is present, which sets sql=None and triggers Metric validation (derived metric requires 'sql' field). As a result, parsing a LookML view that defines a percentile measure without SQL fails the entire adapter parse instead of degrading gracefully (as other unsupported/placeholder measures do).
Useful? React with 👍 / 👎.
1. sql_always_where ref translation: convert ${view.field} references
to {model}.field so generated SQL is valid, not raw LookML syntax.
2. Resilient inheritance resolution: pre-filter to models whose full
extends chain is present so one missing/circular parent doesn't
block valid chains from resolving.
3. Percentile guard: skip percentile measures with no SQL instead of
constructing a Metric(type="derived", sql=None) that fails validation.
1 participant
Summary
extends:to core inheritance resolution and merge refinements (+view) into base viewslabel,value_format,value_format_name,drill_fields,hidden,group_label,tags(viametadict)from:aliasing (both explore-level and join-level),description,label,sql_always_where,always_filtertypesupport (left_outer,inner,full_outer,cross) stored inRelationship.metadata["join_type"]percentile(PERCENTILE_CONT) andlist(STRING_AGG) measure types as derived metrics