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/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/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..d81269624 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, @@ -130,7 +131,17 @@ #: stripped. This is what a hand-authored field's own physical-column #: expression looks like: no dataset qualifier (a field's expression runs #: against its own dataset's source), no operators, no function calls. -_BARE_IDENTIFIER_RE = re.compile(r'^(?:[A-Za-z_][A-Za-z0-9_]{0,127}|"[^"]{1,128}")$') +#: +#: The quoted alternative admits a **doubled** double quote, because that is +#: how a literal one is spelled inside a quoted identifier and it is what the +#: forward direction writes: `_sql_identifier` in `tml_to_ossie` emits a +#: ThoughtSpot column displayed as `Size (")` as `"Size ("")"`. Matching only +#: `[^"]` here rejected exactly the strings this converter itself produces, so +#: such a field was not recognised as physical at all on the way back and was +#: dropped as untranslatable. +_BARE_IDENTIFIER_RE = re.compile( + r'^(?:[A-Za-z_][A-Za-z0-9_]{0,127}|"(?:[^"]|""){1,128}")$' +) #: Any source string containing whitespace outside of a quoted identifier #: reads as a query rather than a `db.schema.table` reference — a real @@ -193,7 +204,10 @@ def _bare_sql_identifier(expression: str) -> str | None: if _BARE_IDENTIFIER_RE.match(text) is None: return None if text.startswith('"') and text.endswith('"'): - return text[1:-1] + # Undouble: `""` inside a quoted identifier is one literal `"`, so the + # column ThoughtSpot displays as `Size (")` arrives as `"Size ("")"`. + # Stripping the outer quotes alone would name a column no warehouse has. + return text[1:-1].replace('""', '"') return text @@ -1586,6 +1600,13 @@ def _build_metric( if preserved_aggregation and "aggregation" not in properties: properties["aggregation"] = preserved_aggregation + # An EXPLICIT `aggregation: NONE`, restored before the conventional path. + # Dropping it is not a cosmetic loss: ThoughtSpot applies its own default to + # an absent key, so a metric the author declared un-aggregated came back as + # one ThoughtSpot rolls up -- a per-row ratio returned as a sum of ratios. + if payload.get(METRIC_STASH_AGGREGATION_NONE) and "aggregation" not in properties: + properties["aggregation"] = "NONE" + if "aggregation" not in properties: conventional = _outer_aggregation_of(ts_expr) if conventional is not None: diff --git a/converters/thoughtspot/src/ossie_thoughtspot/tml_to_ossie.py b/converters/thoughtspot/src/ossie_thoughtspot/tml_to_ossie.py index 257e742d8..666ae2fab 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, @@ -140,9 +141,82 @@ 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_]*$") + +#: 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. + + 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. + + 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) and name.upper() not in _SQL_RESERVED: + return name + return '"' + name.replace('"', '""') + '"' + 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 +308,30 @@ 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 + column_sql = _sql_identifier(warehouse_column) + portable = ( + column_sql if kind == "field" + else f"{_sql_identifier(dataset_name)}.{column_sql}" + ) + entries.append({"dialect": PORTABLE_DIALECT, "expression": portable}) return entries # Anything else is a function call or a multi-reference expression. Producing a @@ -255,7 +352,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 +392,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 +725,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 +983,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 +1057,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: @@ -1029,7 +1126,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 @@ -1048,8 +1162,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( @@ -1105,6 +1232,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" @@ -1132,7 +1266,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( @@ -1150,8 +1290,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, ) @@ -1189,6 +1357,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") @@ -1579,7 +1750,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 +2510,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 +2542,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..70da4f572 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..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: @@ -204,7 +242,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 +274,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_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 diff --git a/converters/thoughtspot/tests/test_roundtrip.py b/converters/thoughtspot/tests/test_roundtrip.py index 13eeb4b0b..2ffaa01bd 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 @@ -481,7 +483,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 +501,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(): @@ -1263,3 +1265,134 @@ 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}" + ) + + +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" 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..32ade7c38 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,180 @@ 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" + ) + + +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})") + + @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)}" + ) 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: 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"