diff --git a/converters/thoughtspot/src/ossie_thoughtspot/tml_to_ossie.py b/converters/thoughtspot/src/ossie_thoughtspot/tml_to_ossie.py index 14692fe90..d1081e575 100644 --- a/converters/thoughtspot/src/ossie_thoughtspot/tml_to_ossie.py +++ b/converters/thoughtspot/src/ossie_thoughtspot/tml_to_ossie.py @@ -141,7 +141,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, @@ -233,7 +233,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 @@ -254,7 +275,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, @@ -294,7 +315,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) @@ -577,7 +598,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: @@ -833,7 +854,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, @@ -907,7 +928,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: @@ -1487,7 +1508,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. @@ -2152,18 +2174,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: @@ -2177,7 +2206,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 ------------------------------------------ formulas: dict[str, dict] = { 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 008f3b367..61c3b8239 100644 --- a/converters/thoughtspot/tests/fixtures/tpcds/expected.ossie.yaml +++ b/converters/thoughtspot/tests/fixtures/tpcds/expected.ossie.yaml @@ -46,7 +46,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 @@ -55,7 +55,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 @@ -64,7 +64,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 @@ -73,7 +73,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 @@ -82,7 +82,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 @@ -91,7 +91,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 @@ -100,7 +100,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 @@ -109,7 +109,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 @@ -131,7 +131,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 @@ -140,7 +140,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 @@ -149,7 +149,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 @@ -158,7 +158,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 @@ -167,7 +167,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 @@ -187,7 +187,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 @@ -196,7 +196,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 @@ -205,7 +205,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 @@ -214,7 +214,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 @@ -233,7 +233,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: カナ名 @@ -242,7 +242,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 @@ -266,7 +266,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 @@ -275,7 +275,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 @@ -284,7 +284,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 @@ -293,7 +293,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 @@ -302,7 +302,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 @@ -311,7 +311,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 @@ -331,7 +331,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 @@ -340,7 +340,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 @@ -349,7 +349,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 @@ -362,7 +362,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 @@ -371,7 +371,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 @@ -380,7 +380,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' @@ -389,7 +389,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: @@ -410,7 +410,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 @@ -419,7 +419,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 @@ -428,7 +428,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 6011c5b7d..8607c188e 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 c60cface9..74df9b229 100644 --- a/converters/thoughtspot/tests/test_tml_to_ossie.py +++ b/converters/thoughtspot/tests/test_tml_to_ossie.py @@ -1273,7 +1273,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")]) @@ -1281,7 +1281,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")]) @@ -1289,7 +1289,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")]) @@ -1305,7 +1305,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 d2f017bdd..cdf9acb03 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]"}} @@ -592,3 +592,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 2aab25f7e..f34c72a82 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: