fix(thoughtspot): portable expressions — unqualify a field's, quote what needs quoting

converters/thoughtspot/docs/vendor-payload.md

@@ -39,6 +39,7 @@ Every `custom_extensions` entry this converter writes uses `vendor_name` `THOUGH
 |---|---|---|---|
 | `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. |
+| `aggregation_explicit_none` | 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. |
 | `formula_name` | Field | information_only | Restored as-is whenever present: nothing on the Ossie side could have diverged from it. |

converters/thoughtspot/pyproject.toml

@@ -27,6 +27,12 @@ dev = [
     # so its absence never fails the suite -- the package's only *runtime* dependency
     # stays PyYAML. Same dev-group placement as converters/nvidia's pyproject.toml.
     "jsonschema>=4.26.0",
+    # Dev/test-only: the parser Apache's own `validation/validate.py` uses, so a
+    # test can assert an emitted portable expression actually PARSES. Without it
+    # that check silently skipped -- and so did the validator's, which reports
+    # "Validation PASSED" while printing "sqlglot not installed, skipping SQL
+    # validation". Guarded by pytest.importorskip, same discipline as the two below.
+    "sqlglot>=25.0",
     # Dev/test-only: drives tests/test_roundtrip_properties.py. Guarded there by
     # pytest.importorskip("hypothesis"), same discipline as jsonschema above --
     # the package's only runtime dependency stays PyYAML. Same dev-group

converters/thoughtspot/src/ossie_thoughtspot/constants.py

@@ -130,6 +130,22 @@
 #: changes the number.
 METRIC_STASH_COLUMN_AGGREGATION = "column_aggregation_value"
 
+#: The surfacing column carried `aggregation: NONE` EXPLICITLY.
+#:
+#: Ossie has one way to say "this metric does not aggregate" -- no aggregate in
+#: the expression -- and TML has two: the key absent, or the key present with
+#: the value NONE. `_AGGREGATION["NONE"]` is `None`, the same thing
+#: `properties.get("aggregation", "NONE")` yields for an absent key, so both
+#: collapsed identically on the way in and the write side could not tell them
+#: apart. An explicit NONE therefore came back ABSENT, and ThoughtSpot applies
+#: its own default to an absent key -- so a per-row ratio declared NONE was
+#: returned as a column ThoughtSpot rolls up, changing the number.
+#:
+#: This records only that the key was there. What an ABSENT key means is a
+#: separate question this converter does not answer either way (see #467): it
+#: still reads absent as NONE, which is what it has always done.
+METRIC_STASH_AGGREGATION_NONE = "aggregation_explicit_none"
+
 #: 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,
@@ -525,6 +541,10 @@ class StashKeyClass(Enum):
     # 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,
+    # INFORMATION_ONLY: Ossie cannot express "explicitly not aggregated" as
+    # distinct from "not aggregated", so there is no live value this can
+    # diverge from and nothing to check it against.
+    METRIC_STASH_AGGREGATION_NONE: StashKeyClass.INFORMATION_ONLY,
     MODEL_STASH_OBJ_ID: StashKeyClass.INFORMATION_ONLY,
     FIELD_STASH_FORMULA_ID: StashKeyClass.INFORMATION_ONLY,
     # No Ossie counterpart: Ossie has no standalone "formula's own name"

converters/thoughtspot/src/ossie_thoughtspot/ossie_to_thoughtspot.py

@@ -92,6 +92,7 @@
     FIELD_STASH_DATA_TYPE_WITNESS,
     FIELD_STASH_DB_COLUMN_NAME,
     FIELD_STASH_DB_COLUMN_NAME_WITNESS,
+    METRIC_STASH_AGGREGATION_NONE,
     METRIC_STASH_COLUMN_AGGREGATION,
     METRIC_SHAPE_COLUMN_AGGREGATION,
     METRIC_SHAPE_FORMULA,
@@ -130,7 +131,17 @@
 #: stripped. This is what a hand-authored field's own physical-column
 #: expression looks like: no dataset qualifier (a field's expression runs
 #: against its own dataset's source), no operators, no function calls.
-_BARE_IDENTIFIER_RE = re.compile(r'^(?:[A-Za-z_][A-Za-z0-9_]{0,127}|"[^"]{1,128}")$')
+#:
+#: The quoted alternative admits a **doubled** double quote, because that is
+#: how a literal one is spelled inside a quoted identifier and it is what the
+#: forward direction writes: `_sql_identifier` in `tml_to_ossie` emits a
+#: ThoughtSpot column displayed as `Size (")` as `"Size ("")"`. Matching only
+#: `[^"]` here rejected exactly the strings this converter itself produces, so
+#: such a field was not recognised as physical at all on the way back and was
+#: dropped as untranslatable.
+_BARE_IDENTIFIER_RE = re.compile(
+    r'^(?:[A-Za-z_][A-Za-z0-9_]{0,127}|"(?:[^"]|""){1,128}")$'
+)
 
 #: Any source string containing whitespace outside of a quoted identifier
 #: reads as a query rather than a `db.schema.table` reference — a real
@@ -193,7 +204,10 @@ def _bare_sql_identifier(expression: str) -> str | None:
     if _BARE_IDENTIFIER_RE.match(text) is None:
         return None
     if text.startswith('"') and text.endswith('"'):
-        return text[1:-1]
+        # Undouble: `""` inside a quoted identifier is one literal `"`, so the
+        # column ThoughtSpot displays as `Size (")` arrives as `"Size ("")"`.
+        # Stripping the outer quotes alone would name a column no warehouse has.
+        return text[1:-1].replace('""', '"')
     return text
 
 
@@ -1586,6 +1600,13 @@ def _build_metric(
     if preserved_aggregation and "aggregation" not in properties:
         properties["aggregation"] = preserved_aggregation
 
+    # An EXPLICIT `aggregation: NONE`, restored before the conventional path.
+    # Dropping it is not a cosmetic loss: ThoughtSpot applies its own default to
+    # an absent key, so a metric the author declared un-aggregated came back as
+    # one ThoughtSpot rolls up -- a per-row ratio returned as a sum of ratios.
+    if payload.get(METRIC_STASH_AGGREGATION_NONE) and "aggregation" not in properties:
+        properties["aggregation"] = "NONE"
+
     if "aggregation" not in properties:
         conventional = _outer_aggregation_of(ts_expr)
         if conventional is not None: