fix(thoughtspot): a bare group_aggregate keeps its column aggregation

Not every already-aggregating formula makes the surfacing column's
`aggregation` inert, and the converter treated them as if it did.

Established by domain review and confirmed against a live cluster:

  sum ( ... )                      column aggregation is a NO-OP
  group_sum / group_average ( ... )  NO-OP -- these behave like ordinary formulas
  sum ( group_aggregate ( ... ) )    NO-OP -- wrapped, so the outer call rules
  group_aggregate ( ... )   BARE     USED -- like a raw column, ThoughtSpot may
                                     apply the column's aggregation to it

The code grouped all four as "the documented no-op", naming
`group_aggregate ( ... )` in that comment explicitly, and discarded the value
with nothing logged -- deliberately, since warning on the genuinely inert
shapes would train readers to ignore the issue log. So the one shape where the
property is load-bearing lost it silently: a changed answer, not a changed
spelling.

Ossie's metric expression has nowhere to put it, so it is preserved verbatim in
the stash and restored, rather than discarded or folded into the expression --
folding would change what the formula means, since wrapping a bare
`group_aggregate` is exactly what makes the property inert.

Also recorded, because it was nearly acted on as a defect: `sum([SALES])` with
`aggregation: SUM` is genuinely a no-op. A converted model carrying both was
imported into a live cluster and returned numbers identical to its source. An
earlier reading of a `NESTED_AGGREGATE_NOT_SUPPORTED` error as a converter
defect was wrong -- the nesting was in the QUERY, which wrapped an
already-aggregated formula in SUM().

Mutation-checked. 932 tests pass.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Author: djwaldo

converters/thoughtspot/docs/vendor-payload.md

