fix(thoughtspot): eight defects found by a max-level review of the converter

All eight reproduce; each is verified against its own failing case and pinned
by a regression test. 848 tests pass (was 824).

SILENT DOUBLE AGGREGATION. `_AGGREGATE_CALL_NAMES` knew `group_aggregate`
alone, above a comment asserting "there is exactly one such construct", while
this package's own reverse inventory registered four shorthands (group_sum,
group_count, group_stddev, group_variance) and two further *_aggregate_op
variants. A MEASURE column using any of them was classified as a scalar formula
and had its own `aggregation` composed on top: `sum ( group_sum ( ... ) )`, no
issue raised, wrong number. The vocabulary is now derived from one source.

CARDINALITY SURVIVED A STALE SWAP WITNESS. An Ossie relationship has no
cardinality field -- `from` is the many side, `to` the one side, so direction IS
cardinality. When the endpoint-swap witness went stale the swap was correctly
abandoned but the stashed cardinality was still read, declaring the join
backwards; ThoughtSpot uses cardinality for fan-out, so the model returned
multiplied rows. Gated on the stale-swap condition, which also honours that
issue's own promise to emit "exactly as a hand-authored relationship would".

A PARENTHESISED JOIN CONDITION LOST ITS RELATIONSHIP. `formula._scan` tracks
paren depth, so `( [A::x] = [B::y] and [A::p] = [B::q] )` -- an ordinary TML
spelling -- put every `and` at depth 1: nothing split, no equality pair matched,
and the whole relationship was demoted to an unrepresentable-join stash entry,
leaving the datasets disconnected and `derive_keys` with no candidate.

AN ALIASED SELF-JOIN EMITTED DUPLICATE TABLE DOCUMENTS. N datasets over one
warehouse table produced N Table documents of the same name; filename
disambiguation then hid the collision and the import created a duplicate Table
object. One document per distinct table now, columns unioned. Found while
fixing it: two model_tables[] entries with no distinguishing alias are
ambiguous on import and were unreported -- now an ERROR.

Also: `zip` truncated a join condition when from_columns/to_columns disagreed,
dropping predicates silently (now refused, matching upstream #375's equal-arity
rule); `_restore_ai_context` overwrote a faithfully stashed synonym_type; shared
list objects made PyYAML emit anchor/alias pairs into every generated document
and both fixtures; `_safe_target_path`'s condition let through the one case it
existed to catch; `read_stash` and a non-UTF-8 input file escaped the CLI's
handler as bare tracebacks.

Fixtures regenerated, proven deep-equal to their previous parsed content. Both
fixtures and emitted output validate against the current upstream schema; the
TPC-DS fixture round-trips end to end.

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

Author: djwaldo

converters/thoughtspot/src/ossie_thoughtspot/cli.py

