Stop LookML export from silently turning measures into COUNT#243
Stop LookML export from silently turning measures into COUNT#243nicosuave wants to merge 1 commit into
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: adf80bca51
ℹ️ 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 metric.agg in type_mapping: | ||
| measure_def["type"] = type_mapping[metric.agg] |
There was a problem hiding this comment.
Guard this branch against metric.type before using metric.agg: cumulative metrics can legitimately carry an agg (for example rolling averages use type='cumulative', agg='avg' in tests/metrics/test_cumulative.py), and cohort metrics require an outer agg. Since only time_comparison/derived/ratio are handled earlier, those complex metrics reach this branch and are exported as ordinary LookML measures (type: average, type: count, etc.), silently dropping their window/cohort semantics instead of hitting the skip-and-warn path below.
Useful? React with 👍 / 👎.
| elif metric.agg in sql_agg_funcs and col_sql: | ||
| measure_def["type"] = "number" | ||
| measure_def["sql"] = f"{sql_agg_funcs[metric.agg]}({col_sql})" |
There was a problem hiding this comment.
When a filtered stddev/variance metric takes this new type: number path, the common filter export below still writes LookML filters, but parsing type: number turns the measure into a derived metric and the derived inline-aggregate generator does not apply metric.filters. A round-trip of Metric(agg='stddev', sql='amount', filters=["{model}.status = 'completed'"]) compiles to STDDEV(amount) over all rows, so the filter needs to be folded into the SQL aggregate (or otherwise represented) on this path.
Useful? React with 👍 / 👎.
nicosuave
force-pushed
the
fix/lookml-reference-resolver
branch
from
6093f6f to
af2d6ba
Compare
nicosuave
force-pushed
the
fix/lookml-export-aggregations
branch
from
adf80bc to
e4e46ef
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e4e46ef4b4
ℹ️ 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 metric.agg in type_mapping: | ||
| measure_def["type"] = type_mapping[metric.agg] |
There was a problem hiding this comment.
Because approx_count_distinct is not recognized by this branch, a model with Metric(agg="approx_count_distinct", sql="user_id") now falls through to the warning/continue path and the measure is omitted from the exported LookML. LookML can represent this as a count_distinct measure with approximate: yes, so skipping it removes a valid metric from the view instead of just avoiding the old COUNT corruption.
Useful? React with 👍 / 👎.
| conds = " AND ".join(f.replace("{model}", "${TABLE}") for f in metric.filters) | ||
| inner = f"CASE WHEN {conds} THEN {col_sql} END" |
There was a problem hiding this comment.
When a filtered stddev/variance metric uses a semantic filter such as {model}.status = 'done' and the exported dimension status has SQL like order_status, this raw replacement emits CASE WHEN ${TABLE}.status = 'done' ... and suppresses the LookML filters block. That bypasses the dimension SQL LookML would normally apply, so the exported measure can reference the wrong or nonexistent physical column; resolve simple filter field references against model.dimensions before folding them into SQL.
Useful? React with 👍 / 👎.
nicosuave
force-pushed
the
fix/lookml-reference-resolver
branch
from
af2d6ba to
5efe508
Compare
nicosuave
force-pushed
the
fix/lookml-export-aggregations
branch
from
e4e46ef to
5efe508
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8dad963a43
ℹ️ 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".
| measure_def["sql"] = f"{sql_agg_funcs[metric.agg]}({inner})" | ||
| elif metric.agg is None and col_sql: | ||
| # Non-aggregating measure (opaque SQL expression, no special type). | ||
| measure_def["type"] = "number" |
There was a problem hiding this comment.
When this branch exports an agg-less sql_is_complete measure whose SQL is a plain column (for example Cube/Tesseract string/time/boolean measures like Metric(agg=None, sql="status", sql_is_complete=True)), it writes type: number with sql: status. Re-importing that LookML creates a normal derived metric with sql_is_complete=False, so the SQL generator treats status as a metric dependency and raises Metric status not found instead of querying the preserved column; preserve the opaque metadata/type or avoid exporting these plain-column measures as derived number measures.
Useful? React with 👍 / 👎.
nicosuave
force-pushed
the
fix/lookml-reference-resolver
branch
from
5efe508 to
eeb68b6
Compare
nicosuave
force-pushed
the
fix/lookml-export-aggregations
branch
from
8dad963 to
4815fce
Compare
nicosuave
force-pushed
the
fix/lookml-reference-resolver
branch
from
eeb68b6 to
af2118c
Compare
nicosuave
force-pushed
the
fix/lookml-export-aggregations
branch
from
4815fce to
7e3fe2e
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7e3fe2eba7
ℹ️ 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 _resolve_filter(fstr, _dim_sql=dim_sql): | ||
| return re.sub( | ||
| r"\{model\}\.(\w+)", | ||
| lambda m: _dim_sql.get(m.group(1), m.group(0)), |
There was a problem hiding this comment.
When a filtered stddev/variance metric targets a dimension whose sql is an expression, this substitution splices the expression into the original predicate without grouping it. Fresh evidence after the dimension-resolution fix: a boolean dimension like eligible with SQL {model}.amount > 10 OR {model}.special and filter {model}.eligible = false exports a condition like ${TABLE}.amount > 10 OR ${TABLE}.special = false, which is not equivalent to filtering on the dimension expression as LookML would; wrap resolved dimension SQL in parentheses before preserving the rest of the predicate.
Useful? React with 👍 / 👎.
nicosuave
force-pushed
the
fix/lookml-reference-resolver
branch
from
af2118c to
553c52c
Compare
nicosuave
force-pushed
the
fix/lookml-export-aggregations
branch
from
7e3fe2e to
4380274
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 43802749de
ℹ️ 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".
| # within the surrounding predicate. | ||
| return re.sub( | ||
| r"\{model\}\.(\w+)", | ||
| lambda m: f"({_dim_sql[m.group(1)]})" if m.group(1) in _dim_sql else m.group(0), |
There was a problem hiding this comment.
When a filtered stddev/variance metric references a dimension whose sql is a bare column, this replacement splices (order_status) into the folded type: number SQL and suppresses the LookML filters block, so the exported measure loses the ${TABLE} qualification and can be ambiguous or invalid in joined explores. Fresh evidence in this revision is that the newly added test_lookml_export_folded_filter_resolves_dimension_sql expects (${TABLE}.order_status) but the exporter emits (order_status), causing the LookML edge-case test file to fail.
Useful? React with 👍 / 👎.
nicosuave
force-pushed
the
fix/lookml-export-aggregations
branch
from
4380274 to
8346ee9
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8346ee92f2
ℹ️ 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 agg_type == "count_distinct" and measure_def.get("approximate") in ("yes", True): | ||
| agg_type = "approx_count_distinct" |
There was a problem hiding this comment.
When a LookML type: count_distinct measure has approximate: yes and is referenced by percent_of_total or percent_of_previous, this branch correctly parses the base metric as approx_count_distinct, but the aggregate lookup used by _parse_post_sql_measure is still built earlier from the raw LookML type and wraps ${base} as COUNT(DISTINCT ...). In that scenario the direct metric compiles approximately, while the derived post-SQL metric compiles to exact COUNT(DISTINCT ...), losing the approximation the PR is trying to preserve; the lookup needs to account for approximate: yes too.
Useful? React with 👍 / 👎.
nicosuave
force-pushed
the
fix/lookml-reference-resolver
branch
from
553c52c to
758f3f5
Compare
nicosuave
force-pushed
the
fix/lookml-export-aggregations
branch
from
8346ee9 to
e7c09f7
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e7c09f74ae
ℹ️ 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".
| fstr, | ||
| ) | ||
|
|
||
| conds = " AND ".join(_resolve_filter(f).replace("{model}", "${TABLE}") for f in metric.filters) |
There was a problem hiding this comment.
When a stddev/variance metric has multiple filters and one filter contains an OR expression (for example ['{model}.a = 1 OR {model}.b = 1', '{model}.c = 1']), this join emits a = 1 OR b = 1 AND c = 1. SQL binds AND before OR, so the second filter only applies to the b branch and the exported aggregate includes rows that should have been excluded. Wrap each resolved filter predicate in parentheses before joining them with AND.
Useful? React with 👍 / 👎.
nicosuave
force-pushed
the
fix/lookml-reference-resolver
branch
from
758f3f5 to
4888d7d
Compare
nicosuave
force-pushed
the
fix/lookml-export-aggregations
branch
from
e7c09f7 to
ca47e68
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ca47e68e0c
ℹ️ 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".
| # -- cumulative, conversion, retention, cohort. Skip it rather than | ||
| # exporting a misleading plain aggregation, even when it carries an | ||
| # agg (e.g. a cumulative rolling average has agg='avg'). | ||
| if metric.type is not None: |
There was a problem hiding this comment.
When exporting a graph that was imported from LookML, a type: running_total measure is parsed by _parse_post_sql_measure as Metric(type="cumulative", meta={"table_calculation": "running_total"}); this blanket skip drops that measure from the re-export even though LookML has a faithful running_total representation. In that round-trip scenario the metric disappears instead of being emitted back as a post-SQL measure, so handle the imported table_calculation == "running_total" case before skipping other cumulative metrics.
Useful? React with 👍 / 👎.
nicosuave
force-pushed
the
fix/lookml-reference-resolver
branch
from
4888d7d to
67d45c3
Compare
nicosuave
force-pushed
the
fix/lookml-export-aggregations
branch
from
ca47e68 to
d029bab
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d029bab187
ℹ️ 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 metric.agg is None and col_sql and not getattr(metric, "sql_is_complete", False): | ||
| # Non-aggregating measure (opaque SQL expression, no special type). | ||
| measure_def["type"] = "number" | ||
| measure_def["sql"] = col_sql |
There was a problem hiding this comment.
When a model contains an opaque complete aggregate measure, such as one imported from Cube/Tesseract with Metric(agg=None, sql="SUM({model}.amount)", sql_is_complete=True), this condition excludes it from the type: number export path and the code falls through to the skip warning, so the measure disappears from the LookML export. Plain-column sql_is_complete measures need special handling, but aggregate complete SQL has a faithful LookML representation as type: number with the aggregate SQL and is already treated as valid/queryable elsewhere in the codebase.
Useful? React with 👍 / 👎.
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: b01a0e733e
ℹ️ 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".
| names_alt = "|".join(re.escape(n) for n in sorted(dim_names, key=len, reverse=True)) | ||
| pattern = rf"(?:\{{model\}}|\b{re.escape(model.name)})\.(\w+)" | ||
| if names_alt: | ||
| pattern += rf"|\b({names_alt})\b" |
There was a problem hiding this comment.
When a folded filter contains a qualified field such as customers.status = 'vip' and the current model also has a status dimension, this bare-dimension alternative matches the status token after the dot and rewrites the predicate to customers.(${TABLE}.order_status) = 'vip'. Since the stddev/variance and complete-aggregate paths then mark the filter as folded and suppress the normal LookML filters block, those exports produce malformed SQL instead of preserving or skipping the cross-qualified predicate.
Useful? React with 👍 / 👎.
nicosuave
force-pushed
the
fix/lookml-reference-resolver
branch
from
89410d4 to
e0395e7
Compare
nicosuave
force-pushed
the
fix/lookml-export-aggregations
branch
from
b01a0e7 to
83a40fa
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 83a40fa313
ℹ️ 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".
| names_alt = "|".join(re.escape(n) for n in sorted(dim_names, key=len, reverse=True)) | ||
| pattern = rf"(?:\{{model\}}|\b{re.escape(model.name)})\.(\w+)" | ||
| if names_alt: | ||
| pattern += rf"|(?<![\w.])({names_alt})\b" |
There was a problem hiding this comment.
When a folded filter uses a SQL function whose name also exists as a dimension, this bare-name alternative rewrites the function token itself. For example, with a date dimension and a filtered stddev/complete aggregate using date(created_at) = ..., export produces (${TABLE}.order_date)(...) and suppresses the normal filters block, so the generated LookML SQL is invalid. Restrict bare substitutions so they do not match identifiers immediately used as function/cast names.
Useful? React with 👍 / 👎.
| # so bail rather than emit malformed `THEN price, qty END`. | ||
| if len(cls._split_top_level_commas(arg)) > 1: | ||
| return None | ||
| return f"{func}(CASE WHEN {conds} THEN {arg} END)" |
There was a problem hiding this comment.
When a filtered complete aggregate is wrapped by a scalar function, e.g. ABS(SUM({model}.amount)), sql_has_aggregate routes it into this folder but this fallback treats the outer scalar as the aggregate and emits ABS(CASE WHEN ... THEN SUM(${TABLE}.amount) END). That moves the aggregate inside a row-level CASE and yields invalid or wrong SQL while also suppressing the separate filters block; verify the outer function is the aggregate and the argument is not a nested aggregate, or skip these expressions.
Useful? React with 👍 / 👎.
nicosuave
force-pushed
the
fix/lookml-reference-resolver
branch
from
e0395e7 to
ce7dfcb
Compare
nicosuave
force-pushed
the
fix/lookml-export-aggregations
branch
from
83a40fa to
d226311
Compare
nicosuave
force-pushed
the
fix/lookml-reference-resolver
branch
from
ce7dfcb to
d555a4f
Compare
nicosuave
force-pushed
the
fix/lookml-export-aggregations
branch
from
d226311 to
8fee1ca
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8fee1ca0f0
ℹ️ 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 len(cls._split_top_level_commas(distinct_arg)) > 1: | ||
| return None |
There was a problem hiding this comment.
When a filtered complete aggregate is a valid single-argument DISTINCT expression that contains a comma string, such as COUNT(DISTINCT {model}.a || ',' || {model}.b), this check treats the comma inside the literal as a top-level separator because _split_top_level_commas only tracks brackets. _fold_filters_into_aggregate then returns None, and the caller skips the measure entirely, so exporting drops common delimited composite-key distinct counts instead of folding the filter; make the splitter quote-aware before using it to reject multi-column DISTINCT.
Useful? React with 👍 / 👎.
nicosuave
force-pushed
the
fix/lookml-reference-resolver
branch
from
d555a4f to
f8a6df7
Compare
nicosuave
force-pushed
the
fix/lookml-export-aggregations
branch
from
8fee1ca to
8f9d121
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8f9d121b00
ℹ️ 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".
| names_alt = "|".join(re.escape(n) for n in sorted(dim_names, key=len, reverse=True)) | ||
| pattern = rf"(?:\{{model\}}|\b{re.escape(model.name)})\.(\w+)" | ||
| if names_alt: | ||
| pattern += rf"|(?<![\w.])({names_alt})\b(?!\s*\()" |
There was a problem hiding this comment.
When a folded metric filter contains SQL syntax that shares a name with a dimension, this bare-name alternative rewrites the syntax token as a dimension reference. For example, with a common date dimension, a filtered stddev/complete aggregate using CAST(created_at AS date) = '2024-01-01' exports CAST(created_at AS (${TABLE}.order_date)) and suppresses the separate filters block, leaving invalid LookML SQL. Restrict bare substitutions so SQL type/keyword contexts such as casts are not treated as column operands.
Useful? React with 👍 / 👎.
nicosuave
force-pushed
the
fix/lookml-reference-resolver
branch
from
f8a6df7 to
dd819ab
Compare
nicosuave
force-pushed
the
fix/lookml-export-aggregations
branch
from
8f9d121 to
2ec0bbe
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2ec0bbe8da
ℹ️ 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".
| parts = re.split(r"""('(?:[^']|'')*'|"(?:[^"]|"")*")""", fstr) | ||
| for i in range(0, len(parts), 2): | ||
| parts[i] = ref_re.sub(_one, parts[i]) |
There was a problem hiding this comment.
When a folded stddev/variance or complete-aggregate filter contains a Liquid/Jinja placeholder whose name matches a dimension, this substitution rewrites inside the template because only SQL quotes are protected. For example, with a status dimension, {model}.status = {{ status }} exports as (${TABLE}.order_status) = {{ (${TABLE}.order_status) }}, corrupting the template variable while the normal filters block is suppressed; skip {{ ... }}/{% ... %} segments when resolving bare dimension names.
Useful? React with 👍 / 👎.
nicosuave
force-pushed
the
fix/lookml-reference-resolver
branch
from
dd819ab to
96722f2
Compare
nicosuave
force-pushed
the
fix/lookml-export-aggregations
branch
from
2ec0bbe to
9c8ddd7
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9c8ddd7949
ℹ️ 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"{func}(CASE WHEN {conds} THEN 1 END)" | ||
| # COUNT(DISTINCT x) -> COUNT(DISTINCT CASE WHEN ... THEN x END): DISTINCT stays | ||
| # outside the CASE (it's part of the aggregate, not the value being filtered). | ||
| dm = re.match(r"(?is)^DISTINCT\s+(.+)$", arg) |
There was a problem hiding this comment.
When a filtered complete aggregate uses the valid spelling COUNT(DISTINCT({model}.user_id)), arg becomes DISTINCT(${TABLE}.user_id) and this regex does not match because it requires whitespace after DISTINCT. The code then falls through to the generic wrapper and exports COUNT(CASE WHEN ... THEN DISTINCT(${TABLE}.user_id) END), which is invalid LookML SQL while the separate filter block is suppressed; accept DISTINCT( or skip this form instead of emitting malformed SQL.
Useful? React with 👍 / 👎.
| # folded into the aggregate; if the expression isn't a single | ||
| # foldable FUNC(arg), skip rather than emit a silently-unfiltered | ||
| # measure. | ||
| if not metric.filters and re.fullmatch(r"(?i)count\(\s*(?:\*|\d+)\s*\)", col_sql.strip()): |
There was a problem hiding this comment.
When a complete row-count metric is written with standard whitespace like COUNT (*) or COUNT (1), this guard misses it because the regex requires count( with no space. The export then uses type: number with zero column references; re-importing creates a complete derived metric with no model dependencies, so query generation cannot anchor the metric, which is the same empty-CTE/no-model failure this guard is intended to avoid.
Useful? React with 👍 / 👎.
nicosuave
force-pushed
the
fix/lookml-reference-resolver
branch
from
96722f2 to
5b6a627
Compare
nicosuave
force-pushed
the
fix/lookml-export-aggregations
branch
from
9c8ddd7 to
8b92870
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8b92870298
ℹ️ 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".
| # of a foreign qualifier (e.g. `status` inside `customers.status`), and a negative | ||
| # lookahead for `(` so it does NOT match a function name (e.g. `date(...)`). | ||
| names_alt = "|".join(re.escape(n) for n in sorted(dim_names, key=len, reverse=True)) | ||
| pattern = rf"(?:\{{model\}}|\b{re.escape(model.name)})\.(\w+)" |
There was a problem hiding this comment.
When a folded filter contains a schema-qualified physical predicate for a model named orders, such as schema.orders.status = 'done', this regex still matches the orders.status suffix because . satisfies the word boundary before the model name. The replacement produces malformed SQL like schema.(${TABLE}.order_status) = 'done', and the stddev/complete-aggregate paths then suppress the separate filters block, so the exported measure is broken instead of preserving or skipping that predicate.
Useful? React with 👍 / 👎.
| # (unresolved cross-view ${other.total}, passed through). An EXPRESSION | ||
| # (e.g. "total + tax") is not a valid single ref, so fall through to the | ||
| # skip-with-warning rather than emit malformed `sql: ${total + tax}`. | ||
| if rt_is_running_total and "${" in rt_sql: |
There was a problem hiding this comment.
If an imported running_total expression contains an unresolved cross-view ref plus anything else, e.g. sql: ${other.total} + ${tax}, parsing leaves metric.sql as ${other.total} + tax. This substring check treats that expression as a valid already-braced single reference and re-exports sql: ${other.total} + tax ;;, even though the code only supports a single base-measure reference and the local ref has lost its ${...} braces; require the whole string to match one ${...} reference or skip it.
Useful? React with 👍 / 👎.
nicosuave
force-pushed
the
fix/lookml-reference-resolver
branch
from
5b6a627 to
905fe86
Compare
nicosuave
force-pushed
the
fix/lookml-export-aggregations
branch
from
8b92870 to
39f0919
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 39f09196c4
ℹ️ 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".
| # rewrite refs only in the remaining (even-index) segments so string VALUES, quoted | ||
| # identifiers, and template variables are untouched. The template patterns require | ||
| # DOUBLE braces / brace-percent, so the single-brace {model} placeholder is unaffected. | ||
| parts = re.split(r"""('(?:[^']|'')*'|"(?:[^"]|"")*"|\{\{.*?\}\}|\{%.*?%\})""", fstr) |
There was a problem hiding this comment.
When a filtered stddev/variance or complete aggregate is exported for a dialect that uses backtick identifiers, e.g. a BigQuery filter `status` = 'done' and the model has a status dimension, this splitter does not treat backtick-quoted tokens as protected. The bare-dimension regex then rewrites inside the quoted identifier and emits invalid SQL like `(${TABLE}.order_status)` = 'done' while the normal filters block is suppressed; include backtick/bracket identifier tokens in the protected split or avoid bare substitutions inside them.
Useful? React with 👍 / 👎.
| for ch in arg: | ||
| if ch == "(": | ||
| depth += 1 | ||
| elif ch == ")": | ||
| depth -= 1 | ||
| if depth < 0: | ||
| return None |
There was a problem hiding this comment.
When folding filters into a valid aggregate whose argument contains a parenthesis inside a string literal, such as COUNT(DISTINCT CONCAT({model}.a, ')')), this depth scan counts the literal ) as syntax and returns None. The caller then skips the measure entirely, so filtered complete aggregate exports can drop otherwise foldable metrics that use string delimiters containing parentheses; make this validation quote-aware before rejecting the expression.
Useful? React with 👍 / 👎.
nicosuave
force-pushed
the
fix/lookml-reference-resolver
branch
from
905fe86 to
459b126
Compare
nicosuave
force-pushed
the
fix/lookml-export-aggregations
branch
from
39f0919 to
bf0b444
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bf0b444ea9
ℹ️ 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 _resolve(fstr: str) -> str: | ||
| def _one(m): | ||
| if m.group(2) is not None: |
There was a problem hiding this comment.
When the model has no declared dimensions, names_alt is empty, so ref_re is compiled with only one capture group, but this callback still reads m.group(2). Exporting a filtered stddev/variance or complete aggregate with a qualified filter such as {model}.status = 'done' then raises IndexError instead of producing LookML; this can happen for models that only define measures or rely on auto-discovered columns.
Useful? React with 👍 / 👎.
The measure export fell back to type_mapping.get(metric.agg, "count"), so any aggregation Looker has no entry for was emitted as type: count -- a silent change to a row count that round-trips back as agg=count: - median / stddev / variance exported as COUNT(col) - complex metric types (cumulative/conversion/retention/cohort, agg=None) exported as COUNT(*) Map median to Looker's native type, emit stddev/variance as type: number with an explicit SQL aggregate, export agg-less opaque measures as type: number, and skip-with-warning anything with no LookML equivalent instead of defaulting to count.
nicosuave
force-pushed
the
fix/lookml-export-aggregations
branch
from
bf0b444 to
6416688
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6416688287
ℹ️ 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".
| names_alt = "|".join(re.escape(n) for n in sorted(dim_names, key=len, reverse=True)) | ||
| pattern = rf"(?:\{{model\}}|(?<![\w.]){re.escape(model.name)})\.(\w+)" | ||
| if names_alt: | ||
| pattern += rf"|(?<![\w.])({names_alt})\b(?!\s*\()" |
There was a problem hiding this comment.
When a model has a dimension named date, a folded filter that uses a valid lowercase SQL typed literal such as created_at >= date '2024-01-01' matches this bare-dimension alternative because the token is not followed by ( and is not in an AS/:: cast context. The export then emits created_at >= (${TABLE}.order_date) '2024-01-01' and suppresses the separate LookML filters block, so stddev/variance or complete-aggregate measures with this common date-literal predicate generate invalid LookML SQL.
Useful? React with 👍 / 👎.
1 participant
Summary
Part of the LookML adapter correctness series. Stacked on #242 (base =
fix/lookml-reference-resolver).Measure export used
type_mapping.get(metric.agg, "count"), so any aggregation without a mapping was silently emitted astype: count— corrupting the measure to a row count, which then round-trips back asagg=count.agg=mediantype: count sql: amounttype: median sql: amountagg=stddevtype: count sql: amounttype: number sql: STDDEV(amount)agg=variancetype: count sql: amounttype: number sql: VAR_SAMP(amount)type=cumulative(etc.)type: count(COUNT(*))Changes
medianto the export type map; emit stddev/variance families astype: numberwith an explicit SQL aggregate.type: number.count.Deferred (follow-ups, documented in the audit)
Other export round-trip issues — measure-filter reverse translation, composite primary keys, sub-day time-grain naming, never-exported joins — are separate PRs.