feat(thoughtspot): bidirectional ThoughtSpot TML <-> Ossie converter - #364
Open
djwaldo wants to merge 81 commits into
Open
feat(thoughtspot): bidirectional ThoughtSpot TML <-> Ossie converter#364djwaldo wants to merge 81 commits into
djwaldo wants to merge 81 commits into
Conversation
Adds ossie_thoughtspot.constants (VENDOR_KEY, DIALECT, FALLBACK_DIALECT, STASH_VERSION, SPEC_SERIES, DIALECT_IS_REGISTERED) per task-2-brief.md. VENDOR_KEY and DIALECT are kept as separate names despite sharing a value today (P6) since they are governed by different upstream processes. Also adds converters/thoughtspot/.gitignore, copied verbatim from converters/honeydew/.gitignore, so a local uv build's egg-info directory can no longer land in a commit (Task 1 had to delete it by hand).
…yYAML controls Complete YAML11_BOOL_TOKENS (add Y, N, YES, NO, OFF) and add two differential tests so the dumper's quoting fix and the loader's 1.2-resolver fix each have a test that fails if that half is removed -- the prior tests only exercised the loader, and 9/11 dumper-quote assertions passed against plain SafeDumper.
…CII-only limitation Review findings on 9ad8fe6: - split_column_ref now raises on a reference containing more than one '::' instead of silently mis-splitting (e.g. a table name formatted with '::' in it corrupted the table/column boundary). Delimiter/escaping redesign is left to Plan C; loud failure is the interim behaviour. - normalise's ASCII-only behaviour (non-ASCII characters dropped, not transliterated) is now documented as a known limitation rather than left implicit, with tests pinning current behaviour for accented Latin and a CJK-only name so it can't silently regress.
…, add duplicate-key test Review findings on task 7: - KD1: a to-one, non-residual relationship with empty to_columns previously vanished silently (no key, no issue) because it still "qualified" for the seen-building loop but was excluded by its own `if cols` guard, while the KD2 loop was gated on seen being non-empty. _qualifies() now requires non-empty to_columns, and the KD2 loop reports an empty-to_columns relationship unconditionally rather than only when a key was derived. - Documented KD3 (orientation re-checked downstream by converters/databricks) in the module docstring, resolving the dangling KD1-KD3 reference. - Added a test for two qualifying relationships agreeing on the same columns (e.g. a dimension joined from two fact tables), which must collapse into one unique key and still yield a primary key rather than being mistaken for disagreement.
Documents the two conversion directions, the 1+N TML document shape, and the Ossie apache#351 dialect caveat. The coverage matrix enumerates what the converter does not carry (L1-L6); L2 (row-level security) is called out as error severity, naming every affected table, since rls_rules is the primary mechanism ThoughtSpot customers are actively migrating onto. The Status section lists the foundations Tasks 1-7 actually shipped (YAML 1.2 codec, issue reporting, custom_extensions stash, identifier and key derivation). test_readme.py asserts structure (both directions named, a Coverage matrix heading with L-numbered rows, the apache#351 reference) rather than prose wording, per P16/P21, so the matrix cannot quietly disappear without a test noticing.
…wording Review findings on the Task 8 README: - Add a Known limitations section documenting identifiers.py's ASCII-only normalise() (drops rather than transliterates non-[0-9a-z] characters, e.g. Cafe -> caf, Urun -> r_n, CJK-only raises ValueError). This lived only in the module docstring; a reader hitting non-English column names would find nothing about it in the README. Kept out of the coverage matrix since it is a different axis (identifier-derivation correctness, not an uncarried TML construct) - the section says so explicitly. - Tighten the L2 (row-level security) Limitation cell: one error-severity issue is raised, its message naming every affected table - not one issue per table, matching the TS_RLS_DROPPED pattern in test_issues.py.
pyproject.toml used setuptools + optional-dependencies where all eight sibling converters use hatchling + PEP 735 dependency-groups, and was missing license/readme/authors/pytest/uv config. The built wheel was missing License-Expression, Author-email, Project-URL, and the embedded README, and leaked pytest/hypothesis into distribution metadata via Requires-Dist. Converted to the sibling shape and dropped hypothesis entirely (unused on this branch). CI ran `pip install -e ".[dev]"` and never touched uv, so the committed uv.lock had no consumer. Switched to `uv sync` + `uv run pytest` and widened the version matrix to 3.10-3.14 to match omni (the sibling with the same requires-python floor). Regenerated uv.lock. SHA-pinned actions left unchanged.
…contract keys.py (I1): the KD2 message unconditionally claimed "Ossie validation will report a to_columns coverage warning" for every disqualified relationship. That's wrong for an empty to_columns (upstream's schema requires minItems: 1, so the document fails validation outright before any coverage check runs — a schema ERROR, not a warning) and often wrong for residual-predicate joins whose columns already cover the derived key (the canonical SCD-2 shape). Split the message: the "not a declared key" half is unconditional, the upstream-prediction half is appended only when `not any(set(key) <= set(rel.to_columns) for key in seen)` — mirroring validate.py:159-165 exactly. Empty to_columns gets its own ERROR-severity issue with a remedy that doesn't say "Expected". _yaml.py: load() now wraps a parse failure in ConversionError (I4), matching stash.py's X4 never-a-bare-traceback contract. dump() now passes allow_unicode=True (I5), so a non-ASCII label round-trips as a literal character instead of an escaped one — every Ossie document Plans C/D emit passes through this function. Minor: keys.py module docstring no longer says databricks' orientation swap is "silent" (it calls _warn()); documented Relationship's frozen=True does not imply safe hashability when to_columns is a list; stash.py's X8 comment now states the guid/obj_id/fqn check is top-level only. Added a test for restore()'s default (no witness_key) shape. Tests: 107 passing (102 + 5 new: 2 keys.py, 3 _yaml.py; the stash.py default-shape test replaces no prior assertion so nets to +1 net file but the count above already reflects all additions).
…ambiguity gap
identifiers.py (R14, revising an earlier ruling): normalise() now applies
Unicode NFKD decomposition before the ASCII lowercase-and-substitute fold.
An earlier ruling treated the ASCII-only behaviour as a stated boundary on
the grounds that transliteration is a product decision — right about
transliteration (Japanese -> romaji), wrong about canonical decomposition,
which is stdlib and needs no policy choice. "Café" -> "cafe", "Ürün" ->
"urun", "Zürich" -> "zurich", "İstanbul" -> "istanbul", "naïve" -> "naive"
now fold correctly; a script with no ASCII decomposition (CJK, Cyrillic)
still raises, and conventional expansions (German "Müller" -> "mueller")
remain an open, separate question. Updated the module/function docstrings,
the README's Known limitations section, and re-pinned the limitation
tests to their new (narrower) expected values.
identifiers.py (M3): split_column_ref's ambiguity guard counted "::" via
str.count, which is non-overlapping — a run of three consecutive colons
(table ending in ':' immediately before the '::' delimiter) counts as one
match and silently mis-split. format_column_ref("ORDERS:", "Col") and
format_column_ref("ORDERS", ":Col") both produce the identical string
"[ORDERS:::Col]" and are genuinely ambiguous; the guard now also rejects
a captured column starting with ':'. Added tests for both origins.
Tests: 112 passing (107 + 5 new: 5 identifiers.py — 2 M3 cases; the R14
limitation tests replace existing pinned cases rather than adding, net
+3 from the expanded parametrize list).
…al ruleset
README.md (I6): "Converts between..." and "Each row raises a structured
ConverterIssue... nothing is dropped silently" both describe behaviour
that does not exist yet — neither conversion direction is implemented,
and no code raises any of L1-L6. Changed to future tense ("will
convert", "is required to raise... once the conversion directions
land"). This is exactly the shape of complaint discussion apache#325 raises
against an unfulfilled README promise.
README.md (I7): the Rules section named "the ThoughtSpot skills
repository" as the normative source for ~30 rule identifiers
(ID1-ID4, X1-X9, KD1-KD3, NM1-NM6, and more) with no URL — unresolvable
from inside this ASF repo, and no sibling converter defers its
normative behaviour to an external vendor-controlled document. Named
the repository (thoughtspot-agent-skills) explicitly, stated it is not
ASF-hosted, acknowledged the vendor-neutrality gap plainly, and recorded
the intent to contribute the mapping tables into this repository rather
than vendoring them now (a larger change needing its own review).
README.md (M9): L2's severity justification was a commercial-trend
argument ("the mechanism customers are actively migrating onto") in an
ASF repo. Restated as the technical property: RLS is unrepresentable in
Ossie core and is security-bearing.
test_packaging.py (M4): the ASF-header test globbed only src/**/*.py and
tests/**/*.py, leaving pyproject.toml, .gitignore, README.md, and the CI
workflow ungated. Added a check for all four, matching on the licence
text itself since each file uses a different comment syntax.
Tests: 113 passing (112 + 1 new).
…ging The Development section still said `pip install -e ".[dev]"` / `python -m pytest tests/ -v`, left over from before the switch to hatchling + PEP 735 dependency-groups. That install now warns the package has no `dev` extra and skips pytest, so the next line fails with ModuleNotFoundError. Replace with the uv invocation CI actually runs.
apache#351 merged 2026-09-01: THOUGHTSPOT is now a registered Dialect enum member and is in SKIP_SQL_VALIDATION. Flip DIALECT_IS_REGISTERED to True, rename FALLBACK_DIALECT to PORTABLE_DIALECT to reflect its new role (emitted alongside THOUGHTSPOT for portable expressions per P8, not instead of it), and update the README status section and both tests that encoded the old assumption.
Task 1 of the expression-translation plan (Plan B). Adds Classification, Variant and Construct (_types.py), an empty CATALOG to be populated across Tasks 3-7, and spec_construct_names() — an oracle that parses the upstream core-spec/expression_language.md so a construct added upstream fails this package's build instead of silently going unsupported. spec_construct_names() returns 137 names, not the plan's target of 146; see task-1-report.md for the family-by-family reconciliation and the concrete gaps (Window's OVER-clause/aggregation prose, a few alias-pair rows). The mapping document Tasks 3-9 are meant to transcribe from (ts-ossie-function-mapping.md) does not exist in this repository yet.
spec_construct_names() (137, parsed live from upstream core-spec) and the mapping document's rule-E1 census (146, one row per construct incl. a few described only in prose) count by different units. Per coordinator ruling, keep the strict spec->catalog gate (catches an upstream addition) and add CONVENTION_DIVERGENCES: the exact 9 constructs the mapping document counts separately that the spec never gives a discrete table row - verified row by row against the mapping document (now located at thoughtspot-agent-skills/docs/ossie/ts-ossie-function-mapping.md, a different repo, per the coordinator). test_the_two_counts_reconcile pins 137 + 9 == 146 so the two counts cannot drift apart silently. Corrects this task's own earlier speculation: CEIL/CEILING, TRUNC/TRUNCATE and TRUE/FALSE are each one row in the mapping document too (not split), so they are a CATALOG-key spelling question for Tasks 3-8, not divergences. Documented prominently in catalog.py's new "Spelling" docstring section.
…templates Review findings on Task 1: - Spelling docstring omitted a AND b / a OR b (-> "expr1 AND expr2" / "expr1 OR expr2", from the Boolean Functions table) and misattributed IS DISTINCT FROM / IS NOT DISTINCT FROM to the top-level summary table when they actually come from the Null-Safe Comparison code fence. Full section rewritten grouped by extractor function and re-audited against a live spec_construct_names() run plus the mapping document's literal row text, with an explicit rule: the live function output is the oracle, never this list or the mapping document's prose. - Deleted unused _SUPPORTED_LIST_CUE_RE and corrected the docstring claim of a "Supported ...:" prose-cue exclusion mechanism that doesn't exist - argument-vocabulary bullet lists are simply invisible to _extract_tables(), which only reads pipe-prefixed lines. - Construct.__post_init__ now rejects a DIRECT/PASSTHROUGH row with no template, so a family task can't add one that passes validation and the coverage gate but fails only when something tries to emit it. New tests/expressions/test_types.py covers all four branches.
…mappable
Adds emit_direct/emit_passthrough/emit_unmappable — one renderer per
Classification, built before the family tasks (3-8) populate CATALOG, so
each family has something real to render into.
Uses the corrected signatures (object_ref required, keyword-only) rather
than the brief's own Interfaces block, which disagreed with its test
snippets on this point:
emit_direct(construct, args) -> str
emit_passthrough(construct, args, log, *, object_ref, has_parameter=False,
partition_column=None) -> str
emit_unmappable(construct, log, *, object_ref) -> None
- E2/E7: emit_direct substitutes positionally via str.format and rejects
an argument-count mismatch rather than tolerating it.
- E4/E7/E12: emit_passthrough renders the SQL body as a quoted template
string (never substituting args into it — ThoughtSpot resolves the
placeholders itself), always raises a WARNING issue naming the function
and the object, and refuses (has_parameter=True) a call that would
carry a runtime ThoughtSpot parameter (E9).
- E8: an optional partition_column wraps the passthrough call in
group_aggregate ( <passthrough> , query_groups ( ) + { <col> } ,
query_filters ( ) ), guaranteeing the partition column reaches GROUP BY.
- E12: emit_unmappable raises an ERROR issue naming the construct and
returns nothing — never a silent drop.
121 passed + 2 xfailed -> 132 passed + 2 xfailed (11 new tests), verified
on the Python 3.10 floor and 3.13.
… convention Review finding (Important): the partition_column kwarg was correct in shape but nothing cross-checked it against the template, so a family task (3-8) forgetting to pass it on a PARTITION BY row would silently emit an unwrapped pass-through that is only sometimes correct — a silent wrong answer, not an error. emit_passthrough now checks "partition by" in the template (case-insensitive) against partition_column in both directions and raises ValueError naming the construct on either mismatch. Two new tests, one per direction. Review finding (Minor): pinned that emit_passthrough's has_parameter refusal never also logs a misleading WARNING, via an explicit assert log.as_dicts() == [] in the existing E9 test. 132 passed + 2 xfailed -> 134 passed + 2 xfailed (2 new tests), verified on the Python 3.10 floor and 3.13. Both xfails confirmed still xfailed (-rxX), not xpassed.
Review follow-up: the guard was a plain substring match on "partition by", so PARTITION BY (two spaces) or a newline between the words silently made carries_partition_by False -- defeating the guard in both directions (a correctly-omitted partition_column leaves the row unwrapped; a correctly- supplied one trips the opposite check as a false blocker). Switched to re.search(r"partition\s+by", ..., re.IGNORECASE) and added a test with irregular internal whitespace that fails against the old substring check and passes against the regex. 134 passed + 2 xfailed -> 135 passed + 2 xfailed (1 new test), verified on the Python 3.10 floor and 3.13. Both xfails confirmed still xfailed.
Construct.__post_init__ now rejects a PASSTHROUGH row whose template already
contains its own variant call (e.g. 'sql_string_op ( "LOWER({0})" , {0} )'
instead of the bare 'LOWER({0})'). emit_passthrough builds the
variant(...) wrapper itself, so a template that already contains it would
double-wrap at emission time - a bug that reads as fine in a static glance at
the catalog and is wrong the moment it runs. Task 5 caught exactly this in its
own first draft; this convention had been written down nowhere, so make it
impossible instead of merely discouraged before two more families land.
Also fixes the pre-existing test_types.py example that (accidentally)
exercised the double-wrapped form and asserted it was valid.
…olumn aggregations Adds convert_metric to tml_to_ossie.py: a column_id + aggregation metric becomes AGG(dataset.field); a scalar formula_id + aggregation composes into AGG(<scalar expr>); an aggregate formula_id (sum(...), etc.) treats the column aggregation as the documented no-op and carries the verbatim expr unchanged. Aggregate detection reads ThoughtSpot's native call names off the same expression catalog used to compose the wrapped rendering, so the two cannot drift apart. Metrics have no label, so a display name that normalises differently is stashed as tml_name.
Records shape (column_aggregation | scalar_formula_plus_aggregation | formula) in the metric's stash alongside tml_name, using the pinned enum spellings, so a return trip can reconstruct the physical-column and scalar-formula-plus- aggregation shapes instead of collapsing every metric into one. formula is omitted rather than written: it is also the reverse direction's own default when no stash is present, so recording it changes nothing about the reconstruction while making the payload heavier.
…nd sql_*_aggregate_op
The outer-call-only aggregate check missed constructs the catalog holds beyond
the eight TML aggregation rows: group_aggregate (ThoughtSpot's own performant
grouped-aggregation pattern) and the sql_*_aggregate_op pass-through family, so
a formula built from either composed a second aggregation on top and produced
a wrong number with no warning.
Two-layer fix. (a) Broadened the aggregate-name set with group_aggregate and
every Variant ending in "_aggregate_op" (derived from the enum, not listed by
hand). (b) Added formula.find_call_names, a whole-expression, any-depth call
scanner, and use it (_contains_aggregate_call) instead of the outer-call-only
check before composing — this also catches an aggregate nested inside a scalar
wrapper (round(sum(x), 2)) that no set of names alone could. Discarding a
load-bearing column aggregation because the expression already aggregates now
always logs a warning naming why, whether the aggregate was the expression's
own outer call or nested inside one.
Also: expression_entries and _physical_datatype hardcoded "field" in issue
text and codes even when building a metric. Both now take a kind parameter
("field" by default, so convert_field's behaviour and codes are byte-for-byte
unchanged) and convert_metric passes kind="metric" at every call site.
…is the outer call An aggregate as a formula's own outer call (sum(...), group_aggregate(...), ...) plus a redundant column-level aggregation is ThoughtSpot's documented, routine no-op -- the previous warning fired on that common, correct shape and would have trained readers to ignore the issue log. Narrowed to warn only when the outer call is not itself an aggregate but one is nested inside it (round(sum(x), 2)), which is the one shape a reader might expect composition and not get. Added _outer_call_is_aggregate for the (silent) outer-call check; _contains_aggregate_call is now only consulted once that check is negative. Strengthened the existing no-op regression test to assert on the issue log (it previously asserted only on the expression), and added explicit coverage for the boundary: sum/average/unique count/group_aggregate as the outer call produce no warning; round(sum(x),2) and a scalar-wrapped group_aggregate each produce exactly one; a genuinely scalar formula still composes. Also corrects find_call_names' docstring and its test's comment, which overstated what the function does: names that are also operator keywords (not, if) are excluded even though both are real catalog functions, and the keyword handling is weaker than split_call's (only a leading run is stripped, so a keyword after a genuine word is not caught). No behaviour change.
…sh and entry point Adds the assembler that ties Tasks 1-5 together: datasets built from model_tables[] + Table/SQL-View documents, the cross-model resolver convert_field/convert_metric take as `resolve`, joins converted to relationships (inline and referencing shapes) with KD1 key derivation (including the ONE_TO_MANY orientation flip), the model/dataset/relationship custom_extensions[THOUGHTSPOT] stash, and the public convert() entry point returning OssieConversion(model, issues). A malformed reference (an ambiguous column_id or join condition) is caught per object, logged, and skipped rather than aborting the whole conversion.
…ing them convert_field/convert_metric never returned a fragment for a column's ThoughtSpot-only properties (index_type, value_casing, ...), so the assembler had nothing to merge and they vanished with no issue -- a direct breach of "nothing dropped silently". Fixed fail-closed in the assembly, without touching either converter's signature: stash the complement of the properties keys the converter actually consumed (column_type, synonyms, ai_context, and aggregation for metrics), under that object's own column_properties extension key, so a future ThoughtSpot property is preserved by construction rather than requiring another enumeration edit. While re-verifying identity leakage for this change, found that copying an unconsumed property's value wholesale can carry a nested identity key a top-level-only guard cannot see (a documented ThoughtSpot shape: a custom map reference nested inside an otherwise plain property). Extended the identity check to scan nested values too, dropping and logging rather than stashing when found.
…ity guard, stash unsurfaced columns Three defects an independent review found and reproduced: - A relationship's target dataset was never checked for existence, only its source -- a join to a table this model failed to build (missing document, bad alias) emitted a Relationship upstream's own validator rejects outright, with no issue logged. Now checked the same way the source already was: dropped, logged, rest of the model stays valid. - The nested identity scan added for column properties covered only that one caller. Moved the scan into stash.write_stash itself -- the single point every stashed payload passes through -- via a new stash.find_forbidden_key(value, forbidden=None), so no present or future caller (model-scope parameters/filters/column_groups/lesson_plans/ action_object_associations/constraints/model-level joins_with included) can bypass it by nesting identity content instead of putting it at a payload's own top level. - Table columns the Model doesn't surface (by column_id, field or metric alike) vanished with no stash and no issue. Now preserved verbatim under the dataset's unsurfaced_columns, per the pinned mapping document. Also: stash.read_stash now rejects an unrecognised custom_extensions shape version (_v) instead of partially reading it as the current shape. Behavioural decision: the boundary (write_stash) still raises on any forbidden key found -- that is what makes the guard impossible to bypass -- but the assembly now catches it (_write_stash_safely), drops the one contaminated top-level stash field, logs which object and which key, and keeps converting the rest of the model. The payload content is TML data the converter did not construct, not a programming error, so it must not abort an otherwise-fine conversion -- consistent with the column-property path, which already dropped and logged rather than raising. Committed the key-derivation edge cases (mixed equality+residual, a self-join, a top-level OR, MANY_TO_MANY) that were previously only scratch-verified during development.
…columns[]
A SQL View document's physical columns live under a different key entirely
(sql_view_columns[], bound to a query output alias via sql_output_column),
not a differently-shaped entry under columns[]. Every reader of "this
dataset's physical columns" -- datatype lookup via table_lookup, and the
unsurfaced-column check -- read columns[] unconditionally, so every SQL
View column vanished: no datatype resolved for a surfaced column, and an
unsurfaced one was lost with no stash and no issue. This has been broken
since the assembler was written; nothing exercised a sql_view document
until now.
Fixed via three small, kind-aware helpers (_raw_physical_columns,
_normalize_physical_column, _normalized_physical_columns) so table_lookup
and the unsurfaced-column pass both read the right key for the document
kind they were actually handed, translating a SQL View column's
sql_output_column into the db_column_name-shaped field the datatype lookup
already expects, while preserving the column verbatim (with its real
sql_output_column key) when it lands in unsurfaced_columns for reverse-
direction fidelity.
sql_output_column itself is now also stashed under the dataset's
sql_output_columns (DatasetLevel schema key: field name -> alias), for
every surfaced field on a SQL View -- unconditionally, not only when it
differs from the field's name, because there is no safe way to re-derive a
query output alias the way a table's db_column_name might be guessed at.
Confirmed source is already correct for a SQL View: it is the raw
sql_query string directly, matching the mapping document's row -- no
change needed there.
Also: routed convert_metric's own stash.write_stash call through
_write_stash_safely, so the wrapper's docstring claim ("every stash site in
this module") is true rather than aspirational; the payload is hardcoded
scalars today so this is currently a no-op, but the next author who adds
TML-derived content to it no longer silently reinstates the
abort-the-whole-conversion behavior the wrapper exists to remove.
Adds build_table(dataset, log) -> TmlDocument, the first module of the Ossie -> TML direction: one Ossie dataset becomes one Table or SQL View document, so the Model document (a later module) has something to reference by name. - db_column_name is always written, even equal to the display name. - db_column_properties.data_type is always written; a datatype-less field infers one rather than omitting the compulsory block. - Each of the four datatypes whose round trip is lossy (Float, Time, DateTimeTz, Opaque) raises an issue naming the loss. - source splits into db/schema/db_table for a table, or is read as a verbatim query for a SQL view; a two- or one-part source that is neither shape raises an issue instead of guessing. - The BOOLEAN/BOOL and DOUBLE/FLOAT warehouse spelling is restored from a field's own stash when present, and defaults otherwise. - A dataset's stashed connection_name, source_parts, table_properties, sql_output_columns and unsurfaced_columns are all honoured, with staleness checked against the live source where a witness exists. A round trip against the forward direction (tml_to_ossie.convert) found a real, previously invisible gap no hand-written fixture had exposed: the forward direction matches a physical column by its display name only, so a surfaced column's true db_column_name is never captured when it differs from that display name. build_table now assumes the two agree in that case -- correct in the common case -- and reports an INFO issue for it rather than staying silent, since the assumption can be wrong. The datatype map's conditional Time -> DATE_TIME mapping (only when the underlying column is timestamp-backed) is deliberately left unimplemented here too: the forward direction never emits Time at all, so a Time-typed field only ever reaches this module hand-authored, with no stashed ThoughtSpot column and no other storage-format signal on an Ossie Field to condition on. VARCHAR is written unconditionally instead, with the declared loss still reported. 589 tests = 556 baseline + 33 new. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…use column, not the Ossie identifier
resolve() built the ANSI_SQL sibling from the Ossie field's own
display-derived identifier ("orders.amount"), not the physical column the
mapping document is explicit about ("the identifier is the physical
column; the display name comes from label/name"). Confident, well-formed
SQL naming a column the warehouse does not have, in every model where a
display name differs from its physical column -- the normal case in any
curated model.
resolve() still uses the model's ATTRIBUTE-surfaced-column index
(attribute_index) to decide WHETHER a reference is one the model actually
surfaces, unchanged; only the value it returns once that gate passes now
looks up the table's own physical column list for the real warehouse
reference (db_column_name for a Table, sql_output_column for a SQL View,
via the existing kind-aware _normalize_physical_column translation). A
computed field's own references resolve through the identical path, since
[TABLE::Column] always names a physical column regardless of what formula
is referencing it.
Also, per the datatype map's own words ("the connection's spelling is
recorded in the field stash's data_type key so the return trip re-emits
the same one"): a physical field/metric's data_type is now stashed under
that documented key whenever it is not the canonical spelling for its
mapped Ossie datatype (BOOL vs BOOLEAN, FLOAT vs DOUBLE) -- never for the
canonical spelling itself, so the empty-payload rule stays reachable.
And: db_column_name, lost because the forward direction matches a
physical column by display name only, is now stashed under a new
field/metric-level db_column_name key when it differs from the display
name (Table-backed columns only -- a SQL View's own sql_output_columns
dataset-level key already covers the same fact). Not in the pinned payload
schema; verified by a real round trip (convert() then build_table()) that
this key is necessary but not yet sufficient on its own: the reverse
direction's _physical_identity prioritises the THOUGHTSPOT bracket
(display name only) and does not yet consult it, so db_column_name still
round-trips as an explicitly logged assumption (TS-FIELD-DB-COLUMN-NAME-ASSUMED)
rather than silently -- closing that consumption gap is reverse-direction
work, out of scope here.
…ree more table-build gaps
- Critical: _physical_identity now reads the field-level stash key the
forward direction writes for a Table column's true db_column_name and
prefers it over the THOUGHTSPOT bracket's display name. Previously it
derived db_column_name from the bracket unconditionally and never
called read_stash for it, so every surfaced column whose display name
differs from its warehouse name round-tripped to a Table document
naming a column that does not exist. The fallback (display name ==
db_column_name) is now used, and reported, only when the stash
genuinely carries nothing -- upgraded from INFO to WARNING, since the
consequence is a column binding that may not exist.
- Dataset-level ai_context has no home in a Table or SQL View document;
build_table now raises TS-DATASET-AI-CONTEXT-UNSUPPORTED for it
instead of dropping it with no trace.
- The new db_column_name stash key is now a shared constant
(FIELD_STASH_DB_COLUMN_NAME in constants.py) imported by both
directions, so they cannot spell it differently. Every other stash key
shared between tml_to_ossie.py and ossie_to_thoughtspot.py is still a
bare string literal independently written on each side -- roughly 15
keys (source_parts, connection_name, unsurfaced_columns,
sql_output_columns, table_properties, tml_name, table_name, tml_object,
data_type, and the relationship/model-scope keys) share the same
drift exposure. Not fixed here; flagged as a follow-up.
- Kind detection ("does this source look like a query?") no longer uses
"contains whitespace" as its first test -- that misclassified a quoted
identifier with an embedded space (SALES.PUBLIC."ORDER TABLE") as a
query, emitting an unimportable sql_view with no issue. A three-part
dotted identifier (quoted segments included) is now tried first, and
only a genuine non-identifier shape falls back to the whitespace test.
Verified with a round trip: a five-column table whose display names all
differ from their db_column_names (plus one computed formula and one
unsurfaced column), through tml_to_ossie.convert and back through
build_table, reproduces every column exactly with zero issues raised.
607 tests = 600 baseline + 7 new.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…] payload key
Closes the follow-up flagged in 35fa490: every stash-payload key besides
db_column_name (FIELD_STASH_DB_COLUMN_NAME) was a bare string literal typed
independently in tml_to_ossie.py (the writer) and ossie_to_thoughtspot.py
(the reader), or independently retyped at two call sites within
tml_to_ossie.py itself. A one-character spelling disagreement between any
of those pairs makes the reader silently find nothing -- no error, just a
value that stops round-tripping.
Mechanical refactor only, no behaviour change: 34 new constants added to
constants.py, grouped by which Ossie object each key's custom_extensions
entry attaches to (model / dataset / relationship / field-metric), matching
FIELD_STASH_DB_COLUMN_NAME's existing naming and documentation style. Every
occurrence of each key as a stash-payload dict literal is replaced with the
constant, in both converter modules and in every test that hardcodes one
(dataset_stash=/field_stash= fixtures, and read_stash()/_own_stash()
assertions) -- a test hardcoding a key string is the same drift risk, and
the place a wrong key is most likely to be enshrined as correct.
Centralized, by scope:
- Shared: STASH_TML_NAME (model + metric; defensively read at dataset scope)
- Model: unattributed_formulas, unrepresentable_joins, model_properties,
parameters, filters, column_groups, lesson_plans,
action_object_associations, constraints, model_joins_with
- Dataset: tml_object, alias, table_name, connection_name, sql_query,
source_parts (+ its db/schema/db_table sub-keys), unsurfaced_columns,
sql_output_columns, table_properties
- Relationship (also reused, unchanged, inside unrepresentable_joins[]
entries): type, cardinality, join_shape, referencing_join, on_expression,
residual_predicates
- Field/metric: data_type, column_properties (joining db_column_name)
- Metric-only: shape
Deliberately NOT centralized, checked rather than assumed:
- The shape-version key (_v) and the vendor name (VENDOR_KEY) -- already
handled: _v is encapsulated entirely inside stash.py's own
read_stash/write_stash pair (one file, adjacent lines), and VENDOR_KEY
already has a shared constant.
- "from"/"to"/"name"/"expr" inside unrepresentable_joins[]/
unattributed_formulas[] entries -- each typed at exactly one site today
(no reader exists yet for either list) and each mirrors an identically-
named core Ossie/TML schema field (Relationship.from/to,
formulas[].name/expr), so there is no independent second typing to drift
against. Left as literals; worth a second look once a Model-scope reader
is written.
- model_properties' own nested keys (is_bypass_rls, join_progressive,
spotter_config.is_spotter_enabled) and column_properties' internal
vocabulary -- copied verbatim from TML's own property names via a single
typing site (a tuple, or a fail-closed complement), not independently
retyped anywhere.
Verified by construction: grepping both converter modules for every
identified key name as a bare string literal outside constants.py returns
only genuine TML-schema reads/writes (e.g. join.get("cardinality"),
db_column_properties.data_type on an emitted Table column) -- zero
remaining stash-payload literals. Both modules import the shared constants
from the same constants.py; no key's string value is defined twice.
607 tests, unchanged from baseline -- no test was rewritten to accommodate
this change, only literal keys swapped for the constant that already
carries that value.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…t invariants Adds build_model (Ossie semantic_model -> ThoughtSpot Model TML) and to_thoughtspot_expression (dialect selection mirroring Task 4's own expression_entries, preferring a verbatim THOUGHTSPOT entry and falling back to structurally translating a catalog-matched ANSI_SQL expression). Every formula/metric gets a formulas[] + columns[] pair (R3), a metric is always a formula and never column_id + aggregation (R4, including the column_aggregation-shaped arrival case), display names are deduplicated across columns[]/formulas[] via a case-preserving allocator (R6/ID4), column_type/synonyms land under properties (R7), is_hidden/ was_auto_generated are never emitted (R8), and a brace-carrying expr is a block scalar (R9). Formula ids are derived from the normalised display name so a THOUGHTSPOT-verbatim cross-reference resolves against a formula this converter itself generates. Also promotes tml_to_ossie.py's private metric-shape constants to constants.py (METRIC_SHAPE_*) so the writer and reader share one spelling, and adds primary_key/unique_keys "unused key" detection to build_model -- a real gap found via round-tripping the construct-mapping document's own worked example. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…d ids build_model regenerates every formula's id from the normalised form of its own display name (formula_net_amount), but a THOUGHTSPOT-verbatim cross-reference elsewhere in the model was written against the source document's own id text ([formula_Net Amount]), which need not match. Left unrewritten, the reference dangles and ThoughtSpot parses it as search tokens rather than failing at parse time, so the emitted document fails import on first attempt. Fixed by deferring the R9 block-scalar wrap and doing one final pass over the fully-assembled formulas[] list, once every formula's id is known: build a normalised-name -> id map from that same list, then rewrite every embedded [formula_X] reference through it (formula._bracketed_spans, X8's identity scan already used the same way elsewhere in this package). A reference matching nothing being built is left as written and logged as an ERROR rather than silently dropped or guessed. Both the id-minting side (_formula_id_from) and the reference side (_rewrite_formula_references) share the same _normalise_or_self fold, so they cannot independently drift. Also logs an INFO issue (TS-MODEL-METRIC-AGGREGATION-CONVENTION) when a metric's surfacing column gets the conventional aggregation property set over an already-aggregating expression -- Ossie's Metric object has no way to record whether the source column carried this property, so the value is always re-derived, and a round trip built for fidelity should surface that judgment rather than making it silently. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
ThoughtSpot accepts only INNER, LEFT_OUTER, RIGHT_OUTER, OUTER for a join type and rejects both FULL OUTER and FULL_OUTER identically; OUTER is its own full outer join, so the rename is semantics-preserving and never a loss. build_model was missing this rename entirely -- a relationship stashed with type: FULL_OUTER emitted FULL_OUTER verbatim into the model's inline join, which would fail on import. Applied at both call sites that write a join type (relationships and unrepresentable_joins[]), case/whitespace-insensitively so any source spelling normalises the same way; every other type value passes through unchanged, with no issue logged for the rename since nothing is lost. This module never writes a Table joins_with[] entry (build_table has no relationship visibility to add one there), so that context needs no corresponding fix. Also closes three minors from the same review: - _formula_id_from now calls _normalise_or_self instead of repeating its try/except, so the id-minting and reference-rewriting sides cannot independently drift onto two different fold rules. - An unrecognised METRIC_STASH_SHAPE value now logs a warning and falls back to the documented default, rather than silently falling through. - Tests import the exported METRIC_SHAPE_* constants instead of hardcoding the shape strings they represent. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
R8 forbids a generated model from ever carrying is_hidden: true or was_auto_generated: true -- a hidden column cannot be surfaced again without a manual edit on the target instance, and re-asserting was_auto_generated on a column this build did not itself generate would misrepresent its provenance. Both properties reach the emitted document through the generic column_properties stash (neither is explicitly consumed by tml_to_ossie.py, so both fall through to the catch-all that preserves any property this converter did not otherwise read), and nothing filtered them back out before this fix. _drop_never_emit_true_properties removes both from a restored column_properties payload before it is merged into the emitted properties dict, at both call sites (fields and metrics). Only a true value is dropped and logged, at WARNING -- matching this module's other declared-loss severities, since a column silently losing its visibility or provenance flag is a real difference the model owner needs to see, not a benign note. A stashed false is simply omitted with no issue: false (or absent) is ThoughtSpot's own default for both properties, so leaving the key out loses nothing. The stash itself is untouched -- it is the Ossie document's own record of what the source TML held, and only the emitted TML side ever drops the flag. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… stale-stash detection Adds the public ossie_to_thoughtspot.convert() entry point (TmlConversion, mirroring tml_to_ossie.OssieConversion in reverse) and applies X5's stash-if-present-and-still-current-else-derive rule via stash.restore(), rather than plain stash-if-present, to the two constructs whose stash shadows a live, editable Ossie value: - A relationship's on_expression (+ residual predicates): witnessed by a snapshot of from_columns/to_columns taken when the stash was written. A retargeted relationship (from_columns/to_columns edited since) drops the stale condition and re-derives the plain equality join instead of silently keeping the old narrowing, with an issue recording it. - A field's stashed warehouse data_type spelling (BOOL/FLOAT): witnessed by the Ossie datatype it was recorded against. An edited datatype drops the stale spelling and re-derives the canonical one. Also fixes a real round-trip bug found by driving both public entry points back to back: a physical column referenced only by a MEASURE metric's column_id (never by any ATTRIBUTE field) was excluded from unsurfaced_columns as "already surfaced", but nothing else preserved its definition -- the reverse direction regenerated a Table missing it while the metric's own formula still referenced it, a dangling column reference in an otherwise-valid document. Fixed by keying the unsurfaced-columns computation on which columns became ATTRIBUTE fields (attribute_index) rather than on every column_id any Model column names. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
find_parameter_refs treated any bracketed name with no `::` as a runtime parameter, but a formula cross-reference ([formula_Name], R3's id form) is the same textual shape and a first-class ThoughtSpot construct, not a parameter — the model builder already recognises exactly this prefix when rewriting references. The two modules had independent, disagreeing notions of the convention. Shares one definition in formula.py (FORMULA_REFERENCE_PREFIX, is_formula_reference, find_formula_refs), which both tml_to_ossie.py and ossie_to_thoughtspot.py now read rather than each keeping its own literal. expression_entries still suppresses the portable ANSI_SQL sibling for a cross-reference — it is not portable without inlining the referenced formula, a transformation this converter does not attempt — but now says so under its own code (TS-EXPR-FORMULA-REFERENCE, INFO) instead of the false TS-EXPR-PARAM claim. An expression carrying both a cross-reference and a genuine parameter logs both, each under its own code. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…very key FIELD_STASH_DB_COLUMN_NAME was written with no witness and read with no currency check: retargeting a field's bracket reference to a different physical column silently kept the OLD warehouse name, producing a duplicate display name the emitted document would not import. Fixed with a witness (the display name db_column_name was recorded against), matching the pattern already used for FIELD_STASH_DATA_TYPE and a relationship's on_expression. Rather than stopping at that one instance, audited every stash key read on the Ossie -> TML direction and classified each in a new STASH_KEY_CLASSIFICATION table (constants.py): does it shadow a value derivable from the live Ossie document (needs a witness/currency check), or is it information that exists nowhere else (stash-if-present is correct)? A new test (test_stash_key_classification.py) fails if a key read anywhere in ossie_to_thoughtspot.py has no entry in the table, and if a SHADOWS_DERIVABLE key has no witness constant or documented self-check -- so the next key added has to declare an answer rather than default to the unsafe one. The audit found two more unwitnessed shadowing keys, both fixed the same way: STASH_TML_NAME at metric and model scope (a renamed metric/model kept serving its stale pre-rename display name -- fixed via a self-verifying check, no separate witness needed, since the stashed name's own normalised form is the comparison), and DATASET_STASH_TML_OBJECT (a dataset whose source was rewritten from a query to a table reference, or back, kept the stale table/sql_view kind -- fixed with a witness on the source string). Also: a display-name collision resolved by the allocator is now logged (naming both the original and the allocated name) instead of silent; an unattributed formula (references span two+ datasets) is now restored fully surfaced with a normal columns[] entry -- TML never required dataset attribution for a formula's surfacing entry in the first place, so nothing stopped this -- rather than re-emitted as an orphan formulas[] entry that ThoughtSpot's own visibility rule would hide, while the old issue claimed only column properties were lost; and a parameter or formula cross-reference used twice in one expression is now named once in its issue, not twice. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…l axis DATASET_STASH_UNSURFACED_COLUMNS is INFORMATION_ONLY in its per-entry content (a physical column's warehouse name/type has no Ossie-side counterpart), but that classification says nothing about whether an entry still BELONGS: a field added or retargeted onto a column that was unsurfaced when the stash was written makes that column surfaced now. Blindly restoring the stale entry alongside the live field's own build duplicated it -- a duplicate Table/SQL-View column name, which does not import. Reproduced and fixed: the reverse direction now drops any restored unsurfaced-column entry whose name collides with a column already built from a live field, silently -- the column is still present once, so there is nothing to name in an issue. The taxonomy could not express this (content-safe, membership-stale), so it gained a second, orthogonal axis: STASH_KEY_CLASSIFICATION still answers "can the value disagree with the live document", and the new STASH_KEYS_WITH_DERIVABLE_MEMBERSHIP frozenset separately answers "can a list entry be superseded by something the live document now covers". Checked every other list-shaped INFORMATION_ONLY key against the second question directly: DATASET_STASH_SQL_OUTPUT_COLUMNS is consulted only as a per-field dict lookup (never appended as a block, so a stale entry is simply never looked up, not duplicated) and does not qualify; MODEL_STASH_UNATTRIBUTED_FORMULAS and MODEL_STASH_UNREPRESENTABLE_JOINS flow through the shared display-name allocator or tolerate multiple joins without an import-breaking collision. Extended test_stash_key_classification.py with a consistency gate (a membership-derivable key must be classified INFORMATION_ONLY) and verified by construction that both it and the original completeness gate still fail closed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Adds tests/fixtures/tpcds/ (5 core datasets, 4 core relationships, 5 core metrics mirroring examples/tpcds_semantic_model.yaml exactly, plus a SQL View dataset and two extra relationships/four extra metrics covering the constructs that have broken during this build: a display name differing from its db_column_name, a YAML 1.1 boolean-token column name, a brace-carrying window formula, a formula cross-reference, a connection-specific BOOL column, a SQL View output alias differing from its column name, a physical column the Model does not surface, a non-equality join condition, a composite-key relationship, and one metric of each of the three TML shapes) and tests/fixtures/minimal/ (the smallest one-model-plus-N-tables split). Each expected.ossie.yaml was generated by running tml_to_ossie.convert() over its TML and reviewed by hand, then also independently checked against validation/validate.py and the upstream JSON schema. tests/test_fixtures.py asserts every TML fixture loads, both expected.ossie.yaml documents validate against core-spec/ossie-schema.json, converting each fixture set reproduces its expected document exactly, and the TPC-DS fixture set exercises every construct listed above. 725 -> 750 tests.
Adds `ossie-thoughtspot to-ossie`/`to-tml` (argparse only) and the `[project.scripts]` entry point, now that `cli:main` exists to point it at. Issues are always emitted as a JSON array -- to `--issues` when given, to stderr otherwise -- and never mixed with document output; exit code is tied to IssueLog.has_errors(), not to the mere presence of a warning/info issue. `to-tml` writes one file per document via `tml.dump_document_set` (tables before the model) into an output directory, with a resolve-then-compare containment check as a second, independent guard against a document name escaping that directory. Neither direction overwrites an existing target without `--force`, and a conflict on any one target refuses the whole run before any file is written.
… and translation separately
…icating a field's description onto its Table column Ossie -> TML -> Ossie renamed a Table-referenced relationship once its join round-tripped through TML's nameless inline join syntax, because the stashed original name was written but never read on the way out. _join_entry_for_relationship now restores the referencing_join pointer and the Table's own joins_with[] entry from that stash, with a currency check against the relationship's live name so a renamed relationship falls back to the old behaviour instead of restoring a stale reference. _physical_table_column/_physical_sql_view_column no longer copy a field's description onto its physical Table column -- every field reaching those functions is already Model-surfaced, and the Model columns[] entry is the description's only correct home.
…iers Adds a Hypothesis-driven Ossie -> TML -> Ossie property suite that draws dataset and field names from a pool deliberately including YAML 1.1 boolean tokens, names colliding only after normalise(), NFKD-foldable and non-foldable non-ASCII, "::"-containing names, whitespace padding, punctuation-only text, very long text, and the empty string. Each document also crosses the real YAML 1.2 codec on both legs, not just the pure conversion functions. Two real, previously uncaught ValueError crashes were found and fixed: - ossie_to_thoughtspot.py's _physical_identity/_field_physical_display_name let an ambiguous [TABLE::Column] bracket's split_column_ref failure propagate out of a document conversion uncaught; now caught, reported as TS-FIELD-COLUMN-REF-MALFORMED, and the field is treated as a formula rather than crashing the whole conversion. - tml_to_ossie.py's convert() (the model's own top-level name) and _index_attribute_columns() called identifiers.normalise() unguarded on a display name with no ASCII alphanumerics to fold onto; now caught, with a reported TS-MODEL-NAME-UNNORMALISABLE fallback for the former and a silent skip (already re-reported downstream by convert()'s own Phase 3 guard) for the latter. hypothesis is dev-group only (never in [project.dependencies]), matching converters/databricks' own placement. 793 baseline + 5 targeted regression tests for the two fixes + 3 property tests = 801 passing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…verter Rewrites the converter README to document both directions, the CLI, the custom_extensions[THOUGHTSPOT] payload, and a dedicated section on what expression translation does not attempt and why (the specification's own pass-through default, no converter in this repo re-renders one SQL dialect into another, and the reference converter behaves the same way). Cleanup for public ASF review: de-identifies ~18 provenance comments that named the internal test instance (se-thoughtspot) while keeping the verification dates and evidence; trims the catalog.py module docstring and several narrative note= entries toward what a maintainer needs. Removes two custom_extensions stash keys that were written and never read (sql_query, which duplicates the live source field; residual_predicates, already fully contained in the verbatim on_expression) -- the payload shape version is not bumped, since no reader ever required either key's presence. Verified pyproject.toml's [project.scripts] entry and hypothesis test extra were already correct; regenerated uv.lock (no changes). Added the THOUGHTSPOT row to converters/README.md and core-spec/spec.md's custom_extensions vendor table. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… references from code The expression mapping, reverse inventory, datatype map and vendor payload were originally hand-authored design documents. The code now implements that mapping, so it is the single source of truth: tools/generate_reference_docs.py reads CATALOG, REVERSE, datatypes.py and constants.py's STASH_KEY_CLASSIFICATION back out into four committed docs/*.md files, and tests/test_reference_docs_current.py regenerates on every run and compares byte-for-byte so a stale doc fails the suite instead of silently drifting. The generator is dev/tooling only (stdlib + this package's own modules, no new dependency, not a wheel package, not a console-script entry point) so the package's only runtime dependency stays PyYAML. test_shipped_references.py now also scans docs/**/*.md and tools/**/*.py for internal-process language and unresolvable rule-id citations, same as every other shipped file.
…p the label The converter's comments, docstrings, and a handful of issue codes cited short rule identifiers (E1-E13, X1-X9, R1-R11, ID1-ID4, A1-A12, KD1-KD3, NM1-NM6, G1-G15) from a hand-authored design reference that is not part of this repository and will not be. Every citation of that shape has been rewritten in place to state what the rule actually requires -- most were already fully explained by their surrounding sentence and just needed the label dropped; a handful (the moving_*/cumulative_* partition loss, LAG's aggregation/ORDER BY constraints, and write_stash's foreign-vendor scenario) had real reasoning added back in. A small number of internal issue codes (`E7-PASSTHROUGH`, `E12-UNMAPPABLE`, `E10-DAYOFWEEK-BASE`, and 12 siblings in reverse.py/emit.py) also embedded the citation as data, not prose. These are renamed to the `TS-EXPR-*` convention every other issue code in this converter already uses -- no test asserts the old literal values, so this is a safe alignment, not a behavioural change. docs/*.md are regenerated from the now-citation-free source via tools/generate_reference_docs.py. README.md's "Rules" section is rewritten to reflect that the shipping decision is resolved rather than pending. The seven now-empty families are removed from test_shipped_references.py's provisional allowlist, so the guard fails closed on any of them reappearing; the "I" (invariant) family is untouched -- resolving it is a separate, unscoped decision. Verified the guard actually fires by temporarily reintroducing one citation and confirming the suite failed before reverting it. 804 tests before, 804 after -- no behaviour change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The previous cleanup rewrote citations to eight rule-id families (A/E/G/ID/KD/NM/R/X) in place and removed them from MAPPING_DOC_RULE_IDS, but left the "I" (invariant) family as an explicitly provisional exception -- I1/I4/I5 appeared 5 times in tests/test_keys.py and tests/test_yaml.py. Those 5 comments already stated the substance of the rule inline, so the fix is the same as before: drop the label, keep the sentence (capitalizing the following word where the label led the sentence). MAPPING_DOC_RULE_IDS is now permanently empty. The name and its two guard tests (test_allowed_token_sets_do_not_overlap, test_no_unresolvable_identifier_shaped_tokens) stay -- deleting them would drop the test count below 804 -- but the surrounding comments no longer describe anything as provisional or pending, and say plainly that repopulating the set needs a fresh, reasoned decision, not a silent addition. Verified the guard still bites: reintroduced "I5" into tests/test_yaml.py, confirmed test_no_unresolvable_identifier_shaped_tokens failed naming that exact token and line, then reverted. A follow-up sweep -- importing the module and running its own TOKEN_SHAPE_RE across its own _shipped_files(), minus only ALLOWED_TOKENS -- found no further unnamed families. 804 tests before and after -- no behaviour change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
jbonofre
self-requested a review
September 7, 2026 11:43
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds
converters/thoughtspot/— a bidirectional converter between ThoughtSpot's semantic model format (Model TML plus its Table and SQL View documents) and the Ossie semantic model.Proposed in #285 (consolidating #269);
THOUGHTSPOTwas added to theDialectenum in #351.Scope
datasetsmodel_tables[]+ a Table or SQL View documentfieldscolumns[]withcolumn_type: ATTRIBUTEmetricscolumns[]withcolumn_type: MEASURE, always viaformulas[]relationshipsjoins_with[]custom_extensions[THOUGHTSPOT]One structural note that shapes the whole design: one Ossie semantic model corresponds to 1 + N TML documents, not one file. The converter reads and writes the set.
Expression handling — the design decision most likely to draw questions
ThoughtSpot's formula language is not SQL. It has its own syntax (
concat ( [a] , [b] ),[TABLE::Column]references,{ }grouping) which no SQL dialect parses.A ThoughtSpot formula is carried verbatim under a
THOUGHTSPOTdialect entry. A portableANSI_SQLsibling is added only where the whole expression is a bare column reference — the one shape where portability is certain. No SQL dialect is ever re-rendered into another, in either direction.That is deliberate, and the reasoning is in
converters/thoughtspot/README.md:dialects[]exists precisely so an untranslatable expression can still travel tagged with the dialect it is valid in.expressions/catalog.pymaps all 146 constructs the Ossie expression language defines to a ThoughtSpot rendering (108 direct, 37 viasql_*passthrough, 1 unmappable) and is used forOssie → TML.expressions/reverse.pyrecords which ThoughtSpot-native functions compose into a portable Ossie expression; it is present for testing and future use and is not yet wired into the shippedTML → Ossiepath — the README says so explicitly rather than leaving it to be inferred.Reference documentation is generated from the code
converters/thoughtspot/docs/is produced bytools/generate_reference_docs.pyfromCATALOG, the reverse inventory, the datatype map and the vendor-payload key classification. A test compares the committed documents byte-for-byte against fresh generator output, so they cannot drift from the implementation.Testing
804 tests, offline, on Python 3.10–3.14.
hypothesis, test-only) over deliberately awkward identifiers — YAML 1.1 boolean tokens, names colliding only after normalisation, non-ASCII, names containing::. This found two crashes the example fixtures never produced.examples/tpcds_semantic_model.yamlso this converter is comparable to its siblings; both expected documents validate againstcore-spec/ossie-schema.jsonandvalidation/validate.py.Dependencies
PyYAML>=6.0and nothing else at runtime.hypothesisandjsonschemaare test-only.Provenance
All TML fixtures and all code are original, authored for this contribution. Nothing derives from ThoughtSpot's
thoughtspot_tmllibrary or any other existing converter.One question for reviewers
The relationship-level mapping and the key-derivation rule admit two readings of whether a
ONE_TO_MANYcardinality inverts a relationship's orientation. The implementation keepsfrom/tostructural and flips only the key-derivation candidate, which satisfiesvalidation/validate.py. I would rather have that settled by someone who owns the specification than guess and encode the guess in tests.Note on history
81 commits, conventional-commit style. Happy to squash to a smaller set of logical commits if that is preferred for review.