fix(thoughtspot): unique formula ids, and stop erroring on a valid self-join

Three findings from a review of the previous nine commits. Two are regressions
those commits introduced — the pattern this round was meant to break.

DUPLICATE `formulas[].id`, EMITTED SILENTLY. `formulas[].id` has two sources
sharing one namespace, and neither checked the other: an id PRESERVED from the
source stash, and one MINTED from the display name. Narrowing
`_DisplayNameAllocator`'s fold to what ThoughtSpot actually treats as equal --
correct in itself — removed the only thing that had been masking the minted
case, so "Order Amount" and "Order-Amount" both emitted `formula_order_amount`
with no issue. A hand-authored metric minting an id equal to a preserved one
collided the same way. Either makes every `[formula_X]` reference ambiguous,
and ThoughtSpot parses an ambiguous bracket reference as search tokens rather
than failing — so the import succeeds and the model is wrong. Ids now come
from one allocator across both sources, the surfacing column's `formula_id` is
rewritten in lockstep, and a rename is reported.

AN ORDINARY SELF-JOIN NOW FAILED. The column-conflict check added last commit
compared whole entries, but a Table's `columns[]` mixes field-derived entries
(name / db_column_name / db_column_properties) with verbatim
`unsurfaced_columns` stash entries carrying raw TML keys. A column surfaced
through one alias and unsurfaced through the other compared unequal: ERROR,
non-zero exit, on a document that was fine — and one that converted cleanly
before the fix. Only the keys that decide WHICH warehouse column is read are
compared now, which is what the message always claimed.

THE CLI CASEFOLD FIX HAD NO TEST. `-o out.yaml --issues OUT.YAML` was pinned by
nothing, so the guard could regress silently. Its sibling in
`dump_document_set` was pinned; this one was missed by the commit that went
looking for exactly this.

All three mutation-checked. 923 tests pass.

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

Author: djwaldo

converters/thoughtspot/src/ossie_thoughtspot/ossie_to_thoughtspot.py

@@ -1242,6 +1242,15 @@ def _field_physical_display_name(field: dict) -> str | None:
 #: name. Two documents carrying it are not evidence of one warehouse object.
 _UNNAMED_TABLE = "<unnamed>"
 
+#: The keys that decide WHICH warehouse column an entry reads. Only a
+#: disagreement on one of these is a conflict; comparing whole entries flagged
+#: an ordinary self-join, because a Table's `columns[]` mixes field-derived
+#: entries (name / db_column_name / db_column_properties) with verbatim
+#: `unsurfaced_columns` stash entries carrying raw TML keys, and a column
+#: surfaced through one alias and unsurfaced through the other compared unequal
+#: -- an ERROR, and a non-zero exit, on a document that was fine.
+_COLUMN_BINDING_KEYS = ("db_column_name", "sql_output_column")
+
 #: Every body key that can hold columns, for callers that must ignore all of them.
 _COLUMN_KEYS = frozenset({"columns", "sql_view_columns"})
 