@@ -124,7 +124,10 @@ def _safe_target_path(directory: Path, filename: str) -> Path:
     """
     resolved_directory = directory.resolve()
     target = (resolved_directory / filename).resolve()
-    if target != resolved_directory and resolved_directory not in target.parents:
+    # `==`, not `!=`: a filename resolving to the output directory ITSELF is the
+    # case this guard exists to catch, and the old `target != resolved_directory
+    # and ...` spelling made that case short-circuit to False and pass.
+    if target == resolved_directory or resolved_directory not in target.parents:
         raise ConversionError(
             f"refusing to write {filename!r}: it resolves outside the output directory "
             f"{resolved_directory}"
@@ -163,7 +166,7 @@ def _cmd_to_ossie(args: argparse.Namespace) -> int:
         texts = [(path, Path(path).read_text(encoding="utf-8")) for path in args.tml_files]
         document_set = tml.load_document_set(texts)
         result = tml_to_ossie.convert(document_set)
-    except (ConversionError, OSError) as e:
+    except (ConversionError, OSError, UnicodeDecodeError) as e:
         print(f"Error: {e}", file=sys.stderr)
         return 1
 
@@ -204,7 +207,7 @@ def _cmd_to_tml(args: argparse.Namespace) -> int:
         # TML/Ossie content, and `dump_document_set` sanitises filenames for exactly this
         # reason (see `_safe_target_path`).
         targets = [(_safe_target_path(output_dir, name), text_) for name, text_ in files]
-    except (ConversionError, OSError) as e:
+    except (ConversionError, OSError, UnicodeDecodeError) as e:
         print(f"Error: {e}", file=sys.stderr)
         return 1
 

converters/thoughtspot/src/ossie_thoughtspot/expressions/__init__.py

@@ -38,6 +38,7 @@
 from .catalog import CATALOG, CONVENTION_DIVERGENCES, spec_construct_names
 from .emit import emit_direct, emit_passthrough, emit_unmappable
 from .reverse import (
+    GROUP_AGGREGATE_CALL_NAMES,
     REVERSE,
     ReverseConstruct,
     ReverseDisposition,
@@ -54,6 +55,7 @@
     "CONVENTION_DIVERGENCES",
     "Classification",
     "Construct",
+    "GROUP_AGGREGATE_CALL_NAMES",
     "REVERSE",
     "ReverseConstruct",
     "ReverseDisposition",

converters/thoughtspot/src/ossie_thoughtspot/expressions/_types.py

@@ -52,6 +52,13 @@ class Variant(str, Enum):
     STRING = "sql_string_op"
     INT_AGGREGATE = "sql_int_aggregate_op"
     NUMBER_AGGREGATE = "sql_number_aggregate_op"
+    # Declared for completeness of the *_aggregate_op family even though no
+    # catalog row targets them today: `tml_to_ossie` derives "what already
+    # aggregates" by filtering this enum on the `_aggregate_op` suffix, so a
+    # missing member there is a missed double-aggregation guard, not merely an
+    # absent rendering option. Both are named in the reverse inventory.
+    STRING_AGGREGATE = "sql_string_aggregate_op"
+    DATE_TIME_AGGREGATE = "sql_date_time_aggregate_op"
 
 
 class VariadicStyle(str, Enum):

converters/thoughtspot/src/ossie_thoughtspot/expressions/reverse.py

@@ -562,6 +562,17 @@ def _dispatch(args: list[str], log: IssueLog, object_ref: str, connection_dialec
         ),
     )
 
+#: Every ThoughtSpot call name that performs a GROUPED aggregation: the general
+#: `group_aggregate` plus the shorthands above. Exported because `tml_to_ossie`
+#: must recognise all of them as "already aggregated" -- it previously named
+#: `group_aggregate` alone, on the stated belief that "there is exactly one such
+#: construct", and so composed a metric's own `aggregation` on top of a
+#: `group_sum(...)` formula: `sum ( group_sum ( ... ) )`, silently doubled.
+#: Derived from the inventory rather than retyped, so a shorthand added above is
+#: covered without a second edit.
+GROUP_AGGREGATE_CALL_NAMES = frozenset({"group_aggregate", *_GROUP_SHORTHAND_AGGREGATES})
+
+
 _SEMI_ADDITIVE_ISSUE = (
     "{name} declares a genuine partition and order axis, and that window clause round-trips "
     "faithfully — but semi-additivity is a roll-up declaration (do not re-sum this measure "

converters/thoughtspot/src/ossie_thoughtspot/keys.py

@@ -149,5 +149,11 @@ def derive_keys(
             ),
         )
 
-    primary_key = seen[0] if len(seen) == 1 else None
+    # `list(...)`, not `seen[0]`: returning the same list OBJECT in both slots
+    # made PyYAML emit a YAML anchor/alias pair -- `primary_key: &id001` with
+    # `unique_keys: [*id001]` -- into every generated document, and into both
+    # committed fixtures, where the tests pinned it rather than caught it.
+    # Aliases are valid YAML but a common hardening default disables them, and a
+    # non-PyYAML reader that does not resolve them reads null.
+    primary_key = list(seen[0]) if len(seen) == 1 else None
     return primary_key, seen

converters/thoughtspot/src/ossie_thoughtspot/ossie_to_thoughtspot.py

@@ -1206,7 +1206,12 @@ def _restore_ai_context(properties: dict, ai_context: object, log: IssueLog, *,
     synonyms = ai_context.get("synonyms")
     if synonyms:
         properties["synonyms"] = list(synonyms)
-        properties["synonym_type"] = "USER_DEFINED"
+        # Only when the source did not state one. This runs AFTER the stashed
+        # column_properties are merged in, so an unconditional assignment
+        # overwrote a faithfully round-tripped `AUTO_GENERATED` with
+        # `USER_DEFINED` and logged nothing -- changing a column's synonym
+        # provenance, which the never-a-silent-loss contract forbids.
+        properties.setdefault("synonym_type", "USER_DEFINED")
     instructions = ai_context.get("instructions")
     if instructions:
         properties["ai_context"] = instructions
@@ -1582,7 +1587,22 @@ def _restore_relationship_condition(
     """The equality-only `on:` condition for a relationship with no stashed
     `on_expression` -- reconstructed from `from_columns`/`to_columns` alone,
     which is all a hand-authored relationship (no stash) has to go on."""
-    pairs = zip(from_columns or [], to_columns or [])
+    from_columns = from_columns or []
+    to_columns = to_columns or []
+    # `zip` truncates to the shorter list, so a relationship whose two arrays
+    # disagree emitted a condition covering only the shorter one -- the extra
+    # predicates vanished with nothing logged, and the imported join then
+    # matched MORE rows than the Ossie document declared. Every other arity
+    # mismatch in this package raises rather than truncating; upstream
+    # apache/ossie#375 made equal arity a validation rule, so a document
+    # reaching here unequal is malformed.
+    if len(from_columns) != len(to_columns):
+        raise ConversionError(
+            f"relationship {from_prefix!r} -> {to_prefix!r} has "
+            f"{len(from_columns)} from_columns and {len(to_columns)} to_columns; "
+            f"they must have equal arity to form a join condition"
+        )
+    pairs = zip(from_columns, to_columns)
     return " and ".join(
         f"{identifiers.format_column_ref(from_prefix, fc)} = "
         f"{identifiers.format_column_ref(to_prefix, tc)}"
@@ -1702,7 +1722,27 @@ def _join_entry_for_relationship(rel: dict, log: IssueLog) -> tuple[str, dict, d
             )
         on_expression = _restore_relationship_condition(from_prefix, to_prefix, from_columns, to_columns)
     join_type = _normalise_join_type(payload.get(RELATIONSHIP_STASH_TYPE) or "INNER")
-    cardinality = payload.get(RELATIONSHIP_STASH_CARDINALITY) or "MANY_TO_ONE"
+    # Gated on the SAME witness as the endpoint swap, because it states the same
+    # fact. An Ossie relationship carries no cardinality field: `from` is the many
+    # side and `to` is the one side, so direction IS cardinality. When the witness
+    # is stale the swap above is deliberately not undone and the orientation is
+    # taken live -- and a stashed `ONE_TO_MANY` read alongside that live
+    # orientation declares the join backwards, which ThoughtSpot uses for
+    # fan-out, so the model returns multiplied rows.
+    #
+    # The stale-swap issue already promises the join is emitted "exactly as a
+    # hand-authored relationship with no stash at all would be". A hand-authored
+    # one gets the MANY_TO_ONE default; reading the stash here broke that promise.
+    # Gated on the stale-swap condition itself, not on the witness directly: a
+    # relationship that never carried a swap stash (hand-authored, never round
+    # -tripped) has a cardinality that stands on its own, and there is nothing
+    # stale about it. Only a swap that WAS stashed and has since gone stale
+    # invalidates it.
+    stashed_swap_is_stale = had_stashed_swap and not endpoints_swapped
+    cardinality = (
+        "MANY_TO_ONE" if stashed_swap_is_stale
+        else (payload.get(RELATIONSHIP_STASH_CARDINALITY) or "MANY_TO_ONE")
+    )
 
     live_name = rel.get("name")
     stashed_referencing_join = payload.get(RELATIONSHIP_STASH_REFERENCING_JOIN)
@@ -1824,6 +1864,35 @@ def build_model(semantic_model: dict, tables: Sequence[TmlDocument], log: IssueL
         table_entry: dict = {"name": table_ref}
         if alias:
             table_entry["alias"] = alias
+        # Two entries naming one table must be told apart by their aliases --
+        # `model_tables[].joins[].with` and every `[PREFIX::Column]` reference
+        # resolve by alias-or-name, so an undistinguished pair is ambiguous on
+        # import. Found while fixing the duplicate-Table-document defect: the
+        # aliased case is the normal self-join and is fine; this is the case
+        # where the aliases did not survive.
+        clash = next(
+            (e for e in model_tables
+             if e["name"] == table_entry["name"]
+             and e.get("alias") == table_entry.get("alias")),
+            None,
+        )
+        if clash is not None:
+            log.add(
+                code="TS-MODEL-TABLE-ENTRY-AMBIGUOUS",
+                severity=Severity.ERROR,
+                message=(
+                    f"datasets {dataset_prefix!r} and an earlier one both resolve "
+                    f"to table {table_ref!r} with the same alias "
+                    f"({table_entry.get('alias')!r}); model_tables[] entries are "
+                    f"referenced by alias-or-name, so the two are indistinguishable "
+                    f"and every reference to either is ambiguous on import"
+                ),
+                object_ref=f"dataset:{dataset_prefix}",
+                remedy=(
+                    "Give the datasets distinct aliases, or a distinct source "
+                    "table each if they are not a self-join."
+                ),
+            )
         model_tables.append(table_entry)
         model_tables_by_prefix[dataset_prefix] = table_entry
 
@@ -2033,6 +2102,65 @@ class TmlConversion:
     issues: IssueLog
 
 
+def _deduplicate_table_documents(
+    tables: list[TmlDocument], log: IssueLog
+) -> list[TmlDocument]:
+    """One Table document per distinct table name, not one per dataset.
+
+    An aliased self-join is N Ossie datasets over ONE warehouse table -- a model
+    joining DATE_DIM twice as "Sold Date" and "Ship Date" is two datasets whose
+    stashed source table is the same. Building a document per dataset emitted
+    two Table documents both named `DATE_DIM`; `dump_document_set` then gave
+    them distinct FILEnames (`DATE_DIM.table.tml`, `DATE_DIM-2.table.tml`),
+    which hid the collision rather than surfacing it, and importing the set
+    created a duplicate ThoughtSpot Table object. The model side was already
+    correct: `model_tables[]` carries one entry per dataset with its own alias,
+    all pointing at the single table name.
+
+    Columns are unioned by name, first occurrence winning, because two aliases
+    of one table may surface different subsets of it and the Table document has
+    to hold every column any alias references. A body that differs beyond its
+    columns cannot be merged that way, so the first is kept and the difference
+    reported rather than silently resolved.
+    """
+    by_name: dict[str, TmlDocument] = {}
+    order: list[str] = []
+    for table in tables:
+        name = table.body.get("name")
+        first = by_name.get(name)
+        if first is None:
+            by_name[name] = table
+            order.append(name)
+            continue
+
+        seen_columns = {c.get("name") for c in first.body.get("columns") or []}
+        for column in table.body.get("columns") or []:
+            if column.get("name") not in seen_columns:
+                first.body.setdefault("columns", []).append(column)
+                seen_columns.add(column.get("name"))
+
+        ignoring_columns = (
+            {k: v for k, v in first.body.items() if k != "columns"},
+            {k: v for k, v in table.body.items() if k != "columns"},
+        )
+        if ignoring_columns[0] != ignoring_columns[1]:
+            log.add(
+                code="TS-TABLE-ALIAS-BODY-DIVERGENT",
+                severity=Severity.WARNING,
+                message=(
+                    f"two datasets resolve to table {name!r} but describe it "
+                    f"differently (connection, description or properties); the "
+                    f"first description is emitted and the second is not"
+                ),
+                object_ref=f"table:{name}",
+                remedy=(
+                    "Make the aliased datasets agree, or give them distinct "
+                    "source tables if they are genuinely different tables."
+                ),
+            )
+    return [by_name[name] for name in order]
+
+
 def convert(ossie_document: dict) -> TmlConversion:
     """Convert one Ossie document into one ThoughtSpot TML document set.
 
@@ -2081,6 +2209,8 @@ def convert(ossie_document: dict) -> TmlConversion:
         raise ConversionError("the Ossie document has no datasets to convert")
 
     log = IssueLog()
-    tables = [build_table(dataset, log) for dataset in semantic_model.get("datasets") or []]
+    tables = _deduplicate_table_documents(
+        [build_table(dataset, log) for dataset in semantic_model.get("datasets") or []], log
+    )
     model = build_model(semantic_model, tables, log)
     return TmlConversion(documents=DocumentSet(model=model, tables=tuple(tables)), issues=log)

converters/thoughtspot/src/ossie_thoughtspot/stash.py

@@ -75,7 +75,21 @@ def find_forbidden_key(value: Any, forbidden: frozenset[str] | None = None) -> s
 
 def read_stash(obj: dict) -> dict[str, Any]:
     """Return this object's parsed THOUGHTSPOT payload, or {} if it has none."""
-    for entry in obj.get("custom_extensions") or []:
+    extensions = obj.get("custom_extensions") or []
+    if not isinstance(extensions, list):
+        raise ConversionError(
+            f"custom_extensions must be a list, not {type(extensions).__name__}"
+        )
+    for entry in extensions:
+        # A hand-authored document can put anything here. Without this the
+        # `.get` below raised a bare AttributeError, which escapes the CLI's
+        # (ConversionError, OSError) handler and prints a traceback -- breaking
+        # this module's own never-a-bare-traceback contract.
+        if not isinstance(entry, dict):
+            raise ConversionError(
+                f"each custom_extensions entry must be a mapping, not "
+                f"{type(entry).__name__}"
+            )
         if entry.get("vendor_name") != VENDOR_KEY:
             continue
         raw = entry.get("data")

