From 89e7af0b47797d877320abb0d618e0465fcda429 Mon Sep 17 00:00:00 2001 From: "damian.waldron" Date: Fri, 25 Sep 2026 10:11:53 +1000 Subject: [PATCH 1/7] fix(thoughtspot): a field's portable expression must not be dataset-qualified `tml_to_ossie` emitted a field's ANSI_SQL sibling as `.` -- `Dim_Customer.Customer_Name`. Both halves are real, but they come from different namespaces, so the pair resolves in neither: - the qualifier is the OSSIE DATASET name, which is ThoughtSpot's Table-object name and need not be the warehouse table. 44 of 164 datasets across 31 real models differ (dataset `Dim_Customer`, source `NEWRETAIL.SMALLRETAIL.NewRetail_Customer_Dimension`), so it is not runnable SQL -- there is no such table; - the column half is the warehouse column, not the Ossie field identifier, so it is not a resolvable logical reference either whenever the display name differs from the column name. Measured over the 31-model corpus: 211 of 612 references named nothing the document declares, in 20 of 31 models, counting case-insensitively. Upstream's validator cannot see it -- it checks that an expression parses as SQL, and `Dim_Customer.Customer_Name` parses fine. Every sibling converter writes a field's expression as the BARE warehouse column: databricks emits `l_linenumber` for a field named `line_number` and `d_year` for `sold_year`, nvidia emits `name` for `customer_name`, and gooddata, omni and orionbelt do the same. The physical column was never the problem; the qualifier was. A METRIC keeps its qualifier, and the asymmetry is scope rather than inconsistency. A field belongs to one dataset, whose `source` already names the warehouse table, so a bare column is unambiguous. A metric is model-scoped and may reference any dataset, so `SUM(amount)` stops being unambiguous as soon as two datasets have an `amount`; nvidia qualifies for that reason (`SUM(orders.subtotal)`). `expression_entries` already receives `kind`, so the distinction needed no new plumbing. `resolve` now returns `(dataset, column)` instead of `"dataset.column"`. Its two callers want different halves -- one emits the column, the other reads the dataset to attribute a formula -- and the joined string meant the second re-parsed what the first then emitted whole. Verified: 211 -> 0 unresolvable references across the corpus; 31/31 still pass upstream's validator; and the returned TML is byte-identical for all 195 documents, since the reverse leg reads the THOUGHTSPOT dialect and never this one. The two expected fixtures are regenerated -- 37 changed lines, all of them `expression:`. 942 tests pass. Co-Authored-By: Claude Opus 5 (1M context) --- .../src/ossie_thoughtspot/tml_to_ossie.py | 71 +++++++++++++------ .../fixtures/minimal/expected.ossie.yaml | 4 +- .../tests/fixtures/tpcds/expected.ossie.yaml | 70 +++++++++--------- converters/thoughtspot/tests/test_fixtures.py | 4 +- .../thoughtspot/tests/test_roundtrip.py | 6 +- .../thoughtspot/tests/test_tml_to_ossie.py | 8 +-- .../tests/test_tml_to_ossie_fields.py | 66 +++++++++++++++-- .../tests/test_tml_to_ossie_metrics.py | 6 +- 8 files changed, 158 insertions(+), 77 deletions(-) diff --git a/converters/thoughtspot/src/ossie_thoughtspot/tml_to_ossie.py b/converters/thoughtspot/src/ossie_thoughtspot/tml_to_ossie.py index 257e742d8..e7f95ee49 100644 --- a/converters/thoughtspot/src/ossie_thoughtspot/tml_to_ossie.py +++ b/converters/thoughtspot/src/ossie_thoughtspot/tml_to_ossie.py @@ -142,7 +142,7 @@ def expression_entries( expr: str, - resolve: Callable[[str, str], str | None], + resolve: Callable[[str, str], tuple[str, str] | None], log: IssueLog, *, object_ref: str, @@ -234,7 +234,28 @@ def expression_entries( object_ref=object_ref, ) return entries - entries.append({"dialect": PORTABLE_DIALECT, "expression": target}) + # A FIELD's expression is the bare warehouse column; a METRIC's keeps the + # dataset qualifier. The asymmetry is scope, and every sibling converter + # shows it: a field belongs to exactly one dataset, so its own `source` + # already says which warehouse table the column sits in and a qualifier + # adds nothing (databricks emits `l_linenumber` for a field named + # `line_number`, nvidia emits `name` for `customer_name`). A metric is + # model-scoped and may reference any dataset, so `SUM(amount)` would be + # ambiguous the moment two datasets both have an `amount`; nvidia + # qualifies for exactly this reason (`SUM(orders.subtotal)`). + # + # Qualifying a FIELD was not merely redundant, it was unresolvable. The + # qualifier is the OSSIE DATASET name -- ThoughtSpot's Table-object name, + # which need not be the warehouse table (44 of 164 datasets across 31 + # real models differ) -- while the column half is the warehouse column. + # `Dim_Customer.Customer_Name` is therefore neither runnable SQL (no such + # table) nor a resolvable logical reference (no such field). 211 of 612 + # emitted references were in that state, every one of them on a field. + dataset_name, warehouse_column = target + portable = ( + warehouse_column if kind == "field" else f"{dataset_name}.{warehouse_column}" + ) + entries.append({"dialect": PORTABLE_DIALECT, "expression": portable}) return entries # Anything else is a function call or a multi-reference expression. Producing a @@ -255,7 +276,7 @@ def expression_entries( def attribute_dataset( expr: str, - resolve: Callable[[str, str], str | None], + resolve: Callable[[str, str], tuple[str, str] | None], log: IssueLog, *, object_ref: str, @@ -295,7 +316,7 @@ def attribute_dataset( if target is None: unresolved.append(identifiers.format_column_ref(table, column)) continue - dataset = target.split(".", 1)[0] + dataset = target[0] if dataset not in datasets: datasets.append(dataset) @@ -628,7 +649,7 @@ def convert_field( column: dict, formulas: dict[str, dict], table_lookup: Callable[[str], dict | None], - resolve: Callable[[str, str], str | None], + resolve: Callable[[str, str], tuple[str, str] | None], log: IssueLog, allocator: identifiers.Allocator | None = None, ) -> dict | None: @@ -886,7 +907,7 @@ def _contains_aggregate_call(expr: str) -> bool: def _compose_aggregate_entries( inner_expr: str, aggregation_raw: str, - resolve: Callable[[str, str], str | None], + resolve: Callable[[str, str], tuple[str, str] | None], log: IssueLog, *, object_ref: str, @@ -960,7 +981,7 @@ def convert_metric( column: dict, formulas: dict[str, dict], table_lookup: Callable[[str], dict | None], - resolve: Callable[[str, str], str | None], + resolve: Callable[[str, str], tuple[str, str] | None], log: IssueLog, allocator: identifiers.Allocator | None = None, ) -> dict | None: @@ -1579,7 +1600,8 @@ def _write_stash_safely(obj: dict, payload: dict, log: IssueLog, object_ref: str def _field_owner_dataset( - column: dict, formulas: dict[str, dict], resolve: Callable[[str, str], str | None] + column: dict, formulas: dict[str, dict], + resolve: Callable[[str, str], tuple[str, str] | None], ) -> str | None: """Which dataset a *successfully built* field belongs in. @@ -2338,18 +2360,25 @@ def table_lookup(name: str) -> dict | None: model_columns = model_body.get("columns") or [] attribute_index = _index_attribute_columns(model_columns, log) - def resolve(table: str, column: str) -> str | None: - # The mapping document is explicit for a bare-identifier field: "the - # identifier is the *physical* column; the display name comes from - # label/name." So the ANSI_SQL sibling this feeds -- built to be - # directly executable against the warehouse -- has to carry the - # actual warehouse column reference (db_column_name, or a SQL - # View's sql_output_column), never the Ossie field's own - # display-derived identifier, which is not a column that exists on - # the underlying table at all. `attribute_index` still gates - # whether this reference is one the model actually surfaces as a - # field -- that scope is unchanged -- only the value returned once - # it passes that gate changes. + def resolve(table: str, column: str) -> tuple[str, str] | None: + """`(dataset name, warehouse column)` for a `[TABLE::Column]` reference. + + Two callers want two different halves, which is why this returns the + pair rather than a joined string: `expression_entries` emits the + warehouse column as the portable expression, and `attribute_dataset` + uses the dataset name to decide which dataset a formula belongs to. + Returning `"dataset.column"` meant the second caller re-parsed a string + the first one then emitted whole -- and emitting it whole was the + defect, because the two halves come from different namespaces. + + The mapping document is explicit for a bare-identifier field: "the + identifier is the *physical* column; the display name comes from + label/name." So the portable sibling carries the actual warehouse + column (db_column_name, or a SQL View's sql_output_column), never the + Ossie field's display-derived identifier, which is not a column on the + underlying table at all. `attribute_index` still gates whether the + reference is one the model surfaces as a field. + """ if table not in dataset_bodies: return None if (table, column) not in attribute_index: @@ -2363,7 +2392,7 @@ def resolve(table: str, column: str) -> str | None: warehouse_reference = physical.get("db_column_name") if warehouse_reference is None: return None - return f"{table}.{warehouse_reference}" + return table, warehouse_reference # -- Phase 3: fields and metrics ------------------------------------------ # A non-string formula expr (an int, a list, or a YAML-parsed date in a diff --git a/converters/thoughtspot/tests/fixtures/minimal/expected.ossie.yaml b/converters/thoughtspot/tests/fixtures/minimal/expected.ossie.yaml index 08baeb407..f2a002a82 100644 --- a/converters/thoughtspot/tests/fixtures/minimal/expected.ossie.yaml +++ b/converters/thoughtspot/tests/fixtures/minimal/expected.ossie.yaml @@ -31,7 +31,7 @@ datasets: - dialect: THOUGHTSPOT expression: '[orders::order_id]' - dialect: ANSI_SQL - expression: orders.order_id + expression: order_id datatype: Integer custom_extensions: - vendor_name: THOUGHTSPOT @@ -54,7 +54,7 @@ datasets: - dialect: THOUGHTSPOT expression: '[customers::customer_name]' - dialect: ANSI_SQL - expression: customers.customer_name + expression: customer_name datatype: String custom_extensions: - vendor_name: THOUGHTSPOT diff --git a/converters/thoughtspot/tests/fixtures/tpcds/expected.ossie.yaml b/converters/thoughtspot/tests/fixtures/tpcds/expected.ossie.yaml index b01035eb3..eae5df7e5 100644 --- a/converters/thoughtspot/tests/fixtures/tpcds/expected.ossie.yaml +++ b/converters/thoughtspot/tests/fixtures/tpcds/expected.ossie.yaml @@ -44,7 +44,7 @@ datasets: - dialect: THOUGHTSPOT expression: '[store_sales::ss_sold_date_sk]' - dialect: ANSI_SQL - expression: store_sales.ss_sold_date_sk + expression: ss_sold_date_sk datatype: Integer - name: ss_item_sk label: ss_item_sk @@ -53,7 +53,7 @@ datasets: - dialect: THOUGHTSPOT expression: '[store_sales::ss_item_sk]' - dialect: ANSI_SQL - expression: store_sales.ss_item_sk + expression: ss_item_sk datatype: Integer - name: ss_customer_sk label: ss_customer_sk @@ -62,7 +62,7 @@ datasets: - dialect: THOUGHTSPOT expression: '[store_sales::ss_customer_sk]' - dialect: ANSI_SQL - expression: store_sales.ss_customer_sk + expression: ss_customer_sk datatype: Integer - name: ss_store_sk label: ss_store_sk @@ -71,7 +71,7 @@ datasets: - dialect: THOUGHTSPOT expression: '[store_sales::ss_store_sk]' - dialect: ANSI_SQL - expression: store_sales.ss_store_sk + expression: ss_store_sk datatype: Integer - name: ss_quantity label: ss_quantity @@ -80,7 +80,7 @@ datasets: - dialect: THOUGHTSPOT expression: '[store_sales::ss_quantity]' - dialect: ANSI_SQL - expression: store_sales.ss_quantity + expression: ss_quantity datatype: Integer - name: ss_sales_price label: ss_sales_price @@ -89,7 +89,7 @@ datasets: - dialect: THOUGHTSPOT expression: '[store_sales::ss_sales_price]' - dialect: ANSI_SQL - expression: store_sales.ss_sales_price + expression: ss_sales_price datatype: Decimal - name: ss_ext_sales_price label: ss_ext_sales_price @@ -98,7 +98,7 @@ datasets: - dialect: THOUGHTSPOT expression: '[store_sales::ss_ext_sales_price]' - dialect: ANSI_SQL - expression: store_sales.ss_ext_sales_price + expression: ss_ext_sales_price datatype: Decimal - name: ss_net_profit label: ss_net_profit @@ -107,7 +107,7 @@ datasets: - dialect: THOUGHTSPOT expression: '[store_sales::ss_net_profit]' - dialect: ANSI_SQL - expression: store_sales.ss_net_profit + expression: ss_net_profit datatype: Decimal custom_extensions: - vendor_name: THOUGHTSPOT @@ -129,7 +129,7 @@ datasets: - dialect: THOUGHTSPOT expression: '[date_dim::d_date_sk]' - dialect: ANSI_SQL - expression: date_dim.d_date_sk + expression: d_date_sk datatype: Integer - name: d_date label: d_date @@ -138,7 +138,7 @@ datasets: - dialect: THOUGHTSPOT expression: '[date_dim::d_date]' - dialect: ANSI_SQL - expression: date_dim.d_date + expression: d_date datatype: Date - name: d_year label: d_year @@ -147,7 +147,7 @@ datasets: - dialect: THOUGHTSPOT expression: '[date_dim::d_year]' - dialect: ANSI_SQL - expression: date_dim.d_year + expression: d_year datatype: Integer - name: d_quarter_name label: d_quarter_name @@ -156,7 +156,7 @@ datasets: - dialect: THOUGHTSPOT expression: '[date_dim::d_quarter_name]' - dialect: ANSI_SQL - expression: date_dim.d_quarter_name + expression: d_quarter_name datatype: String - name: d_moy label: d_moy @@ -165,7 +165,7 @@ datasets: - dialect: THOUGHTSPOT expression: '[date_dim::d_moy]' - dialect: ANSI_SQL - expression: date_dim.d_moy + expression: d_moy datatype: Integer custom_extensions: - vendor_name: THOUGHTSPOT @@ -185,7 +185,7 @@ datasets: - dialect: THOUGHTSPOT expression: '[customer::c_customer_sk]' - dialect: ANSI_SQL - expression: customer.c_customer_sk + expression: c_customer_sk datatype: Integer - name: c_customer_id label: c_customer_id @@ -194,7 +194,7 @@ datasets: - dialect: THOUGHTSPOT expression: '[customer::c_customer_id]' - dialect: ANSI_SQL - expression: customer.c_customer_id + expression: c_customer_id datatype: String - name: c_first_name label: c_first_name @@ -203,7 +203,7 @@ datasets: - dialect: THOUGHTSPOT expression: '[customer::c_first_name]' - dialect: ANSI_SQL - expression: customer.c_first_name + expression: c_first_name datatype: String - name: c_last_name label: c_last_name @@ -212,7 +212,7 @@ datasets: - dialect: THOUGHTSPOT expression: '[customer::c_last_name]' - dialect: ANSI_SQL - expression: customer.c_last_name + expression: c_last_name datatype: String - name: customer_full_name label: customer_full_name @@ -231,7 +231,7 @@ datasets: - dialect: THOUGHTSPOT expression: '[customer::c_email_address]' - dialect: ANSI_SQL - expression: customer.c_email_address + expression: c_email_address datatype: String - name: c_kana_name label: カナ名 @@ -240,7 +240,7 @@ datasets: - dialect: THOUGHTSPOT expression: '[customer::カナ名]' - dialect: ANSI_SQL - expression: customer.c_kana_name + expression: c_kana_name datatype: String custom_extensions: - vendor_name: THOUGHTSPOT @@ -264,7 +264,7 @@ datasets: - dialect: THOUGHTSPOT expression: '[item::i_item_sk]' - dialect: ANSI_SQL - expression: item.i_item_sk + expression: i_item_sk datatype: Integer - name: i_item_id label: i_item_id @@ -273,7 +273,7 @@ datasets: - dialect: THOUGHTSPOT expression: '[item::i_item_id]' - dialect: ANSI_SQL - expression: item.i_item_id + expression: i_item_id datatype: String - name: i_item_desc label: i_item_desc @@ -282,7 +282,7 @@ datasets: - dialect: THOUGHTSPOT expression: '[item::i_item_desc]' - dialect: ANSI_SQL - expression: item.i_item_desc + expression: i_item_desc datatype: String - name: i_brand label: i_brand @@ -291,7 +291,7 @@ datasets: - dialect: THOUGHTSPOT expression: '[item::i_brand]' - dialect: ANSI_SQL - expression: item.i_brand + expression: i_brand datatype: String - name: i_category label: i_category @@ -300,7 +300,7 @@ datasets: - dialect: THOUGHTSPOT expression: '[item::i_category]' - dialect: ANSI_SQL - expression: item.i_category + expression: i_category datatype: String - name: i_current_price label: i_current_price @@ -309,7 +309,7 @@ datasets: - dialect: THOUGHTSPOT expression: '[item::i_current_price]' - dialect: ANSI_SQL - expression: item.i_current_price + expression: i_current_price datatype: Decimal custom_extensions: - vendor_name: THOUGHTSPOT @@ -329,7 +329,7 @@ datasets: - dialect: THOUGHTSPOT expression: '[store::s_store_sk]' - dialect: ANSI_SQL - expression: store.s_store_sk + expression: s_store_sk datatype: Integer - name: s_store_id label: s_store_id @@ -338,7 +338,7 @@ datasets: - dialect: THOUGHTSPOT expression: '[store::s_store_id]' - dialect: ANSI_SQL - expression: store.s_store_id + expression: s_store_id datatype: String - name: s_store_name label: s_store_name @@ -347,7 +347,7 @@ datasets: - dialect: THOUGHTSPOT expression: '[store::s_store_name]' - dialect: ANSI_SQL - expression: store.STORE_NM + expression: STORE_NM datatype: String custom_extensions: - vendor_name: THOUGHTSPOT @@ -360,7 +360,7 @@ datasets: - dialect: THOUGHTSPOT expression: '[store::s_city]' - dialect: ANSI_SQL - expression: store.s_city + expression: s_city datatype: String - name: s_state label: s_state @@ -369,7 +369,7 @@ datasets: - dialect: THOUGHTSPOT expression: '[store::s_state]' - dialect: ANSI_SQL - expression: store.s_state + expression: s_state datatype: String - name: s_number_employees label: s_number_employees @@ -378,7 +378,7 @@ datasets: - dialect: THOUGHTSPOT expression: '[store::s_number_employees]' - dialect: ANSI_SQL - expression: store.s_number_employees + expression: s_number_employees datatype: Integer - name: 'on' label: 'on' @@ -387,7 +387,7 @@ datasets: - dialect: THOUGHTSPOT expression: '[store::on]' - dialect: ANSI_SQL - expression: store.on + expression: 'on' datatype: Boolean description: Whether the store is currently active and open for business. custom_extensions: @@ -408,7 +408,7 @@ datasets: - dialect: THOUGHTSPOT expression: '[store_returns_sv::sr_item_sk]' - dialect: ANSI_SQL - expression: store_returns_sv.sr_item_sk + expression: sr_item_sk datatype: Integer - name: sr_ticket_number label: sr_ticket_number @@ -417,7 +417,7 @@ datasets: - dialect: THOUGHTSPOT expression: '[store_returns_sv::sr_ticket_number]' - dialect: ANSI_SQL - expression: store_returns_sv.sr_ticket_number + expression: sr_ticket_number datatype: Integer - name: sr_return_amt label: sr_return_amt @@ -426,7 +426,7 @@ datasets: - dialect: THOUGHTSPOT expression: '[store_returns_sv::sr_return_amt]' - dialect: ANSI_SQL - expression: store_returns_sv.RETURN_AMT + expression: RETURN_AMT datatype: Decimal custom_extensions: - vendor_name: THOUGHTSPOT diff --git a/converters/thoughtspot/tests/test_fixtures.py b/converters/thoughtspot/tests/test_fixtures.py index dbe2d888f..3e76d298f 100644 --- a/converters/thoughtspot/tests/test_fixtures.py +++ b/converters/thoughtspot/tests/test_fixtures.py @@ -204,7 +204,7 @@ def test_store_has_a_display_name_differing_from_its_db_column_name(self, datase store = next(d for d in dataset["datasets"] if d["name"] == "store") field = next(f for f in store["fields"] if f["name"] == "s_store_name") dialects = {d["dialect"]: d["expression"] for d in field["expression"]["dialects"]} - assert dialects[PORTABLE_DIALECT] == "store.STORE_NM" + assert dialects[PORTABLE_DIALECT] == "STORE_NM" def test_the_on_column_survives_as_a_string_not_a_boolean(self, dataset): store = next(d for d in dataset["datasets"] if d["name"] == "store") @@ -236,7 +236,7 @@ def test_a_sql_view_is_present_with_a_differing_output_alias(self, dataset): sv = next(d for d in dataset["datasets"] if d["name"] == "store_returns_sv") field = next(f for f in sv["fields"] if f["name"] == "sr_return_amt") dialects = {d["dialect"]: d["expression"] for d in field["expression"]["dialects"]} - assert dialects[PORTABLE_DIALECT] == "store_returns_sv.RETURN_AMT" + assert dialects[PORTABLE_DIALECT] == "RETURN_AMT" def test_a_physical_column_the_model_does_not_surface_is_stashed(self, dataset): store_sales = next(d for d in dataset["datasets"] if d["name"] == "store_sales") diff --git a/converters/thoughtspot/tests/test_roundtrip.py b/converters/thoughtspot/tests/test_roundtrip.py index 13eeb4b0b..419a703db 100644 --- a/converters/thoughtspot/tests/test_roundtrip.py +++ b/converters/thoughtspot/tests/test_roundtrip.py @@ -481,7 +481,7 @@ def test_minimal_physical_field_translates_to_its_dataset_dot_column(): field = _field(_dataset(model, "orders"), "order_id") dialects = _dialects(field) assert dialects[DIALECT] == "[orders::order_id]" - assert dialects[PORTABLE_DIALECT] == "orders.order_id" + assert dialects[PORTABLE_DIALECT] == "order_id" def test_minimal_known_unportable_metric_has_no_portable_sibling_and_an_issue(): @@ -499,12 +499,12 @@ def test_tpcds_physical_fields_translate_to_their_warehouse_column_even_when_the # (STORE_NM); the portable expression has to carry the warehouse name, # not the display-derived Ossie identifier. store_name = _field(_dataset(model, "store"), "s_store_name") - assert _dialects(store_name)[PORTABLE_DIALECT] == "store.STORE_NM" + assert _dialects(store_name)[PORTABLE_DIALECT] == "STORE_NM" # sr_return_amt is a SQL View column whose output alias (RETURN_AMT) # differs from its own field name -- the same fact, for a query rather # than a table. return_amt = _field(_dataset(model, "store_returns_sv"), "sr_return_amt") - assert _dialects(return_amt)[PORTABLE_DIALECT] == "store_returns_sv.RETURN_AMT" + assert _dialects(return_amt)[PORTABLE_DIALECT] == "RETURN_AMT" def test_tpcds_known_unportable_metrics_have_no_portable_sibling_and_an_issue(): diff --git a/converters/thoughtspot/tests/test_tml_to_ossie.py b/converters/thoughtspot/tests/test_tml_to_ossie.py index 38c58c6a7..a12d65f05 100644 --- a/converters/thoughtspot/tests/test_tml_to_ossie.py +++ b/converters/thoughtspot/tests/test_tml_to_ossie.py @@ -1432,7 +1432,7 @@ def test_a_differing_db_column_name_is_used_in_the_portable_expression(self): result = convert(_document_set(model, orders)) field = result.model["datasets"][0]["fields"][0] dialects = {d["dialect"]: d["expression"] for d in field["expression"]["dialects"]} - assert dialects["ANSI_SQL"] == "ORDERS.AMT_RAW" + assert dialects["ANSI_SQL"] == "AMT_RAW" def test_an_equal_db_column_name_still_resolves(self): orders = _table("ORDERS", columns=[_column("Amount", "Amount", "DOUBLE")]) @@ -1440,7 +1440,7 @@ def test_an_equal_db_column_name_still_resolves(self): result = convert(_document_set(model, orders)) field = result.model["datasets"][0]["fields"][0] dialects = {d["dialect"]: d["expression"] for d in field["expression"]["dialects"]} - assert dialects["ANSI_SQL"] == "ORDERS.Amount" + assert dialects["ANSI_SQL"] == "Amount" def test_a_sql_view_reference_uses_sql_output_column_not_db_column_name(self): vw = _sql_view("VW", columns=[_sql_view_column("CID", "c_id", "INT64")]) @@ -1448,7 +1448,7 @@ def test_a_sql_view_reference_uses_sql_output_column_not_db_column_name(self): result = convert(_document_set(model, vw)) field = result.model["datasets"][0]["fields"][0] dialects = {d["dialect"]: d["expression"] for d in field["expression"]["dialects"]} - assert dialects["ANSI_SQL"] == "VW.c_id" + assert dialects["ANSI_SQL"] == "c_id" def test_a_computed_field_referencing_a_renamed_column_still_resolves(self): orders = _table("ORDERS", columns=[_column("Amount", "AMT_RAW", "DOUBLE")]) @@ -1464,7 +1464,7 @@ def test_a_computed_field_referencing_a_renamed_column_still_resolves(self): result = convert(_document_set(model, orders)) fields = {f["name"]: f for f in result.model["datasets"][0]["fields"]} doubled_dialects = {d["dialect"]: d["expression"] for d in fields["doubled"]["expression"]["dialects"]} - assert doubled_dialects["ANSI_SQL"] == "ORDERS.AMT_RAW" + assert doubled_dialects["ANSI_SQL"] == "AMT_RAW" assert result.issues.as_dicts() == [] diff --git a/converters/thoughtspot/tests/test_tml_to_ossie_fields.py b/converters/thoughtspot/tests/test_tml_to_ossie_fields.py index f39194a12..66bbf5a27 100644 --- a/converters/thoughtspot/tests/test_tml_to_ossie_fields.py +++ b/converters/thoughtspot/tests/test_tml_to_ossie_fields.py @@ -21,9 +21,9 @@ def _resolve(table, column): - """Every reference lands in the dataset named after its table, lower-cased, - unless the table is named "MISSING" — then it resolves to nothing at all.""" - return None if table == "MISSING" else f"{table.lower()}.{column.lower()}" + """`(dataset, warehouse column)` -- the dataset is the table lower-cased, + unless the table is named "MISSING", which resolves to nothing at all.""" + return None if table == "MISSING" else (table.lower(), column.lower()) class TestExpressionEntries: @@ -32,7 +32,7 @@ def test_a_bare_reference_gets_both_dialects(self): out = expression_entries("[ORDERS::Amount]", _resolve, log, object_ref="f") assert out == [ {"dialect": "THOUGHTSPOT", "expression": "[ORDERS::Amount]"}, - {"dialect": "ANSI_SQL", "expression": "orders.amount"}, + {"dialect": "ANSI_SQL", "expression": "amount"}, ] assert log.as_dicts() == [] @@ -202,7 +202,7 @@ def test_references_spanning_two_datasets_are_not_attributed(self): log = IssueLog() def resolve(table, column): - return f"other.{column.lower()}" if table == "OTHER" else f"orders.{column.lower()}" + return ("other", column.lower()) if table == "OTHER" else ("orders", column.lower()) result = attribute_dataset( "[ORDERS::Amount] + [OTHER::Fee]", resolve, log, object_ref="f" @@ -226,7 +226,7 @@ def test_references_to_the_same_dataset_via_different_tables_are_attributed(self log = IssueLog() def resolve(table, column): - return f"orders.{column.lower()}" # ORDERS and ORDERS_ALIAS both land here + return "orders", column.lower() # ORDERS and ORDERS_ALIAS both land here result = attribute_dataset( "[ORDERS::Amount] + [ORDERS_ALIAS::Tax]", resolve, log, object_ref="f" @@ -422,7 +422,7 @@ def test_a_computed_field_spanning_two_datasets_is_not_built(self): log = IssueLog() def resolve(table, column): - return f"other.{column.lower()}" if table == "OTHER" else f"orders.{column.lower()}" + return ("other", column.lower()) if table == "OTHER" else ("orders", column.lower()) formulas = {"formula_Combined": {"id": "formula_Combined", "expr": "[ORDERS::Amount] + [OTHER::Fee]"}} @@ -652,3 +652,55 @@ def test_a_present_but_unmapped_data_type_logs_exactly_one_issue_naming_it(self) issues = log.as_dicts() assert len(issues) == 1 assert "GEOGRAPHY" in issues[0]["message"] + + +class TestPortableReferenceScope: + """A field's portable expression is bare; a metric's keeps its dataset. + + The asymmetry is scope, and it is what every sibling converter does. A + field belongs to one dataset, whose `source` already names the warehouse + table, so `l_linenumber` (databricks) or `name` (nvidia) is unambiguous on + its own. A metric is model-scoped and may reference any dataset, so + `SUM(amount)` stops being unambiguous the moment two datasets have an + `amount`; nvidia qualifies for that reason (`SUM(orders.subtotal)`). + + Qualifying a FIELD was not just redundant, it was unresolvable: the + qualifier is the Ossie dataset name (ThoughtSpot's Table-object name, which + need not be the warehouse table) while the column half is the warehouse + column, so `Dim_Customer.Customer_Name` was neither runnable SQL nor a + resolvable logical reference. 211 of 612 references across 31 real models + were in that state, every one of them on a field. + """ + + def test_a_field_reference_carries_the_bare_warehouse_column(self): + out = expression_entries( + "[ORDERS::Amount]", _resolve, IssueLog(), object_ref="f", kind="field" + ) + portable = next(e for e in out if e["dialect"] == "ANSI_SQL") + assert portable["expression"] == "amount", ( + "a field's portable expression must not carry a dataset qualifier" + ) + + def test_a_metric_reference_keeps_its_dataset_qualifier(self): + out = expression_entries( + "[ORDERS::Amount]", _resolve, IssueLog(), object_ref="m", kind="metric" + ) + portable = next(e for e in out if e["dialect"] == "ANSI_SQL") + assert portable["expression"] == "orders.amount", ( + "a metric is model-scoped, so its reference must stay qualified" + ) + + def test_the_two_kinds_genuinely_differ(self): + # Guards the asymmetry itself: making both branches emit the same thing + # would satisfy either test above on its own. + as_field = expression_entries( + "[ORDERS::Amount]", _resolve, IssueLog(), object_ref="f", kind="field" + ) + as_metric = expression_entries( + "[ORDERS::Amount]", _resolve, IssueLog(), object_ref="m", kind="metric" + ) + field_expr = next(e["expression"] for e in as_field if e["dialect"] == "ANSI_SQL") + metric_expr = next(e["expression"] for e in as_metric if e["dialect"] == "ANSI_SQL") + assert field_expr != metric_expr, ( + f"field and metric both emitted {field_expr!r}; the scope distinction is gone" + ) diff --git a/converters/thoughtspot/tests/test_tml_to_ossie_metrics.py b/converters/thoughtspot/tests/test_tml_to_ossie_metrics.py index 231816d89..52ef6946b 100644 --- a/converters/thoughtspot/tests/test_tml_to_ossie_metrics.py +++ b/converters/thoughtspot/tests/test_tml_to_ossie_metrics.py @@ -27,9 +27,9 @@ def _resolve(table, column): - """Every reference lands in the dataset named after its table, lower-cased, - unless the table is named "MISSING" — then it resolves to nothing at all.""" - return None if table == "MISSING" else f"{table.lower()}.{column.lower()}" + """`(dataset, warehouse column)` -- the dataset is the table lower-cased, + unless the table is named "MISSING", which resolves to nothing at all.""" + return None if table == "MISSING" else (table.lower(), column.lower()) class TestConvertMetric: From 4fa2ea8832ae66947d04d1f30d8596899f4ded3f Mon Sep 17 00:00:00 2001 From: "damian.waldron" Date: Fri, 25 Sep 2026 17:35:17 +1000 Subject: [PATCH 2/7] fix(thoughtspot): quote a portable identifier that is not a regular one A ThoughtSpot display name is not automatically a valid SQL identifier. Names carry spaces, colons, percent signs and parentheses, and emitted raw they do not merely look wrong -- they do not parse. `SUM(cargo.Custom Clearance Time (min))` and `SUM(HV: STORES.LATITUDE)` are both rejected by sqlglot, which is the parser Apache's own `validation/validate.py` uses. Per `core-spec/expression_language.md`, identifiers follow ANSI SQL naming and the Ossie dialect's quote character is the double quote. A name that is not a regular identifier is therefore double-quoted, with embedded quotes doubled. Quoted only WHEN NEEDED, which is semantic rather than cosmetic: the spec notes regular identifiers compare case-insensitively while quoted ones compare verbatim, so quoting a name that does not need it changes how a consumer matches it. Measured over 31 real models with the validator's SQL checks actually running: before 29 passed 2 failed 7 [SQL] findings after 31 passed 0 failed 0 [SQL] findings `sqlglot` joins the dev group, because none of that was visible without it. `validation/validate.py` prints "sqlglot not installed, skipping SQL validation" and then "Validation PASSED" -- a skipped check reported as a pass, which is why these 7 findings sat unseen. The new parse test is `importorskip`-guarded like its siblings; the dependency means it runs. 950 tests pass. Co-Authored-By: Claude Opus 5 (1M context) --- converters/thoughtspot/pyproject.toml | 6 ++ .../src/ossie_thoughtspot/tml_to_ossie.py | 32 ++++++++- .../tests/test_tml_to_ossie_fields.py | 68 +++++++++++++++++++ converters/thoughtspot/uv.lock | 11 +++ 4 files changed, 116 insertions(+), 1 deletion(-) diff --git a/converters/thoughtspot/pyproject.toml b/converters/thoughtspot/pyproject.toml index fe09593a6..070af3583 100644 --- a/converters/thoughtspot/pyproject.toml +++ b/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 diff --git a/converters/thoughtspot/src/ossie_thoughtspot/tml_to_ossie.py b/converters/thoughtspot/src/ossie_thoughtspot/tml_to_ossie.py index e7f95ee49..c86b9bd77 100644 --- a/converters/thoughtspot/src/ossie_thoughtspot/tml_to_ossie.py +++ b/converters/thoughtspot/src/ossie_thoughtspot/tml_to_ossie.py @@ -140,6 +140,34 @@ from .tml import DocumentSet + +#: A regular ANSI SQL identifier: letter or underscore, then letters, digits or +#: underscores. Anything else has to be double-quoted to survive a SQL parser. +_REGULAR_IDENTIFIER = re.compile(r"^[A-Za-z_][A-Za-z0-9_]*$") + + +def _sql_identifier(name: str) -> str: + """`name` as an ANSI SQL identifier, quoted only when it has to be. + + ThoughtSpot column and table names are display names: they carry spaces, + colons, percent signs and parentheses freely. Emitted raw into a portable + expression they do not merely look wrong, they do not parse -- + `SUM(cargo.Custom Clearance Time (min))` and `SUM(HV: STORES.LATITUDE)` + are both rejected by sqlglot, which is the parser Apache's own validator + uses. + + The specification is explicit that identifiers follow ANSI SQL naming and + that the Ossie dialect's quote character is the double quote + (`core-spec/expression_language.md`), so a name that is not a regular + identifier is double-quoted, with any embedded double quote doubled. + A name that IS regular is left bare -- the spec notes regular identifiers + are case-insensitive while quoted ones are compared verbatim, so quoting + unnecessarily would change how a consumer matches it. + """ + if _REGULAR_IDENTIFIER.match(name): + return name + return '"' + name.replace('"', '""') + '"' + def expression_entries( expr: str, resolve: Callable[[str, str], tuple[str, str] | None], @@ -252,8 +280,10 @@ def expression_entries( # table) nor a resolvable logical reference (no such field). 211 of 612 # emitted references were in that state, every one of them on a field. dataset_name, warehouse_column = target + column_sql = _sql_identifier(warehouse_column) portable = ( - warehouse_column if kind == "field" else f"{dataset_name}.{warehouse_column}" + column_sql if kind == "field" + else f"{_sql_identifier(dataset_name)}.{column_sql}" ) entries.append({"dialect": PORTABLE_DIALECT, "expression": portable}) return entries diff --git a/converters/thoughtspot/tests/test_tml_to_ossie_fields.py b/converters/thoughtspot/tests/test_tml_to_ossie_fields.py index 66bbf5a27..8e985a5db 100644 --- a/converters/thoughtspot/tests/test_tml_to_ossie_fields.py +++ b/converters/thoughtspot/tests/test_tml_to_ossie_fields.py @@ -704,3 +704,71 @@ def test_the_two_kinds_genuinely_differ(self): assert field_expr != metric_expr, ( f"field and metric both emitted {field_expr!r}; the scope distinction is gone" ) + + +class TestPortableIdentifierQuoting: + """A ThoughtSpot display name is not automatically a valid SQL identifier. + + Columns and tables carry spaces, colons, percent signs and parentheses + freely. Emitted raw they do not merely look wrong, they do not parse: + `SUM(cargo.Custom Clearance Time (min))` and `SUM(HV: STORES.LATITUDE)` are + both rejected by sqlglot -- the parser Apache's own validator uses. Across + 31 real models this was 5 documents failing validation outright and 30 SQL + findings; quoting takes both to zero. + + Quoted only WHEN NEEDED, which matters semantically rather than + cosmetically: the specification notes regular identifiers are compared + case-insensitively while quoted ones are compared verbatim, so quoting a + name that does not need it changes how a consumer matches it. + """ + + @staticmethod + def _resolver(dataset, column): + return lambda _t, _c: (dataset, column) + + def test_a_name_needing_quotes_is_quoted(self): + out = expression_entries( + "[T::C]", self._resolver("cargo", "Custom Clearance Time (min)"), + IssueLog(), object_ref="m", kind="metric", + ) + portable = next(e["expression"] for e in out if e["dialect"] == "ANSI_SQL") + assert portable == 'cargo."Custom Clearance Time (min)"', portable + + def test_a_dataset_name_needing_quotes_is_quoted_too(self): + out = expression_entries( + "[T::C]", self._resolver("HV: DIM_STORES", "LATITUDE"), + IssueLog(), object_ref="m", kind="metric", + ) + portable = next(e["expression"] for e in out if e["dialect"] == "ANSI_SQL") + assert portable == '"HV: DIM_STORES".LATITUDE', portable + + def test_a_regular_identifier_is_left_bare(self): + # Not cosmetic: quoting changes case-matching semantics, so a name that + # does not need quotes must not get them. + out = expression_entries( + "[T::C]", self._resolver("orders", "amount"), + IssueLog(), object_ref="m", kind="metric", + ) + portable = next(e["expression"] for e in out if e["dialect"] == "ANSI_SQL") + assert portable == "orders.amount", portable + assert '"' not in portable + + def test_an_embedded_double_quote_is_doubled(self): + out = expression_entries( + "[T::C]", self._resolver("orders", 'He said "hi"'), + IssueLog(), object_ref="f", kind="field", + ) + portable = next(e["expression"] for e in out if e["dialect"] == "ANSI_SQL") + assert portable == '"He said ""hi"""', portable + + def test_the_quoted_output_actually_parses_as_sql(self): + # The point of the exercise. Skipped rather than silently passing when + # sqlglot is absent -- the validator's own "not installed" path reports + # PASSED, which is how this defect survived unseen. + sqlglot = pytest.importorskip("sqlglot") + out = expression_entries( + "[T::C]", self._resolver("HV: DIM_STORES", "Custom Time (min)"), + IssueLog(), object_ref="m", kind="metric", + ) + portable = next(e["expression"] for e in out if e["dialect"] == "ANSI_SQL") + sqlglot.parse_one(f"SELECT SUM({portable})") diff --git a/converters/thoughtspot/uv.lock b/converters/thoughtspot/uv.lock index 5563eb59d..e21794e51 100644 --- a/converters/thoughtspot/uv.lock +++ b/converters/thoughtspot/uv.lock @@ -19,6 +19,7 @@ dev = [ { name = "hypothesis" }, { name = "jsonschema" }, { name = "pytest" }, + { name = "sqlglot" }, ] [package.metadata] @@ -29,6 +30,7 @@ dev = [ { name = "hypothesis", specifier = ">=6.0" }, { name = "jsonschema", specifier = ">=4.26.0" }, { name = "pytest", specifier = ">=8.0" }, + { name = "sqlglot", specifier = ">=25.0" }, ] [[package]] @@ -578,6 +580,15 @@ wheels = [ { url = "https://files.pythonhosted.org/packages/32/46/9cb0e58b2deb7f82b84065f37f3bffeb12413f947f9388e4cac22c4621ce/sortedcontainers-2.4.0-py2.py3-none-any.whl", hash = "sha256:a163dcaede0f1c021485e957a39245190e74249897e2ae4b2aa38595db237ee0", size = 29575, upload-time = "2021-05-16T22:03:41.177Z" }, ] +[[package]] +name = "sqlglot" +version = "30.19.0" +source = { registry = "https://pypi.org/simple" } +sdist = { url = "https://files.pythonhosted.org/packages/fe/48/996687ba146c07deee6cc3528738e645d2e72805e023ed9672b483648378/sqlglot-30.19.0.tar.gz", hash = "sha256:6fcfbe93c5ee76e07faadf0930dd119eebb545c4f317f8e4f205f177305d8b08", size = 6042945, upload-time = "2026-09-22T15:19:58.64Z" } +wheels = [ + { url = "https://files.pythonhosted.org/packages/10/94/094d83ec808b50374f33a3024c9e60ca3074595e1f9581c1692591fda81e/sqlglot-30.19.0-py3-none-any.whl", hash = "sha256:b4500fcced395359feea582f09d2d4433bf1f36362a3a9d0d6d5ebbc1f84242e", size = 756203, upload-time = "2026-09-22T15:19:56.761Z" }, +] + [[package]] name = "tomli" version = "2.4.1" From 9d9feee7d686d85ae73306f3d0cf542ace4722ad Mon Sep 17 00:00:00 2001 From: "damian.waldron" Date: Fri, 25 Sep 2026 18:27:25 +1000 Subject: [PATCH 3/7] fix(thoughtspot): preserve an explicit `aggregation: NONE` across a round trip Ossie has one way to say "this metric does not aggregate"; TML has two -- the `aggregation` key absent, or present with the value NONE. `_AGGREGATION["NONE"]` is `None`, which is also what `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. A per-row ratio the author declared un-aggregated was returned as a column ThoughtSpot rolls up: the sum of ratios instead of the ratio. Exit 0 both directions, INFO-level logging only. Recorded in the stash, which is the mechanism this converter already uses for a ThoughtSpot value Ossie has nowhere to put. Only where the aggregation is LOAD-BEARING: a raw column, or a formula that does not already aggregate. On a formula whose outer call is an aggregate the column property is a documented no-op, so NONE and absent genuinely do mean the same thing there, and stashing it would put a payload on a document that needs none. An existing test (`..._stashes_nothing`) catches that, and caught it here. Recognising it has to happen in the `aggregation is None` branch rather than the scalar branch below: an explicit NONE takes the first branch and never reaches the second, which is what made the first attempt at this a no-op. Part of #467. The OTHER half of that issue -- whether an ABSENT key should mean SUM rather than NONE -- is NOT changed here. That rests on ThoughtSpot's documented default, and neither this repo's TML schema reference nor the developer documentation I can reach states one. It changes numbers, so it wants a live confirmation rather than an inference; the issue records what was checked. Found by an independent adversarial review. Mutation-checked: removing the restoration fails the new round-trip test. 952 tests pass; the 31-model corpus is unchanged. Co-Authored-By: Claude Opus 5 (1M context) --- converters/thoughtspot/docs/vendor-payload.md | 1 + .../src/ossie_thoughtspot/constants.py | 20 ++++++ .../ossie_thoughtspot/ossie_to_thoughtspot.py | 8 +++ .../src/ossie_thoughtspot/tml_to_ossie.py | 24 +++++++ .../thoughtspot/tests/test_roundtrip.py | 68 +++++++++++++++++++ 5 files changed, 121 insertions(+) diff --git a/converters/thoughtspot/docs/vendor-payload.md b/converters/thoughtspot/docs/vendor-payload.md index 08df506ac..028de01b6 100644 --- a/converters/thoughtspot/docs/vendor-payload.md +++ b/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. | diff --git a/converters/thoughtspot/src/ossie_thoughtspot/constants.py b/converters/thoughtspot/src/ossie_thoughtspot/constants.py index d594b7a9b..899bd3006 100644 --- a/converters/thoughtspot/src/ossie_thoughtspot/constants.py +++ b/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" diff --git a/converters/thoughtspot/src/ossie_thoughtspot/ossie_to_thoughtspot.py b/converters/thoughtspot/src/ossie_thoughtspot/ossie_to_thoughtspot.py index d1d5a7ea6..723217f9c 100644 --- a/converters/thoughtspot/src/ossie_thoughtspot/ossie_to_thoughtspot.py +++ b/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, @@ -1586,6 +1587,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: diff --git a/converters/thoughtspot/src/ossie_thoughtspot/tml_to_ossie.py b/converters/thoughtspot/src/ossie_thoughtspot/tml_to_ossie.py index c86b9bd77..fa3978ba1 100644 --- a/converters/thoughtspot/src/ossie_thoughtspot/tml_to_ossie.py +++ b/converters/thoughtspot/src/ossie_thoughtspot/tml_to_ossie.py @@ -106,6 +106,7 @@ FIELD_STASH_FORMULA_ID, FIELD_STASH_FORMULA_NAME, FIELD_STASH_DB_COLUMN_NAME_WITNESS, + METRIC_STASH_AGGREGATION_NONE, METRIC_STASH_COLUMN_AGGREGATION, METRIC_SHAPE_COLUMN_AGGREGATION, METRIC_SHAPE_FORMULA, @@ -1099,8 +1100,21 @@ def convert_metric( aggregation_raw = "NONE" aggregation = _AGGREGATION[aggregation_raw] load_bearing_aggregation: str | None = None + #: The key was PRESENT and said NONE, as opposed to being absent. Read from + #: `properties` directly because `aggregation_raw` cannot tell the two apart + #: -- `.get("aggregation", "NONE")` yields "NONE" for both, which is the + #: collapse that lost this value in the first place. + explicit_none = properties.get("aggregation") == "NONE" + #: ...and worth recording only where the aggregation would otherwise DO + #: something: a raw column, or a scalar formula. Where the expression + #: already aggregates, the column property is a documented no-op, so NONE + #: and absent genuinely mean the same thing and stashing it would put a + #: payload on a document that needs none. + explicit_none_is_load_bearing = False if "column_id" in column: + # A raw column aggregates by its property alone, so NONE is load-bearing. + explicit_none_is_load_bearing = explicit_none metric_shape = METRIC_SHAPE_COLUMN_AGGREGATION table_name, column_name = identifiers.split_column_ref(f"[{column['column_id']}]") metric_name = _field_or_metric_identifier( @@ -1156,6 +1170,13 @@ def convert_metric( metric: dict = {"name": metric_name} if aggregation is None: # Nothing to compose: the verbatim expr, untouched, is the whole metric. + # An EXPLICIT NONE reaches here too, because `_AGGREGATION["NONE"]` + # is `None` -- so this is where it has to be recognised, not in the + # scalar branch below, which it never reaches. It is load-bearing + # exactly when the expression does not already aggregate. + explicit_none_is_load_bearing = explicit_none and not ( + _outer_call_is_aggregate(expr) or _contains_aggregate_call(expr) + ) metric_shape = METRIC_SHAPE_FORMULA dialects = expression_entries( expr, resolve, log, object_ref=object_ref, kind="metric" @@ -1240,6 +1261,9 @@ def convert_metric( stash_payload[METRIC_STASH_SHAPE] = metric_shape if load_bearing_aggregation is not None: stash_payload[METRIC_STASH_COLUMN_AGGREGATION] = load_bearing_aggregation + if explicit_none_is_load_bearing: + # Recorded because Ossie cannot: see METRIC_STASH_AGGREGATION_NONE. + stash_payload[METRIC_STASH_AGGREGATION_NONE] = True metric = _write_stash_safely(metric, stash_payload, log, object_ref) description = column.get("description") diff --git a/converters/thoughtspot/tests/test_roundtrip.py b/converters/thoughtspot/tests/test_roundtrip.py index 419a703db..53b82420e 100644 --- a/converters/thoughtspot/tests/test_roundtrip.py +++ b/converters/thoughtspot/tests/test_roundtrip.py @@ -62,6 +62,8 @@ """ from __future__ import annotations +import json + from pathlib import Path import pytest @@ -1263,3 +1265,69 @@ def test_it_is_not_given_a_surfacing_column(self): def test_the_conversion_reports_no_error(self): assert not self._round_trip().issues.has_errors() + + +def test_an_explicit_aggregation_none_survives_a_round_trip(): + """`aggregation: NONE` is not the same as no `aggregation` key. + + Ossie has one way to say "does not aggregate"; TML has two, and + `_AGGREGATION["NONE"]` is `None` -- the same value + `properties.get("aggregation", "NONE")` yields for an absent key. Both + collapsed identically on the way in, so an explicit NONE came back ABSENT, + and ThoughtSpot applies its own default to an absent key. A per-row ratio + the author declared un-aggregated was returned as a column ThoughtSpot + rolls up: the sum of ratios instead of the ratio. + + Only where the aggregation is LOAD-BEARING. On a formula that already + aggregates the property is a documented no-op, so NONE and absent do mean + the same thing there, and stashing it would put a payload on a document + that needs none -- see the companion test below. + """ + table = tml.TmlDocument(kind="table", guid=None, body={ + "name": "orders", "db": "D", "schema": "S", "db_table": "ORDERS", + "connection": {"name": "C"}, + "columns": [ + {"name": "amount", "db_column_name": "amount", + "db_column_properties": {"data_type": "INT64"}}, + {"name": "qty", "db_column_name": "qty", + "db_column_properties": {"data_type": "INT64"}}, + ], + }) + model = tml.TmlDocument(kind="model", guid=None, body={ + "name": "m", "model_tables": [{"name": "orders"}], + "formulas": [{"id": "f1", "name": "Ratio", "expr": "[orders::amount] / [orders::qty]"}], + "columns": [{"name": "Ratio", "formula_id": "f1", + "properties": {"column_type": "MEASURE", "aggregation": "NONE"}}], + }) + ossie = tml_to_ossie.convert(tml.DocumentSet(model=model, tables=(table,))) + back = ossie_to_thoughtspot.convert(ossie.model) + emitted = next(c for c in back.documents.model.body["columns"] if c["name"] == "Ratio") + assert emitted["properties"].get("aggregation") == "NONE", ( + f"an explicit NONE was dropped; ThoughtSpot would apply its own default: {emitted}" + ) + + +def test_an_inert_aggregation_none_is_not_stashed(): + """The other direction: on a formula that already aggregates, NONE is a + no-op, so recording it would dirty a document that needs no payload.""" + table = tml.TmlDocument(kind="table", guid=None, body={ + "name": "orders", "db": "D", "schema": "S", "db_table": "ORDERS", + "connection": {"name": "C"}, + "columns": [{"name": "amount", "db_column_name": "amount", + "db_column_properties": {"data_type": "INT64"}}], + }) + model = tml.TmlDocument(kind="model", guid=None, body={ + "name": "m", "model_tables": [{"name": "orders"}], + "formulas": [{"id": "f1", "name": "Total", "expr": "sum ( [orders::amount] )"}], + "columns": [{"name": "Total", "formula_id": "f1", + "properties": {"column_type": "MEASURE", "aggregation": "NONE"}}], + }) + ossie = tml_to_ossie.convert(tml.DocumentSet(model=model, tables=(table,))) + metric = next(m for m in ossie.model["metrics"] if m["name"] == "total") + payload = {} + for ext in metric.get("custom_extensions") or []: + if ext.get("vendor_name") == "THOUGHTSPOT": + payload = json.loads(ext["data"]) + assert "aggregation_explicit_none" not in payload, ( + f"an inert NONE was stashed: {payload}" + ) From ead5cac8f9d2fd6bb31f0722fc9e859a6b0401cb Mon Sep 17 00:00:00 2001 From: "damian.waldron" Date: Fri, 25 Sep 2026 18:32:30 +1000 Subject: [PATCH 4/7] fix(thoughtspot): an absent aggregation is ThoughtSpot's default, SUM, not NONE Completes #467. ThoughtSpot's default for a MEASURE column with no `aggregation` key is SUM (confirmed by ThoughtSpot). This converter read absent as NONE, so a column the product sums came across as a raw per-row value -- a different number, silently. The default may only be APPLIED where this converter can see that nothing already aggregates, and a blanket default is unsafe. Composed everywhere, the tpcds fixture's `profit_margin` became sum ( [formula_total_profit] / [formula_total_sales] ) A ratio of two aggregates, wrapped in another one. That is the silent double-aggregation this converter exists to avoid, and it is invisible to `_contains_aggregate_call`, which cannot see through a formula cross-reference. So the default is applied to a raw column and to a genuinely scalar formula, and declined where the expression references other formulas -- with TS-METRIC-AGGREGATION-DEFAULT-UNRESOLVED naming them. A loud loss rather than a quiet wrong answer, which is this converter's standing preference. Two smaller corrections fall out: - TS-METRIC-AGGREGATION-ALREADY-AGGREGATED said "the column-level aggregation 'SUM' was ignored" for columns whose author wrote no aggregation at all. It now distinguishes a defaulted value from a declared one. - A DEFAULTED aggregation is no longer stashed for a bare `group_aggregate`. Preserving it would add an `aggregation` key where the source had none -- a byte the round trip should not invent -- and absent still means SUM on the way back, so nothing is lost by leaving it absent. Real exports are unaffected: 0 of 476 MEASURE columns across 31 real models omit the key, and the corpus output is byte-identical before and after. This bites hand-authored and minimal TML, which is exactly what a portable format invites. 952 tests pass; four new ones, including the cross-reference case. Co-Authored-By: Claude Opus 5 (1M context) --- .../src/ossie_thoughtspot/tml_to_ossie.py | 59 +++++++++++++++-- .../thoughtspot/tests/test_roundtrip.py | 65 +++++++++++++++++++ 2 files changed, 120 insertions(+), 4 deletions(-) diff --git a/converters/thoughtspot/src/ossie_thoughtspot/tml_to_ossie.py b/converters/thoughtspot/src/ossie_thoughtspot/tml_to_ossie.py index fa3978ba1..d2dde3652 100644 --- a/converters/thoughtspot/src/ossie_thoughtspot/tml_to_ossie.py +++ b/converters/thoughtspot/src/ossie_thoughtspot/tml_to_ossie.py @@ -1081,7 +1081,24 @@ def convert_metric( return None object_ref = f"metric:{display_name}" - aggregation_raw = properties.get("aggregation", "NONE") + # ThoughtSpot's default for a MEASURE column with no `aggregation` key is + # SUM, not "no aggregation" (confirmed by ThoughtSpot). Reading absent as + # NONE emitted a metric with no aggregate at all, so a column the product + # sums came across as a raw per-row value: a different number, silently. + # + # But the default may only be APPLIED where this converter can see that + # nothing already aggregates. It cannot see through a formula + # cross-reference: `[formula_total_profit] / [formula_total_sales]` is a + # ratio of two aggregates and looks scalar, so composing the default around + # it yields `sum ( [formula_a] / [formula_b] )` -- double aggregation, which + # is the silent-wrong-number class this converter exists to avoid. Where the + # default cannot be applied safely it is left off and an issue says so, + # which is a loud loss rather than a quiet wrong answer. + # + # `explicit_none` below still distinguishes a key that SAYS NONE, which is a + # real instruction rather than an absence, and which must round-trip. + aggregation_is_default = "aggregation" not in properties + aggregation_raw = properties.get("aggregation", "SUM") # A non-string aggregation (a list or dict in a hand-edited document) would # make the `not in _AGGREGATION` membership test raise on an unhashable # value; treat it as unrecognised and fall back to NONE like any other @@ -1204,7 +1221,13 @@ def convert_metric( # 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): + if _is_bare_group_aggregate(expr) and not aggregation_is_default: + # Only an EXPLICIT value is preserved. A defaulted one is not + # information the source document carried, and re-emitting it + # would add an `aggregation` key where the author wrote none -- + # a byte the round trip should not invent. Absent still means + # SUM to ThoughtSpot on the way back, so nothing is lost by + # leaving it absent, and fidelity is kept. load_bearing_aggregation = aggregation_raw metric_shape = METRIC_SHAPE_FORMULA dialects = expression_entries( @@ -1222,8 +1245,36 @@ def convert_metric( severity=Severity.WARNING, message=( f"column {display_name!r}'s expression already contains an " - f"aggregate; the column-level aggregation {aggregation_raw!r} " - f"was ignored to avoid double-aggregating" + f"aggregate; " + + ( + f"ThoughtSpot's default aggregation is not applied" + if aggregation_is_default + else f"the column-level aggregation {aggregation_raw!r} was ignored" + ) + + " to avoid double-aggregating" + ), + object_ref=object_ref, + ) + metric_shape = METRIC_SHAPE_FORMULA + dialects = expression_entries( + expr, resolve, log, object_ref=object_ref, kind="metric" + ) + elif aggregation_is_default and formula.find_formula_refs(expr): + # Scalar-looking, but it references other formulas whose own + # expressions may aggregate. ThoughtSpot's default would resolve + # against the real definitions; this converter cannot, so it + # declines rather than guessing a wrapper around them. + log.add( + code="TS-METRIC-AGGREGATION-DEFAULT-UNRESOLVED", + severity=Severity.WARNING, + message=( + f"column {display_name!r} has no explicit aggregation, and its " + f"expression references the formula(s) " + f"{', '.join(formula.find_formula_refs(expr))}, whose own " + f"aggregation this converter cannot resolve; ThoughtSpot's " + f"default aggregation is not applied, because composing one " + f"around an expression that already aggregates would " + f"double-aggregate" ), object_ref=object_ref, ) diff --git a/converters/thoughtspot/tests/test_roundtrip.py b/converters/thoughtspot/tests/test_roundtrip.py index 53b82420e..2ffaa01bd 100644 --- a/converters/thoughtspot/tests/test_roundtrip.py +++ b/converters/thoughtspot/tests/test_roundtrip.py @@ -1331,3 +1331,68 @@ def test_an_inert_aggregation_none_is_not_stashed(): assert "aggregation_explicit_none" not in payload, ( f"an inert NONE was stashed: {payload}" ) + + +class TestAnAbsentAggregationDefaultsToSum: + """ThoughtSpot's default for a MEASURE column with no `aggregation` key is + SUM. Reading absent as NONE emitted a metric with no aggregate at all, so a + column the product sums came across as a raw per-row value. + + The default may only be APPLIED where nothing already aggregates. It must + NOT be composed around an expression that reaches its aggregate through a + formula cross-reference -- the converter cannot resolve those, and wrapping + them produces `sum ( [formula_a] / [formula_b] )`, which is the silent + double-aggregation this converter exists to avoid. + """ + + @staticmethod + def _convert(expr=None, column_id=None, formulas=None): + table = tml.TmlDocument(kind="table", guid=None, body={ + "name": "orders", "db": "D", "schema": "S", "db_table": "ORDERS", + "connection": {"name": "C"}, + "columns": [{"name": "amount", "db_column_name": "amount", + "db_column_properties": {"data_type": "INT64"}}, + {"name": "qty", "db_column_name": "qty", + "db_column_properties": {"data_type": "INT64"}}], + }) + column = {"name": "M", "properties": {"column_type": "MEASURE"}} # no aggregation + body = {"name": "m", "model_tables": [{"name": "orders"}], "columns": [column]} + if column_id: + column["column_id"] = column_id + else: + column["formula_id"] = "f1" + body["formulas"] = (formulas or []) + [{"id": "f1", "name": "M", "expr": expr}] + model = tml.TmlDocument(kind="model", guid=None, body=body) + return tml_to_ossie.convert(tml.DocumentSet(model=model, tables=(table,))) + + @staticmethod + def _ts(result): + metric = next(m for m in result.model["metrics"]) + return next(d["expression"] for d in metric["expression"]["dialects"] + if d["dialect"] == "THOUGHTSPOT") + + def test_a_raw_column_gains_the_default(self): + assert self._ts(self._convert(column_id="orders::amount")) == "sum ( [orders::amount] )" + + def test_a_scalar_formula_gains_the_default(self): + assert self._ts(self._convert(expr="[orders::amount] / [orders::qty]")) == ( + "sum ( [orders::amount] / [orders::qty] )" + ) + + def test_an_already_aggregating_formula_does_not(self): + assert self._ts(self._convert(expr="sum ( [orders::amount] )")) == ( + "sum ( [orders::amount] )" + ) + + def test_a_cross_referencing_formula_is_left_alone_and_reported(self): + # THE case a blanket default gets wrong. `[a] / [b]` looks scalar and is + # a ratio of two aggregates; composing around it double-aggregates. + result = self._convert( + expr="[formula_a] / [formula_b]", + formulas=[{"id": "formula_a", "name": "A", "expr": "sum ( [orders::amount] )"}, + {"id": "formula_b", "name": "B", "expr": "sum ( [orders::qty] )"}], + ) + assert self._ts(result) == "[formula_a] / [formula_b]", "the default was composed around aggregates" + assert "TS-METRIC-AGGREGATION-DEFAULT-UNRESOLVED" in [ + i["code"] for i in result.issues.as_dicts() + ], "declining to apply the default must be reported, not silent" From f7de8f565237cd29b44aa6c8fa45f6b904737fe8 Mon Sep 17 00:00:00 2001 From: "damian.waldron" Date: Tue, 6 Oct 2026 09:24:35 +1100 Subject: [PATCH 5/7] fix(thoughtspot): a reserved word is not a bare portable identifier `_REGULAR_IDENTIFIER` tests an identifier's *shape*, and a reserved word passes it: `on` is a letter followed by letters. So a ThoughtSpot column named `on` emitted `expression: on`, which makes `SELECT on`, and `validation/validate.py` rejects it: [SQL] Field 'store.on' in model 'tpcds_retail_model' (ANSI_SQL): Invalid expression / Unexpected token. Line 1, Col: 9. Not hypothetical: TPC-DS `store` has that column, so this converter's own fixture has been failing the project's validator. Reported by @jbonofre in review. `_SQL_RESERVED` carries the second test, from two sources because each covers the other's blind spot: * the words **sqlglot rejects** as a bare column reference -- that is the parser the validator runs, so one of these left unquoted fails the gate. 16 words no standards list would have caught, among them ALTER, GRANT, LATERAL, QUALIFY, RETURNING. * the **ANSI reserved words sqlglot tolerates** -- `SELECT order` parses under sqlglot and is rejected by Snowflake and PostgreSQL alike, so the validator passing says nothing about a warehouse running it. 25 words the parser alone would have missed, among them ORDER, GROUP, TABLE, USER. Deliberately not the whole ANSI:2016 list (~450 words): that would quote YEAR, MONTH and VALUE, which are ordinary ThoughtSpot column names, and quoting is not free -- the spec makes a quoted identifier match verbatim where a regular one is case-insensitive, so a needlessly quoted "Amount" stops matching a warehouse column stored as AMOUNT. A test pins that those stay bare. A further test re-derives the sqlglot half of the list from the installed sqlglot and fails if the literal no longer covers it, so an upgrade that reserves a new word breaks a test rather than quietly emitting an expression the validator turns away. No effect on the 31-model corpus -- output is byte-identical, none of those columns is a reserved word. Only the fixture had one, which is the argument for the fixture. Co-Authored-By: Claude Opus 5 (1M context) --- .../src/ossie_thoughtspot/tml_to_ossie.py | 47 ++++++++++++++- .../tests/fixtures/tpcds/expected.ossie.yaml | 2 +- .../tests/test_tml_to_ossie_fields.py | 57 +++++++++++++++++++ 3 files changed, 104 insertions(+), 2 deletions(-) diff --git a/converters/thoughtspot/src/ossie_thoughtspot/tml_to_ossie.py b/converters/thoughtspot/src/ossie_thoughtspot/tml_to_ossie.py index d2dde3652..666ae2fab 100644 --- a/converters/thoughtspot/src/ossie_thoughtspot/tml_to_ossie.py +++ b/converters/thoughtspot/src/ossie_thoughtspot/tml_to_ossie.py @@ -146,6 +146,46 @@ #: underscores. Anything else has to be double-quoted to survive a SQL parser. _REGULAR_IDENTIFIER = re.compile(r"^[A-Za-z_][A-Za-z0-9_]*$") +#: Words a bare column reference cannot be, however regular its spelling. +#: +#: Two sources, because each covers the other's blind spot and neither alone +#: is enough: +#: +#: * The words **sqlglot rejects** as a bare column reference. That is the +#: parser `validation/validate.py` runs, so one of these left unquoted does +#: not merely read oddly -- it fails the project's own validator. `ON` is +#: real: TPC-DS `store` has a column of that name and it emitted +#: `expression: on`, which makes `SELECT on`. These are the 16 words no +#: standards list would have caught, among them `ALTER`, `GRANT`, `LATERAL`, +#: `QUALIFY` and `RETURNING`. +#: * The **ANSI SQL reserved words sqlglot happens to tolerate**. `SELECT order` +#: parses under sqlglot and is rejected by Snowflake and PostgreSQL alike, so +#: the validator passing says nothing about whether a warehouse will run it. +#: These are the 25 the parser alone would have missed, among them `ORDER`, +#: `GROUP`, `TABLE`, `USER` and `NULL`. +#: +#: `tests/test_tml_to_ossie_fields.py` re-derives the first source from the +#: installed sqlglot and asserts this set still covers it, so a sqlglot upgrade +#: that reserves a new word fails loudly rather than quietly emitting an +#: expression the validator rejects. +#: +#: Deliberately NOT the whole ANSI:2016 reserved list (~450 words). That would +#: quote `YEAR`, `MONTH` and `VALUE` -- ordinary ThoughtSpot column names -- +#: and quoting is not free: the spec makes a quoted identifier match verbatim +#: where a regular one is case-insensitive, so a needlessly quoted `"Amount"` +#: stops matching a warehouse column stored as `AMOUNT`. +_SQL_RESERVED = frozenset({ + "ALL", "ALTER", "AND", "ANY", "AS", "ASC", "BETWEEN", "BY", "CASE", "CHECK", + "CREATE", "CROSS", "DEFAULT", "DESC", "DISTINCT", "DROP", "ELSE", "END", + "EXCEPT", "EXISTS", "FALSE", "FOR", "FROM", "FULL", "GLOB", "GRANT", + "GROUP", "HAVING", "ILIKE", "IN", "INNER", "INSERT", "INTERSECT", "INTO", + "IS", "JOIN", "LATERAL", "LEFT", "LIKE", "LIMIT", "NOT", "NOTNULL", "NULL", + "OFFSET", "ON", "OR", "ORDER", "OUTER", "PARTITIONED_BY", "PRIMARY", + "QUALIFY", "REGEXP", "RETURNING", "REVOKE", "RIGHT", "RLIKE", "ROLLBACK", + "SELECT", "SOME", "STRAIGHT_JOIN", "TABLE", "THEN", "TRUE", "UNCACHE", + "UNION", "UNIQUE", "USER", "USING", "WHEN", "WHERE", "WITH", "XOR", +}) + def _sql_identifier(name: str) -> str: """`name` as an ANSI SQL identifier, quoted only when it has to be. @@ -164,8 +204,13 @@ def _sql_identifier(name: str) -> str: A name that IS regular is left bare -- the spec notes regular identifiers are case-insensitive while quoted ones are compared verbatim, so quoting unnecessarily would change how a consumer matches it. + + Regular *spelling* is not sufficient on its own: a reserved word is a + regular identifier by shape and still cannot stand bare where a column is + expected (`SELECT on`). `_SQL_RESERVED` carries that second test, and the + comparison is case-insensitive because reserved words are. """ - if _REGULAR_IDENTIFIER.match(name): + if _REGULAR_IDENTIFIER.match(name) and name.upper() not in _SQL_RESERVED: return name return '"' + name.replace('"', '""') + '"' diff --git a/converters/thoughtspot/tests/fixtures/tpcds/expected.ossie.yaml b/converters/thoughtspot/tests/fixtures/tpcds/expected.ossie.yaml index eae5df7e5..70da4f572 100644 --- a/converters/thoughtspot/tests/fixtures/tpcds/expected.ossie.yaml +++ b/converters/thoughtspot/tests/fixtures/tpcds/expected.ossie.yaml @@ -387,7 +387,7 @@ datasets: - dialect: THOUGHTSPOT expression: '[store::on]' - dialect: ANSI_SQL - expression: 'on' + expression: '"on"' datatype: Boolean description: Whether the store is currently active and open for business. custom_extensions: diff --git a/converters/thoughtspot/tests/test_tml_to_ossie_fields.py b/converters/thoughtspot/tests/test_tml_to_ossie_fields.py index 8e985a5db..32ade7c38 100644 --- a/converters/thoughtspot/tests/test_tml_to_ossie_fields.py +++ b/converters/thoughtspot/tests/test_tml_to_ossie_fields.py @@ -772,3 +772,60 @@ def test_the_quoted_output_actually_parses_as_sql(self): ) portable = next(e["expression"] for e in out if e["dialect"] == "ANSI_SQL") sqlglot.parse_one(f"SELECT SUM({portable})") + + @pytest.mark.parametrize("word", ["on", "where", "ORDER", "Select", "qualify"]) + def test_a_reserved_word_column_is_quoted_and_parses(self, word): + """A reserved word is a regular identifier by *shape*, so the regex + alone left it bare. `on` is not hypothetical -- TPC-DS `store` has a + column of that name, and it emitted `expression: on`, which the + project's own `validation/validate.py` rejects. + + Case-mixed spellings are included because reserved words are matched + case-insensitively: `Select` must be quoted exactly as `select` is. + """ + sqlglot = pytest.importorskip("sqlglot") + out = expression_entries( + "[T::C]", self._resolver("orders", word), + IssueLog(), object_ref="f", kind="field", + ) + portable = next(e["expression"] for e in out if e["dialect"] == "ANSI_SQL") + assert portable == f'"{word}"', portable + sqlglot.parse_one(f"SELECT {portable}") + + def test_an_ordinary_word_that_merely_looks_keyword_ish_stays_bare(self): + # The guard against over-correcting. These are reserved in ANSI:2016 + # and are ordinary ThoughtSpot column names; quoting them would stop + # them matching a warehouse column stored as `YEAR`, because a quoted + # identifier is compared verbatim where a regular one is not. + for word in ("year", "month", "value", "amount", "name"): + out = expression_entries( + "[T::C]", self._resolver("orders", word), + IssueLog(), object_ref="f", kind="field", + ) + portable = next(e["expression"] for e in out if e["dialect"] == "ANSI_SQL") + assert portable == word, portable + + def test_the_reserved_set_still_covers_every_word_sqlglot_rejects(self): + """`_SQL_RESERVED` is a literal, so it can go stale when sqlglot moves. + This re-derives the oracle from the installed sqlglot and fails if a + word it now rejects would still be emitted bare -- loudly, at test + time, rather than as an expression the validator turns away. + """ + sqlglot = pytest.importorskip("sqlglot") + from sqlglot.tokens import Tokenizer + + from ossie_thoughtspot.tml_to_ossie import _sql_identifier + + missed = [] + for word in Tokenizer.KEYWORDS: + if not word.replace("_", "").isalpha(): + continue + try: + sqlglot.parse_one(f"SELECT {word}") + except Exception: + if _sql_identifier(word) == word: + missed.append(word) + assert not missed, ( + f"sqlglot rejects these as a bare column reference but " + f"_SQL_RESERVED does not cover them: {sorted(missed)}" + ) From 8d8a2a4e768aecd3cf9628cb98bce8d56894977e Mon Sep 17 00:00:00 2001 From: "damian.waldron" Date: Tue, 6 Oct 2026 09:24:48 +1100 Subject: [PATCH 6/7] test(thoughtspot): run the upstream SQL validation over the fixtures The fixture test validated against `ossie-schema.json` and stopped there. The schema says nothing about whether an emitted expression PARSES, so the `store.on` defect above sat in a committed fixture with 936 tests green -- the suite could not see it, and only running `validation/validate.py` by hand did. This closes the gap at the point it opened. It imports the project's own validator by path rather than reimplementing its checks: a local copy would be a second account of the gate, free to agree with this converter while the gate itself disagrees. `validate_sql` RETURNS a warning instead of raising when sqlglot is absent, so a bare assertion on emptiness would pass on the one input that proves nothing. The test asserts the skip warning is absent before asserting no errors -- the same discipline the two importorskip guards already use, and the reason this class of defect survived: "Validation PASSED" is also what the validator prints when it validated nothing. Verified it fails on the pre-fix fixture with @jbonofre's exact error and passes after, with zero skip warnings. Co-Authored-By: Claude Opus 5 (1M context) --- converters/thoughtspot/tests/test_fixtures.py | 38 +++++++++++++++++++ 1 file changed, 38 insertions(+) diff --git a/converters/thoughtspot/tests/test_fixtures.py b/converters/thoughtspot/tests/test_fixtures.py index 3e76d298f..eeb10ddae 100644 --- a/converters/thoughtspot/tests/test_fixtures.py +++ b/converters/thoughtspot/tests/test_fixtures.py @@ -93,6 +93,25 @@ def _load_expected(fixture_dir: Path) -> dict: return document +def _upstream_validator(): + """`validation/validate.py` loaded as a module. + + It is a top-level script rather than an installed package, so it is loaded + by path. Importing the real thing rather than copying its checks is the + point: a local reimplementation would be a second account of the gate, + free to agree with this converter while the gate itself disagrees. + """ + import importlib.util + + path = Path(__file__).resolve().parents[3] / "validation" / "validate.py" + if not path.exists(): # pragma: no cover - only outside a full checkout + pytest.skip(f"upstream validator not found at {path}") + spec = importlib.util.spec_from_file_location("_ossie_validate", path) + module = importlib.util.module_from_spec(spec) + spec.loader.exec_module(module) + return module + + @pytest.mark.parametrize("fixture_name", FIXTURE_SETS) class TestFixtureSetsLoad: def test_the_fixture_directory_has_tml_documents(self, fixture_name): @@ -127,6 +146,25 @@ def test_expected_output_validates_against_the_upstream_schema(self, fixture_nam expected = _load_expected(FIXTURES_ROOT / fixture_name) jsonschema.Draft202012Validator(schema).validate(expected) + def test_expected_output_passes_the_upstream_sql_validation(self, fixture_name): + """The schema check above says nothing about whether an emitted + expression PARSES, and that gap shipped a real defect: TPC-DS `store` + has a column named `on`, which emitted `expression: on` and made + `validation/validate.py` fail with `Invalid expression / Unexpected + token` while the whole suite stayed green. + + This runs the project's own validator, not a local reimplementation, + so the test cannot drift from the gate it stands in for. + """ + pytest.importorskip("sqlglot") + validate = _upstream_validator() + errors = [str(e) for e in validate.validate_sql(_load_expected(FIXTURES_ROOT / fixture_name))] + # `validate_sql` RETURNS a warning rather than raising when sqlglot is + # absent, so an unguarded assertion on emptiness would pass on the one + # input that proves nothing. Fail loudly instead of skipping silently. + assert not any("skipping SQL validation" in e for e in errors), errors + assert errors == [] + @pytest.mark.parametrize("fixture_name", FIXTURE_SETS) class TestConversionMatchesExpected: From 51b795fb01d73cd42dec56bce8636068f20ef509 Mon Sep 17 00:00:00 2001 From: "damian.waldron" Date: Tue, 6 Oct 2026 09:24:59 +1100 Subject: [PATCH 7/7] fix(thoughtspot): undouble a quoted identifier on the way back to TML Quoting more identifiers on the forward leg exposed an asymmetry in the reverse one. `_sql_identifier` doubles an embedded double quote, so a column ThoughtSpot displays as `Size (")` is emitted `"Size ("")"`. The reverse `_BARE_IDENTIFIER_RE` matched `"[^"]{1,128}"`, which rejects a doubled quote outright -- so the field was not recognised as a physical column at all on the way back, and was dropped as untranslatable. The regex turned away exactly the strings this converter itself writes. The regex now admits `""`, and `_bare_sql_identifier` undoubles on extraction: stripping the outer quotes alone would put `Size ("")` in `db_column_name` and name a column no warehouse has. Also adds the first test that feeds a **foreign vendor dialect** through `build_table`. When an Ossie document carries no THOUGHTSPOT dialect and no stash -- another converter's output -- the physical column can only come from the field's own `expression`, which is what core-spec PR #486 settles it to mean, and the code already does. But every test on that branch used ANSI_SQL, leaving it correct by construction and unproven. DATABRICKS is the real case: `metric_view_to_ossie` emits its dimensions in that dialect, so this is the shape a TML -> Ossie -> Metric View -> Ossie -> TML trip hands back. Each of the three fixes in this PR was reverted in turn against the suite; every one is caught. Co-Authored-By: Claude Opus 5 (1M context) --- .../ossie_thoughtspot/ossie_to_thoughtspot.py | 17 ++++++- .../tests/test_ossie_to_thoughtspot_tables.py | 47 +++++++++++++++++++ 2 files changed, 62 insertions(+), 2 deletions(-) diff --git a/converters/thoughtspot/src/ossie_thoughtspot/ossie_to_thoughtspot.py b/converters/thoughtspot/src/ossie_thoughtspot/ossie_to_thoughtspot.py index 723217f9c..d81269624 100644 --- a/converters/thoughtspot/src/ossie_thoughtspot/ossie_to_thoughtspot.py +++ b/converters/thoughtspot/src/ossie_thoughtspot/ossie_to_thoughtspot.py @@ -131,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 @@ -194,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 diff --git a/converters/thoughtspot/tests/test_ossie_to_thoughtspot_tables.py b/converters/thoughtspot/tests/test_ossie_to_thoughtspot_tables.py index dcae68b7a..879e2c219 100644 --- a/converters/thoughtspot/tests/test_ossie_to_thoughtspot_tables.py +++ b/converters/thoughtspot/tests/test_ossie_to_thoughtspot_tables.py @@ -121,6 +121,53 @@ def test_db_column_name_is_present_even_when_equal_to_name(self): column = table.body["columns"][0] assert column["name"] == column["db_column_name"] == "status" + def test_a_quoted_warehouse_column_is_unquoted_into_db_column_name(self): + # A reserved word reaches the document quoted (`"on"`), because bare it + # would not parse. `db_column_name` must carry the warehouse's own + # spelling, so the quotes come back off -- left on, it would name a + # column called `"on"`, quotes included, which no warehouse has. + dataset = _dataset( + "store", "TPCDS.PUBLIC.STORE", fields=[_physical("is_open", '"on"')] + ) + table = build_table(dataset, IssueLog()) + assert table.body["columns"][0]["db_column_name"] == "on" + + def test_a_doubled_quote_inside_a_quoted_column_is_undoubled(self): + # The forward direction writes a column displayed as `Size (")` as + # `"Size ("")"`. Matching only non-quote characters rejected exactly + # the strings this converter itself produces, so the field was not + # recognised as physical at all and was dropped as untranslatable. + dataset = _dataset( + "parts", "SALES.PUBLIC.PARTS", fields=[_physical("size_in", '"Size ("")"')] + ) + table = build_table(dataset, IssueLog()) + assert table.body["columns"][0]["db_column_name"] == 'Size (")' + + def test_a_foreign_vendor_dialect_still_supplies_db_column_name(self): + """An Ossie document from another converter carries neither a + THOUGHTSPOT dialect nor a ThoughtSpot stash, so the physical column can + only come from the field's own `expression` -- which is exactly what + core-spec PR #486 settles it to mean. + + `DATABRICKS` is the real case: `metric_view_to_ossie` emits its + dimensions in that dialect, so this is the shape a + TML -> Ossie -> Metric View -> Ossie -> TML trip hands back. Every + other test on this path uses ANSI_SQL, which left the dialect-agnostic + branch correct by construction and unproven. + """ + field = _field( + "total_amount", + [{"dialect": "DATABRICKS", "expression": "o_totalprice"}], + label="Total Amount", + ) + dataset = _dataset("orders", "SALES.PUBLIC.ORDERS", fields=[field]) + log = IssueLog() + table = build_table(dataset, log) + column = table.body["columns"][0] + assert column["name"] == "Total Amount" + assert column["db_column_name"] == "o_totalprice" + assert not log.has_errors(), log.as_dicts() + def test_a_round_tripped_bracket_reference_supplies_db_column_name(self): # A prior TML -> Ossie trip leaves the table's own physical column # *display* name inside the verbatim THOUGHTSPOT bracket, not in