@@ -1921,6 +1930,8 @@ def build_model(semantic_model: dict, tables: Sequence[TmlDocument], log: IssueL
     tables_by_name = {t.body.get("name"): t for t in tables}
 
     model_tables: list[dict] = []
+    #: Every `formulas[].id` already handed out, across BOTH sources of them.
+    taken_formula_ids: set[str] = set()
     model_tables_by_prefix: dict[str, dict] = {}
     table_doc_by_prefix: dict[str, TmlDocument | None] = {}
 
@@ -2020,13 +2031,15 @@ def build_model(semantic_model: dict, tables: Sequence[TmlDocument], log: IssueL
             columns_entry, formulas_entry = built
             columns.append(columns_entry)
             if formulas_entry is not None:
+                _allocate_formula_id(formulas_entry, columns_entry, taken_formula_ids, log)
                 formulas.append(formulas_entry)
 
     for metric in semantic_model.get("metrics") or []:
         built = _build_metric(metric, allocator, resolve_field, log)
         if built is None:
             continue
         formulas_entry, columns_entry = built
+        _allocate_formula_id(formulas_entry, columns_entry, taken_formula_ids, log)
         formulas.append(formulas_entry)
         columns.append(columns_entry)
 
@@ -2211,6 +2224,55 @@ class TmlConversion:
     issues: IssueLog
 
 
+def _allocate_formula_id(
+    formulas_entry: dict, columns_entry: dict, taken: set[str], log: IssueLog
+) -> None:
+    """Give this formula an id no other formula in the model holds.
+
+    `formulas[].id` comes from two places that share one namespace and neither
+    of which checks the other: a PRESERVED source id (the stash), and one MINTED
+    from the display name. Two ways for them to collide, both silent until now:
+
+    - two display names folding to one minted id. `_DisplayNameAllocator` used
+      to mask this by renaming one of the display names first, so narrowing its
+      fold to what ThoughtSpot actually treats as equal -- correct in itself --
+      exposed it;
+    - a hand-authored metric whose minted id happens to equal a preserved one.
+
+    A duplicate id makes every `[formula_X]` reference to it ambiguous, and
+    ThoughtSpot resolves an ambiguous bracket reference by parsing it as search
+    tokens rather than failing, so the import succeeds and the model is wrong.
+    The surfacing column's `formula_id` is rewritten in lockstep, which is why
+    this runs where both halves are in hand.
+    """
+    original = formulas_entry["id"]
+    candidate, suffix = original, 1
+    while candidate in taken:
+        suffix += 1
+        candidate = f"{original}_{suffix}"
+    taken.add(candidate)
+    if candidate == original:
+        return
+    log.add(
+        code="TS-MODEL-FORMULA-ID-COLLISION",
+        severity=Severity.WARNING,
+        message=(
+            f"formula {formulas_entry.get('name')!r} would take id {original!r}, "
+            f"which another formula in this model already holds; it is emitted as "
+            f"{candidate!r} instead, because a duplicate id makes every reference "
+            f"to it ambiguous"
+        ),
+        object_ref=f"formula:{formulas_entry.get('name')}",
+        remedy=(
+            "Rename one of the colliding formulas if the generated id matters to "
+            "a cross-reference written by hand."
+        ),
+    )
+    formulas_entry["id"] = candidate
+    if columns_entry is not None and columns_entry.get("formula_id") == original:
+        columns_entry["formula_id"] = candidate
+
+
 def _deduplicate_table_documents(
     tables: list[TmlDocument], log: IssueLog
 ) -> list[TmlDocument]:
@@ -2282,7 +2344,11 @@ def _deduplicate_table_documents(
             if previous is None:
                 first.body.setdefault(column_key, []).append(column)
                 existing[column_name] = column
-            elif previous != column:
+            elif any(
+                previous.get(key) != column.get(key)
+                and key in previous and key in column
+                for key in _COLUMN_BINDING_KEYS
+            ):
                 log.add(
                     code="TS-TABLE-COLUMN-CONFLICT",
                     severity=Severity.ERROR,

converters/thoughtspot/tests/test_cli.py

@@ -385,6 +385,18 @@ def test_to_tml_refuses_issues_aimed_at_a_generated_document(self, tmp_path):
             "--issues", str(out_dir / "orders.table.tml"),
         ]) == 1
 
+    def test_paths_differing_only_by_case_still_collide(self, tmp_path):
+        # `Path.resolve()` does not fold case and the default filesystem on
+        # macOS and Windows does, so this named ONE file and the issue log
+        # overwrote the converted document with exit 0 -- the defect the guard
+        # was written for, surviving its own fix. The parallel fix in
+        # `dump_document_set` was pinned; this one was not.
+        assert cli.main([
+            "to-ossie", *[str(p) for p in _tml_paths("minimal")],
+            "-o", str(tmp_path / "out.yaml"), "--issues", str(tmp_path / "OUT.YAML"),
+        ]) == 1
+        assert not list(tmp_path.iterdir()), "nothing should have been written"
+
     def test_distinct_paths_are_still_accepted(self, tmp_path):
         target, issues = tmp_path / "out.yaml", tmp_path / "issues.json"
         assert cli.main([

converters/thoughtspot/tests/test_ossie_to_thoughtspot.py

@@ -1098,3 +1098,111 @@ def test_an_unrenamed_dataset_keeps_its_alias_and_says_nothing(self):
         ]})
         assert result.documents.model.body["model_tables"][0]["alias"] == "emp"
         assert "TS-DATASET-ALIAS-STALE" not in [i["code"] for i in result.issues.as_dicts()]
+
+
+class TestFormulaIdsAreUniqueAcrossBothSources:
+    """`formulas[].id` has two sources sharing one namespace.
+
+    An id is either PRESERVED from the source document's stash or MINTED from
+    the display name, and neither checked the other. Two ways to collide:
+    two display names folding to one minted id (which `_DisplayNameAllocator`
+    used to mask, so narrowing its fold to what ThoughtSpot actually treats as
+    equal exposed it), and a hand-authored metric whose minted id equals a
+    preserved one. A duplicate id makes every `[formula_X]` reference to it
+    ambiguous, and ThoughtSpot parses an ambiguous bracket reference as search
+    tokens rather than failing -- so the import succeeds and the model is wrong.
+    """
+
+    @staticmethod
+    def _document(*metrics):
+        return {
+            "version": "0.2.0.dev0", "name": "M",
+            "datasets": [{"name": "t", "source": "D.S.T", "fields": [
+                {"name": "a", "expression": {"dialects": [
+                    {"dialect": "THOUGHTSPOT", "expression": "[t::a]"}]}}]}],
+            "metrics": list(metrics),
+        }
+
+    @staticmethod
+    def _metric(name, expression, preserved_id=None):
+        metric = {"name": name, "expression": {"dialects": [
+            {"dialect": "THOUGHTSPOT", "expression": expression}]}}
+        if preserved_id:
+            metric["custom_extensions"] = [{"vendor_name": "THOUGHTSPOT", "data": json.dumps(
+                {"_v": 1, "formula_id": preserved_id})}]
+        return metric
+
+    def test_two_names_minting_one_id_are_separated(self):
+        result = convert(self._document(
+            self._metric("Order Amount", "sum ( [t::a] )"),
+            self._metric("Order-Amount", "max ( [t::a] )"),
+        ))
+        ids = [f["id"] for f in result.documents.model.body["formulas"]]
+        assert len(ids) == len(set(ids)), f"duplicate formula ids: {ids}"
+
+    def test_a_minted_id_never_collides_with_a_preserved_one(self):
+        result = convert(self._document(
+            self._metric("Net Margin", "sum ( [t::a] )", preserved_id="formula_margin"),
+            self._metric("Margin", "max ( [t::a] )"),
+        ))
+        ids = [f["id"] for f in result.documents.model.body["formulas"]]
+        assert ids[0] == "formula_margin", "the preserved id must win"
+        assert len(ids) == len(set(ids)), f"duplicate formula ids: {ids}"
+
+    def test_the_surfacing_column_follows_the_renamed_id(self):
+        # A renamed id that the column still points at by its OLD value would
+        # leave the column bound to nothing.
+        result = convert(self._document(
+            self._metric("Order Amount", "sum ( [t::a] )"),
+            self._metric("Order-Amount", "max ( [t::a] )"),
+        ))
+        body = result.documents.model.body
+        emitted = {f["id"] for f in body["formulas"]}
+        referenced = {c["formula_id"] for c in body["columns"] if c.get("formula_id")}
+        assert referenced <= emitted, f"columns point at missing ids: {referenced - emitted}"
+
+    def test_the_rename_is_reported(self):
+        result = convert(self._document(
+            self._metric("Order Amount", "sum ( [t::a] )"),
+            self._metric("Order-Amount", "max ( [t::a] )"),
+        ))
+        assert "TS-MODEL-FORMULA-ID-COLLISION" in [i["code"] for i in result.issues.as_dicts()]
+
+
+class TestAnAliasedSelfJoinWithUnsurfacedColumnsIsNotAConflict:
+    """A Table's `columns[]` mixes two shapes; only the BINDING keys conflict.
+
+    Field-derived entries carry exactly name/db_column_name/db_column_properties;
+    verbatim `unsurfaced_columns` stash entries carry raw TML (`properties`,
+    `description`, ...). Comparing whole entries made a column surfaced through
+    one alias and unsurfaced through the other compare unequal -- an ERROR, and
+    a non-zero exit, on a document that was perfectly fine.
+    """
+
+    def test_an_ordinary_aliased_self_join_reports_no_conflict(self):
+        table = TmlDocument(kind="table", guid=None, body={
+            "name": "DATE_DIM", "db": "D", "schema": "S", "db_table": "DATE_DIM",
+            "connection": {"name": "Conn"},
+            "columns": [
+                {"name": c, "db_column_name": c.upper(),
+                 "properties": {"column_type": "ATTRIBUTE"},
+                 "db_column_properties": {"data_type": "DATE"}}
+                for c in ("d_date", "d_year")
+            ]})
+        model = TmlDocument(kind="model", guid=None, body={
+            "name": "M",
+            "model_tables": [{"name": "DATE_DIM", "alias": "sold"},
+                             {"name": "DATE_DIM", "alias": "ship"}],
+            "columns": [
+                {"name": "Sold Date", "column_id": "sold::d_date",
+                 "properties": {"column_type": "ATTRIBUTE"}},
+                {"name": "Sold Year", "column_id": "sold::d_year",
+                 "properties": {"column_type": "ATTRIBUTE"}},
+                {"name": "Ship Date", "column_id": "ship::d_date",
+                 "properties": {"column_type": "ATTRIBUTE"}},
+            ]})
+        ossie = tml_to_ossie_convert(DocumentSet(model=model, tables=(table,)))
+        result = convert(ossie.model)
+        codes = [i["code"] for i in result.issues.as_dicts()]
+        assert "TS-TABLE-COLUMN-CONFLICT" not in codes
+        assert not result.issues.has_errors()