converters/thoughtspot/src/ossie_thoughtspot/tml_to_ossie.py

@@ -129,7 +129,7 @@
     STASH_TML_NAME,
 )
 from .errors import ConversionError
-from .expressions import CATALOG, Variant, emit_direct
+from .expressions import CATALOG, GROUP_AGGREGATE_CALL_NAMES, Variant, emit_direct
 from .issues import IssueLog, Severity
 from .tml import DocumentSet
 
@@ -711,12 +711,19 @@ def convert_field(
 #: it appears in an expression — see `_contains_aggregate_call`.
 _NATIVE_AGGREGATE_SPECS = (*_AGGREGATION_CATALOG_SPEC.values(), "MEDIAN(expr)")
 
-#: `group_aggregate` is ThoughtSpot's own construct for a grouped/windowed
-#: aggregation — the *performant* pattern the catalog's window-function rows
-#: prefer over a raw `sql_*_aggregate_op` pass-through — and it is not a target of
-#: any TML `aggregation` enum value, so it cannot come from `_AGGREGATION_CATALOG_SPEC`.
-#: There is exactly one such construct, so it is named directly rather than derived.
-_GROUP_AGGREGATE_CALL = "group_aggregate"
+#: ThoughtSpot's grouped/windowed aggregations — the *performant* pattern the
+#: catalog's window-function rows prefer over a raw `sql_*_aggregate_op`
+#: pass-through. None is the target of any TML `aggregation` enum value, so they
+#: cannot come from `_AGGREGATION_CATALOG_SPEC`.
+#:
+#: Imported from the reverse inventory rather than named here. This constant
+#: previously read `"group_aggregate"` alone, above a comment asserting "there is
+#: exactly one such construct" — while the same package's reverse inventory
+#: registered four more (`group_sum`, `group_count`, `group_stddev`,
+#: `group_variance`). A MEASURE column carrying `group_sum ( ... )` was therefore
+#: classified as a scalar formula and had its own `aggregation` composed on top,
+#: emitting `sum ( group_sum ( ... ) )` with no issue raised.
+_GROUP_AGGREGATE_CALLS = GROUP_AGGREGATE_CALL_NAMES
 
 #: Every `Variant` that denotes an *aggregate* `sql_*_op` pass-through wrapper,
 #: derived by filtering the enum on its own `_aggregate_op` naming convention
@@ -740,7 +747,7 @@ def convert_field(
         formula.split_call(emit_direct(CATALOG[spec], ["x"]))[0].lower()
         for spec in _NATIVE_AGGREGATE_SPECS
     )
-    | {_GROUP_AGGREGATE_CALL}
+    | _GROUP_AGGREGATE_CALLS
     | _SQL_AGGREGATE_OP_CALLS
 )
 
@@ -1567,6 +1574,32 @@ def _split_top_level_and(text: str) -> list[str]:
     return [p for p in parts if p]
 
 
+def _strip_wrapping_parens(text: str) -> str:
+    """`( x )` -> `x`, repeatedly, but only when the parens truly wrap the whole.
+
+    `(a) and (b)` is left alone: its first `(` closes before the end, so the
+    outer pair is not a wrapper. Depth comes from `formula._scan`, the same
+    tracker the rest of this module splits on, so a paren inside a quoted
+    literal or a `[TABLE::Column]` body never counts.
+    """
+    stripped = text.strip()
+    while stripped.startswith("(") and stripped.endswith(")"):
+        # `_scan` reports depth 0 AT the opening paren and again AT its match,
+        # so the wrapper test is whether depth returns to 0 strictly between
+        # them -- index 0 and the final index are both 0 for a true wrapper.
+        depth_reaches_zero_early = any(
+            depth == 0 and 0 < index < len(stripped) - 1
+            for index, _ch, depth, _in_quote in formula._scan(stripped)
+        )
+        if depth_reaches_zero_early:
+            return stripped
+        inner = stripped[1:-1].strip()
+        if not inner:
+            return stripped
+        stripped = inner
+    return stripped
+
+
 def _parse_join_condition(
     on_expression: str, from_prefix: str, to_prefix: str
 ) -> tuple[list[tuple[str, str]], list[str]]:
