Fix LookML ${view.field} reference resolution and cycle handling#242
Fix LookML ${view.field} reference resolution and cycle handling#242nicosuave wants to merge 1 commit into
Conversation
nicosuave
force-pushed
the
fix/lookml-filters
branch
from
345f8c2 to
00ff35d
Compare
nicosuave
force-pushed
the
fix/lookml-reference-resolver
branch
from
6093f6f to
af2d6ba
Compare
![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: af2d6ba158
ℹ️ 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_explore(explore_def, graph) | ||
|
|
||
| # Matches ${field} and ${view.field}. ${TABLE} is handled specially. | ||
| _REF_RE = re.compile(r"\$\{(?:([a-zA-Z_]\w*)\.)?([a-zA-Z_]\w*)\}") |
There was a problem hiding this comment.
This new dotted-reference resolver is not used by the type: number measure path, which still applies the old bare-only regex in _parse_measure. In a LookML view with a derived number measure such as measure: margin_pct { type: number sql: ${customers.total} / ${orders.total} ;; }, the self-view ref is normalized but the cross-view ref remains a literal ${customers.total}, so generated SQL still contains invalid LookML syntax despite the new cross-view handling. Route that branch through the same dotted-reference resolver or explicitly handle the two-part match there.
Useful? React with 👍 / 👎.
nicosuave
force-pushed
the
fix/lookml-reference-resolver
branch
from
af2d6ba to
5efe508
Compare
nicosuave
force-pushed
the
fix/lookml-filters
branch
from
00ff35d to
0c91a36
Compare
nicosuave
force-pushed
the
fix/lookml-reference-resolver
branch
from
5efe508 to
eeb68b6
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eeb68b61df
ℹ️ 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".
| view, | ||
| view, | ||
| ) | ||
| return f"{view}.{name}" |
There was a problem hiding this comment.
When a LookML dimension uses a joined-view field such as sql: ${customers.full_name} ;;, this replacement stores the orders dimension SQL as customers.full_name, but querying orders.customer_name only marks orders as required because the generator tracks the requested dimension qualifier rather than parsing dimension.sql. I checked the generated path and it builds an orders_cte that selects customers.full_name AS customer_name from public.orders with no customers join, so imported cross-view dimensions become unqueryable instead of merely avoiding the literal ${...} leak.
Useful? React with 👍 / 👎.
| view, | ||
| ref_name, | ||
| ) | ||
| return f"{view}.{ref_name}" |
There was a problem hiding this comment.
For type: number measures that reference a joined-view dimension, e.g. sql: ${customers.credit_limit} / ${orders.total} ;;, returning customers.credit_limit causes Metric.get_dependencies() to treat the credit_limit column as an unresolved metric dependency and generation raises ValueError: Metric credit_limit not found. This is the same dotted-ref scenario as the earlier comment, but fresh evidence is that the new resolver now converts the literal to customers.credit_limit and the derived-metric builder fails on that column rather than producing valid SQL.
Useful? React with 👍 / 👎.
nicosuave
force-pushed
the
fix/lookml-filters
branch
from
0c91a36 to
8654cb6
Compare
nicosuave
force-pushed
the
fix/lookml-reference-resolver
branch
from
eeb68b6 to
af2118c
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: af2118cb04
ℹ️ 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".
|
|
||
| # Normalize self-view-qualified references (${this_view.field} -> ${field}) | ||
| # so they resolve like bare references instead of leaking literal ${...}. | ||
| view_def = self._strip_self_view_qualifiers(view_def, name.lstrip("+")) |
There was a problem hiding this comment.
When a type: number measure references a self-qualified dimension such as sql: ${orders.amount} / 2 ;;, this normalization now turns it into a bare dimension ref, and the number-measure resolver emits ({model}.amount) / 2. The derived metric dependency parser reads {model}.amount as a dependency named model, so compiling/querying that imported measure raises ValueError: Metric model not found instead of producing SQL; the new tests only inspect metric.sql and don't exercise compilation.
Useful? React with 👍 / 👎.
nicosuave
force-pushed
the
fix/lookml-filters
branch
from
8654cb6 to
9c31524
Compare
nicosuave
force-pushed
the
fix/lookml-reference-resolver
branch
from
af2118c to
553c52c
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 553c52c346
ℹ️ 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".
| number_refs_only_columns = ( | ||
| bool(sql) and not referenced_measure and "{model}" in sql and not sql_has_aggregate(sql) |
There was a problem hiding this comment.
When a LookML type: number measure mixes a self-qualified measure ref with a self-qualified dimension ref, e.g. sql: ${orders.total} / NULLIF(${orders.amount}, 0) ;;, referenced_measure is true so this condition does not mark the expression opaque. The resolved SQL still contains ({model}.amount), and dependency extraction then treats model as a metric dependency, so compiling the imported metric raises ValueError: Metric model not found; the new tests cover measure-only and dimension-only cases but not this mixed form.
Useful? React with 👍 / 👎.
nicosuave
force-pushed
the
fix/lookml-filters
branch
from
9c31524 to
ab41cd8
Compare
nicosuave
force-pushed
the
fix/lookml-reference-resolver
branch
from
553c52c to
758f3f5
Compare
nicosuave
force-pushed
the
fix/lookml-filters
branch
from
ab41cd8 to
0325713
Compare
nicosuave
force-pushed
the
fix/lookml-reference-resolver
branch
from
758f3f5 to
4888d7d
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4888d7dead
ℹ️ 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".
| referenced_measure = True | ||
| return ref_name | ||
|
|
||
| sql = self._REF_RE.sub(resolve_reference, sql) |
There was a problem hiding this comment.
When a self-qualified ref points at a compact LookML dimension with no explicit sql (a supported fixture style), this substitution now turns ${inventory_items.cost} into ${cost}, but dimension_sql_lookup has no entry for compact dimensions, so the resolver falls through and treats cost as a measure dependency. A type: number measure such as sql: ${inventory_items.cost} / 2 ;; imports as cost / 2 and compile() raises ValueError: Metric cost not found; use dimension_names/the dimension's default SQL when resolving these refs instead of only consulting the SQL lookup.
Useful? React with 👍 / 👎.
nicosuave
force-pushed
the
fix/lookml-filters
branch
from
0325713 to
87fbf6d
Compare
nicosuave
force-pushed
the
fix/lookml-reference-resolver
branch
from
4888d7d to
67d45c3
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 67d45c3450
ℹ️ 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".
| name, | ||
| ) | ||
| return match.group(0) | ||
| if name in dimension_sql_lookup: |
There was a problem hiding this comment.
When a self-qualified reference points at a compact dimension, e.g. dimension: cost {} plus dimension: cost_x2 { sql: ${inventory_items.cost} * 2 ;; }, _strip_self_view_qualifiers rewrites it to ${cost} but compact dimensions are never added to dimension_sql_lookup. This branch therefore leaves the literal ${cost} in the imported dimension SQL, and querying that dimension emits invalid LookML syntax in the generated SQL instead of using the dimension's default column expression.
Useful? React with 👍 / 👎.
| number_refs_only_columns = ( | ||
| bool(sql) and not referenced_measure and "{model}" in sql and not sql_has_aggregate(sql) | ||
| ) |
There was a problem hiding this comment.
For LookML dimensions whose SQL is a valid bare column such as sql: amount ;;, a number measure like sql: ${orders.amount} / 2 ;; resolves to (amount) / 2 with referenced_measure == False, but this condition refuses to set sql_is_complete because there is no {model} placeholder. The dependency analyzer then treats amount as a metric dependency and compiling the imported measure raises ValueError: Metric amount not found; the column-only check should not depend on the resolved expression containing {model}.
Useful? React with 👍 / 👎.
nicosuave
force-pushed
the
fix/lookml-filters
branch
from
87fbf6d to
0b87991
Compare
nicosuave
force-pushed
the
fix/lookml-reference-resolver
branch
from
67d45c3 to
50dce1c
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 50dce1c7b5
ℹ️ 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 measure's raw column at generation), so the surrounding | ||
| # row-level columns and this aggregate live in one opaque SQL. | ||
| agg_template = measure_agg_lookup.get(ref_name, "{0}") | ||
| return agg_template.format(f"{{model}}.{ref_name}") |
There was a problem hiding this comment.
When a type: number measure mixes a measure ref with a dimension ref, this expands ${total} to SUM({model}.total), but total is the metric name, not necessarily a physical column. I checked the compiled path for measure: total { type: sum sql: ${TABLE}.amount ;; } plus sql: ${orders.total} / NULLIF(${orders.amount}, 0) ;;: the complete-SQL CTE projects total AS m__total__cmpl instead of using amount AS total_raw, so queries fail or read the wrong column whenever the measure name differs from the source column (and for type: count, it projects a nonexistent count column).
Useful? React with 👍 / 👎.
nicosuave
force-pushed
the
fix/lookml-filters
branch
from
0b87991 to
7b09cac
Compare
nicosuave
force-pushed
the
fix/lookml-reference-resolver
branch
from
50dce1c to
0798ac1
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0798ac1f71
ℹ️ 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 mixed: | ||
| number_refs_only_columns = bool(sql) |
There was a problem hiding this comment.
When a type: number measure mixes a measure ref and a dimension ref, this marks the rewritten SQL complete even though resolve_reference has expanded the measure ref to an aggregate. The complete-SQL builder preserves aggregate expressions and only wraps fully non-aggregate expressions, so a measure like sql: ${orders.total} / NULLIF(${orders.amount}, 0) ;; generates SUM(...)/amount when queried without grouping by amount (or grouped by another dimension), which DuckDB/Postgres reject because the raw dimension column is neither grouped nor aggregated. Keep mixed aggregate/raw expressions out of this complete-SQL path or aggregate/wrap the dimension side before setting this flag.
Useful? React with 👍 / 👎.
nicosuave
force-pushed
the
fix/lookml-filters
branch
from
7b09cac to
3e60168
Compare
nicosuave
force-pushed
the
fix/lookml-reference-resolver
branch
from
0798ac1 to
92dfd41
Compare
nicosuave
force-pushed
the
fix/lookml-reference-resolver
branch
from
d226f3a to
e63cca4
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e63cca4e5c
ℹ️ 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".
| number_refs_only_columns = mixed or ( | ||
| bool(sql) and not referenced_measure and referenced_dimension and not sql_has_aggregate(sql) |
There was a problem hiding this comment.
When a dimension-only type: number measure such as sql: ${orders.amount} / 2 ;; hits this branch, it is marked sql_is_complete, but the complete-SQL generator only wraps non-aggregate expressions when there are grouping dimensions. For a normal metric query with no dimensions, it emits the raw row-level expression from the model CTE, so two input orders return two half_amount rows instead of one scalar metric result; the new test only checks that compilation succeeds, not execution/cardinality. Either skip these row-level expressions or make the complete-SQL path aggregate/collapse them for metric-only queries.
Useful? React with 👍 / 👎.
nicosuave
force-pushed
the
fix/lookml-reference-resolver
branch
from
e63cca4 to
89410d4
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 89410d46e3
ℹ️ 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".
| type=metric_type, | ||
| agg=agg_type, | ||
| sql=sql, | ||
| sql_is_complete=number_refs_only_columns, |
There was a problem hiding this comment.
When an aggregate-safe mixed type: number measure itself has filters on a renamed dimension, e.g. dimension: state { sql: ${TABLE}.status ;; } plus measure: m { type: number sql: ${total} / NULLIF(SUM(${amount}), 0) ;; filters: [state: "completed"] }, this sets sql_is_complete=True while leaving filters as {model}.state = .... The complete-SQL CTE path applies these filters by simply stripping {model} before projecting raw columns, so the generated SQL reads a raw state column instead of the dimension SQL status, failing or filtering on the wrong column; resolve the measure's own filters the same way _folded_measure_filter resolves referenced base-measure filters before marking it complete.
Useful? React with 👍 / 👎.
nicosuave
force-pushed
the
fix/lookml-reference-resolver
branch
from
89410d4 to
e0395e7
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e0395e7812
ℹ️ 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".
| # It's a measure reference - use plain measure_name; the | ||
| # dependency analyzer will resolve this. | ||
| return ref_name |
There was a problem hiding this comment.
When a self-qualified measure ref is combined with an inline aggregate but no dimension refs, e.g. sql: ${orders.completed_count} / NULLIF(COUNT(*), 0) ;;, mixed is false so this leaves completed_count as a bare token. Because the formula contains COUNT(*), the generator takes its inline-aggregate path and skips dependency replacement, emitting completed_count / COUNT(*) even though the CTE only has completed_count_raw, so the query fails at execution. Expand these refs to their full aggregate SQL (or force dependency replacement) whenever the remaining number expression has inline aggregates.
Useful? React with 👍 / 👎.
| # Aggregate-safe mixed exprs (measure refs expanded above) are opaque | ||
| # complete SQL; pure metric-of-metrics ratios stay derived metrics. | ||
| number_refs_only_columns = mixed |
There was a problem hiding this comment.
For a type: number measure that contains only dimension refs inside an aggregate and has LookML filters, such as sql: SUM(${orders.amount}) ;; filters: [status: "completed"], mixed is false so the measure is not marked complete. The generator's inline-aggregate derived path does not apply metric.filters, so after this PR normalizes ${orders.amount} the imported metric returns the unfiltered SUM(amount) instead of the filtered amount. These aggregate dimension-only number measures need the same complete/filter handling or equivalent filter folding.
Useful? React with 👍 / 👎.
nicosuave
force-pushed
the
fix/lookml-reference-resolver
branch
from
e0395e7 to
ce7dfcb
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ce7dfcb4dc
ℹ️ 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".
| has_inline_agg = sql_has_aggregate(sql) | ||
| needs_complete = (referenced_measure and referenced_dimension) or has_inline_agg |
There was a problem hiding this comment.
When a type: number measure uses an inline aggregate with no column references, e.g. sql: COUNT(*) ;; filters: [status: "completed"], this routes it through the complete-SQL path, but that path only applies filters by nulling projected raw columns. _complete_sql_columns returns no columns for COUNT(*), so the generated query remains COUNT(*) over all rows and silently ignores the LookML filter; fold the filter into zero-column aggregates or avoid this complete path for them.
Useful? React with 👍 / 👎.
nicosuave
force-pushed
the
fix/lookml-reference-resolver
branch
from
ce7dfcb to
d555a4f
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d555a4fbfe
ℹ️ 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".
| (c.find_ancestor(exp.AggFunc) is not None or c.find_ancestor(exp.Filter) is not None) | ||
| and c.find_ancestor(exp.Window) is None |
There was a problem hiding this comment.
When a type: number measure uses an ordered-set aggregate such as PERCENTILE_CONT(0.5) WITHIN GROUP (ORDER BY amount), this safety check treats the ORDER BY column as a raw ungrouped column because sqlglot places it under exp.WithinGroup rather than as a descendant of exp.AggFunc. The importer then skips a valid aggregate measure with the “raw ungrouped column” warning, so LookML percentile-style number measures are silently dropped instead of imported as complete aggregate SQL.
Useful? React with 👍 / 👎.
nicosuave
force-pushed
the
fix/lookml-reference-resolver
branch
from
d555a4f to
f8a6df7
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f8a6df71e9
ℹ️ 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 arg is None or isinstance(arg, exp.Star): | ||
| a.set("this", _case(_combined(), exp.Literal.number(1))) | ||
| else: | ||
| a.set("this", _case(_combined(), arg.copy())) |
There was a problem hiding this comment.
When a filtered type: number measure uses an ordered-set aggregate, e.g. PERCENTILE_CONT(0.5) WITHIN GROUP (ORDER BY ${amount}) with filters: [status: "completed"], this fallback rewrites the aggregate's this argument rather than the ordered value. The metric imports as PERCENTILE_CONT(CASE WHEN ... THEN 0.5 END) WITHIN GROUP (ORDER BY amount) and clears the filters, so compilation produces a non-constant percentile parameter (DuckDB rejects it with QUANTILE can only take constant parameters) instead of filtering the ordered values. Handle exp.WithinGroup by applying the filter to the ORDER BY expression or leave this path to the generator rather than wrapping the percentile constant.
Useful? React with 👍 / 👎.
nicosuave
force-pushed
the
fix/lookml-reference-resolver
branch
from
f8a6df7 to
dd819ab
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dd819abad5
ℹ️ 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".
| return all( | ||
| ( | ||
| c.find_ancestor(exp.AggFunc) is not None | ||
| or c.find_ancestor(exp.Filter) is not None | ||
| or c.find_ancestor(exp.WithinGroup) is not None | ||
| ) |
There was a problem hiding this comment.
For type: number measures that use aggregate functions sqlglot parses as exp.Anonymous but sidemantic already recognizes as aggregates (for example PRODUCT, ENTROPY, GEOMETRIC_MEAN, or WEIGHTED_AVG in aggregation_detection), has_inline_agg becomes true and this guard runs, but the argument column is not considered aggregate-scoped because only exp.AggFunc/Filter/WithinGroup ancestors are accepted. A LookML measure such as measure: prod_amt { type: number sql: PRODUCT(${amount}) ;; } is therefore skipped on import as a raw ungrouped column even though DuckDB executes PRODUCT(amount) as an aggregate; include those anonymous aggregate nodes in the scope check.
Useful? React with 👍 / 👎.
nicosuave
force-pushed
the
fix/lookml-reference-resolver
branch
from
dd819ab to
96722f2
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 96722f2a7c
ℹ️ 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".
| folded = self._fold_zero_col_filters(sql, filters) | ||
| if folded is not None: | ||
| sql = folded | ||
| filters = None |
There was a problem hiding this comment.
When a complete LookML number measure has its own filter, this only folds filters for zero-column aggregates and otherwise leaves them for the generator's column-nulling path. If the expanded SQL already contains a referenced base-measure filter such as COUNT(CASE WHEN {model}.status IS NULL THEN 1 END) and the number measure has filters: [country: "US"], rows outside the country filter get status nulled in the CTE, making status IS NULL true and inflating the numerator with out-of-scope rows. These filters need to be folded into the aggregate predicate instead of nulling columns used by nullable predicates.
Useful? React with 👍 / 👎.
| unexpandable = expand_measures and any( | ||
| m.group(1) is None | ||
| and m.group(2) != "TABLE" | ||
| and not _is_dim_ref(m.group(2)) | ||
| and m.group(2) not in measure_full_sql_lookup |
There was a problem hiding this comment.
When a type: number measure needs the complete-SQL path and references a supported distinct base measure plus an aggregate dimension expression, for example ${distinct_total} / NULLIF(SUM(${amount}), 0), this unexpandable check treats the distinct measure as unsupported because only _SQL_AGG_FUNC measures are inserted into measure_full_sql_lookup. The adapter then returns None, silently dropping a valid LookML metric even though _parse_distinct_measure already knows how to generate the distinct aggregate SQL.
Useful? React with 👍 / 👎.
nicosuave
force-pushed
the
fix/lookml-reference-resolver
branch
from
96722f2 to
5b6a627
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5b6a6275f4
ℹ️ 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".
| dm = self._parse_distinct_measure(m_name, m_type, m, resolved_dimension_sql, declared_dim_names) | ||
| if dm and dm.sql: | ||
| measure_full_sql_lookup[m_name] = dm.sql |
There was a problem hiding this comment.
When an aggregate-safe type: number measure references a filtered distinct base measure, this prepass stores only dm.sql, but _parse_distinct_measure does not fold or carry the base measure's filters. For example, completed_sd { type: sum_distinct ... filters: [status: "completed"] } used in rate { type: number sql: ${completed_sd} / NULLIF(SUM(${amount}), 0) ;; } imports as an unfiltered distinct sum, so the rate becomes all amount divided by all amount instead of completed amount divided by all amount. Fold the distinct measure's filters before adding it to measure_full_sql_lookup, or avoid expanding filtered distinct measures here.
Useful? React with 👍 / 👎.
nicosuave
force-pushed
the
fix/lookml-reference-resolver
branch
from
5b6a627 to
905fe86
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 905fe86d91
ℹ️ 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".
| # Otherwise, if every aggregate already references a column the generator can null | ||
| # (nulling the value/ORDER-BY column filters it; aggregates ignore NULLs), the | ||
| # existing path is correct AND consistent -> don't rewrite. | ||
| if not null_sensitive and all(any(True for _ in _scope(a).find_all(exp.Column)) for a in aggs): |
There was a problem hiding this comment.
When a complete type: number measure references an unfiltered keyed sum_distinct/average_distinct measure and has its own filter, this fast path leaves filtering to the generator's raw-column nulling because both symmetric-aggregate SUMs contain columns. For rows outside the filter the key alias becomes NULL, but the generated subtraction still evaluates HASH(NULL) in the offset term, so a filtered expression such as ${sd_total} / SUM(${amount}) can return a huge negative numerator instead of the filtered distinct sum. Detect the keyed/HASH distinct form and fold the filter into the aggregate arguments rather than using column-nulling.
Useful? React with 👍 / 👎.
| col = f"CASE WHEN {joined} THEN {col} END" | ||
| measure_full_sql_lookup[m_name] = agg_template.format(col) |
There was a problem hiding this comment.
When a referenced base count measure explicitly uses sql: * and has filters, this branch wraps the star in CASE WHEN ... THEN * END before expanding it into a complete number measure, producing invalid SQL like COUNT(CASE WHEN status = 'completed' THEN * END). The generator already treats count measures with sql == '*' as the no-SQL count case, so this expansion should route * through the same COUNT(CASE WHEN ... THEN 1 END) path.
Useful? React with 👍 / 👎.
nicosuave
force-pushed
the
fix/lookml-reference-resolver
branch
from
905fe86 to
459b126
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 459b126673
ℹ️ 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".
| return None | ||
| # Complete exprs (mixed or inline-aggregate, measure refs expanded above) | ||
| # are opaque SQL; pure metric-of-metrics ratios stay derived metrics. | ||
| number_refs_only_columns = needs_complete |
There was a problem hiding this comment.
When a LookML type: number measure contains an inline aggregate plus a scalar subquery, e.g. SUM(${amount}) / NULLIF((SELECT SUM(amount) FROM targets), 0), this marks it as complete SQL. The complete-SQL builder rewrites every parsed Column to this measure's CTE raw alias, including the amount inside the targets subquery, so the compiled query becomes a correlated SELECT SUM(orders_cte.pct_target__amount__cmpl) FROM targets and can error or divide by the wrong value. Keep subquery columns out of the complete-SQL alias rewrite or leave these expressions on the derived inline-aggregate path.
Useful? React with 👍 / 👎.
nicosuave
force-pushed
the
fix/lookml-reference-resolver
branch
from
459b126 to
ed32db3
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ed32db3e63
ℹ️ 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".
| parsed_conds = [sqlglot.parse_one(f.replace("{model}", "__MODEL__")) for f in filters] | ||
| except Exception: | ||
| return None | ||
| aggs = list(tree.find_all(exp.AggFunc)) |
There was a problem hiding this comment.
When a filtered complete type: number measure combines a recognized anonymous aggregate with a zero-column aggregate, e.g. PRODUCT(${amount}) / NULLIF(COUNT(*), 0) ;; filters: [country: "US"], this collects only exp.AggFunc nodes, so _fold_complete_sql_filters wraps the COUNT(*), clears filters, and leaves PRODUCT(amount) unfiltered. The generated SQL then computes the product over all rows while dividing by only filtered rows (for amounts 2, 3 in US and 100 in CA it returns 300 instead of 3), so these anonymous aggregate nodes need the same filter folding or the filters must not be cleared.
Useful? React with 👍 / 👎.
nicosuave
force-pushed
the
fix/lookml-filters
branch
from
08f2939 to
7421d74
Compare
nicosuave
force-pushed
the
fix/lookml-reference-resolver
branch
from
ed32db3 to
a09fd24
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a09fd24c5a
ℹ️ 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".
| return f"({measure_full_sql_lookup[ref_name]})" | ||
| # It's a measure reference - use plain measure_name; the | ||
| # dependency analyzer will resolve this. | ||
| return ref_name |
There was a problem hiding this comment.
When ref_name is a filtered inline-aggregate number measure that this adapter marked sql_is_complete, leaving the reference as an ordinary derived dependency makes the generator expand that measure against the raw CTE columns instead of the filtered __cmpl aliases. For example, completed_sum { type: number sql: SUM(${amount}) ;; filters: [status: "completed"] } queried directly returns the completed sum, but double_sum { type: number sql: ${completed_sum} * 2 ;; } compiles as SUM(orders_cte.amount) * 2 and includes pending rows; refs to complete measures need to preserve their complete/filter handling or be expanded through the complete-SQL path.
Useful? React with 👍 / 👎.
The reference resolver regex (${name}) never matched dotted refs, so:
- self-view-qualified refs (${this_view.field}) leaked the literal ${...}
into generated SQL (a hard syntax error); for measures it also broke the
metric. Real models (e.g. the GA360 block) use this form pervasively.
- cross-view refs (${other_view.field}) likewise leaked literally.
- chains deeper than 10 silently truncated; self-references expanded 10x.
Normalize self-view qualifiers to bare refs before resolution, make both
resolvers dotted-ref aware (cross-view -> qualified column + warning, since
sidemantic cannot represent an inline cross-model column), and replace the
fixed depth cap with cycle detection so acyclic chains of any depth resolve
and circular refs terminate.
Note: cross-view field references remain a modeling gap (sidemantic has no
inline cross-model column); the adapter now emits a qualified column and
warns instead of producing invalid ${...} SQL.
nicosuave
force-pushed
the
fix/lookml-reference-resolver
branch
from
a09fd24 to
2e3ab84
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2e3ab84489
ℹ️ 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".
| def fix(value): | ||
| return pat.sub(r"${\1}", value) if isinstance(value, str) else value | ||
|
|
||
| for key in ("dimensions", "dimension_groups", "measures"): |
There was a problem hiding this comment.
When a LookML filter: field is imported as a segment with SQL like sql: ${orders.status} = 'completed' ;;, this normalization path never visits view_def['filters']; _parse_view later only replaces ${TABLE} for those segments. Using that segment therefore leaves ${orders.status} in the generated WHERE clause, which the SQL parser/database rejects, even though the same self-qualified reference now works for dimensions and measures.
Useful? React with 👍 / 👎.
| unexpandable = expand_measures and any( | ||
| m.group(1) is None | ||
| and m.group(2) != "TABLE" | ||
| and not _is_dim_ref(m.group(2)) | ||
| and m.group(2) not in measure_full_sql_lookup | ||
| for m in self._REF_RE.finditer(sql) |
There was a problem hiding this comment.
If an inline-aggregate type: number measure references another derived number measure, e.g. gross_margin { sql: ${revenue} - ${cost} ;; } and avg_margin { sql: ${gross_margin} / NULLIF(COUNT(*), 0) ;; }, this check treats gross_margin as unexpandable because measure_full_sql_lookup is populated only for simple aggregate/distinct measures. The importer then drops a valid aggregate-level LookML measure that Sidemantic can otherwise represent through the derived measure's dependencies; expand derived refs recursively or avoid skipping these cases.
Useful? React with 👍 / 👎.
1 participant
Summary
Part of the LookML adapter correctness series. Stacked on #241 (base =
fix/lookml-filters).The reference resolver's regex
\${name}never matched dotted references, so any${view.field}leaked the literal${...}into generated SQL — a guaranteed database syntax error. Confirmed firsthand: the canonical GA360 block fixture left 26 fields unresolved.Changes
${this_view.field}) are normalized to${field}before resolution, so they resolve like bare refs (common in machine-generated LookML).${other_view.field}) now emit a qualified columnview.fieldplus a warning, instead of leaking${...}.Known limitation
Cross-view field references are a deeper modeling gap: sidemantic cannot represent an inline cross-model column (the generator emits
customers.nameagainstFROM orderswith no join). This PR stops the silent invalid-SQL leak and surfaces a warning; full support is a separate effort.