Fix LookML ${view.field} reference resolution and cycle handling#242
Fix LookML ${view.field} reference resolution and cycle handling#242nicosuave wants to merge 11 commits into
Conversation
345f8c2 to
00ff35d
Compare
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 👍 / 👎.
af2d6ba to
5efe508
Compare
00ff35d to
0c91a36
Compare
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 👍 / 👎.
0c91a36 to
8654cb6
Compare
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 👍 / 👎.
8654cb6 to
9c31524
Compare
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 👍 / 👎.
9c31524 to
ab41cd8
Compare
553c52c to
758f3f5
Compare
ab41cd8 to
0325713
Compare
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 👍 / 👎.
0325713 to
87fbf6d
Compare
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 👍 / 👎.
87fbf6d to
0b87991
Compare
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 👍 / 👎.
0b87991 to
7b09cac
Compare
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 👍 / 👎.
7b09cac to
3e60168
Compare
0798ac1 to
92dfd41
Compare
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 👍 / 👎.
08f2939 to
7421d74
Compare
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 👍 / 👎.
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 👍 / 👎.
2e3ab84 to
a82c26b
Compare
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
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.
A view-level filter: segment's sql (e.g. ${orders.status} = 'completed') was only
${TABLE}-substituted, never run through dimension-reference resolution like
dimensions and measures. Self-qualified and bare ${field} refs therefore leaked an
unresolved ${...} into the generated WHERE clause, which the database rejects.
Add filters to the keys _strip_self_view_qualifiers normalizes, and resolve
dimension references in the segment loop so ${orders.status} becomes the real
column SQL.
Two correctness bugs when a LookML type: number measure references another measure: - A reference to a DERIVED number measure (gross_margin = revenue - cost) was treated as unexpandable because measure_full_sql_lookup only held simple aggregate/distinct measures, so a valid measure like avg_margin = gross_margin / NULLIF(COUNT(*), 0) was silently dropped on import. - A reference to a FILTERED complete number measure (completed_sum with filters: [status: completed]) left as a plain derived dependency expanded against the raw column, so double_sum = completed_sum * 2 computed over ALL rows (360) instead of the filtered value (260). Add a fixpoint second pass that expands type: number measures -- inline-aggregate complete measures (filters folded into a CASE) and pure derived metric-of-metrics -- into measure_full_sql_lookup, resolving chains regardless of declaration order. Track filter-sensitive measures (those carrying a folded filter) and force a referencing measure through the complete-SQL path so the filter is inlined, not dropped. Add a force flag to _fold_complete_sql_filters so an inlined aggregate always bakes its filter into a CASE (no generator column-nulling downstream).
9493294 to
5fe08d7
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5fe08d7cd4
ℹ️ 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 sql_has_aggregate(expanded.replace("{model}", "x")): | ||
| return None |
There was a problem hiding this comment.
When an earlier type: number measure mixes an aggregate measure with a raw dimension, this prepass adds it to measure_full_sql_lookup solely because the expanded SQL contains an aggregate, even though _parse_measure later skips that same measure as invalid. If a later inline-aggregate number measure references it, e.g. bad: ${total} + ${amount} followed by outer: ${bad} / NULLIF(COUNT(*), 0), the later measure expands to SUM(amount) + amount and imports as complete SQL, so grouped queries fail on the raw ungrouped amount column instead of the unsupported intermediate measure being unavailable.
Useful? React with 👍 / 👎.
The type:number expansion prepass added a measure to measure_full_sql_lookup
whenever its expanded SQL contained an aggregate, without applying the
aggregate-safety check _parse_measure uses. A measure mixing an aggregate measure
with a RAW dimension -- bad: ${total} + ${amount} -- is correctly dropped on
import, but the prepass still cached SUM(amount) + amount, so a later
outer: ${bad} / NULLIF(COUNT(*), 0) inlined that raw ungrouped column and imported
as complete SQL, failing on grouped queries instead of being unavailable.
Run _mixed_is_aggregate_safe over the measure's raw SQL before caching it, so the
prepass and _parse_measure agree on which measures are representable.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cf4c21d157
ℹ️ 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 m.group(2) not in measure_full_sql_lookup | ||
| for m in self._REF_RE.finditer(sql) | ||
| ) | ||
| if cross_view or unexpandable or not self._mixed_is_aggregate_safe(sql, _is_dim_ref): |
There was a problem hiding this comment.
When a type: number measure mixes a measure ref with a dimension whose SQL is a constant, this safety check still probes the unresolved ${tax_rate} as a raw column and drops the measure. For example, dimension: tax_rate { sql: 0.07 ;; } plus measure: tax { type: number sql: ${total} * ${tax_rate} ;; } is valid aggregate SQL after resolution (SUM(amount) * 0.07), but it is skipped as if tax_rate were an ungrouped column.
Useful? React with 👍 / 👎.
| ) | ||
| expand_measures = needs_complete and referenced_measure | ||
| if needs_complete: | ||
| if re.search(r"(?is)\bselect\b", sql): |
There was a problem hiding this comment.
This raw regex treats any occurrence of the word select as a subquery, so valid inline aggregate measures are skipped when a string literal contains that value. For example, SUM(CASE WHEN ${status} = 'select' THEN ${amount} END) has no subquery and otherwise imports as complete SQL, but this branch logs the subquery warning and returns None.
Useful? React with 👍 / 👎.
…ect'
Two aggregate-safety checks rejected measures that are perfectly valid:
- The mixed-measure probe substituted every dimension ref with a synthetic
t.<name> column, so a CONSTANT-valued dimension (sql: 0.07 ;;) looked like a raw
ungrouped column and dropped tax = ${total} * ${tax_rate}, which really resolves
to SUM(amount) * 0.07. Probe with the dimension's RESOLVED SQL instead (falling
back to t.<name> only for a compact dimension with no explicit sql).
- Subquery detection scanned for \bselect\b over the raw SQL, so the word inside a
string VALUE -- SUM(CASE WHEN status = 'select' THEN amount END) -- was mistaken
for a subquery. Blank out single-quoted literals before scanning, via a shared
_has_subquery helper used by both the expansion prepass and _parse_measure.
Real subqueries and genuinely raw ungrouped dimension columns are still skipped.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c278e0b653
ℹ️ 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 | ||
| or c.find_ancestor(exp.WithinGroup) is not None | ||
| or _in_anon_agg(c) |
There was a problem hiding this comment.
When a type: number measure uses DuckDB's LIST aggregate inside a numeric expression, such as ARRAY_LENGTH(LIST(${amount})), sql_has_aggregate treats exp.List as an aggregate (sidemantic/sql/aggregation_detection.py:37), so the new complete-SQL path runs. This safety check only accepts AggFunc, aggregate FILTER/WITHIN GROUP, or known anonymous aggregates, and sqlglot parses LIST(...) as exp.List, so the argument column is misclassified as raw and the adapter skips a valid aggregate measure that DuckDB executes; include exp.List as an aggregate-scoped ancestor here.
Useful? React with 👍 / 👎.
aggregation_detection counts sqlglot's exp.List (DuckDB's LIST collector) as an
aggregate, so a measure like ARRAY_LENGTH(LIST(${amount})) takes the complete-SQL
path -- but the safety check only accepted AggFunc/Filter/WITHIN GROUP/anonymous
aggregates as aggregate scope. The argument column was therefore misclassified as
raw and ungrouped, and a valid measure DuckDB executes was skipped on import.
Accept an exp.List ancestor so the two agree. A raw column OUTSIDE the LIST is
still rejected, and a column under a WINDOW is still unsafe.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ef8c602380
ℹ️ 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".
| aggs = list(tree.find_all(exp.AggFunc)) | ||
| aggs += [n for n in tree.find_all(exp.Anonymous) if (n.name or "").lower() in _ANONYMOUS_AGGREGATE_FUNCTIONS] |
There was a problem hiding this comment.
When a filtered type: number measure uses DuckDB's LIST aggregate (for example ARRAY_LENGTH(LIST(${amount})) with filters: [status: "completed"]), the new aggregate-safety path imports it as sql_is_complete, but this filter-folder only collects AggFunc and known anonymous aggregates. Since LIST(...) is parsed separately and DuckDB's list aggregate keeps NULL inputs, the fallback column-nulling filter leaves one list element per base row instead of per matching row, so the metric counts/uses out-of-scope rows. Include exp.List in the aggregates folded here, or avoid the complete path for filtered LIST expressions.
Useful? React with 👍 / 👎.
Making LIST(...) count as aggregate scope let a filtered measure such as
ARRAY_LENGTH(LIST(${amount})) with filters: [status: completed] import as complete
SQL -- but its filter was silently ignored, returning 3 instead of 2.
LIST is unlike every other aggregate: it KEEPS NULL inputs, so no filtering strategy
excludes a row. Column-nulling and a folded CASE both leave one NULL element per
non-matching row (LIST(CASE WHEN s='x' THEN amount END) -> [100, NULL, 30]), so
ARRAY_LENGTH still counts them; only a dialect-specific FILTER (WHERE ...) clause
would work. Folding exp.List into the CASE would therefore NOT fix this, so refuse
the combination: skip with a warning. Unfiltered LIST measures, and filtered
non-LIST aggregates, still import.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a45c43f89e
ℹ️ 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".
| perfectly valid inline aggregate. Blank out single-quoted literals first so only real | ||
| SQL keywords are seen. | ||
| """ | ||
| return bool(re.search(r"(?is)\bselect\b", re.sub(r"'(?:[^']|'')*'", "''", sql or ""))) |
There was a problem hiding this comment.
When an inline-aggregate type: number measure references a quoted column whose name is a reserved word, e.g. sql: SUM(${TABLE}."select") ;;, this regex still sees select and _parse_measure skips the metric as if it contained a scalar subquery. That expression has no subquery and used to be representable, so the importer will silently drop valid LookML measures for schemas with quoted select columns; strip quoted identifiers/comments or use the parsed SQL tree to detect actual subqueries.
Useful? React with 👍 / 👎.
Subquery detection blanked single-quoted literals but not quoted IDENTIFIERS, so a
column named after a reserved word -- SUM(${TABLE}."select"), SUM(`select`),
SUM([select]) -- matched the \bselect\b scan and the valid inline-aggregate measure
was dropped as if it contained a scalar subquery.
Blank every quoted form before scanning (single-quoted literals plus double-quote,
backtick and bracket identifiers -- the same protected set the folded-filter
splitter uses). Real subqueries are still detected, including alongside a quoted
identifier.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a756bebf72
ℹ️ 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 force and not unsafe_nulling and all(any(True for _ in _scope(a).find_all(exp.Column)) for a in aggs): | ||
| return None |
There was a problem hiding this comment.
When a filtered complete LookML number measure uses an aggregate argument that can still return a non-NULL value after column-nulling, this early return leaves the filter to the generator and excluded rows are still counted. For example, type: number sql: COUNT(CASE WHEN ${status} = 'completed' THEN 1 ELSE 0 END) ;; filters: [country: "US"] compiles the CTE as CASE WHEN country='US' THEN status ELSE NULL END, but the outer CASE ... ELSE 0 makes every non-US row non-NULL, so COUNT returns all rows instead of the Looker-equivalent filtered rows; detect CASE defaults (especially under COUNT) as unsafe and fold the filter into the aggregate.
Useful? React with 👍 / 👎.
The generator filters a complete measure by nulling the columns its SQL reads, relying on the aggregate ignoring NULLs. A CASE with an ELSE default defeats that: nulling the predicate's column only makes the WHEN false, and ELSE still yields a non-NULL value, so COUNT(CASE WHEN status='completed' THEN 1 ELSE 0 END) with filters: [country: US] counted EVERY row (3) instead of the filtered ones (2). Treat a CASE carrying an ELSE as unsafe-to-null, alongside the existing IS NULL / COALESCE / HASH cases, so the filter is folded into the aggregate. A CASE without an ELSE, and plain aggregates, keep using the generator's column-nulling.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f3fadc477c
ℹ️ 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_complete_sql_filters(expanded, [joined], force=True) | ||
| if folded is None: | ||
| return None # can't fold the filter safely -> leave unexpandable | ||
| return folded, True |
There was a problem hiding this comment.
When a filtered type: number measure contains LIST(...) plus another aggregate, such as ARRAY_LENGTH(LIST(${amount})) / NULLIF(COUNT(*), 0) with filters: [status: "completed"], the direct parser correctly skips it later because LIST cannot be filtered by CASE/nulling. This prepass still calls _fold_complete_sql_filters, folds only the COUNT(*), caches the partially filtered SQL, and a later measure referencing it imports an unfiltered LIST(amount) numerator over all rows; add the same LIST+filter guard here before storing folded.
Useful? React with 👍 / 👎.
_parse_measure skips a filtered LIST measure because LIST keeps NULL inputs and no
filtering strategy excludes a row. The expansion prepass lacked that guard: for
ARRAY_LENGTH(LIST(${amount})) / NULLIF(COUNT(*), 0) with filters it folded only the
COUNT(*) and cached the partially filtered SQL, so a referencing measure inlined an
UNFILTERED LIST numerator over a filtered denominator -- a silently wrong number.
Apply the same LIST+filter guard before caching, keeping the prepass and the parser
in step (as with the earlier aggregate-safety guard). A referencing measure now
fails loudly on the missing metric instead of returning a wrong value; unfiltered
LIST measures and their referencers are unaffected.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 81c83cea30
ℹ️ 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 makes the WHEN false, and ELSE still yields a non-NULL value, so the aggregate | ||
| # keeps counting the excluded row -- e.g. COUNT(CASE WHEN status='completed' THEN 1 | ||
| # ELSE 0 END) returns EVERY row rather than the filtered ones. | ||
| case_with_default = any(c.args.get("default") is not None for c in tree.find_all(exp.Case)) |
There was a problem hiding this comment.
When a filtered complete type: number measure uses COUNT(IF(${status} = 'completed', 1, 0)) (or a dialect equivalent where NULL takes the false branch), this only treats CASE ... ELSE as unsafe, so _fold_complete_sql_filters returns None because the aggregate contains columns. The generator then applies the measure filter by nulling status; outside the filter IF(NULL, 1, 0) yields 0, and COUNT still counts those rows, inflating the filtered metric. Treat non-null IF/IFF false branches like CASE defaults and fold the filter into the aggregate.
Useful? React with 👍 / 👎.
Only CASE ... ELSE was treated as unsafe to null, so the IF spellings of the same shape slipped through: COUNT(IF(status='completed', 1, 0)) with filters left the generator to null 'status', but IF(NULL, 1, 0) is 0 -- non-NULL -- so COUNT still counted the excluded rows and inflated the metric. Treat a conditional with a non-NULL DEFAULT branch as unsafe: exp.If with a non-None false branch, and Snowflake's IFF (parsed as a 3-arg Anonymous). A CASE's own WHEN clauses are exp.If with false=None, so a plain CASE (no ELSE) and IF(cond, x) with no false branch still use column-nulling and are not over-folded. NVL/IFNULL already parse as Coalesce and were covered.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3246aa7ea8
ℹ️ 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".
| needs_complete = ( | ||
| (referenced_measure and referenced_dimension) or has_inline_agg or refs_filter_sensitive | ||
| ) |
There was a problem hiding this comment.
When a type: number measure references another unfiltered inline-aggregate number measure, e.g. avg_amt { sql: SUM(${amount}) / COUNT(*) } and double_avg { sql: ${avg_amt} * 2 }, this condition leaves double_avg on the plain derived path because refs_filter_sensitive is false. The dependency collector will project only the complete measure's dedicated raw aliases, but _build_metric_sql(avg_amt) expands the nested ref as SUM(orders_cte.amount) / COUNT(*); with no amount dimension requested that column is absent from the CTE, so querying double_avg fails. Force refs to any complete/inline-aggregate number measure through the inlining path, not only filtered ones.
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.