Stop LookML export from silently turning measures into COUNT#243
Stop LookML export from silently turning measures into COUNT#243nicosuave wants to merge 17 commits into
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 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 👍 / 👎.
6093f6f to
af2d6ba
Compare
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 👍 / 👎.
af2d6ba to
5efe508
Compare
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 👍 / 👎.
5efe508 to
eeb68b6
Compare
8dad963 to
4815fce
Compare
eeb68b6 to
af2118c
Compare
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 👍 / 👎.
af2118c to
553c52c
Compare
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 👍 / 👎.
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 👍 / 👎.
553c52c to
758f3f5
Compare
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 👍 / 👎.
758f3f5 to
4888d7d
Compare
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 👍 / 👎.
4888d7d to
67d45c3
Compare
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 👍 / 👎.
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: d1f824ddd7
ℹ️ 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 folded filters contain a qualified predicate whose qualifier is also a dimension, e.g. an orders model with a customers dimension and a filtered stddev/complete aggregate using customers.status = 'vip', this bare-name alternative still matches the customers token because the lookahead only excludes function calls, not a following dot. The export then suppresses the normal filters block and emits malformed SQL such as (${TABLE}.customer_id).status = 'vip'; the current fix only guards identifiers after a dot, not qualifiers before one.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8c548ecbd1
ℹ️ 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 ( | ||
| re.search(r"(?i)\bextract\s*\(\s*$", pre) | ||
| or re.match(r"(?is)\s+from\b", suf) | ||
| or re.search(r"(?i)\binterval\s+(?:[+-]?\d+(?:\.\d+)?|'(?:[^']|'')*')\s*$", pre) | ||
| ): |
There was a problem hiding this comment.
Fresh evidence is that this guard only protects EXTRACT/INTERVAL positions, so folded filters still rewrite unquoted date-part arguments used by dialects such as BigQuery. With a month dimension, a filtered stddev/complete aggregate using DATE_TRUNC(created_at, month) = DATE '2024-01-01' exports as DATE_TRUNC(created_at, (${TABLE}.order_month)) and suppresses the normal filters block, leaving invalid LookML SQL; protect truncation/date-part function arguments before treating bare tokens as dimensions.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 29dac1cd93
ℹ️ 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".
| filters_folded = True | ||
| else: | ||
| measure_def["sql"] = agg_sql | ||
| elif metric.agg is None and col_sql and _sql_has_aggregate(metric.sql or ""): |
There was a problem hiding this comment.
When an opaque complete-SQL metric uses an aggregate such as APPROX_COUNT_DISTINCT({model}.user_id), this check runs aggregate detection on the raw {model} SQL. Sqlglot cannot parse that placeholder, and the fallback aggregate regex does not include approx_count_distinct, so the metric falls through to the skip branch below instead of being exported as type: number; approximate-distinct imports can therefore disappear from LookML export.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7270eaf2d8
ℹ️ 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".
| agg_template = self._SQL_AGG_FUNC.get(m_type) | ||
| # An approximate count_distinct must aggregate approximately when wrapped | ||
| # by a post-SQL measure (percent_of_total/previous), matching the direct metric. | ||
| if m_type == "count_distinct" and m.get("approximate") in ("yes", True): |
There was a problem hiding this comment.
When a LookML count_distinct measure is both approximate: yes and filtered, a post-SQL measure such as percent_of_total over ${uu} now resolves through this aggregate template as APPROX_COUNT_DISTINCT({model}.uu), but the base measure's filters are not included. The first pass already builds a filtered measure_full_sql_lookup for uu, yet _parse_post_sql_measure only receives measure_agg_lookup, so uu's filter is dropped and the percent is computed over all rows instead of the filtered population.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 63a2801638
ℹ️ 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".
| or ( | ||
| bare.lower() in cls._DATE_PART_KEYWORDS | ||
| and (cls._enclosing_function(pre) or "").lower() in cls._DATE_PART_FUNCS | ||
| ) |
There was a problem hiding this comment.
When the real column argument is itself named like a date part, e.g. DATE_TRUNC(date, month) with Dimension(name="date", sql="order_date"), this guard treats date as a date-part solely because it is inside DATE_TRUNC. The folded measure then suppresses the normal filters block and emits DATE_TRUNC(date, month) instead of DATE_TRUNC(${TABLE}.order_date, month), which is wrong or nonexistent for renamed date dimensions; the skip needs to be limited to the function argument positions that are actually date-part tokens.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 80a268969c
ℹ️ 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".
| # EXTRACT(part FROM x) and INTERVAL n part are handled by their own position checks; quoted | ||
| # forms (DATE_TRUNC('day', x)) are already protected as string literals. | ||
| _DATE_PART_ARG_POS = { | ||
| "date_trunc": -1, "datetrunc": -1, "timestamp_trunc": -1, "datetime_trunc": -1, |
There was a problem hiding this comment.
When a folded filter uses SQL Server-style DATETRUNC(month, created_at) and the model also has a month dimension, this table marks datetrunc as if the date part were the last argument. _is_date_part_argument therefore does not protect the first month token, so the bare-dimension replacement rewrites the filter to something like DATETRUNC((${TABLE}.order_month), created_at) while the normal LookML filters block is suppressed, producing invalid exported SQL for that dialect.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 076eb33613
ℹ️ 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".
| # deliberately absent: listing one would protect whatever sits in that slot, including a real | ||
| # keyword-named column. EXTRACT(part FROM x) and INTERVAL n part have their own position checks. | ||
| _DATE_PART_ARG_POS = { | ||
| "date_trunc": -1, "timestamp_trunc": -1, "datetime_trunc": -1, "time_trunc": -1, |
There was a problem hiding this comment.
When exporting folded filters for Snowflake-style DATE_TRUNC(part, expr) predicates, this map treats underscored date_trunc as if the date part is always the last argument. Snowflake documents the syntax as DATE_TRUNC(<date_or_time_part>, <date_or_time_expr>), so a filter like DATE_TRUNC(month, created_at) = ... on a model with a month dimension is rewritten to DATE_TRUNC((${TABLE}.order_month), (${TABLE}.created_at)) while the separate filters block is suppressed, producing invalid/wrong LookML SQL for that dialect.
Useful? React with 👍 / 👎.
| try: | ||
| tree = sqlglot.parse_one(sql.replace("{model}", "__m__").replace("${TABLE}", "__m__")) | ||
| except Exception: | ||
| return True |
There was a problem hiding this comment.
Fresh evidence is that the new column-reference helper treats any parse failure as column-bearing. When a folded row-count filter is only a Liquid/Jinja predicate such as {{ user_filter }}, sqlglot cannot parse the folded COUNT(CASE WHEN ({{ user_filter }}) THEN 1 END), this path returns True, and the zero-column guard lets a type: number measure export; re-importing then treats the template token as raw SQL/columns and compiling the round-tripped metric emits invalid SQL instead of skipping or anchoring the unsupported form.
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.
…d filters
Two LookML export bugs producing invalid re-import SQL:
- COUNT over a non-null constant other than *//digit (COUNT(TRUE), COUNT('x'),
COUNT(1.0)) fell through to type: number and re-imported as a zero-column
complete-SQL metric, whose query hits an empty model CTE (SELECT FROM ...).
Classify every non-null constant COUNT as a native row count (type: count);
COUNT(NULL) is excluded since it is always 0.
- A folded-filter bare-name resolver rewrote date-part / interval-unit keywords
that match a dimension name -- e.g. EXTRACT(day FROM ...) or INTERVAL 7 day on
a model with a 'day' dimension -- into the dimension SQL, emitting invalid LookML
like EXTRACT((${TABLE}.order_day) FROM ...). Protect the EXTRACT-part,
extract-FROM, and INTERVAL-unit positions. Context checks now use absolute
offsets into the full predicate so a split literal (INTERVAL '7' day) is handled.
…en type:number
The constant-count guard only diverted non-null COUNT constants. Other zero-column
aggregates -- COUNT(NULL), COUNT(DISTINCT 1), SUM(1), MAX('x') -- still exported as
type: number with SQL that references no column, so re-importing built an opaque
complete-SQL metric with an empty referenced-column set and compiling it produced an
empty model CTE (SELECT ... FROM with no select list).
Skip an UNFILTERED aggregate whose SQL references no column, with a warning. A
FILTERED zero-column aggregate still folds (COUNT(*) with a filter ->
COUNT(CASE WHEN ... THEN 1 END)), which references the filter's columns and runs.
COUNT(ALL x) with filters folded to COUNT(CASE WHEN ... THEN ALL x END): ALL is an aggregate modifier, not a row expression, so the exported LookML SQL was malformed while the separate filters block was suppressed. Handle ALL exactly like DISTINCT -- keep the modifier outside the CASE (COUNT(ALL CASE WHEN ... THEN x END)) and bail on a multi-column argument. The existing lookahead keeps a column actually named 'all' a plain argument.
Two filtered-complete-aggregate export bugs: - The zero-column guard ran only when the metric had NO filters, but a filter need not reference a column: COUNT(*) with '1 = 1' (or a pure template predicate) folds to COUNT(CASE WHEN (1 = 1) THEN 1 END), which still references none and re-imports as a metric over an empty model CTE. Re-run the zero-column check on the FOLDED SQL; a filter that does reference a column still exports. - An aggregate-local ORDER BY (SUM(amount ORDER BY created_at)) belongs to the aggregate call, not the argument, so wrapping the whole arg emitted SUM(CASE WHEN ... THEN amount ORDER BY created_at END). Detect a top-level ORDER BY (paren-depth 0, outside string literals, word-boundary anchored) and bail so the caller skips instead of exporting malformed SQL.
The bare-dimension alternative's negative lookbehind guards the field AFTER a dot
(status in customers.status), but nothing guarded the QUALIFIER before one. On a
model with a 'customers' dimension, a filter like customers.status = 'vip' rewrote
the qualifier and emitted (${TABLE}.customer_id).status = 'vip' while the separate
filters block was suppressed.
Add a negative lookahead for a following dot: a bare name followed by '.' is a
table qualifier, not a column. The same name used as a real column (no dot) is
still rewritten, as are {model}. and own-model qualified refs.
The date-part guard only covered EXTRACT/INTERVAL positions, but dialects such as
BigQuery pass the part as an unquoted argument: DATE_TRUNC(created_at, month) or
DATE_DIFF(a, b, day). On a model with a 'month' dimension that exported
DATE_TRUNC(created_at, (${TABLE}.order_month)) while suppressing the filters block.
Generalize instead of adding another position pattern: protect a bare token that is
a known date-part KEYWORD appearing inside a call to a known date/time function
(resolved by scanning back for the enclosing call). Gating on the keyword set keeps
a real column argument of the SAME call resolvable (created_at above), and a
keyword-named column outside a date function is still rewritten.
A percent_of_total / percent_of_previous over a base measure that carries its own
LookML filters expanded via the bare <AGG>({model}.<measure>) template, which has no
filter -- so e.g. a percent over a count_distinct filtered to status: completed was
computed across every row instead of the filtered population.
The first pass already builds the FILTERED aggregate for such a base in
measure_full_sql_lookup; expand through it when the base is known to carry filters.
Unfiltered bases keep the existing template form, so the change is limited to the
measures that were losing a filter.
Protecting any date-part KEYWORD inside a date/time function over-protected real columns: DATE_TRUNC(date, month) on a model with both a 'date' and a 'month' dimension left the 'date' COLUMN unresolved, so the folded measure read a nonexistent/renamed column while the filters block stayed suppressed. Protect only the date-part ARGUMENT SLOT, which differs by dialect: BigQuery puts the part LAST (DATE_TRUNC(value, part), DATE_DIFF(a, b, part)); SQL Server FIRST (DATEDIFF(part, a, b), DATEADD(part, n, x)). EXTRACT(part FROM x) and INTERVAL n part keep their own position checks, and quoted parts are already protected as string literals.
…n-part functions
Two errors in the date-part argument-position table:
- DATETRUNC was mapped as part-LAST, conflating it with BigQuery's DATE_TRUNC. The
two spellings differ: underscored DATE_TRUNC is BigQuery's (value, part), while
the unspaced DATETRUNC is SQL Server's (part, value). SQL Server's
DATETRUNC(month, created_at) therefore rewrote the 'month' PART as a column.
(Postgres/DuckDB date_trunc('part', value) quotes the part -- already protected.)
- time_bucket / timestamp_bucket take an INTERVAL, not a bare date part. Listing
them protected whatever sat in that slot, which would silently leave a real
keyword-named COLUMN unresolved. Drop them: only functions that actually take a
bare date-part keyword belong in the table.
… filters
- DATE_TRUNC has NO fixed date-part position: BigQuery is DATE_TRUNC(value, part)
while Snowflake is DATE_TRUNC(part, expr) -- the same name with opposite orders,
and the adapter has no dialect context. Mapping it as part-last rewrote
Snowflake's leading part as a column. Decide by CONTENT instead: whichever
argument is a date-part keyword is the part. When BOTH are keywords
(DATE_TRUNC(date, month)) it is genuinely ambiguous, so the LAST is taken as the
part (BigQuery's order) and the other still resolves as a column -- keeping the
previously-fixed case working.
- The column check reads a sqlglot parse failure as 'has columns', and a Liquid
segment does not parse, so a folded filter that is ONLY a template
(COUNT(CASE WHEN ({{ user_filter }}) THEN 1 END)) slipped past the zero-column
guard and exported a measure with no real column. Neutralise templates to NULL
before parsing; a template combined with a real column still exports, and
genuinely unparseable SQL keeps the safe default.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5b4db67e78
ℹ️ 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 metric.filters and re.fullmatch( | ||
| rf"(?i)count\s*\(\s*(?:{_count_const})\s*\)", col_sql.strip() | ||
| ): |
There was a problem hiding this comment.
When a complete aggregate uses the standard ALL modifier with a non-null constant, e.g. COUNT(ALL 1) or COUNT(ALL TRUE), this native-count guard does not match because it only accepts the constant immediately after COUNT(. The next zero-column check then skips the measure entirely, even though it is semantically the same row count that could be exported as type: count; accept an optional ALL before _count_const instead of dropping these metrics.
Useful? React with 👍 / 👎.
An explicit ALL modifier is the default and does not change the count, but the native-count guard only accepted a constant immediately after COUNT(, so COUNT(ALL 1) / COUNT(ALL TRUE) fell through to the zero-column check and were dropped instead of exporting as type: count. Accept an optional ALL. Also strip the modifier before the column check: sqlglot cannot parse an aggregate's ALL, so EVERY COUNT(ALL ...) was hitting the has-columns fallback rather than a real check -- which let COUNT(ALL NULL) export a broken zero-column type: number. It now skips like COUNT(NULL), while COUNT(ALL <column>) still exports. A column literally named 'all' remains a plain argument.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ffd93af693
ℹ️ 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 arg_index == 1: | ||
| return True |
There was a problem hiding this comment.
Fresh evidence is this new ambiguous DATE_TRUNC branch: when a folded filter uses Snowflake order like DATE_TRUNC(month, date) and date is a renamed dimension, the second argument is treated as the date-part solely because it is argument 2. _fold_filter_conds then leaves DATE_TRUNC(month, date) instead of resolving date to ${TABLE}.order_date, and the separate filters block is suppressed for folded stddev/complete aggregates, so renamed keyword-named date columns compile to wrong or invalid SQL.
Useful? React with 👍 / 👎.
Taking the LAST argument as the part when both are date-part keywords only read BigQuery's DATE_TRUNC(value, part). Under Snowflake's DATE_TRUNC(part, expr) on a model with both a 'date' and a 'month' dimension, DATE_TRUNC(month, date) then resolved 'month' as a column and protected 'date' -- exactly backwards. Neither position is decisive when both arguments look like parts, but coarseness is: truncation goes finer -> coarser, so the COARSER keyword is the part and the other is the value column. That reads DATE_TRUNC(date, month) and DATE_TRUNC(month, date) identically -- month truncates date -- which is correct under either dialect's order, and generalizes (year beats month either way). Ties fall back to BigQuery's order; a single keyword argument still decides on its own.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 22011eadf1
ℹ️ 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".
| first, second = args[0].strip().lower(), args[1].strip().lower() | ||
| first_kw = first in cls._DATE_PART_KEYWORDS | ||
| second_kw = second in cls._DATE_PART_KEYWORDS |
There was a problem hiding this comment.
When a folded filter uses Postgres/DuckDB-style DATE_TRUNC('month', date) and the model has a renamed date dimension, the quoted first argument is protected by the splitter but first_kw stays false because the quotes are still present; the second argument date is then treated as the date-part keyword and left unresolved. Since the filters block is suppressed after folding, the exported SQL keeps date instead of ${TABLE}.order_date, producing wrong or invalid LookML for keyword-named date columns.
Useful? React with 👍 / 👎.
Postgres/DuckDB write the part quoted: DATE_TRUNC('month', date). The literal
splitter already protects that token from rewriting, but the keyword check matched
the raw argument text, so "'month'" never matched the keyword set. The OTHER
argument then looked like the only keyword, and a real column named 'date' was
treated as the part and left unresolved -- while the filters block stayed
suppressed.
Strip surrounding quotes before matching, so a quoted part is recognized and the
column argument resolves. Covers single and double quotes, and leaves the unquoted
BigQuery/Snowflake orders and the both-keyword coarseness tiebreak intact.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 898e28c814
ℹ️ 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"] = col_sql | ||
| elif metric.agg in sql_agg_funcs and col_sql: | ||
| measure_def["type"] = "number" | ||
| agg_sql = f"{sql_agg_funcs[metric.agg]}({col_sql})" |
There was a problem hiding this comment.
When this new stddev/variance branch receives a constant expression, e.g. Metric(agg="stddev", sql="1") or a folded filter that still references no columns, it emits a type: number measure such as STDDEV(1). Unlike the agg is None path below, this branch never calls _aggregate_references_column, so re-importing creates a complete SQL metric with no source columns and querying it hits the empty model CTE failure that the new zero-column guards are trying to avoid; apply the same skip/check after building or folding agg_sql here.
Useful? React with 👍 / 👎.
The stddev/variance branch also emits a type: number measure, but unlike the agg-less path it never checked whether the SQL references a column. A constant expression -- Metric(agg=stddev, sql=1) -> STDDEV(1) -- therefore exported a measure whose re-import builds a complete-SQL metric with no source columns, so compiling it hits the empty model CTE this guard exists to avoid. Apply the same check here: skip an unfiltered aggregate that references no column, and re-check after folding since a filter need not add one either (a constant predicate or a pure template). Column-based aggregates, filtered or not, and natively-mapped aggregates are unaffected.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f945e57bcf
ℹ️ 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"] = folded | ||
| filters_folded = True | ||
| else: | ||
| measure_def["sql"] = col_sql |
There was a problem hiding this comment.
When an opaque complete aggregate uses the explicit ALL modifier over a real column, such as COUNT(ALL {model}.id) or SUM(ALL {model}.amount), this path writes the SQL unchanged as type: number. Re-importing that LookML then drops the measure because the type:number import safety check cannot parse aggregate ALL, even though ALL is the default modifier and these are valid aggregates; strip/normalize ALL before emitting the SQL or teach the import path to accept it.
Useful? React with 👍 / 👎.
An opaque complete aggregate written with the explicit default modifier -- COUNT(ALL id), SUM(ALL amount) -- was exported verbatim as type: number. sqlglot cannot parse an aggregate's ALL, so the type: number import safety check dropped the measure and the round-trip silently lost it. ALL is the default and changes nothing, so strip it on export, leaving equivalent SQL that re-imports. DISTINCT is NOT a no-op and is preserved; a column literally named `all` is untouched (a `(` must precede the modifier, which also leaves `= ALL (SELECT ...)` alone).
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3d6121c347
ℹ️ 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".
| "timestamp_trunc": -1, "datetime_trunc": -1, "time_trunc": -1, | ||
| "date_diff": -1, "timestamp_diff": -1, "datetime_diff": -1, "time_diff": -1, | ||
| "datetrunc": 0, "datediff": 0, "dateadd": 0, "datepart": 0, "date_part": 0, |
There was a problem hiding this comment.
Fresh evidence is that this date-part map omits unspaced TIMESTAMPADD/TIMESTAMPDIFF forms, which use the date part as the first argument. When a model has a day dimension, a folded filter such as TIMESTAMPADD(day, 1, created_at) > ... is rewritten to TIMESTAMPADD((${TABLE}.order_day), 1, ...); since the stddev/complete-aggregate export path then suppresses the separate filters block, the exported LookML SQL is invalid or changes meaning. Add these aliases to the first-argument guard before treating bare tokens as dimensions.
Useful? React with 👍 / 👎.
The date-part argument map omitted the unspaced TIMESTAMPADD / TIMESTAMPDIFF
aliases, which take the part as their FIRST argument. On a model with a `day`
dimension a folded filter like TIMESTAMPADD(day, 1, created_at) rewrote the part
as a column -- TIMESTAMPADD((${TABLE}.order_day), 1, ...) -- while the folded
export suppressed the separate filters block, so the emitted LookML SQL was
invalid.
Add them to the first-argument guard. The column arguments of the same call still
resolve, and a keyword-named column outside a date function is unaffected.
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.