@@ -38,6 +38,7 @@ Every `custom_extensions` entry this converter writes uses `vendor_name` `THOUGH
 | Key | Scope | Classification | Treatment on the return trip |
 |---|---|---|---|
 | `tml_name` | Shared | shadows_derivable | Restored only if reconstructing it from the live document still agrees with the stashed value (self-verifying — no separate witness key); disagreement re-derives instead. |
+| `column_aggregation_value` | Metric | information_only | Restored as-is whenever present — nothing on the Ossie side could have diverged from it. |
 | `tml_obj_id` | Model | information_only | Restored as-is whenever present — nothing on the Ossie side could have diverged from it. |
 | `formula_id` | Field | information_only | Restored as-is whenever present — nothing on the Ossie side could have diverged from it. |
 | `db_column_name` | Field | shadows_derivable | Restored only if its witness companion key still matches the live document's current value; a mismatch means the document changed since the stash was written, so the value is re-derived instead. |

converters/thoughtspot/src/ossie_thoughtspot/constants.py

@@ -99,6 +99,20 @@
 #: must agree on the exact spelling and nothing else enforces that.
 FIELD_STASH_DB_COLUMN_NAME = "db_column_name"
 
+#: A surfacing column's `aggregation` that is LOAD-BEARING and has no home in
+#: the Ossie metric's own expression, so it is preserved verbatim instead.
+#:
+#: The one shape this is for: a formula whose outer call is a BARE
+#: `group_aggregate ( ... )`. Every other already-aggregating shape makes the
+#: column's aggregation a genuine no-op -- `sum ( ... )`, and the
+#: `group_sum`/`group_average` shorthands, all behave that way (verified on a
+#: live cluster: a model carrying `sum([SALES])` WITH `aggregation: SUM`
+#: returns the same number as the source). A bare `group_aggregate` does not:
+#: like a raw column, ThoughtSpot may APPLY the column aggregation to it. It
+#: was being discarded with the others and with nothing logged, which silently
+#: changes the number.
+METRIC_STASH_COLUMN_AGGREGATION = "column_aggregation_value"
+
 #: The source TML `obj_id` -- ThoughtSpot's own PORTABLE object handle, e.g.
 #: `SampleRetail-Apparel-LH-58435d2b` (display name, then the first segment of
 #: the GUID). Stashed under a distinct payload key so the forbidden-key scan,
@@ -473,6 +487,8 @@ class StashKeyClass(Enum):
     # it -- TML's id is independent of its name, so renaming the metric in Ossie
     # must NOT change the id, or every cross-reference written against it breaks.
     # Ossie has no object-identity concept, so nothing here can diverge from it.
+    # No Ossie counterpart: the metric's expression cannot carry it.
+    METRIC_STASH_COLUMN_AGGREGATION: StashKeyClass.INFORMATION_ONLY,
     MODEL_STASH_OBJ_ID: StashKeyClass.INFORMATION_ONLY,
     FIELD_STASH_FORMULA_ID: StashKeyClass.INFORMATION_ONLY,
     FIELD_STASH_DB_COLUMN_NAME: StashKeyClass.SHADOWS_DERIVABLE,

converters/thoughtspot/src/ossie_thoughtspot/ossie_to_thoughtspot.py

@@ -91,6 +91,7 @@
     FIELD_STASH_DATA_TYPE_WITNESS,
     FIELD_STASH_DB_COLUMN_NAME,
     FIELD_STASH_DB_COLUMN_NAME_WITNESS,
+    METRIC_STASH_COLUMN_AGGREGATION,
     METRIC_SHAPE_COLUMN_AGGREGATION,
     METRIC_SHAPE_FORMULA,
     METRIC_SHAPE_SCALAR_FORMULA_PLUS_AGGREGATION,
@@ -1562,6 +1563,15 @@ def _build_metric(
         else:
             properties["aggregation"], formula_expr = decomposed
 
+    # A LOAD-BEARING aggregation, preserved verbatim because the metric's
+    # expression had nowhere to carry it: a bare `group_aggregate ( ... )`
+    # takes its column's aggregation the way a raw column does, unlike every
+    # other already-aggregating shape. Restored before the conventional path
+    # below, which would otherwise not set one at all for this shape.
+    preserved_aggregation = payload.get(METRIC_STASH_COLUMN_AGGREGATION)
+    if preserved_aggregation and "aggregation" not in properties:
+        properties["aggregation"] = preserved_aggregation
+
     if "aggregation" not in properties:
         conventional = _outer_aggregation_of(ts_expr)
         if conventional is not None:

converters/thoughtspot/src/ossie_thoughtspot/tml_to_ossie.py

@@ -105,6 +105,7 @@
     FIELD_STASH_DB_COLUMN_NAME,
     FIELD_STASH_FORMULA_ID,
     FIELD_STASH_DB_COLUMN_NAME_WITNESS,
+    METRIC_STASH_COLUMN_AGGREGATION,
     METRIC_SHAPE_COLUMN_AGGREGATION,
     METRIC_SHAPE_FORMULA,
     METRIC_SHAPE_SCALAR_FORMULA_PLUS_AGGREGATION,
@@ -755,6 +756,19 @@ def convert_field(
 )
 
 
+def _is_bare_group_aggregate(expr: str) -> bool:
+    """Whether `expr`'s outer call is `group_aggregate` itself.
+
+    The distinction that matters: a bare `group_aggregate ( ... )` takes its
+    surfacing column's `aggregation` the way a raw column does, while
+    `sum ( group_aggregate ( ... ) )` -- wrapped -- does not, and neither do the
+    `group_sum`/`group_average` shorthands, which behave like ordinary
+    formulas.
+    """
+    call = formula.split_call(expr)
+    return call is not None and call[0].strip().lower() == "group_aggregate"
+
+
 def _outer_call_is_aggregate(expr: str) -> bool:
     """Whether `expr`'s own outer call (not something nested inside it) is a
     native ThoughtSpot aggregate.
@@ -959,6 +973,7 @@ def convert_metric(
         )
         aggregation_raw = "NONE"
     aggregation = _AGGREGATION[aggregation_raw]
+    load_bearing_aggregation: str | None = None
 
     if "column_id" in column:
         metric_shape = METRIC_SHAPE_COLUMN_AGGREGATION
@@ -1021,13 +1036,30 @@ def convert_metric(
                 expr, resolve, log, object_ref=object_ref, kind="metric"
             )
         elif _outer_call_is_aggregate(expr):
-            # The documented no-op: the expression's own call already
-            # aggregates (sum ( ... ), group_aggregate ( ... ), ...), and
-            # ThoughtSpot's UI sets a column aggregation on a formula column
-            # like this routinely, whether or not it is redundant. Discarding
-            # it here is expected, not a loss, so nothing is logged --
-            # warning on this common, correct shape would train readers to
-            # ignore the issue log entirely.
+            # Mostly the documented no-op: the expression's own call already
+            # aggregates, and ThoughtSpot's UI sets a column aggregation on a
+            # formula column like this routinely whether or not it is
+            # redundant. Discarding it is expected, not a loss, so nothing is
+            # logged -- warning on this common, correct shape would train
+            # readers to ignore the issue log entirely. Confirmed on a live
+            # cluster: a model carrying `sum([SALES])` WITH `aggregation: SUM`
+            # returns the same numbers as the source.
+            #
+            # ONE EXCEPTION, and it changes the answer. A BARE
+            # `group_aggregate ( ... )` -- not itself wrapped in an aggregate --
+            # behaves like a raw column: ThoughtSpot may APPLY the column's
+            # aggregation to it. (`sum ( group_aggregate ( ... ) )` is wrapped,
+            # so there the property is inert again, as are the
+            # `group_sum`/`group_average` shorthands, which behave like any
+            # other formula.) This comment used to name `group_aggregate` among
+            # the no-ops, and the value was dropped with the rest -- silently,
+            # since nothing is logged on this path.
+            #
+            # Ossie's metric expression has nowhere to put it, so it is
+            # preserved verbatim in the stash rather than discarded or folded
+            # into the expression, which would change what the formula means.
+            if _is_bare_group_aggregate(expr):
+                load_bearing_aggregation = aggregation_raw
             metric_shape = METRIC_SHAPE_FORMULA
             dialects = expression_entries(
                 expr, resolve, log, object_ref=object_ref, kind="metric"
@@ -1081,6 +1113,8 @@ def convert_metric(
         stash_payload[STASH_TML_NAME] = display_name
     if metric_shape != METRIC_SHAPE_FORMULA:
         stash_payload[METRIC_STASH_SHAPE] = metric_shape
+    if load_bearing_aggregation is not None:
+        stash_payload[METRIC_STASH_COLUMN_AGGREGATION] = load_bearing_aggregation
     metric = _write_stash_safely(metric, stash_payload, log, object_ref)
 
     description = column.get("description")

converters/thoughtspot/tests/test_roundtrip.py

@@ -957,3 +957,55 @@ def test_the_models_obj_id_survives_the_round_trip():
     # The guid must still never travel.
     assert "11111111-2222" not in emitted
     assert "guid:" not in emitted
+
+
+class TestABareGroupAggregateKeepsItsColumnAggregation:
+    """Not every already-aggregating formula makes the column property inert.
+
+    Verified against a live ThoughtSpot cluster and against domain review:
+
+    - `sum ( ... )`, and the `group_sum`/`group_average` shorthands, behave like
+      ordinary formulas -- the surfacing column's `aggregation` is a NO-OP. A
+      model carrying `sum([SALES])` WITH `aggregation: SUM` returns the same
+      numbers as its source.
+    - `sum ( group_aggregate ( ... ) )` -- wrapped -- is inert for the same reason.
+    - a BARE `group_aggregate ( ... )` is NOT. Like a raw column, ThoughtSpot may
+      APPLY the column's aggregation to it.
+
+    The converter grouped all of them as "the documented no-op" and discarded
+    the value, with nothing logged, so the one shape where it is load-bearing
+    silently lost it -- a changed answer, not a changed spelling.
+    """
+
+    BARE = "group_aggregate ( sum ( [T::a] ) , query_groups ( ) , query_filters ( ) )"
+
+    @staticmethod
+    def _round_trip(expr, aggregation):
+        table = TmlDocument(kind="table", guid=None, body={
+            "name": "T", "db": "D", "schema": "S", "db_table": "T",
+            "connection": {"name": "Conn"},
+            "columns": [{"name": c, "db_column_name": c.upper(),
+                         "db_column_properties": {"data_type": "DOUBLE"}} for c in ("a", "r")]})
+        properties = {"column_type": "MEASURE"}
+        if aggregation:
+            properties["aggregation"] = aggregation
+        model = TmlDocument(kind="model", guid=None, body={
+            "name": "M", "model_tables": [{"name": "T"}],
+            "formulas": [{"id": "f1", "name": "Metric", "expr": expr}],
+            "columns": [{"name": "Metric", "formula_id": "f1", "properties": properties}]})
+        ossie = tml_to_ossie.convert(DocumentSet(model=model, tables=(table,)))
+        returned = ossie_to_thoughtspot.convert(ossie.model).documents.model.body
+        column = next(c for c in returned["columns"] if c["name"] == "Metric")
+        return (column.get("properties") or {}).get("aggregation")
+
+    def test_a_bare_group_aggregate_keeps_it(self):
+        assert self._round_trip(self.BARE, "SUM") == "SUM"
+
+    def test_a_bare_group_aggregate_without_one_gains_none(self):
+        assert self._round_trip(self.BARE, None) is None
+
+    def test_a_wrapped_group_aggregate_is_unaffected(self):
+        assert self._round_trip(f"sum ( {self.BARE} )", "SUM") == "SUM"
+
+    def test_a_plain_aggregate_is_unaffected(self):
+        assert self._round_trip("sum ( [T::a] )", "SUM") == "SUM"