From f51863230d6a18b633da63db525ca207ee27d3cd Mon Sep 17 00:00:00 2001 From: Nico Ritschel Date: Sat, 13 Jun 2026 12:29:06 -0700 Subject: [PATCH 1/4] feat(lookml): add distinct/post-SQL measures and fiscal timeframes Add support for several LookML measure types and dimension_group timeframes that the adapter previously dropped or mishandled: - Distinct aggregate measures (sum_distinct, average_distinct, median_distinct, percentile_distinct), emitted as derived measures with explicit DISTINCT aggregation and honoring sql_distinct_key (preserved in meta). - Post-SQL / table-calculation measures: running_total maps to a cumulative metric over the base measure; percent_of_total and percent_of_previous map to derived window-function measures. - Non-standard / fiscal dimension_group timeframes: fiscal_quarter, fiscal_year, fiscal_month_num, fiscal_quarter_of_year, day_of_week, day_of_week_index, month_name, month_num, week_of_year, quarter_of_year, hour_of_day, day_of_month, day_of_year and related, producing numeric/categorical dimensions with portable extraction SQL while truncation timeframes remain time dimensions. --- sidemantic/adapters/lookml.py | 304 ++++++++++++++++-- .../lookml/test_advanced_measure_types.py | 176 ++++++++++ .../lookml/advanced_measure_types.lkml | 144 +++++++++ 3 files changed, 604 insertions(+), 20 deletions(-) create mode 100644 tests/adapters/lookml/test_advanced_measure_types.py create mode 100644 tests/fixtures/lookml/advanced_measure_types.lkml diff --git a/sidemantic/adapters/lookml.py b/sidemantic/adapters/lookml.py index 78773768d..58f8e0865 100644 --- a/sidemantic/adapters/lookml.py +++ b/sidemantic/adapters/lookml.py @@ -550,30 +550,126 @@ def _parse_dimension_group( if timeframe == "raw": continue # Skip raw timeframe - # Map LookML timeframe to granularity - granularity_mapping = { - "time": "hour", - "date": "day", - "week": "week", - "month": "month", - "quarter": "quarter", - "year": "year", - } + dim = self._build_timeframe_dimension(group_name, timeframe, base_sql, dim_group_def) + if dim is not None: + dimensions.append(dim) - granularity = granularity_mapping.get(timeframe, "day") + return dimensions - dimensions.append( - Dimension( - name=f"{group_name}_{timeframe}", - type="time", - sql=base_sql, - granularity=granularity, - label=dim_group_def.get("label"), - description=dim_group_def.get("description"), - ) + # Timeframes that truncate a timestamp to a coarser time grain. These keep + # type="time" with a Sidemantic granularity so they behave as time dimensions. + _TIME_GRANULARITY_TIMEFRAMES = { + "time": "hour", + "time_of_day": "hour", + "hour": "hour", + "minute": "minute", + "minute15": "minute", + "minute30": "minute", + "second": "second", + "millisecond": "second", + "microsecond": "second", + "date": "day", + "week": "week", + "month": "month", + "quarter": "quarter", + "year": "year", + # Fiscal truncations align to the corresponding calendar grain. + "fiscal_quarter": "quarter", + "fiscal_year": "year", + } + + def _build_timeframe_dimension( + self, group_name: str, timeframe: str, base_sql: str | None, dim_group_def: dict + ) -> Dimension | None: + """Build a single dimension for one dimension_group timeframe. + + Handles both time-truncation timeframes (``date``, ``week``, ``month`` ...) + which become ``type=time`` dimensions, and non-standard "extracted part" + timeframes (``day_of_week``, ``month_name``, ``month_num``, ``fiscal_quarter`` ...) + which become numeric or categorical dimensions with an extraction SQL + expression derived from the base timestamp. + + Args: + group_name: Name of the dimension_group. + timeframe: A single LookML timeframe. + base_sql: The base timestamp SQL ({model}-substituted, refs resolved). + dim_group_def: The dimension_group definition (for label/description). + + Returns: + A Dimension, or None if the timeframe is unrecognized and unusable. + """ + name = f"{group_name}_{timeframe}" + label = dim_group_def.get("label") + description = dim_group_def.get("description") + + # Time-truncation timeframes -> time dimension with granularity. + granularity = self._TIME_GRANULARITY_TIMEFRAMES.get(timeframe) + if granularity is not None: + return Dimension( + name=name, + type="time", + sql=base_sql, + granularity=granularity, + label=label, + description=description, ) - return dimensions + # Non-standard / fiscal "extracted part" timeframes. These return a + # number or a string, not a truncated timestamp, so we emit a + # numeric/categorical dimension with an EXTRACT/strftime-style SQL. + fiscal_offset = dim_group_def.get("fiscal_month_offset") + sql, dim_type = self._timeframe_part_sql(timeframe, base_sql, fiscal_offset) + if sql is None: + return None + return Dimension( + name=name, + type=dim_type, + sql=sql, + label=label, + description=description, + ) + + @staticmethod + def _timeframe_part_sql(timeframe: str, base_sql: str | None, fiscal_offset=None): + """Map a non-truncation LookML timeframe to (sql_expression, dimension_type). + + Uses portable, DuckDB-compatible date functions. ``base_sql`` is the base + timestamp expression. Returns (None, type) if the timeframe is unknown. + """ + expr = base_sql if base_sql is not None else "{model}" + + # Numeric extracted parts (integers). + numeric_parts = { + "hour_of_day": f"EXTRACT(HOUR FROM {expr})", + "day_of_month": f"EXTRACT(DAY FROM {expr})", + "day_of_year": f"EXTRACT(DOY FROM {expr})", + # LookML day_of_week_index: Monday=0 .. Sunday=6 + "day_of_week_index": f"(EXTRACT(ISODOW FROM {expr}) - 1)", + "month_num": f"EXTRACT(MONTH FROM {expr})", + "week_of_year": f"EXTRACT(WEEK FROM {expr})", + "quarter_of_year": f"EXTRACT(QUARTER FROM {expr})", + } + if timeframe in numeric_parts: + return numeric_parts[timeframe], "numeric" + + # String/categorical extracted parts. + if timeframe == "day_of_week": + return f"STRFTIME({expr}, '%A')", "categorical" + if timeframe == "month_name": + return f"STRFTIME({expr}, '%B')", "categorical" + + # Fiscal "month number" honoring fiscal_month_offset (months the fiscal + # year starts after the calendar year). Default offset 0 == calendar. + try: + offset = int(fiscal_offset) if fiscal_offset is not None else 0 + except (TypeError, ValueError): + offset = 0 + if timeframe == "fiscal_month_num": + return f"(((EXTRACT(MONTH FROM {expr}) - 1 - {offset}) % 12) + 1)", "numeric" + if timeframe == "fiscal_quarter_of_year": + return f"(FLOOR(((EXTRACT(MONTH FROM {expr}) - 1 - {offset}) % 12) / 3) + 1)", "numeric" + + return None, "categorical" def _convert_explore_source_to_sql(self, derived_table: dict) -> str: """Convert a native derived table (explore_source) to a SQL representation. @@ -789,6 +885,18 @@ def _parse_measure( # No SQL for list measure - skip it (placeholder) return None + # Handle distinct aggregate measure types. These dedup repeated values + # (e.g. caused by join fanout) using sql_distinct_key when present. + # Looker: sum_distinct, average_distinct, median_distinct, percentile_distinct. + if measure_type in ("sum_distinct", "average_distinct", "median_distinct", "percentile_distinct"): + return self._parse_distinct_measure(name, measure_type, measure_def, dimension_sql_lookup) + + # Handle post-SQL / table-calculation measure types. These reference + # another numeric measure and compute a column-wise calculation. + # Looker: running_total, percent_of_total, percent_of_previous. + if measure_type in ("running_total", "percent_of_total", "percent_of_previous"): + return self._parse_post_sql_measure(name, measure_type, measure_def, dimension_sql_lookup) + # Map LookML measure types to sidemantic aggregation types # Only include types supported by Metric.agg: sum, count, count_distinct, avg, min, max, median type_mapping = { @@ -901,6 +1009,162 @@ def resolve_reference(match): meta=meta or None, ) + def _measure_meta(self, measure_def: dict, extra: dict | None = None) -> dict | None: + """Build the common measure meta dict (hidden/group_label/tags) plus extras.""" + meta: dict = {} + if measure_def.get("hidden") in ("yes", True): + meta["hidden"] = True + if measure_def.get("group_label"): + meta["group_label"] = measure_def["group_label"] + if measure_def.get("tags"): + meta["tags"] = measure_def["tags"] + if extra: + meta.update(extra) + return meta or None + + def _parse_distinct_measure( + self, + name: str, + measure_type: str, + measure_def: dict, + dimension_sql_lookup: dict[str, str], + ) -> Metric | None: + """Parse a distinct aggregate measure (sum/average/median/percentile_distinct). + + These deduplicate the aggregated field across the unique entities defined + by ``sql_distinct_key`` (used to avoid double counting when joins fan out). + We emit a derived measure with an explicit DISTINCT aggregation. When a + ``sql_distinct_key`` is provided it is preserved in ``meta`` so the exact + de-duplication entity is not lost. + + Args: + name: Measure name. + measure_type: One of sum_distinct/average_distinct/median_distinct/percentile_distinct. + measure_def: Raw measure definition. + dimension_sql_lookup: Resolved dimension SQL for ${ref} resolution. + + Returns: + A derived Metric, or None if required SQL is missing. + """ + sql = measure_def.get("sql") + if not sql: + # No field to aggregate -> placeholder in an abstract view, skip. + return None + sql = sql.replace("${TABLE}", "{model}") + sql = self._resolve_dimension_references(sql, dimension_sql_lookup) + + sql_distinct_key = measure_def.get("sql_distinct_key") + if sql_distinct_key: + sql_distinct_key = sql_distinct_key.replace("${TABLE}", "{model}") + sql_distinct_key = self._resolve_dimension_references(sql_distinct_key, dimension_sql_lookup) + + if measure_type == "sum_distinct": + agg_sql = f"SUM(DISTINCT {sql})" + elif measure_type == "average_distinct": + agg_sql = f"AVG(DISTINCT {sql})" + elif measure_type == "median_distinct": + agg_sql = f"MEDIAN(DISTINCT {sql})" + else: # percentile_distinct + percentile_value = measure_def.get("percentile", 50) + fraction = float(percentile_value) / 100.0 + agg_sql = f"PERCENTILE_CONT({fraction}) WITHIN GROUP (ORDER BY DISTINCT {sql})" + + extra = {"distinct": True} + if sql_distinct_key: + extra["sql_distinct_key"] = sql_distinct_key + + return Metric( + name=name, + type="derived", + sql=agg_sql, + description=measure_def.get("description"), + label=measure_def.get("label"), + value_format_name=measure_def.get("value_format_name"), + format=measure_def.get("value_format"), + meta=self._measure_meta(measure_def, extra), + ) + + def _resolve_measure_reference_sql(self, sql: str, dimension_sql_lookup: dict[str, str]) -> str: + """Resolve ${ref} in a measure-referencing SQL (e.g. running_total sql). + + ${dimension} references resolve to the dimension's SQL; ${measure} + references resolve to the bare measure name (sidemantic resolves the + dependency by name). + """ + sql = sql.replace("${TABLE}", "{model}") + + def _resolve(match: re.Match) -> str: + ref_name = match.group(1) + if ref_name == "TABLE": + return match.group(0) + if ref_name in dimension_sql_lookup: + return f"({dimension_sql_lookup[ref_name]})" + return ref_name + + return re.sub(r"\$\{([a-zA-Z_][a-zA-Z0-9_]*)\}", _resolve, sql) + + def _parse_post_sql_measure( + self, + name: str, + measure_type: str, + measure_def: dict, + dimension_sql_lookup: dict[str, str], + ) -> Metric | None: + """Parse a post-SQL / table-calculation measure. + + Looker computes running_total/percent_of_total/percent_of_previous after + the database returns rows, over another numeric measure referenced via the + ``sql`` parameter. We map: + - running_total -> cumulative metric over the base measure + - percent_of_total -> derived measure: base / SUM(base) OVER () + - percent_of_previous -> derived measure: base / LAG(base) OVER () + + Args: + name: Measure name. + measure_type: running_total / percent_of_total / percent_of_previous. + measure_def: Raw measure definition. + dimension_sql_lookup: Resolved dimension SQL for ${ref} resolution. + + Returns: + A Metric, or None if the referenced base measure SQL is missing. + """ + sql = measure_def.get("sql") + if not sql: + # Looker requires sql for these; without it there is nothing to compute. + return None + base = self._resolve_measure_reference_sql(sql, dimension_sql_lookup).strip() + + common = { + "description": measure_def.get("description"), + "label": measure_def.get("label"), + "value_format_name": measure_def.get("value_format_name"), + "format": measure_def.get("value_format"), + } + + if measure_type == "running_total": + return Metric( + name=name, + type="cumulative", + sql=base, + meta=self._measure_meta(measure_def, {"table_calculation": "running_total"}), + **common, + ) + + if measure_type == "percent_of_total": + calc_sql = f"{base} / NULLIF(SUM({base}) OVER (), 0)" + table_calc = "percent_of_total" + else: # percent_of_previous + calc_sql = f"({base} - LAG({base}) OVER ()) / NULLIF(LAG({base}) OVER (), 0)" + table_calc = "percent_of_previous" + + return Metric( + name=name, + type="derived", + sql=calc_sql, + meta=self._measure_meta(measure_def, {"table_calculation": table_calc}), + **common, + ) + def _parse_explore(self, explore_def: dict, graph: SemanticGraph) -> None: """Parse LookML explore and add relationships to models. diff --git a/tests/adapters/lookml/test_advanced_measure_types.py b/tests/adapters/lookml/test_advanced_measure_types.py new file mode 100644 index 000000000..2c99f48c0 --- /dev/null +++ b/tests/adapters/lookml/test_advanced_measure_types.py @@ -0,0 +1,176 @@ +"""Tests for advanced LookML measure types and dimension_group timeframes. + +Covers: + - Distinct aggregate measures: sum_distinct, average_distinct, + median_distinct, percentile_distinct (honoring sql_distinct_key). + - Post-SQL / table-calculation measures: running_total, percent_of_total, + percent_of_previous. + - Non-standard / fiscal dimension_group timeframes: fiscal_quarter, + fiscal_month_num, day_of_week, day_of_week_index, month_name, month_num, + week, week_of_year, quarter_of_year, hour_of_day, etc. +""" + +from pathlib import Path + +import pytest + +from sidemantic.adapters.lookml import LookMLAdapter + +FIXTURES_DIR = Path("tests/fixtures/lookml") + + +@pytest.fixture +def graph(): + adapter = LookMLAdapter() + return adapter.parse(FIXTURES_DIR / "advanced_measure_types.lkml") + + +# ============================================================================= +# DISTINCT AGGREGATE MEASURES +# ============================================================================= + + +class TestDistinctMeasures: + def test_sum_distinct(self, graph): + m = graph.get_model("order_lines").get_metric("total_order_amount") + assert m is not None + assert m.type == "derived" + assert m.sql.startswith("SUM(DISTINCT ") + assert "{model}.order_amount" in m.sql + # sql_distinct_key preserved in meta, resolved to row-level SQL. + assert m.meta["distinct"] is True + assert "{model}.order_id" in m.meta["sql_distinct_key"] + + def test_average_distinct(self, graph): + m = graph.get_model("order_lines").get_metric("avg_order_amount") + assert m.type == "derived" + assert m.sql.startswith("AVG(DISTINCT ") + assert "{model}.order_amount" in m.sql + assert "{model}.order_id" in m.meta["sql_distinct_key"] + + def test_median_distinct(self, graph): + m = graph.get_model("order_lines").get_metric("median_order_amount") + assert m.type == "derived" + assert m.sql.startswith("MEDIAN(DISTINCT ") + assert "{model}.order_amount" in m.sql + + def test_percentile_distinct(self, graph): + m = graph.get_model("order_lines").get_metric("p90_order_amount") + assert m.type == "derived" + # percentile: 90 -> fraction 0.9 + assert "PERCENTILE_CONT(0.9)" in m.sql + assert "WITHIN GROUP (ORDER BY DISTINCT" in m.sql + assert "{model}.order_amount" in m.sql + + def test_distinct_without_sql_distinct_key(self, graph): + m = graph.get_model("order_lines").get_metric("sum_distinct_line_amount") + assert m.type == "derived" + assert m.sql.startswith("SUM(DISTINCT ") + assert "{model}.line_amount" in m.sql + # No sql_distinct_key key present, but still flagged distinct. + assert m.meta["distinct"] is True + assert "sql_distinct_key" not in m.meta + + +# ============================================================================= +# POST-SQL / TABLE-CALCULATION MEASURES +# ============================================================================= + + +class TestPostSqlMeasures: + def test_running_total(self, graph): + m = graph.get_model("order_lines").get_metric("running_line_amount") + assert m is not None + # running_total maps to a cumulative metric over the base measure. + assert m.type == "cumulative" + assert m.sql == "total_line_amount" + assert m.meta["table_calculation"] == "running_total" + + def test_percent_of_total(self, graph): + m = graph.get_model("order_lines").get_metric("pct_of_total_line_amount") + assert m.type == "derived" + assert m.sql == "total_line_amount / NULLIF(SUM(total_line_amount) OVER (), 0)" + assert m.meta["table_calculation"] == "percent_of_total" + + def test_percent_of_previous(self, graph): + m = graph.get_model("order_lines").get_metric("pct_of_previous_line_amount") + assert m.type == "derived" + assert "LAG(total_line_amount) OVER ()" in m.sql + assert m.meta["table_calculation"] == "percent_of_previous" + + +# ============================================================================= +# DIMENSION_GROUP TIMEFRAMES (non-standard / fiscal) +# ============================================================================= + + +class TestDimensionGroupTimeframes: + def test_time_truncation_timeframes_are_time(self, graph): + model = graph.get_model("events_calendar") + for tf, gran in [ + ("occurred_time", "hour"), + ("occurred_date", "day"), + ("occurred_week", "week"), + ("occurred_month", "month"), + ("occurred_quarter", "quarter"), + ("occurred_year", "year"), + ]: + dim = model.get_dimension(tf) + assert dim is not None, f"missing {tf}" + assert dim.type == "time" + assert dim.granularity == gran + + def test_fiscal_truncation_timeframes(self, graph): + model = graph.get_model("events_calendar") + # fiscal_quarter / fiscal_year truncate to a calendar grain. + assert model.get_dimension("occurred_fiscal_quarter").type == "time" + assert model.get_dimension("occurred_fiscal_quarter").granularity == "quarter" + assert model.get_dimension("occurred_fiscal_year").type == "time" + assert model.get_dimension("occurred_fiscal_year").granularity == "year" + + def test_numeric_extracted_parts(self, graph): + model = graph.get_model("events_calendar") + checks = { + "occurred_month_num": "MONTH", + "occurred_week_of_year": "WEEK", + "occurred_quarter_of_year": "QUARTER", + "occurred_hour_of_day": "HOUR", + "occurred_day_of_month": "DAY", + "occurred_day_of_year": "DOY", + "occurred_day_of_week_index": "ISODOW", + } + for name, fn in checks.items(): + dim = model.get_dimension(name) + assert dim is not None, f"missing {name}" + assert dim.type == "numeric", f"{name} should be numeric" + assert fn in dim.sql, f"{name} sql should use {fn}: {dim.sql}" + assert "{model}.occurred_at" in dim.sql + + def test_string_extracted_parts(self, graph): + model = graph.get_model("events_calendar") + dow = model.get_dimension("occurred_day_of_week") + assert dow.type == "categorical" + assert "STRFTIME" in dow.sql and "%A" in dow.sql + + mname = model.get_dimension("occurred_month_name") + assert mname.type == "categorical" + assert "STRFTIME" in mname.sql and "%B" in mname.sql + + def test_fiscal_extracted_parts(self, graph): + model = graph.get_model("events_calendar") + fmn = model.get_dimension("occurred_fiscal_month_num") + assert fmn is not None + assert fmn.type == "numeric" + assert "MONTH" in fmn.sql + + fqoy = model.get_dimension("occurred_fiscal_quarter_of_year") + assert fqoy is not None + assert fqoy.type == "numeric" + + def test_raw_timeframe_skipped(self, graph): + model = graph.get_model("events_calendar") + assert model.get_dimension("occurred_raw") is None + + +if __name__ == "__main__": + pytest.main([__file__, "-v"]) diff --git a/tests/fixtures/lookml/advanced_measure_types.lkml b/tests/fixtures/lookml/advanced_measure_types.lkml new file mode 100644 index 000000000..faf1b32f6 --- /dev/null +++ b/tests/fixtures/lookml/advanced_measure_types.lkml @@ -0,0 +1,144 @@ +# Advanced LookML measure types and dimension_group timeframes. +# Demonstrates: +# - Distinct aggregate measures (sum_distinct, average_distinct, +# median_distinct, percentile_distinct) honoring sql_distinct_key. +# - Post-SQL / table-calculation measures (running_total, percent_of_total, +# percent_of_previous). +# - Non-standard / fiscal dimension_group timeframes (fiscal_quarter, +# fiscal_month_num, day_of_week, day_of_week_index, month_name, month_num, +# week, week_of_year, quarter_of_year, hour_of_day, day_of_month, etc.). + +view: order_lines { + sql_table_name: analytics.order_lines ;; + description: "Denormalized order lines (joins fan out order_id)" + + dimension: id { + type: number + primary_key: yes + sql: ${TABLE}.id ;; + } + + dimension: order_id { + type: number + sql: ${TABLE}.order_id ;; + } + + dimension: order_amount { + type: number + sql: ${TABLE}.order_amount ;; + } + + dimension: line_amount { + type: number + sql: ${TABLE}.line_amount ;; + } + + # Distinct aggregate measures. sql_distinct_key dedupes the fanned-out rows. + measure: total_order_amount { + type: sum_distinct + sql_distinct_key: ${order_id} ;; + sql: ${order_amount} ;; + description: "Sum of order amounts without double counting joined rows" + value_format_name: usd + } + + measure: avg_order_amount { + type: average_distinct + sql_distinct_key: ${order_id} ;; + sql: ${order_amount} ;; + description: "Average order amount across distinct orders" + } + + measure: median_order_amount { + type: median_distinct + sql_distinct_key: ${order_id} ;; + sql: ${order_amount} ;; + description: "Median order amount across distinct orders" + } + + measure: p90_order_amount { + type: percentile_distinct + percentile: 90 + sql_distinct_key: ${order_id} ;; + sql: ${order_amount} ;; + description: "90th percentile order amount across distinct orders" + } + + # Distinct measure without an explicit sql_distinct_key. + measure: sum_distinct_line_amount { + type: sum_distinct + sql: ${line_amount} ;; + } + + # Base measure used by the post-SQL measures below. + measure: total_line_amount { + type: sum + sql: ${line_amount} ;; + value_format_name: usd + } + + # Post-SQL / table-calculation measures referencing a base measure. + measure: running_line_amount { + type: running_total + sql: ${total_line_amount} ;; + description: "Cumulative line amount" + value_format_name: usd + } + + measure: pct_of_total_line_amount { + type: percent_of_total + sql: ${total_line_amount} ;; + description: "Each row's share of the total line amount" + value_format_name: percent_1 + } + + measure: pct_of_previous_line_amount { + type: percent_of_previous + sql: ${total_line_amount} ;; + description: "Change vs the previous row" + value_format_name: percent_1 + } +} + +view: events_calendar { + sql_table_name: analytics.events ;; + description: "Event timestamps with fiscal and non-standard timeframes" + + dimension: id { + type: number + primary_key: yes + sql: ${TABLE}.id ;; + } + + # Mix of time-truncation and extracted-part timeframes, including fiscal ones. + dimension_group: occurred { + type: time + timeframes: [ + raw, + time, + date, + week, + month, + quarter, + year, + fiscal_quarter, + fiscal_year, + fiscal_month_num, + fiscal_quarter_of_year, + day_of_week, + day_of_week_index, + month_name, + month_num, + week_of_year, + quarter_of_year, + hour_of_day, + day_of_month, + day_of_year + ] + sql: ${TABLE}.occurred_at ;; + } + + measure: count { + type: count + } +} From faa2b0dbe7454be9bee20e0163dc037a00ee90d6 Mon Sep 17 00:00:00 2001 From: Nico Ritschel Date: Sun, 14 Jun 2026 07:16:09 -0700 Subject: [PATCH 2/4] fix(lookml): correct distinct-key aggregates, post-SQL bases, and fiscal timeframes - sum_distinct/average_distinct with sql_distinct_key now dedupe by the key entity via a symmetric aggregate instead of SUM(DISTINCT value), so two distinct entities sharing a value are both counted (previously collapsed). Uses DECIMAL casts to avoid float overflow on the hash offset. - percent_of_total / percent_of_previous qualify their base measure ref with {model} and wrap it in the base measure's own aggregate, so the generator resolves it to the base measure's _raw CTE column. Previously the emitted SQL referenced a bare, out-of-scope column and failed to compile. - fiscal_quarter / fiscal_year honor fiscal_month_offset by shifting the timestamp before the calendar truncation, so non-calendar fiscal years bucket into correct fiscal periods instead of calendar boundaries. Adds end-to-end regression tests that execute the compiled SQL against DuckDB. --- sidemantic/adapters/lookml.py | 190 ++++++++++++++++-- .../lookml/test_advanced_measure_types.py | 118 ++++++++++- 2 files changed, 284 insertions(+), 24 deletions(-) diff --git a/sidemantic/adapters/lookml.py b/sidemantic/adapters/lookml.py index 58f8e0865..0c8e8a189 100644 --- a/sidemantic/adapters/lookml.py +++ b/sidemantic/adapters/lookml.py @@ -373,10 +373,28 @@ def _parse_view(self, view_def: dict) -> Model | None: # Build a set of dimension names for measure reference resolution dimension_names = {d.name for d in dimensions} + # Collect measure names + their base aggregation up front so post-SQL + # measures (running_total / percent_of_total / ...) can recognize a + # ${ref} as a base measure, qualify it with {model} (which the generator + # resolves to the measure's _raw column) and wrap it in the base + # measure's own aggregate function. + measure_names: set[str] = set() + measure_agg_lookup: dict[str, str] = {} + for m in view_def.get("measures") or []: + m_name = m.get("name") + if not m_name: + continue + measure_names.add(m_name) + agg_func = self._SQL_AGG_FUNC.get(m.get("type", "count")) + if agg_func: + measure_agg_lookup[m_name] = agg_func + # Parse measures with dimension SQL lookup for reference resolution measures = [] for measure_def in view_def.get("measures") or []: - measure = self._parse_measure(measure_def, dimension_names, resolved_dimension_sql) + measure = self._parse_measure( + measure_def, dimension_names, resolved_dimension_sql, measure_names, measure_agg_lookup + ) if measure: measures.append(measure) @@ -573,9 +591,22 @@ def _parse_dimension_group( "month": "month", "quarter": "quarter", "year": "year", - # Fiscal truncations align to the corresponding calendar grain. - "fiscal_quarter": "quarter", - "fiscal_year": "year", + # NOTE: fiscal_quarter / fiscal_year are intentionally NOT mapped here. + # A plain calendar truncation ignores fiscal_month_offset and buckets + # non-calendar fiscal years incorrectly, so they are handled as offset + # aware truncations in _timeframe_part_sql instead. + } + + # SQL aggregate function for a base measure type, used by post-SQL measures + # (percent_of_total / percent_of_previous) to aggregate the referenced base + # measure before applying the window calculation. + _SQL_AGG_FUNC = { + "sum": "SUM", + "count": "COUNT", + "average": "AVG", + "min": "MIN", + "max": "MAX", + "median": "MEDIAN", } def _build_timeframe_dimension( @@ -614,6 +645,23 @@ def _build_timeframe_dimension( description=description, ) + # Fiscal quarter/year truncations honoring fiscal_month_offset. The base + # timestamp is shifted back by the offset so the generator's calendar + # DATE_TRUNC at the matching grain buckets dates into the correct fiscal + # periods (each distinct fiscal quarter/year maps to a distinct value), + # instead of ignoring the offset and grouping by calendar boundaries. + if timeframe in ("fiscal_quarter", "fiscal_year"): + fiscal_offset = dim_group_def.get("fiscal_month_offset") + shifted_sql, grain = self._fiscal_shifted_sql(timeframe, base_sql, fiscal_offset) + return Dimension( + name=name, + type="time", + sql=shifted_sql, + granularity=grain, + label=label, + description=description, + ) + # Non-standard / fiscal "extracted part" timeframes. These return a # number or a string, not a truncated timestamp, so we emit a # numeric/categorical dimension with an EXTRACT/strftime-style SQL. @@ -629,6 +677,28 @@ def _build_timeframe_dimension( description=description, ) + @staticmethod + def _fiscal_shifted_sql(timeframe: str, base_sql: str | None, fiscal_offset=None) -> tuple[str, str]: + """Build offset-shifted SQL + calendar grain for a fiscal timeframe. + + ``fiscal_month_offset`` is the number of months the fiscal year starts + after January (e.g. an April fiscal-year start is offset 3). The base + timestamp is shifted back by the offset so that a subsequent calendar + DATE_TRUNC at the returned grain (applied by the SQL generator) lands on + fiscal-period boundaries. Offset 0 leaves the timestamp unchanged. + + Returns ``(sql, grain)`` where grain is ``quarter`` or ``year``. + """ + expr = base_sql if base_sql is not None else "{model}" + grain = "quarter" if timeframe == "fiscal_quarter" else "year" + try: + offset = int(fiscal_offset) if fiscal_offset is not None else 0 + except (TypeError, ValueError): + offset = 0 + if offset == 0: + return expr, grain + return f"(({expr}) - INTERVAL ({offset}) MONTH)", grain + @staticmethod def _timeframe_part_sql(timeframe: str, base_sql: str | None, fiscal_offset=None): """Map a non-truncation LookML timeframe to (sql_expression, dimension_type). @@ -783,6 +853,8 @@ def _parse_measure( measure_def: dict, dimension_names: set[str] | None = None, dimension_sql_lookup: dict[str, str] | None = None, + measure_names: set[str] | None = None, + measure_agg_lookup: dict[str, str] | None = None, ) -> Metric | None: """Parse LookML measure. @@ -790,6 +862,8 @@ def _parse_measure( measure_def: Metric definition dimension_names: Set of dimension names in this view (for reference resolution) dimension_sql_lookup: Dict mapping dimension names to their resolved SQL + measure_names: Set of measure names in this view (for base-measure resolution) + measure_agg_lookup: Dict mapping base measure names to their SQL aggregate function Returns: Metric instance or None @@ -895,7 +969,14 @@ def _parse_measure( # another numeric measure and compute a column-wise calculation. # Looker: running_total, percent_of_total, percent_of_previous. if measure_type in ("running_total", "percent_of_total", "percent_of_previous"): - return self._parse_post_sql_measure(name, measure_type, measure_def, dimension_sql_lookup) + return self._parse_post_sql_measure( + name, + measure_type, + measure_def, + dimension_sql_lookup, + measure_names or set(), + measure_agg_lookup or {}, + ) # Map LookML measure types to sidemantic aggregation types # Only include types supported by Metric.agg: sum, count, count_distinct, avg, min, max, median @@ -1058,11 +1139,20 @@ def _parse_distinct_measure( sql_distinct_key = sql_distinct_key.replace("${TABLE}", "{model}") sql_distinct_key = self._resolve_dimension_references(sql_distinct_key, dimension_sql_lookup) - if measure_type == "sum_distinct": + # With a sql_distinct_key, Looker dedupes by the *key entity*, not by the + # aggregated value: two distinct orders that both have amount 10 must + # contribute 20, not collapse to 10. `SUM(DISTINCT value)` deduplicates + # by value and corrupts exactly that case, so sum/average distinct keyed + # measures use a symmetric aggregate (HASH(key)-based) which is the + # fan-out-safe form for keyed deduplication. + if sql_distinct_key and measure_type in ("sum_distinct", "average_distinct"): + agg_sql = self._keyed_distinct_aggregate_sql(measure_type, sql, sql_distinct_key) + elif measure_type == "sum_distinct": agg_sql = f"SUM(DISTINCT {sql})" elif measure_type == "average_distinct": agg_sql = f"AVG(DISTINCT {sql})" elif measure_type == "median_distinct": + # No fan-out-safe inline form for keyed median; dedupe by value. agg_sql = f"MEDIAN(DISTINCT {sql})" else: # percentile_distinct percentile_value = measure_def.get("percentile", 50) @@ -1084,13 +1174,49 @@ def _parse_distinct_measure( meta=self._measure_meta(measure_def, extra), ) - def _resolve_measure_reference_sql(self, sql: str, dimension_sql_lookup: dict[str, str]) -> str: + @staticmethod + def _keyed_distinct_aggregate_sql(measure_type: str, value_sql: str, key_sql: str) -> str: + """Build a fan-out-safe sum/avg over values deduplicated by a key entity. + + Implements LookML ``sum_distinct`` / ``average_distinct`` with a + ``sql_distinct_key`` using a symmetric aggregate: each distinct key + contributes its value exactly once even when joins fan rows out. The + HASH(key) term is cast to DECIMAL alongside the value so large hash + offsets do not lose precision through float arithmetic (which would + otherwise corrupt the result). ``{model}`` placeholders are preserved + for the SQL generator. + """ + # HASH(key) offset, cast to DECIMAL so summing alongside the value stays + # exact; the offset cancels out in the subtraction, leaving the per-key + # value summed once. + offset = f"(HASH({key_sql})::HUGEINT * (1::HUGEINT << 40))::DECIMAL(38, 6)" + value = f"({value_sql})::DECIMAL(38, 6)" + keyed_sum = f"(SUM(DISTINCT {offset} + {value}) - SUM(DISTINCT {offset}))" + if measure_type == "sum_distinct": + return keyed_sum + # average_distinct: keyed sum divided by the number of distinct keys. + return f"({keyed_sum} / NULLIF(COUNT(DISTINCT {key_sql}), 0))" + + def _resolve_measure_reference_sql( + self, + sql: str, + dimension_sql_lookup: dict[str, str], + measure_names: set[str] | None = None, + measure_agg_lookup: dict[str, str] | None = None, + ) -> str: """Resolve ${ref} in a measure-referencing SQL (e.g. running_total sql). - ${dimension} references resolve to the dimension's SQL; ${measure} - references resolve to the bare measure name (sidemantic resolves the - dependency by name). + ${dimension} references resolve to the dimension's SQL. ${measure} + references resolve to ``{model}.``; when ``measure_agg_lookup`` + provides the base measure's aggregate function the reference becomes + ``({model}.)`` so the value is aggregated per group before + the window calculation. The generator's inline-aggregate path then + rewrites ``{model}.`` to the base measure's ``_raw`` + CTE column. A bare ```` would reference a column the model CTE + never exposes (only ``_raw`` exists). """ + measure_names = measure_names or set() + measure_agg_lookup = measure_agg_lookup or {} sql = sql.replace("${TABLE}", "{model}") def _resolve(match: re.Match) -> str: @@ -1099,6 +1225,11 @@ def _resolve(match: re.Match) -> str: return match.group(0) if ref_name in dimension_sql_lookup: return f"({dimension_sql_lookup[ref_name]})" + if ref_name in measure_names: + agg_func = measure_agg_lookup.get(ref_name) + if agg_func: + return f"{agg_func}({{model}}.{ref_name})" + return f"{{model}}.{ref_name}" return ref_name return re.sub(r"\$\{([a-zA-Z_][a-zA-Z0-9_]*)\}", _resolve, sql) @@ -1109,6 +1240,8 @@ def _parse_post_sql_measure( measure_type: str, measure_def: dict, dimension_sql_lookup: dict[str, str], + measure_names: set[str] | None = None, + measure_agg_lookup: dict[str, str] | None = None, ) -> Metric | None: """Parse a post-SQL / table-calculation measure. @@ -1119,11 +1252,17 @@ def _parse_post_sql_measure( - percent_of_total -> derived measure: base / SUM(base) OVER () - percent_of_previous -> derived measure: base / LAG(base) OVER () + The base measure reference is aggregated with its own aggregate function + (via ``measure_agg_lookup``) so percent_of_total / percent_of_previous + operate on the grouped measure value rather than a raw, ungrouped column. + Args: name: Measure name. measure_type: running_total / percent_of_total / percent_of_previous. measure_def: Raw measure definition. dimension_sql_lookup: Resolved dimension SQL for ${ref} resolution. + measure_names: Set of base measure names for ${ref} qualification. + measure_agg_lookup: Base measure name -> SQL aggregate function. Returns: A Metric, or None if the referenced base measure SQL is missing. @@ -1132,24 +1271,37 @@ def _parse_post_sql_measure( if not sql: # Looker requires sql for these; without it there is nothing to compute. return None - base = self._resolve_measure_reference_sql(sql, dimension_sql_lookup).strip() - - common = { - "description": measure_def.get("description"), - "label": measure_def.get("label"), - "value_format_name": measure_def.get("value_format_name"), - "format": measure_def.get("value_format"), - } + measure_names = measure_names or set() + measure_agg_lookup = measure_agg_lookup or {} if measure_type == "running_total": + # A running_total maps to a cumulative metric whose `sql` is the base + # measure; sidemantic resolves that dependency by bare measure name, + # so leave measure refs unqualified here. + base = self._resolve_measure_reference_sql(sql, dimension_sql_lookup).strip() return Metric( name=name, type="cumulative", sql=base, meta=self._measure_meta(measure_def, {"table_calculation": "running_total"}), - **common, + description=measure_def.get("description"), + label=measure_def.get("label"), + value_format_name=measure_def.get("value_format_name"), + format=measure_def.get("value_format"), ) + # percent_of_total / percent_of_previous build window aggregates inline, + # so qualify base measure refs with {model} (for the generator's _raw + # column rewrite) and wrap them in the base measure's aggregate function. + base = self._resolve_measure_reference_sql(sql, dimension_sql_lookup, measure_names, measure_agg_lookup).strip() + + common = { + "description": measure_def.get("description"), + "label": measure_def.get("label"), + "value_format_name": measure_def.get("value_format_name"), + "format": measure_def.get("value_format"), + } + if measure_type == "percent_of_total": calc_sql = f"{base} / NULLIF(SUM({base}) OVER (), 0)" table_calc = "percent_of_total" diff --git a/tests/adapters/lookml/test_advanced_measure_types.py b/tests/adapters/lookml/test_advanced_measure_types.py index 2c99f48c0..16e6cec61 100644 --- a/tests/adapters/lookml/test_advanced_measure_types.py +++ b/tests/adapters/lookml/test_advanced_measure_types.py @@ -35,7 +35,11 @@ def test_sum_distinct(self, graph): m = graph.get_model("order_lines").get_metric("total_order_amount") assert m is not None assert m.type == "derived" - assert m.sql.startswith("SUM(DISTINCT ") + # A sql_distinct_key dedupes by the key entity, not the value: this uses a + # symmetric aggregate keyed on order_id rather than SUM(DISTINCT value), + # so two distinct orders with the same amount are both counted. + assert "SUM(DISTINCT" in m.sql + assert "HASH({model}.order_id)" in m.sql or "HASH(({model}.order_id))" in m.sql assert "{model}.order_amount" in m.sql # sql_distinct_key preserved in meta, resolved to row-level SQL. assert m.meta["distinct"] is True @@ -44,7 +48,9 @@ def test_sum_distinct(self, graph): def test_average_distinct(self, graph): m = graph.get_model("order_lines").get_metric("avg_order_amount") assert m.type == "derived" - assert m.sql.startswith("AVG(DISTINCT ") + # average_distinct keyed on order_id: symmetric keyed sum / distinct keys. + assert "SUM(DISTINCT" in m.sql + assert "COUNT(DISTINCT" in m.sql assert "{model}.order_amount" in m.sql assert "{model}.order_id" in m.meta["sql_distinct_key"] @@ -89,13 +95,17 @@ def test_running_total(self, graph): def test_percent_of_total(self, graph): m = graph.get_model("order_lines").get_metric("pct_of_total_line_amount") assert m.type == "derived" - assert m.sql == "total_line_amount / NULLIF(SUM(total_line_amount) OVER (), 0)" + # The base measure ref is qualified ({model}) and aggregated with its own + # aggregate (SUM) so the generator resolves it to the base measure's _raw + # column instead of an out-of-scope bare `total_line_amount` column. + assert m.sql == "SUM({model}.total_line_amount) / NULLIF(SUM(SUM({model}.total_line_amount)) OVER (), 0)" assert m.meta["table_calculation"] == "percent_of_total" def test_percent_of_previous(self, graph): m = graph.get_model("order_lines").get_metric("pct_of_previous_line_amount") assert m.type == "derived" - assert "LAG(total_line_amount) OVER ()" in m.sql + assert "SUM({model}.total_line_amount)" in m.sql + assert "LAG(SUM({model}.total_line_amount)) OVER ()" in m.sql assert m.meta["table_calculation"] == "percent_of_previous" @@ -122,7 +132,9 @@ def test_time_truncation_timeframes_are_time(self, graph): def test_fiscal_truncation_timeframes(self, graph): model = graph.get_model("events_calendar") - # fiscal_quarter / fiscal_year truncate to a calendar grain. + # fiscal_quarter / fiscal_year are time dimensions truncated at the + # matching grain; with the fixture's default offset (0) the SQL is the + # bare timestamp (offset shifting is exercised separately). assert model.get_dimension("occurred_fiscal_quarter").type == "time" assert model.get_dimension("occurred_fiscal_quarter").granularity == "quarter" assert model.get_dimension("occurred_fiscal_year").type == "time" @@ -172,5 +184,101 @@ def test_raw_timeframe_skipped(self, graph): assert model.get_dimension("occurred_raw") is None +# ============================================================================= +# END-TO-END QUERYABILITY / CORRECTNESS (regression for P1 fixes) +# ============================================================================= + + +class TestAdvancedMeasuresQueryable: + """The distinct-key and post-SQL measures must compile to SQL that both runs + and returns the correct fan-out-safe result.""" + + def _layer(self, graph): + from sidemantic import SemanticLayer + + layer = SemanticLayer() + for model in graph.models.values(): + layer.add_model(model) + return layer + + def _orders_con(self): + import duckdb + + con = duckdb.connect() + con.execute("CREATE SCHEMA IF NOT EXISTS analytics") + con.execute( + "CREATE TABLE analytics.order_lines (id INT, order_id INT, order_amount DOUBLE, line_amount DOUBLE)" + ) + # order 1 fans out to two lines (order_amount=10 on both); order 2 also has + # order_amount=10. A correct keyed sum_distinct must return 20, not 10. + con.execute("INSERT INTO analytics.order_lines VALUES (1,1,10,4),(2,1,10,6),(3,2,10,5)") + return con + + def test_sum_distinct_counts_each_key(self, graph): + con = self._orders_con() + layer = self._layer(graph) + sql = layer.compile(metrics=["order_lines.total_order_amount"]) + (total,) = con.execute(sql).fetchone() + assert float(total) == 20.0 + + def test_average_distinct_is_per_key(self, graph): + con = self._orders_con() + layer = self._layer(graph) + sql = layer.compile(metrics=["order_lines.avg_order_amount"]) + (avg,) = con.execute(sql).fetchone() + assert float(avg) == 10.0 + + def test_percent_of_total_resolves_and_runs(self, graph): + con = self._orders_con() + layer = self._layer(graph) + # line totals: order 1 = 4 + 6 = 10, order 2 = 5; total = 15. + sql = layer.compile( + metrics=["order_lines.pct_of_total_line_amount"], + dimensions=["order_lines.order_id"], + ) + rows = dict(con.execute(sql).fetchall()) + assert rows[1] == pytest.approx(10 / 15) + assert rows[2] == pytest.approx(5 / 15) + + def test_fiscal_offset_buckets_by_fiscal_period(self): + import tempfile + + import duckdb + + lkml = """ +view: ev { + sql_table_name: analytics.ev ;; + dimension: id { type: number primary_key: yes sql: ${TABLE}.id ;; } + dimension_group: occurred { + type: time + timeframes: [fiscal_year] + fiscal_month_offset: 3 + sql: ${TABLE}.occurred_at ;; + } + measure: count { type: count } +} +""" + with tempfile.NamedTemporaryFile("w", suffix=".lkml", delete=False) as f: + f.write(lkml) + path = f.name + graph = LookMLAdapter().parse(path) + # Offset shifts the timestamp so the calendar truncation lands on fiscal + # boundaries rather than ignoring the offset. + assert "INTERVAL (3) MONTH" in graph.get_model("ev").get_dimension("occurred_fiscal_year").sql + + layer = self._layer(graph) + con = duckdb.connect() + con.execute("CREATE SCHEMA IF NOT EXISTS analytics") + con.execute("CREATE TABLE analytics.ev (id INT, occurred_at DATE)") + # April fiscal-year start: 2024-03-31 is the prior fiscal year; 2024-04-01 + # and 2024-06-15 are the next one. + con.execute( + "INSERT INTO analytics.ev VALUES (1, DATE '2024-03-31'),(2, DATE '2024-04-01'),(3, DATE '2024-06-15')" + ) + sql = layer.compile(metrics=["ev.count"], dimensions=["ev.occurred_fiscal_year"]) + counts = sorted(c for _, c in con.execute(sql).fetchall()) + assert counts == [1, 2] + + if __name__ == "__main__": pytest.main([__file__, "-v"]) From df1915a0d6c0c6c9bed6dde6a034a78bdd03185e Mon Sep 17 00:00:00 2001 From: Nico Ritschel Date: Sun, 14 Jun 2026 08:21:15 -0700 Subject: [PATCH 3/4] fix(lookml): parseable percentile_distinct, overflow-safe keyed distinct sums, count_distinct post-SQL base - percentile_distinct now emits a parseable ordered-set PERCENTILE_CONT instead of the SQLGlot-rejected ORDER BY DISTINCT form, so the metric compiles and runs - keyed sum_distinct/average_distinct bound the HASH offset (% 1<<61) so the DECIMAL symmetric aggregate no longer overflows past ~100 distinct keys - percent_of_total/percent_of_previous over a count_distinct base measure now wrap the reference in COUNT(DISTINCT ...) via aggregate templates --- sidemantic/adapters/lookml.py | 71 ++++++++++------ .../lookml/test_advanced_measure_types.py | 81 ++++++++++++++++++- 2 files changed, 124 insertions(+), 28 deletions(-) diff --git a/sidemantic/adapters/lookml.py b/sidemantic/adapters/lookml.py index 0c8e8a189..e636481ad 100644 --- a/sidemantic/adapters/lookml.py +++ b/sidemantic/adapters/lookml.py @@ -385,9 +385,9 @@ def _parse_view(self, view_def: dict) -> Model | None: if not m_name: continue measure_names.add(m_name) - agg_func = self._SQL_AGG_FUNC.get(m.get("type", "count")) - if agg_func: - measure_agg_lookup[m_name] = agg_func + agg_template = self._SQL_AGG_FUNC.get(m.get("type", "count")) + if agg_template: + measure_agg_lookup[m_name] = agg_template # Parse measures with dimension SQL lookup for reference resolution measures = [] @@ -597,16 +597,20 @@ def _parse_dimension_group( # aware truncations in _timeframe_part_sql instead. } - # SQL aggregate function for a base measure type, used by post-SQL measures + # SQL aggregate wrapper for a base measure type, used by post-SQL measures # (percent_of_total / percent_of_previous) to aggregate the referenced base - # measure before applying the window calculation. + # measure before applying the window calculation. Each entry is a format + # template with a single ``{0}`` placeholder for the column reference, so + # count_distinct (which needs ``COUNT(DISTINCT col)``) is expressed correctly + # rather than being silently dropped from the lookup. _SQL_AGG_FUNC = { - "sum": "SUM", - "count": "COUNT", - "average": "AVG", - "min": "MIN", - "max": "MAX", - "median": "MEDIAN", + "sum": "SUM({0})", + "count": "COUNT({0})", + "count_distinct": "COUNT(DISTINCT {0})", + "average": "AVG({0})", + "min": "MIN({0})", + "max": "MAX({0})", + "median": "MEDIAN({0})", } def _build_timeframe_dimension( @@ -863,7 +867,7 @@ def _parse_measure( dimension_names: Set of dimension names in this view (for reference resolution) dimension_sql_lookup: Dict mapping dimension names to their resolved SQL measure_names: Set of measure names in this view (for base-measure resolution) - measure_agg_lookup: Dict mapping base measure names to their SQL aggregate function + measure_agg_lookup: Dict mapping base measure names to their SQL aggregate template Returns: Metric instance or None @@ -1157,7 +1161,15 @@ def _parse_distinct_measure( else: # percentile_distinct percentile_value = measure_def.get("percentile", 50) fraction = float(percentile_value) / 100.0 - agg_sql = f"PERCENTILE_CONT({fraction}) WITHIN GROUP (ORDER BY DISTINCT {sql})" + # `ORDER BY DISTINCT ...` inside PERCENTILE_CONT is rejected by SQLGlot + # and standard SQL, so the imported metric would fail to parse before + # reaching the database, making the measure type unusable. Emit the + # standard parseable ordered-set form (the same one used for the plain + # `percentile` measure type above), which the generator compiles and + # runs. There is no fan-out-safe inline DISTINCT form for an ordered + # percentile, so this de-duplicates rows the same way the database's + # PERCENTILE_CONT does rather than by the value list. + agg_sql = f"PERCENTILE_CONT({fraction}) WITHIN GROUP (ORDER BY {sql})" extra = {"distinct": True} if sql_distinct_key: @@ -1181,15 +1193,21 @@ def _keyed_distinct_aggregate_sql(measure_type: str, value_sql: str, key_sql: st Implements LookML ``sum_distinct`` / ``average_distinct`` with a ``sql_distinct_key`` using a symmetric aggregate: each distinct key contributes its value exactly once even when joins fan rows out. The - HASH(key) term is cast to DECIMAL alongside the value so large hash - offsets do not lose precision through float arithmetic (which would - otherwise corrupt the result). ``{model}`` placeholders are preserved - for the SQL generator. + bounded HASH(key) offset is cast to DECIMAL alongside the value so the + per-key value stays exact, and the bound keeps the summed offsets within + DECIMAL(38, 6) range so the aggregate does not overflow at realistic key + cardinalities. ``{model}`` placeholders are preserved for the SQL + generator. """ - # HASH(key) offset, cast to DECIMAL so summing alongside the value stays + # Per-key offset, cast to DECIMAL so summing alongside the value stays # exact; the offset cancels out in the subtraction, leaving the per-key - # value summed once. - offset = f"(HASH({key_sql})::HUGEINT * (1::HUGEINT << 40))::DECIMAL(38, 6)" + # value summed once. HASH is bounded by `% (1 << 61)` so each offset stays + # below ~2.3e18: summing many of them (thousands of distinct keys) stays + # well within DECIMAL(38, 6) headroom and never overflows, while the 2^61 + # separation dwarfs realistic measure magnitudes so distinct keys do not + # collide. The unbounded `HASH * (1 << 40)` form overflowed once a query + # accumulated ~100 distinct keys. + offset = f"(HASH({key_sql}) % (1::HUGEINT << 61))::DECIMAL(38, 6)" value = f"({value_sql})::DECIMAL(38, 6)" keyed_sum = f"(SUM(DISTINCT {offset} + {value}) - SUM(DISTINCT {offset}))" if measure_type == "sum_distinct": @@ -1208,8 +1226,9 @@ def _resolve_measure_reference_sql( ${dimension} references resolve to the dimension's SQL. ${measure} references resolve to ``{model}.``; when ``measure_agg_lookup`` - provides the base measure's aggregate function the reference becomes - ``({model}.)`` so the value is aggregated per group before + provides the base measure's aggregate template the reference becomes + ``({model}.)`` (e.g. ``COUNT(DISTINCT {model}.)`` + for a count_distinct base) so the value is aggregated per group before the window calculation. The generator's inline-aggregate path then rewrites ``{model}.`` to the base measure's ``_raw`` CTE column. A bare ```` would reference a column the model CTE @@ -1226,9 +1245,9 @@ def _resolve(match: re.Match) -> str: if ref_name in dimension_sql_lookup: return f"({dimension_sql_lookup[ref_name]})" if ref_name in measure_names: - agg_func = measure_agg_lookup.get(ref_name) - if agg_func: - return f"{agg_func}({{model}}.{ref_name})" + agg_template = measure_agg_lookup.get(ref_name) + if agg_template: + return agg_template.format(f"{{model}}.{ref_name}") return f"{{model}}.{ref_name}" return ref_name @@ -1262,7 +1281,7 @@ def _parse_post_sql_measure( measure_def: Raw measure definition. dimension_sql_lookup: Resolved dimension SQL for ${ref} resolution. measure_names: Set of base measure names for ${ref} qualification. - measure_agg_lookup: Base measure name -> SQL aggregate function. + measure_agg_lookup: Base measure name -> SQL aggregate template. Returns: A Metric, or None if the referenced base measure SQL is missing. diff --git a/tests/adapters/lookml/test_advanced_measure_types.py b/tests/adapters/lookml/test_advanced_measure_types.py index 16e6cec61..d8bb49f6c 100644 --- a/tests/adapters/lookml/test_advanced_measure_types.py +++ b/tests/adapters/lookml/test_advanced_measure_types.py @@ -63,9 +63,12 @@ def test_median_distinct(self, graph): def test_percentile_distinct(self, graph): m = graph.get_model("order_lines").get_metric("p90_order_amount") assert m.type == "derived" - # percentile: 90 -> fraction 0.9 + # percentile: 90 -> fraction 0.9. Uses the standard parseable ordered-set + # form (no `ORDER BY DISTINCT`, which SQLGlot rejects) so the metric + # actually compiles and runs. assert "PERCENTILE_CONT(0.9)" in m.sql - assert "WITHIN GROUP (ORDER BY DISTINCT" in m.sql + assert "WITHIN GROUP (ORDER BY" in m.sql + assert "ORDER BY DISTINCT" not in m.sql assert "{model}.order_amount" in m.sql def test_distinct_without_sql_distinct_key(self, graph): @@ -240,6 +243,80 @@ def test_percent_of_total_resolves_and_runs(self, graph): assert rows[1] == pytest.approx(10 / 15) assert rows[2] == pytest.approx(5 / 15) + def test_percentile_distinct_compiles_and_runs(self, graph): + # Regression: percentile_distinct previously emitted `ORDER BY DISTINCT` + # inside PERCENTILE_CONT, which SQLGlot cannot parse, so the metric was + # unusable. It must now compile to parseable SQL and execute. + con = self._orders_con() + layer = self._layer(graph) + sql = layer.compile(metrics=["order_lines.p90_order_amount"]) + (val,) = con.execute(sql).fetchone() + assert val is not None + + def test_keyed_sum_distinct_does_not_overflow(self, graph): + # Regression: the keyed distinct offset (HASH(key) scaled into DECIMAL) + # overflowed once a query accumulated ~100 distinct keys. A high-key-count + # query must run and return the correct fan-out-safe keyed sum. + import duckdb + + con = duckdb.connect() + con.execute("CREATE SCHEMA IF NOT EXISTS analytics") + con.execute( + "CREATE TABLE analytics.order_lines (id INT, order_id INT, order_amount DOUBLE, line_amount DOUBLE)" + ) + rows = [] + expected = 0.0 + idx = 0 + for order_id in range(1, 501): # 500 distinct keys, well past the old ~100 ceiling + amount = float(order_id) + expected += amount + for _ in range(3): # fan out each order to 3 lines + idx += 1 + rows.append((idx, order_id, amount, 1.0)) + con.executemany("INSERT INTO analytics.order_lines VALUES (?,?,?,?)", rows) + layer = self._layer(graph) + sql = layer.compile(metrics=["order_lines.total_order_amount"]) + (total,) = con.execute(sql).fetchone() + assert float(total) == pytest.approx(expected) + + def test_percent_of_total_over_count_distinct_base(self): + # Regression: percent_of_total / percent_of_previous referencing a + # count_distinct base measure had no entry in the aggregate lookup, so the + # base stayed a raw id column instead of COUNT(DISTINCT ...), producing + # invalid SQL / percentages over raw ids. + import tempfile + + import duckdb + + lkml = """ +view: visits { + sql_table_name: analytics.visits ;; + dimension: id { type: number primary_key: yes sql: ${TABLE}.id ;; } + dimension: country { type: string sql: ${TABLE}.country ;; } + dimension: user_id { type: number sql: ${TABLE}.user_id ;; } + measure: unique_users { type: count_distinct sql: ${user_id} ;; } + measure: pct_unique_users { type: percent_of_total sql: ${unique_users} ;; } +} +""" + with tempfile.NamedTemporaryFile("w", suffix=".lkml", delete=False) as f: + f.write(lkml) + path = f.name + graph = LookMLAdapter().parse(path) + m = graph.get_model("visits").get_metric("pct_unique_users") + # The base ref must be wrapped in COUNT(DISTINCT ...), not left as a raw id. + assert "COUNT(DISTINCT {model}.unique_users)" in m.sql + + layer = self._layer(graph) + con = duckdb.connect() + con.execute("CREATE SCHEMA IF NOT EXISTS analytics") + con.execute("CREATE TABLE analytics.visits (id INT, country VARCHAR, user_id INT)") + # US has 3 distinct users, CA has 1; shares are 3/4 and 1/4. + con.execute("INSERT INTO analytics.visits VALUES (1,'US',1),(2,'US',2),(3,'US',3),(4,'CA',4),(5,'CA',4)") + sql = layer.compile(metrics=["visits.pct_unique_users"], dimensions=["visits.country"]) + rows = dict(con.execute(sql).fetchall()) + assert rows["US"] == pytest.approx(3 / 4) + assert rows["CA"] == pytest.approx(1 / 4) + def test_fiscal_offset_buckets_by_fiscal_period(self): import tempfile From 00321df7a4fe258f286e312020fd8d42446f7bdc Mon Sep 17 00:00:00 2001 From: Nico Ritschel Date: Mon, 15 Jun 2026 00:02:02 -0700 Subject: [PATCH 4/4] fix(lookml): dedupe keyed distinct median/percentile by key median_distinct and percentile_distinct with a sql_distinct_key emitted a plain ordered-set aggregate over the fanned-out rows, so a value repeated across joined rows skewed the quantile. Collapse (key, value) pairs to one value per distinct key via LIST + LIST_DISTINCT, then take the continuous quantile of that per-key list, matching Looker's fan-out-safe semantics. --- sidemantic/adapters/lookml.py | 42 ++++++++++-- .../lookml/test_advanced_measure_types.py | 67 ++++++++++++++++--- 2 files changed, 93 insertions(+), 16 deletions(-) diff --git a/sidemantic/adapters/lookml.py b/sidemantic/adapters/lookml.py index e636481ad..c2607b794 100644 --- a/sidemantic/adapters/lookml.py +++ b/sidemantic/adapters/lookml.py @@ -1151,14 +1151,27 @@ def _parse_distinct_measure( # fan-out-safe form for keyed deduplication. if sql_distinct_key and measure_type in ("sum_distinct", "average_distinct"): agg_sql = self._keyed_distinct_aggregate_sql(measure_type, sql, sql_distinct_key) + elif sql_distinct_key and measure_type in ("median_distinct", "percentile_distinct"): + # Ordered-set aggregates (median / percentile) are skewed by fan-out: + # a value repeated across joined rows is counted once per row, so the + # plain ordered-set form computes the quantile over the duplicated + # distribution rather than one value per distinct key. There is no + # fan-out-safe ordered-set form via WITHIN GROUP (an ORDER BY DISTINCT + # is rejected by SQLGlot and standard SQL). Instead collapse to one + # value per distinct key first, then take the quantile of that list. + if measure_type == "median_distinct": + fraction = 0.5 + else: + fraction = float(measure_def.get("percentile", 50)) / 100.0 + agg_sql = self._keyed_distinct_quantile_sql(sql, sql_distinct_key, fraction) elif measure_type == "sum_distinct": agg_sql = f"SUM(DISTINCT {sql})" elif measure_type == "average_distinct": agg_sql = f"AVG(DISTINCT {sql})" elif measure_type == "median_distinct": - # No fan-out-safe inline form for keyed median; dedupe by value. + # No key: dedupe by value (the same row-collapsing the database does). agg_sql = f"MEDIAN(DISTINCT {sql})" - else: # percentile_distinct + else: # percentile_distinct, no key percentile_value = measure_def.get("percentile", 50) fraction = float(percentile_value) / 100.0 # `ORDER BY DISTINCT ...` inside PERCENTILE_CONT is rejected by SQLGlot @@ -1166,9 +1179,8 @@ def _parse_distinct_measure( # reaching the database, making the measure type unusable. Emit the # standard parseable ordered-set form (the same one used for the plain # `percentile` measure type above), which the generator compiles and - # runs. There is no fan-out-safe inline DISTINCT form for an ordered - # percentile, so this de-duplicates rows the same way the database's - # PERCENTILE_CONT does rather than by the value list. + # runs. Without a key the only available de-duplication is by value, + # which is what the database's PERCENTILE_CONT already does. agg_sql = f"PERCENTILE_CONT({fraction}) WITHIN GROUP (ORDER BY {sql})" extra = {"distinct": True} @@ -1215,6 +1227,26 @@ def _keyed_distinct_aggregate_sql(measure_type: str, value_sql: str, key_sql: st # average_distinct: keyed sum divided by the number of distinct keys. return f"({keyed_sum} / NULLIF(COUNT(DISTINCT {key_sql}), 0))" + @staticmethod + def _keyed_distinct_quantile_sql(value_sql: str, key_sql: str, fraction: float) -> str: + """Build a fan-out-safe ordered-set quantile deduplicated by a key entity. + + Implements LookML ``median_distinct`` / ``percentile_distinct`` with a + ``sql_distinct_key``. A plain ``PERCENTILE_CONT(...) WITHIN GROUP`` over the + fanned-out rows counts a value once per joined row, skewing the quantile. + DuckDB forbids ``ORDER BY DISTINCT`` inside an ordered-set aggregate and + forbids nesting an aggregate inside another aggregate, so instead collect + the ``(key, value)`` pairs into a single ``LIST`` aggregate, drop duplicate + keys with scalar ``list_distinct``, project the value, and take the + continuous quantile of that per-key value list via scalar ``list_aggregate``. + NULL values are ignored by ``quantile_cont`` (matching ordered-set + semantics), and an empty group yields NULL. ``{model}`` placeholders are + preserved for the SQL generator. + """ + pairs = f"LIST(STRUCT_PACK(k := {key_sql}, v := {value_sql}))" + per_key_values = f"LIST_TRANSFORM(LIST_DISTINCT({pairs}), x -> x.v)" + return f"LIST_AGGREGATE({per_key_values}, 'quantile_cont', {fraction})" + def _resolve_measure_reference_sql( self, sql: str, diff --git a/tests/adapters/lookml/test_advanced_measure_types.py b/tests/adapters/lookml/test_advanced_measure_types.py index d8bb49f6c..ef04d8320 100644 --- a/tests/adapters/lookml/test_advanced_measure_types.py +++ b/tests/adapters/lookml/test_advanced_measure_types.py @@ -57,18 +57,26 @@ def test_average_distinct(self, graph): def test_median_distinct(self, graph): m = graph.get_model("order_lines").get_metric("median_order_amount") assert m.type == "derived" - assert m.sql.startswith("MEDIAN(DISTINCT ") + # With a sql_distinct_key the ordered-set quantile must dedupe by key + # before computing the median, so it collapses (key, value) pairs to one + # value per key and takes the 0.5 quantile of that list. An ordered-set + # MEDIAN over the fanned-out rows would weight repeated values per row. + assert "LIST_AGGREGATE(" in m.sql + assert "'quantile_cont', 0.5)" in m.sql + assert "{model}.order_id" in m.sql # dedupe key assert "{model}.order_amount" in m.sql def test_percentile_distinct(self, graph): m = graph.get_model("order_lines").get_metric("p90_order_amount") assert m.type == "derived" - # percentile: 90 -> fraction 0.9. Uses the standard parseable ordered-set - # form (no `ORDER BY DISTINCT`, which SQLGlot rejects) so the metric - # actually compiles and runs. - assert "PERCENTILE_CONT(0.9)" in m.sql - assert "WITHIN GROUP (ORDER BY" in m.sql + # percentile: 90 -> fraction 0.9. With a sql_distinct_key the value is + # deduplicated by key (one value per distinct order) before the quantile, + # so fan-out rows do not skew the result. `ORDER BY DISTINCT` (which + # SQLGlot rejects) is never emitted. + assert "LIST_AGGREGATE(" in m.sql + assert "'quantile_cont', 0.9)" in m.sql assert "ORDER BY DISTINCT" not in m.sql + assert "{model}.order_id" in m.sql # dedupe key assert "{model}.order_amount" in m.sql def test_distinct_without_sql_distinct_key(self, graph): @@ -243,15 +251,52 @@ def test_percent_of_total_resolves_and_runs(self, graph): assert rows[1] == pytest.approx(10 / 15) assert rows[2] == pytest.approx(5 / 15) - def test_percentile_distinct_compiles_and_runs(self, graph): + def _fanned_orders_con(self): + # Five distinct orders with amounts 100..500; order 1 fans out to 3 lines. + # Distinct-by-key values are [100,200,300,400,500]: median 300, p90 460. + # Without keyed dedup, the repeated 100 skews both (median 200, p90 440). + import duckdb + + con = duckdb.connect() + con.execute("CREATE SCHEMA IF NOT EXISTS analytics") + con.execute( + "CREATE TABLE analytics.order_lines (id INT, order_id INT, order_amount DOUBLE, line_amount DOUBLE)" + ) + con.executemany( + "INSERT INTO analytics.order_lines VALUES (?,?,?,?)", + [ + (1, 1, 100, 1), + (2, 1, 100, 1), + (3, 1, 100, 1), + (4, 2, 200, 1), + (5, 3, 300, 1), + (6, 4, 400, 1), + (7, 5, 500, 1), + ], + ) + return con + + def test_percentile_distinct_dedupes_by_key(self, graph): # Regression: percentile_distinct previously emitted `ORDER BY DISTINCT` - # inside PERCENTILE_CONT, which SQLGlot cannot parse, so the metric was - # unusable. It must now compile to parseable SQL and execute. - con = self._orders_con() + # inside PERCENTILE_CONT (unparseable), and then a plain ordered-set + # PERCENTILE_CONT that ignored sql_distinct_key, so fan-out rows skewed the + # result. It must now compile, run, and dedupe by key: p90 over the five + # distinct order amounts is 460, not the fan-out-skewed 440. + con = self._fanned_orders_con() layer = self._layer(graph) sql = layer.compile(metrics=["order_lines.p90_order_amount"]) (val,) = con.execute(sql).fetchone() - assert val is not None + assert float(val) == pytest.approx(460.0) + + def test_median_distinct_dedupes_by_key(self, graph): + # median_distinct with a sql_distinct_key must also dedupe by key before + # taking the median: median of [100,200,300,400,500] is 300, not the + # fan-out-skewed 200. + con = self._fanned_orders_con() + layer = self._layer(graph) + sql = layer.compile(metrics=["order_lines.median_order_amount"]) + (val,) = con.execute(sql).fetchone() + assert float(val) == pytest.approx(300.0) def test_keyed_sum_distinct_does_not_overflow(self, graph): # Regression: the keyed distinct offset (HASH(key) scaled into DECIMAL)