@@ -1587,7 +1620,15 @@ def _parse_join_condition(
     """
     equality_pairs: list[tuple[str, str]] = []
     residuals: list[str] = []
-    for part in _split_top_level_and(on_expression):
+    # Redundant wrapping parentheses are stripped before the split and again per
+    # part. `formula._scan` tracks paren depth -- correct for a general
+    # expression, and exactly wrong here: a condition written
+    # `( [A::x] = [B::y] and [A::p] = [B::q] )`, an ordinary TML spelling, puts
+    # every `and` at depth 1, so nothing split, no equality pair matched, and the
+    # whole relationship was demoted to an unrepresentable-join stash entry --
+    # leaving the Ossie datasets disconnected and `derive_keys` with no candidate.
+    for part in _split_top_level_and(_strip_wrapping_parens(on_expression)):
+        part = _strip_wrapping_parens(part)
         match = _EQUALITY_PAIR_RE.match(part)
         if match is None:
             residuals.append(part)

converters/thoughtspot/tests/fixtures/minimal/expected.ossie.yaml

@@ -42,10 +42,10 @@ datasets:
       {"data_type": "DOUBLE"}, "name": "order_total"}]}'
 - name: customers
   source: MINIMAL.PUBLIC.CUSTOMERS
-  primary_key: &id001
+  primary_key:
   - customer_id
   unique_keys:
-  - *id001
+  - - customer_id
   fields:
   - name: customer_name
     label: customer_name