diff --git a/DECISIONS.md b/DECISIONS.md index 026cef94..e5c1c75b 100644 --- a/DECISIONS.md +++ b/DECISIONS.md @@ -116,3 +116,4 @@ implementation detail. Include issue refs when known. - 2026-08-11 — Deletion, sweep, docs: P-J states 2+3 executed (DEV-1749, PR 6 of 6, closing the DEV-1742 consolidation). Every mechanism PRs 1–5 + DEV-1763 left production-unreferenced (P-J state 1) is now **deleted** together with its pinning tests (state 3), after confirming the desired behaviour is pinned by tests on the new code. Removed from `generator.py`: the five legacy per-path `ValueKey` renderers and the three arithmetic composer shims (`render_value_key` / `render_arithmetic` are the sole paths, P-G); the first/last host-base ranked machinery — `_build_first_last_base_select`, `_build_ranked_subquery_from_planned`, `_has_first_last_aggregate`, the rn-suffix + filtered-rn/match maps and raw-filter-leak fallback in `_build_agg`, `FirstLastRenderState` and its `first_last_state` threading, and the production-dead `is_first_or_last` arm of `_render_cross_model_cte` (first/last is a `RankedAggregatePlan` CTE since DEV-1748, P-C); the Mode-A model-filter qualify chain (`_render_model_filter_sql`, `_qualify_mode_a_sql_filter`, `_render_mode_a_predicate`, `_filter_join_paths`, `_expand_degenerate_derived_root`, `_column_ref_is_derived`, `_predicate_references_derived`) plus the dead `FilterPhase.text_columns` field — the Mode-A door (`ScopeFrame.enter_predicate`, DEV-1745) is the one door, P-A; the four legacy ORDER BY resolvers (`_build_combined_order_by_sql`, `_resolve_combined_order_term`, `_planned_order_by_sql`, `_apply_order_limit_from_planned`) superseded by `resolve_order_term`; `_null_safe_join_pair_sql` (string round-trip superseded by `render/joins.py`, P-I); `_build_transform_sql`+`_SELF_JOIN_TRANSFORMS`, `_build_outer_wrap`+`_strip_trailing_pagination` (planned outer-wrap delegates to `SqlDialect.emit_outer_wrap`, P-H), and `_cte_name_from_alias` (superseded by `naming.cte_name_from_alias`, P-F). Removed from `cross_model_planner.py`: the formula-text re-rooting island (`_local_agg_formula`, `_render_ref_formula`, `_scalar_formula_literal`, `_reroot_ref`, `_host_ref_path`, `_REROOT_BIND_ERRORS`) and two further dead helpers (`_classify_subplan_filters`, `_filter_ref_paths`) — cross-model re-rooting is typed keys end to end (`reroot_aggregate_key` / `reroot_value_key`), no text round-trip, P-E. **B12 (ratified):** `_build_agg`'s dispatch now reads the single `AGG_REGISTRY` classification table (DEV-1744) — `_AGG_FUNCTION_MAP` (with its dead `COUNT_DISTINCT`/`MEDIAN` string values), the second inline class map, and the generator-local stat-name frozenset are gone; the two-phase resolution order (own-inner builders → shared inner + filter wrap → distinct/median/simple) is preserved byte-for-byte, SQL-identical. **Consolidations landed:** transform-op registries single-sourced (`RANK_FAMILY_TRANSFORMS`, `TIME_TRANSFORMS` from `core/formula.py`), the cube identifier regex re-pointed at `core/refs.IDENTIFIER_RE`; the `_bare_column_refs` regex kept for its remaining validation role only. **Deferred to DEV-1777** (pure refactors, no SQL change, carry regression risk needing their own byte-identity checks): step-CTE emission extraction, the throwaway-`ScopeFrame` consolidation + `_resolve_explicit_time_col` dead-branch removal, and the positional-index couplings. The remaining bare-identifier divergences (`schema_drift`, `.isidentifier()` sites) went to DEV-1771. `docs/architecture/sql-generation.md` was rewritten as the P-A – P-J principles document. Every deleted symbol's docstring/comment references were swept to present-tense truth across 16 files. No emitted SQL changed; the full non-integration suite is green (3 xfails, all ticketed — DEV-1752 / DEV-1729 / DEV-1445 — retained per the F18 inventory). - 2026-08-12 — Single-source bare-identifier detection narrowed to `schema_drift` only (DEV-1771). Of the three ad-hoc detectors the issue named, only `schema_drift._is_bare_identifier` was re-pointed at the canonical `core/refs.IDENTIFIER_RE` (its char-loop `all(c.isalnum() or c == "_")` accepted non-ASCII in every position; the regex rejects only a non-ASCII *leading* char, since `\w*` still matches Unicode after the ASCII-only lead class). The two `stage_planner` `.isidentifier()` sites (`_saved_model_measure_type`, `_bare_saved_measure_name`) were left as-is: they gate `get_measure(name)` and `ModelMeasure.name` is already ASCII-constrained by `_NAME_PATTERN`, so any string that could match a real measure passes both predicates identically — the flip is observationally a no-op, so no test could fail without it. The two `generator` `.isidentifier()` sites (`_resolve_sql`; the no-bundle branch reached via the `col.name` fallback) were also left as-is: `Column.name`/`Column.sql` are NOT ASCII-constrained, and routing a non-ASCII-leading physical column (e.g. Cyrillic `год`, common in RU/UA schemas) through `_parse()` drops the model-relation qualifier (`m."год"` → bare `год`) — a correctness regression on legitimate input, exactly the issue's "surface and stop" guard. The one shipped behavior change is a drift false-negative: a base column aliasing a bare non-ASCII-leading physical name (`Column(name="year", sql="год")`) reclassifies base→derived, so a dropped physical `год` is no longer flagged by `_diff_sql_table_columns` and is instead scanned as a ref by `_first_dropped_sql_column_ref` (author-accepted; advisory-only, never query correctness). `.match()` (not `fullmatch`) and the retained `.strip()` match the existing `cube`/`dbt`/`osi` reuse sites. +- 2026-08-16 — Aggregated slot-type and display-format inference share one classifier (DEV-1788, follow-up to DEV-1784's Option A). `aggregated_type` (slot `DataType`) and `_infer_aggregated_format` (response `NumberFormat`) had disagreed on the stat/parametric family: type said `DOUBLE` while format fell through to inherit the source column's format, so `revenue:stddev_samp` was typed `DOUBLE` yet displayed as currency. Both now read a single `classify_aggregation` (`core/enums.py`) returning one of four `AggregationValueClass` buckets, and each function maps the bucket to its own output — no per-name branching survives, so the two axes cannot drift. The four builtin frozensets (`INTEGER_AGGREGATIONS`, `PRESERVING_AGGREGATIONS`, `FLOAT_SOURCE_UNIT_AGGREGATIONS`, `FLOAT_PLAIN_AGGREGATIONS`) partition `BUILTIN_AGGREGATIONS`, pinned by a completeness test; custom/model-defined aggregations hit the `PRESERVING` fallback (inherit type & format), unchanged. **Semantics chosen (Option B, unit-correct):** `avg`/`median`/`weighted_avg`/`percentile`/`stddev*` are `DOUBLE` but keep the source's UNITS, so display format inherits the source (falling back to `FLOAT` when the source has none — keeping type `DOUBLE` and format `FLOAT` coherent for unformatted measures, and confining the change to formatted ones); `corr`/`var*`/`covar*` are dimensionless/squared/product units, so they display as plain `FLOAT` regardless of source. `aggregated_type` is behaviourally unchanged (only restructured). **Net user-visible change, all in `_infer_aggregated_format`:** avg-family of a FORMATTED measure now inherits that format (was `FLOAT`); `corr`/`var*`/`covar*` now `FLOAT` (was inherit); `stddev*`/`percentile` unchanged (already inherited). Drift guard extended to the full four-bucket table and routed through the public callers (`measure_key_type` / `measure_key_format_description`), plus response-metadata assertions for the stat/parametric family. diff --git a/docs/architecture/sql-generation.md b/docs/architecture/sql-generation.md index e863bc53..9cc93521 100644 --- a/docs/architecture/sql-generation.md +++ b/docs/architecture/sql-generation.md @@ -287,8 +287,11 @@ confirmed pinned. each public result key mapped to its `FieldMetadata(label, format)`. `_slot_result_keys` mirrors `_full_alias_for_slot`; only keys actually present in the rendered SQL are surfaced. Aggregate formats come from - `_infer_aggregated_format` (INTEGER for count/star, FLOAT for avg-family, - source-column format for sum/min/max). + `_infer_aggregated_format`, which shares one classifier (`classify_aggregation`, + DEV-1788) with `aggregated_type` (slot `DataType`) so the type and format axes + cannot drift: INTEGER for count/star; plain FLOAT for `corr`/`var`/`covar`; + the source column's format (else FLOAT) for the avg-family/`percentile`/`stddev`; + the source column's format (else None) for `sum`/`min`/`max`/`first`/`last`. `FieldMetadata` / `ResponseAttributes` / `_infer_aggregated_format` live here (not in `query_engine`) so the module imports nothing from the engine; `query_engine` diff --git a/slayer/core/enums.py b/slayer/core/enums.py index e5e6bd88..3a760ab5 100644 --- a/slayer/core/enums.py +++ b/slayer/core/enums.py @@ -3,7 +3,7 @@ import datetime # noqa: F401 (kept for downstream imports of TimeGranularity) import difflib from enum import Enum -from typing import Any +from typing import Any, Optional class StrEnum(str, Enum): @@ -174,15 +174,61 @@ class JoinType(StrEnum): "corr", "covar_samp", "covar_pop", }) -# Aggregations whose result is always an integer count, independent of the -# source column's type/format. Single source of truth shared by -# ``aggregated_type`` (slot DataType) and ``_infer_aggregated_format`` (response -# NumberFormat) so the two cannot drift on the integer bucket; a drift-guard -# test pins both against it. The float/inherit split for stat/parametric aggs -# legitimately differs between type and display format (see DEV-1788). +# Aggregation value classification (DEV-1788). One classifier, +# ``classify_aggregation``, buckets every aggregation by how its result relates +# to the source column. Both ``aggregated_type`` (slot DataType) and +# ``_infer_aggregated_format`` (display NumberFormat) read the bucket and map it +# to their own output, so the type and format axes cannot drift apart. The four +# builtin sets partition ``BUILTIN_AGGREGATIONS`` (pinned by a drift-guard test); +# custom/model-defined aggregations hit the PRESERVING fallback. + +# Result is always an integer count, independent of the source column. INTEGER_AGGREGATIONS: frozenset[str] = frozenset({ "count", "count_distinct", "count_distinct_approx", }) +# Result is a float in the SAME units as the source (display format inherited). +FLOAT_SOURCE_UNIT_AGGREGATIONS: frozenset[str] = frozenset({ + "avg", "weighted_avg", "median", "percentile", + "stddev_samp", "stddev_pop", +}) +# Result is a float in different units (dimensionless / squared / product), so it +# carries a plain FLOAT format, not the source's units. +FLOAT_PLAIN_AGGREGATIONS: frozenset[str] = frozenset({ + "corr", "var_samp", "var_pop", "covar_samp", "covar_pop", +}) +# Result preserves the source column's type AND format. +PRESERVING_AGGREGATIONS: frozenset[str] = frozenset({ + "sum", "min", "max", "first", "last", +}) + + +class AggregationValueClass(StrEnum): + """How an aggregation's result relates to its source column, for slot-type + and display-format inference (DEV-1788).""" + + COUNT = "count" # INT type, INTEGER format + PRESERVING = "preserving" # source type & format + FLOAT_SOURCE_UNITS = "float_source_units" # DOUBLE type, source format (else FLOAT) + FLOAT_PLAIN = "float_plain" # DOUBLE type, plain FLOAT format + + +def classify_aggregation( + *, measure_name: Optional[str], aggregation: str +) -> AggregationValueClass: + """Bucket an aggregation for slot-type / display-format inference. + + ``measure_name == "*"`` (``*:count``) is COUNT; custom/unknown aggregations + fall through to PRESERVING (inherit source type & format). + """ + if measure_name == "*": + return AggregationValueClass.COUNT + if aggregation in INTEGER_AGGREGATIONS: + return AggregationValueClass.COUNT + if aggregation in FLOAT_SOURCE_UNIT_AGGREGATIONS: + return AggregationValueClass.FLOAT_SOURCE_UNITS + if aggregation in FLOAT_PLAIN_AGGREGATIONS: + return AggregationValueClass.FLOAT_PLAIN + return AggregationValueClass.PRESERVING # DEV-1576: unambiguous aggregation-name aliases that LLM agents routinely # emit. ``normalize_aggregation_name`` lowercases the incoming token and maps diff --git a/slayer/engine/prebound.py b/slayer/engine/prebound.py index 7e61db05..7d813c21 100644 --- a/slayer/engine/prebound.py +++ b/slayer/engine/prebound.py @@ -28,11 +28,15 @@ from __future__ import annotations -from typing import FrozenSet, List, Optional, Tuple +from typing import List, Optional, Tuple from pydantic import BaseModel, ConfigDict, Field, model_validator -from slayer.core.enums import DataType, INTEGER_AGGREGATIONS +from slayer.core.enums import ( + AggregationValueClass, + DataType, + classify_aggregation, +) from slayer.core.format import NumberFormat from slayer.core.keys import ( AggregateKey, @@ -163,36 +167,31 @@ def __getattr__(self, item: str): # Key -> slot metadata # --------------------------------------------------------------------------- -_FLOAT_AGGREGATIONS: FrozenSet[str] = frozenset({ - "avg", "weighted_avg", "median", - "stddev_samp", "stddev_pop", "var_samp", "var_pop", - "corr", "covar_samp", "covar_pop", "percentile", -}) - - def aggregated_type( *, model: SlayerModel, measure_name: Optional[str], aggregation: str, ) -> Optional[DataType]: - """Type for an aggregated measure slot. Mirrors - ``_infer_aggregated_format`` (decision #2 of the Stage B plan): + """Type for an aggregated measure slot, via the shared + ``classify_aggregation`` (DEV-1788), so it cannot drift from + ``_infer_aggregated_format``: - * ``*:count`` (measure_name=``"*"``) → ``INT`` - * ``count`` / ``count_distinct`` / ``count_distinct_approx`` → ``INT`` - * ``avg`` / ``weighted_avg`` / ``median`` / parametric / stat aggs → + * ``COUNT`` (``*:count`` / count-family) → ``INT`` + * ``FLOAT_SOURCE_UNITS`` / ``FLOAT_PLAIN`` (avg-family, stat, parametric) → ``DOUBLE`` - * ``sum`` / ``min`` / ``max`` / ``first`` / ``last`` → inherit from - source column type (DOUBLE if absent). + * ``PRESERVING`` (sum / min / max / first / last, and custom aggs) → inherit + source column type (``None`` if absent). """ - if measure_name == "*": - return DataType.INT - if aggregation in INTEGER_AGGREGATIONS: + cls = classify_aggregation(measure_name=measure_name, aggregation=aggregation) + if cls is AggregationValueClass.COUNT: return DataType.INT - if aggregation in _FLOAT_AGGREGATIONS: + if cls in ( + AggregationValueClass.FLOAT_SOURCE_UNITS, + AggregationValueClass.FLOAT_PLAIN, + ): return DataType.DOUBLE - # sum / min / max / first / last — preserve source column type. + # PRESERVING — inherit source column type. if measure_name is None: return None col = model.get_column(measure_name) diff --git a/slayer/engine/response_meta.py b/slayer/engine/response_meta.py index 2898f2d6..6eee9eee 100644 --- a/slayer/engine/response_meta.py +++ b/slayer/engine/response_meta.py @@ -26,7 +26,7 @@ import sqlglot from pydantic import BaseModel, Field as PydanticField -from slayer.core.enums import INTEGER_AGGREGATIONS +from slayer.core.enums import AggregationValueClass, classify_aggregation from slayer.core.format import NumberFormat, NumberFormatType from slayer.core.keys import ( AggregateKey, @@ -73,28 +73,28 @@ def _infer_aggregated_format( measure_name: str, aggregation: str, ) -> Optional[NumberFormat]: - """Infer NumberFormat for an aggregated measure based on aggregation type and source measure format. - - Rules: - - count, count_distinct, count_distinct_approx: always INTEGER - - avg, weighted_avg, median: always FLOAT - - sum, min, max, first, last: inherit from source measure - - *:count (measure_name="*"): INTEGER + """Infer the display NumberFormat for an aggregated measure via the shared + ``classify_aggregation`` (DEV-1788), so it cannot drift from + ``aggregated_type``: + + - COUNT (``*:count`` / count-family): INTEGER + - FLOAT_PLAIN (corr / var / covar): plain FLOAT + - FLOAT_SOURCE_UNITS (avg-family / percentile / stddev): inherit source + format, else FLOAT (the result is fractional even absent source units) + - PRESERVING (sum / min / max / first / last, and custom aggs): inherit + source format, else None """ - if measure_name == "*": - return NumberFormat(type=NumberFormatType.INTEGER) - - if aggregation in INTEGER_AGGREGATIONS: + cls = classify_aggregation(measure_name=measure_name, aggregation=aggregation) + if cls is AggregationValueClass.COUNT: return NumberFormat(type=NumberFormatType.INTEGER) - - if aggregation in ("avg", "weighted_avg", "median"): + if cls is AggregationValueClass.FLOAT_PLAIN: return NumberFormat(type=NumberFormatType.FLOAT) - # sum, min, max, first, last: inherit from source column's format source_col = model.get_column(measure_name) if source_col and source_col.format: return source_col.format - + if cls is AggregationValueClass.FLOAT_SOURCE_UNITS: + return NumberFormat(type=NumberFormatType.FLOAT) return None @@ -202,8 +202,9 @@ def _measure_format( """Number format for a measure slot. Aggregate slots inherit via ``_infer_aggregated_format`` (INTEGER for - count(-distinct) / star, FLOAT for avg-family, source-column format for - sum/min/max). Transform / arithmetic / scalar-call slots default to FLOAT. + count(-distinct) / star, plain FLOAT for corr / var / covar, source format + for the avg-family / percentile / stddev and for sum / min / max). + Transform / arithmetic / scalar-call slots default to FLOAT. """ key = slot.key if isinstance(key, AggregateKey): diff --git a/tests/test_format_propagation.py b/tests/test_format_propagation.py index cbc2e74c..39b16e13 100644 --- a/tests/test_format_propagation.py +++ b/tests/test_format_propagation.py @@ -3,14 +3,28 @@ import pytest from slayer.core.format import NumberFormat, NumberFormatType -from slayer.core.enums import BUILTIN_AGGREGATIONS, DataType, INTEGER_AGGREGATIONS +from slayer.core.enums import ( + AggregationValueClass, + BUILTIN_AGGREGATIONS, + DataType, + FLOAT_PLAIN_AGGREGATIONS, + FLOAT_SOURCE_UNIT_AGGREGATIONS, + INTEGER_AGGREGATIONS, + PRESERVING_AGGREGATIONS, + classify_aggregation, +) +from slayer.core.keys import AggregateKey, ColumnKey, StarKey from slayer.core.models import Column, SlayerModel from slayer.engine.query_engine import FieldMetadata # DEV-1485 Stage D: imported through ``query_engine`` while the legacy # ``_query_as_model`` re-exported it; now imported from its owning module. from slayer.engine.response_meta import _infer_aggregated_format -from slayer.engine.prebound import aggregated_type +from slayer.engine.prebound import ( + aggregated_type, + measure_key_format_description, + measure_key_type, +) # --------------------------------------------------------------------------- @@ -57,8 +71,15 @@ def test_count_distinct_returns_integer(self, model): fmt = _infer_aggregated_format(model=model, measure_name="revenue", aggregation="count_distinct") assert fmt.type == NumberFormatType.INTEGER - def test_avg_returns_float(self, model): + def test_avg_inherits_currency(self, model): + # DEV-1788: avg-family now inherits the source column's units (was FLOAT). fmt = _infer_aggregated_format(model=model, measure_name="revenue", aggregation="avg") + assert fmt.type == NumberFormatType.CURRENCY + assert fmt.symbol == "€" + + def test_avg_unformatted_falls_back_to_float(self, model): + # No source units to inherit → FLOAT (result is fractional). Option 1. + fmt = _infer_aggregated_format(model=model, measure_name="quantity", aggregation="avg") assert fmt.type == NumberFormatType.FLOAT def test_sum_inherits_currency(self, model): @@ -75,7 +96,7 @@ def test_max_inherits_format(self, model): assert fmt.type == NumberFormatType.CURRENCY def test_sum_no_format_returns_none(self, model): - """Measure without format returns None for inheriting aggregations.""" + """Preserving aggregation over a measure without format returns None.""" fmt = _infer_aggregated_format(model=model, measure_name="quantity", aggregation="sum") assert fmt is None @@ -146,16 +167,17 @@ def test_format_meta_omits_none_fields(self): # --------------------------------------------------------------------------- -# Drift guard: ``aggregated_type`` (slot DataType) and ``_infer_aggregated_format`` -# (display NumberFormat) both classify aggregations, and the integer bucket is -# the one classification they MUST agree on. Pinned here against the shared -# ``INTEGER_AGGREGATIONS`` constant so neither can drift on it. The float / -# inherit split for stat aggregations legitimately differs (type vs display) -# and is deliberately left as-is — reconciling it is tracked in DEV-1788. +# DEV-1788 shared classifier drift guard. +# +# ``aggregated_type`` (slot DataType) and ``_infer_aggregated_format`` (display +# NumberFormat) both read the single ``classify_aggregation`` classifier, so the +# type and format axes cannot drift apart again. These tests pin the full +# four-bucket table across formatted / unformatted / int-typed sources and +# through the public prebound callers. # --------------------------------------------------------------------------- -class TestIntegerBucketSharedByTypeAndFormat: +class TestSharedClassifier: @pytest.fixture def model(self): return SlayerModel( @@ -165,40 +187,182 @@ def model(self): columns=[ Column( name="revenue", - sql="amount", type=DataType.DOUBLE, format=NumberFormat(type=NumberFormatType.CURRENCY, symbol="€"), ), + Column(name="quantity", type=DataType.DOUBLE), + Column(name="qty_int", type=DataType.INT), ], ) - def test_shared_constant_names_only_builtin_aggregations(self): - assert INTEGER_AGGREGATIONS <= BUILTIN_AGGREGATIONS + # --- classification table (hard-coded, independent of the frozensets) --- + + @pytest.mark.parametrize( + "aggregation,expected", + [ + ("count", AggregationValueClass.COUNT), + ("count_distinct", AggregationValueClass.COUNT), + ("count_distinct_approx", AggregationValueClass.COUNT), + ("sum", AggregationValueClass.PRESERVING), + ("min", AggregationValueClass.PRESERVING), + ("max", AggregationValueClass.PRESERVING), + ("first", AggregationValueClass.PRESERVING), + ("last", AggregationValueClass.PRESERVING), + ("avg", AggregationValueClass.FLOAT_SOURCE_UNITS), + ("weighted_avg", AggregationValueClass.FLOAT_SOURCE_UNITS), + ("median", AggregationValueClass.FLOAT_SOURCE_UNITS), + ("percentile", AggregationValueClass.FLOAT_SOURCE_UNITS), + ("stddev_samp", AggregationValueClass.FLOAT_SOURCE_UNITS), + ("stddev_pop", AggregationValueClass.FLOAT_SOURCE_UNITS), + ("corr", AggregationValueClass.FLOAT_PLAIN), + ("var_samp", AggregationValueClass.FLOAT_PLAIN), + ("var_pop", AggregationValueClass.FLOAT_PLAIN), + ("covar_samp", AggregationValueClass.FLOAT_PLAIN), + ("covar_pop", AggregationValueClass.FLOAT_PLAIN), + ], + ) + def test_classification_table(self, aggregation, expected): + assert classify_aggregation(measure_name="revenue", aggregation=aggregation) == expected + + def test_star_classifies_as_count(self): + assert ( + classify_aggregation(measure_name="*", aggregation="count") + == AggregationValueClass.COUNT + ) + + def test_custom_aggregation_falls_back_to_preserving(self, model): + # Model-defined aggregations (not builtin) inherit source type & format + # through both consumers, not only the classifier. + assert ( + classify_aggregation(measure_name="revenue", aggregation="my_custom") + == AggregationValueClass.PRESERVING + ) + assert aggregated_type(model=model, measure_name="revenue", aggregation="my_custom") == DataType.DOUBLE + fmt = _infer_aggregated_format(model=model, measure_name="revenue", aggregation="my_custom") + assert fmt.type == NumberFormatType.CURRENCY + assert fmt.symbol == "€" + + # --- partition completeness: forces any new builtin agg to be classified --- + def test_four_sets_partition_builtin_aggregations(self): + sets = [ + INTEGER_AGGREGATIONS, + PRESERVING_AGGREGATIONS, + FLOAT_SOURCE_UNIT_AGGREGATIONS, + FLOAT_PLAIN_AGGREGATIONS, + ] + union = set().union(*sets) + assert union == BUILTIN_AGGREGATIONS + assert sum(len(s) for s in sets) == len(union) # pairwise disjoint + + # --- COUNT bucket: INT / INTEGER on every source --- + + @pytest.mark.parametrize("measure", ["revenue", "quantity", "qty_int"]) @pytest.mark.parametrize("aggregation", sorted(INTEGER_AGGREGATIONS)) - def test_integer_aggs_are_int_type_and_integer_format(self, model, aggregation): - assert aggregated_type( - model=model, measure_name="revenue", aggregation=aggregation, - ) == DataType.INT - assert _infer_aggregated_format( - model=model, measure_name="revenue", aggregation=aggregation, - ).type == NumberFormatType.INTEGER - - def test_star_count_is_int_type_and_integer_format(self, model): - assert aggregated_type( - model=model, measure_name="*", aggregation="count", - ) == DataType.INT - assert _infer_aggregated_format( - model=model, measure_name="*", aggregation="count", - ).type == NumberFormatType.INTEGER - - def test_stat_agg_type_format_divergence_is_pinned(self, model): - # stddev is always a float, so the slot TYPE is DOUBLE — but the display - # FORMAT still inherits the source column's format (currency here), not - # integer/float. This intentional divergence is what DEV-1788 revisits. - assert aggregated_type( - model=model, measure_name="revenue", aggregation="stddev_samp", - ) == DataType.DOUBLE - assert _infer_aggregated_format( - model=model, measure_name="revenue", aggregation="stddev_samp", - ).type == NumberFormatType.CURRENCY + def test_count_bucket(self, model, aggregation, measure): + assert aggregated_type(model=model, measure_name=measure, aggregation=aggregation) == DataType.INT + assert ( + _infer_aggregated_format(model=model, measure_name=measure, aggregation=aggregation).type + == NumberFormatType.INTEGER + ) + + def test_star_count_bucket(self, model): + assert aggregated_type(model=model, measure_name="*", aggregation="count") == DataType.INT + assert ( + _infer_aggregated_format(model=model, measure_name="*", aggregation="count").type + == NumberFormatType.INTEGER + ) + + # --- PRESERVING bucket: inherit source type & format --- + + @pytest.mark.parametrize("aggregation", sorted(PRESERVING_AGGREGATIONS)) + def test_preserving_currency_source(self, model, aggregation): + assert aggregated_type(model=model, measure_name="revenue", aggregation=aggregation) == DataType.DOUBLE + fmt = _infer_aggregated_format(model=model, measure_name="revenue", aggregation=aggregation) + assert fmt.type == NumberFormatType.CURRENCY + assert fmt.symbol == "€" + + @pytest.mark.parametrize("aggregation", sorted(PRESERVING_AGGREGATIONS)) + def test_preserving_unformatted_double_source(self, model, aggregation): + assert aggregated_type(model=model, measure_name="quantity", aggregation=aggregation) == DataType.DOUBLE + assert _infer_aggregated_format(model=model, measure_name="quantity", aggregation=aggregation) is None + + @pytest.mark.parametrize("aggregation", sorted(PRESERVING_AGGREGATIONS)) + def test_preserving_int_source_inherits_int(self, model, aggregation): + # The type axis: PRESERVING inherits the source's INT type ... + assert aggregated_type(model=model, measure_name="qty_int", aggregation=aggregation) == DataType.INT + assert _infer_aggregated_format(model=model, measure_name="qty_int", aggregation=aggregation) is None + + # --- FLOAT_SOURCE_UNITS bucket: DOUBLE type always; format inherits, else FLOAT --- + + @pytest.mark.parametrize("aggregation", sorted(FLOAT_SOURCE_UNIT_AGGREGATIONS)) + def test_float_source_units_currency(self, model, aggregation): + assert aggregated_type(model=model, measure_name="revenue", aggregation=aggregation) == DataType.DOUBLE + fmt = _infer_aggregated_format(model=model, measure_name="revenue", aggregation=aggregation) + assert fmt.type == NumberFormatType.CURRENCY + assert fmt.symbol == "€" + + @pytest.mark.parametrize("measure", ["quantity", "qty_int"]) + @pytest.mark.parametrize("aggregation", sorted(FLOAT_SOURCE_UNIT_AGGREGATIONS)) + def test_float_source_units_unformatted_falls_back_to_float(self, model, aggregation, measure): + # ... whereas FLOAT_SOURCE_UNITS FORCES DOUBLE, even over an INT source. + assert aggregated_type(model=model, measure_name=measure, aggregation=aggregation) == DataType.DOUBLE + assert ( + _infer_aggregated_format(model=model, measure_name=measure, aggregation=aggregation).type + == NumberFormatType.FLOAT + ) + + # --- FLOAT_PLAIN bucket: DOUBLE type; plain FLOAT format on every source --- + + @pytest.mark.parametrize("measure", ["revenue", "quantity", "qty_int"]) + @pytest.mark.parametrize("aggregation", sorted(FLOAT_PLAIN_AGGREGATIONS)) + def test_float_plain_bucket(self, model, aggregation, measure): + assert aggregated_type(model=model, measure_name=measure, aggregation=aggregation) == DataType.DOUBLE + assert ( + _infer_aggregated_format(model=model, measure_name=measure, aggregation=aggregation).type + == NumberFormatType.FLOAT + ) + + # --- missing source column --- + + def test_preserving_missing_column(self, model): + assert aggregated_type(model=model, measure_name="nope", aggregation="sum") is None + assert _infer_aggregated_format(model=model, measure_name="nope", aggregation="sum") is None + + def test_float_source_units_missing_column_falls_back_to_float(self, model): + assert aggregated_type(model=model, measure_name="nope", aggregation="avg") == DataType.DOUBLE + assert ( + _infer_aggregated_format(model=model, measure_name="nope", aggregation="avg").type + == NumberFormatType.FLOAT + ) + + def test_float_plain_missing_column(self, model): + assert aggregated_type(model=model, measure_name="nope", aggregation="corr") == DataType.DOUBLE + assert ( + _infer_aggregated_format(model=model, measure_name="nope", aggregation="corr").type + == NumberFormatType.FLOAT + ) + + # --- drift guard through the public prebound callers --- + + @pytest.mark.parametrize( + "aggregation,exp_type,exp_fmt", + [ + ("count", DataType.INT, NumberFormatType.INTEGER), + ("sum", DataType.DOUBLE, NumberFormatType.CURRENCY), # inherit revenue's currency + ("avg", DataType.DOUBLE, NumberFormatType.CURRENCY), # FLOAT_SOURCE_UNITS inherits currency + ("var_samp", DataType.DOUBLE, NumberFormatType.FLOAT), # FLOAT_PLAIN drops currency + ], + ) + def test_public_callers_agree_per_bucket(self, model, aggregation, exp_type, exp_fmt): + key = AggregateKey(source=ColumnKey(leaf="revenue"), agg=aggregation) + assert measure_key_type(model=model, key=key) == exp_type + fmt, _desc = measure_key_format_description(model=model, key=key) + assert fmt.type == exp_fmt + + def test_public_callers_star_count(self, model): + key = AggregateKey(source=StarKey(), agg="count") + assert measure_key_type(model=model, key=key) == DataType.INT + fmt, desc = measure_key_format_description(model=model, key=key) + assert fmt.type == NumberFormatType.INTEGER + assert desc is None diff --git a/tests/test_response_meta.py b/tests/test_response_meta.py index 4fda2c83..ec3bc244 100644 --- a/tests/test_response_meta.py +++ b/tests/test_response_meta.py @@ -31,6 +31,7 @@ def _orders() -> SlayerModel: type=DataType.DOUBLE, format=NumberFormat(type=NumberFormatType.CURRENCY, symbol="€"), ), + Column(name="quantity", type=DataType.DOUBLE), Column(name="status", type=DataType.TEXT, label="Order status"), ], joins=[ @@ -145,3 +146,41 @@ def test_cross_model_aggregate_format_integer_for_count(): assert fm.format is not None assert fm.format.type == NumberFormatType.CURRENCY assert fm.format.symbol == "$" + + +# --------------------------------------------------------------------------- +# DEV-1788: stat/parametric aggregation display format through the full +# response-metadata path (query → plan → build_response_metadata → attributes). +# --------------------------------------------------------------------------- + + +def test_float_plain_aggregation_is_plain_float(): + # var_samp is in squared units — the source currency is NOT inherited. + q = SlayerQuery(source_model="orders", measures=[{"formula": "amount:var_samp"}]) + attrs, _cols, _sql = _meta_for(q) + fm = attrs.measures["orders.amount_var_samp"] + assert fm.format.type == NumberFormatType.FLOAT + + +def test_avg_inherits_currency_format(): + q = SlayerQuery(source_model="orders", measures=[{"formula": "amount:avg"}]) + attrs, _cols, _sql = _meta_for(q) + fm = attrs.measures["orders.amount_avg"] + assert fm.format.type == NumberFormatType.CURRENCY + assert fm.format.symbol == "€" + + +def test_avg_unformatted_measure_is_plain_float(): + q = SlayerQuery(source_model="orders", measures=[{"formula": "quantity:avg"}]) + attrs, _cols, _sql = _meta_for(q) + fm = attrs.measures["orders.quantity_avg"] + assert fm.format.type == NumberFormatType.FLOAT + + +def test_preserving_unformatted_measure_omits_format_entry(): + # quantity:sum has no source format and no label → no attributes entry, + # though the column is still projected in expected_columns. + q = SlayerQuery(source_model="orders", measures=[{"formula": "quantity:sum"}]) + attrs, cols, _sql = _meta_for(q) + assert "orders.quantity_sum" in cols + assert "orders.quantity_sum" not in attrs.measures