feat(thoughtspot): bidirectional ThoughtSpot TML <-> Ossie converter - #364
Conversation
56bb50a to
e4ee68d
Compare
|
Rebased onto current One thing I cannot do from my side: no CI has run on this PR since it was opened. I believe |
d905a3f to
0803a4d
Compare
|
i've Rebased onto current 823 tests pass on 3.10–3.14, offline. Both fixtures and the emitted output validate against the current Two things I can't do from this side:
Could a committer approve CI and take a look? |
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).
… to itself
`build_model` read the stashed dataset alias with no currency check, while
`column_id` and every join target were built from the LIVE dataset name. So
renaming a dataset emitted a model whose `model_tables[]` carried the old alias
and whose references named the new one -- pointing at nothing:
model_tables: [{name: EMPLOYEES, alias: emp}, {name: EMPLOYEES, alias: mgr}]
column_ids: ['emp::employee_id', 'manager::employee_name'] <- dangles
issues: nothing about it
The alias is SELF-VERIFYING: `tml_to_ossie._build_dataset` writes the Ossie
dataset's `name` from it, so `stashed == dataset["name"]` is the whole check --
the same shape `RELATIONSHIP_STASH_REFERENCING_JOIN` already uses. When they
disagree the live name wins and the divergence is reported.
The root cause was a misclassification, so the classification is corrected too:
`DATASET_STASH_ALIAS` was INFORMATION_ONLY, defined as "no Ossie-native
counterpart at all ... nothing there could have diverged from it". It has one,
and it diverges. It is SHADOWS_DERIVABLE now, listed among the self-verifying
keys, which is what makes the enforcement table agree with the code again.
Mutation-checked. 884 tests pass.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Showing the same date as "Sale Date" and, with a format pattern, as
"Sale Month" is ordinary modelling, but it is still ONE physical column. Both
body builders appended an entry per field, so:
sql_view_columns:
- {name: d, sql_output_column: d} <- alias GUESSED and lowercased
- {name: d, sql_output_column: D} <- duplicate name
- {name: amt, sql_output_column: AMT}
Two defects in one. The duplicate entry, and -- worse -- a corrupted alias: only
one of the two fields carried the stashed `sql_output_column`, so the other had
one derived from its display name. Lowercased, it binds to nothing on a
case-sensitive warehouse. Both were silent.
Entries are now folded by column name. Where the two agree, the first stands and
nothing is reported -- they are the same column. Where they disagree, a value the
SOURCE document recorded beats one this converter guessed, so the stashed alias
wins; a disagreement between two equally-trusted values is reported rather than
resolved by position.
Mutation-checked. 886 tests pass.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…Spot has not got
`_DisplayNameAllocator` assigns TML DISPLAY NAMES, but folded each candidate
through `identifiers.normalise` -- an OSSIE IDENTIFIER rule, which lowercases,
strips punctuation and drops non-ASCII after NFKD. Applied to the wrong side of
the converter it invented collisions between names that are perfectly distinct
in ThoughtSpot:
Order Amount / Order-Amount both folded to order_amount
Cafe / Café both folded to cafe
収益 2024 / 売上 2024 both folded to n_2024
One of each pair was renamed to `<name>_2`, and the issue explaining it asserted
"to satisfy ThoughtSpot's uniqueness requirement" -- a requirement that does not
apply, since those are different strings to ThoughtSpot. The allocator is also
model-wide while the forward leg de-collides fields per dataset, so two fields
in DIFFERENT datasets could trigger it.
The fold is now a plain casefold: case is the only thing ThoughtSpot ignores.
Strictly narrower, so it renames less, and everything it still renames is a
genuine collision.
The property test that covered this generated punctuation variants and asserted
they collide, pinning the defect. It now generates only case and whitespace
variants, and a new property pins the other direction -- that punctuation
variants are left alone.
Mutation-checked. 888 tests pass.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… not during
`_FISCAL_CAPABLE_FUNCTIONS` was a module-level constant evaluated partway down
`reverse.py` -- while `REVERSE` kept being populated for another hundred lines
below it. Twelve date functions registered after that point were therefore
excluded from the gate, and a fiscal call on one of them raised an uncaught
`ValueError` where it had previously reported a declared loss:
month, year_name, day_of_week, month_number_of_quarter,
day_number_of_quarter, week_number_of_month, week_number_of_quarter,
is_weekend, start_of_hour, start_of_min, date
Deriving the set was the right instinct -- the hand-list it replaced was
already missing two names its own tests exercised -- but deriving it before its
source existed made it worse, not better. It is computed on first call now.
The token match was also unbounded, so `min(?:ute)?` matched the AGGREGATE
`min`, taking `min_if`, `cumulative_min` and `moving_min` with it, and `time`
matched inside "runtime". All four were classed fiscal-capable, which is the
same over-inclusion the gate exists to prevent. Tokens are word-shaped now,
with `start_of_*`/`end_of_*` admitted by prefix since `start_of_min`
abbreviates "minute" to a string that cannot be a token without re-admitting
the aggregate.
The old test used `year`, `quarter_number` and `diff_months` -- all registered
BEFORE the constant, so it was blind to the ordering by construction. The new
one is parametrised from late-registered names for that reason.
Mutation-checked: pinning the set to an early-only source fails 22 tests.
913 tests pass.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The existing hypothesis properties generate adversarial NAMES -- unicode, display-name collisions, warehouse names. Every structural defect found in review was a SHAPE: a table aliased to itself, two datasets over one SQL View, a parenthesised join condition, case-colliding document names. A name generator cannot reach any of them, which is why reviewers kept finding what 800-odd tests did not. This generator varies dataset count, table vs SQL View, aliasing several datasets onto one source, relationships (including composite), already -aggregated metrics, and case-varying source names. What it asserts is the package's own stated contract, which holds over any shape: nothing is silently dropped -- every field survives or an issue names it -- no two documents would be written to one path, and a declared relationship survives or is reported. It earned its place on the first run, twice over. It surfaced that an Ossie relationship's NAME is silently lost for an inline join (TML joins carry no name, so the return leg re-derives `<from>_to_<to>`; only the "referencing" shape stashes one). And it caught a bad assertion of my own: the property originally compared relationships by name, which is too strict for exactly that reason -- it now compares structure. 6,000 examples across three properties, all passing. 917 tests total. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…lf-join Three findings from a review of the previous nine commits. Two are regressions those commits introduced — the pattern this round was meant to break. DUPLICATE `formulas[].id`, EMITTED SILENTLY. `formulas[].id` has two sources sharing one namespace, and neither checked the other: an id PRESERVED from the source stash, and one MINTED from the display name. Narrowing `_DisplayNameAllocator`'s fold to what ThoughtSpot actually treats as equal -- correct in itself — removed the only thing that had been masking the minted case, so "Order Amount" and "Order-Amount" both emitted `formula_order_amount` with no issue. A hand-authored metric minting an id equal to a preserved one collided the same way. Either makes every `[formula_X]` reference ambiguous, and ThoughtSpot parses an ambiguous bracket reference as search tokens rather than failing — so the import succeeds and the model is wrong. Ids now come from one allocator across both sources, the surfacing column's `formula_id` is rewritten in lockstep, and a rename is reported. AN ORDINARY SELF-JOIN NOW FAILED. The column-conflict check added last commit compared whole entries, but a Table's `columns[]` mixes field-derived entries (name / db_column_name / db_column_properties) with verbatim `unsurfaced_columns` stash entries carrying raw TML keys. A column surfaced through one alias and unsurfaced through the other compared unequal: ERROR, non-zero exit, on a document that was fine — and one that converted cleanly before the fix. Only the keys that decide WHICH warehouse column is read are compared now, which is what the message always claimed. THE CLI CASEFOLD FIX HAD NO TEST. `-o out.yaml --issues OUT.YAML` was pinned by nothing, so the guard could regress silently. Its sibling in `dump_document_set` was pinned; this one was missed by the commit that went looking for exactly this. All three mutation-checked. 923 tests pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ck of order
Three findings from a review of the previous commit, two of them defects that
commit introduced. Fifth consecutive round in which the fixes carried new
defects; recording that plainly rather than describing this as converged.
A PRESERVED ID WAS DECIDED BY PROCESSING ORDER. `_allocate_formula_id` renamed
whichever formula it reached second, so a preserved id -- the identity a source
cross-reference was written against -- could be handed to a newly minted one.
The field loop runs before the metric loop, so adding an ordinary formula field
to a round-tripped document was enough:
formula_margin Margin [t::a] + 1 <- the new field took it
formula_margin_2 Net Margin sum ( [t::a] ) <- the preserved one renamed
Double Net = 2 * [formula_margin] <- now 2 x the wrong formula
Preserved ids are reserved in a pass before any minting, and are never renamed.
The test that was meant to cover this asserted `ids[0] == "formula_margin"` and
passed only because the preserved metric happened to be listed first; it now
checks both orderings.
THE THIRD SOURCE OF IDS BYPASSED THE ALLOCATOR. A formula spanning two datasets
is stashed as name+expr only -- its id dropped -- and re-minted on return. That
loop appended directly, so a pure round trip of a VALID document still emitted
duplicate ids, silently. The allocator's own docstring said ids come from "two
places".
A TEST SURVIVED ITS FIX BEING REVERTED. `test_the_surfacing_column_follows_the_
renamed_id` asserted `referenced <= emitted`, which cannot see the failure: when
the column keeps the old id, that id is still emitted by the OTHER formula, so
the subset holds while the column surfaces the wrong expression. It asserts the
pair now.
Also widened `_COLUMN_BINDING_KEYS` to include `db_column_properties`: narrowing
it to fix the self-join false positive went further than needed and made a
datatype disagreement -- which binds a field to the wrong data just as surely as
a wrong column name -- completely silent. Confirmed the wider key does not
re-break the self-join.
All four mutation-checked, including the one that previously survived.
926 tests pass.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Found by importing a converted REAL model into a live ThoughtSpot cluster.
`with` is mandatory on every `model_tables[].joins[]` entry. The referencing
branch emitted only `referencing_join` — but that pointer names the Table's
`joins_with[]` entry, it does not replace the join target:
emitted {"referencing_join": "C_DIM_RETAPP_PRODUCTS"}
real TML {"with": "DIM_RETAPP_PRODUCTS", "referencing_join": "C_DIM_RETAPP_PRODUCTS"}
ThoughtSpot: "Compulsory Field worksheet->model_tables(2nd)->joins(1st)->with
is not populated."
Nothing caught this, and the reason matters more than the defect. The document
passes the Ossie schema. It passes upstream's `validation/validate.py`. Both
validate the OSSIE side, and the missing field is on the ThoughtSpot side. And
the round-trip tests compared the converter's output against HAND-AUTHORED
FIXTURES THAT OMITTED `with` TOO — so code and tests shared one
misunderstanding and agreed with each other. 926 tests passed throughout.
Both fixtures now carry `with`, matching what a real export contains, and a new
test asserts that every emitted joins[] entry has one, over every fixture rather
than at two hardcoded sites.
Caught on the first real model imported. Structural round-tripping of 30 real
models -- 161 tables, 1,471 columns, 146 joins, all passing upstream's validator
-- did not surface it, because no amount of Ossie-side validation can.
927 tests pass.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`guid`, `fqn` and `obj_id` were grouped together as "instance-local identity never travels in a portable document". They are not the same kind of thing. `guid` is a raw cluster UUID. `fqn` is a reference to one -- and this repo's own schema reference already records that a viz-level `fqn` is DROPPED on import, leaving the object with no data source. Both are correctly refused. `obj_id` is the opposite: ThoughtSpot introduced it precisely so objects can be referenced across environments, and it survives import. It is a readable handle -- `SampleRetail-Apparel-LH-58435d2b`, the display name plus the GUID's first segment -- not a bare identifier. Discarding it meant a converted model re-imported as a NEW object beside the one it came from, rather than updating it, which breaks the promote-between-environments workflow this converter exists to serve. It is now stashed under its own payload key and restored at the document root. The forbidden-key scan still names `obj_id`, deliberately: that scan stops one riding along unnoticed inside a block copied wholesale from source TML, while the stash preserves it on purpose, under a distinct key. The two are not in conflict and the comment now says so. `guid` is still stripped at every depth, and the test asserts that. Observed on a real export while investigating why a converted model could not be re-imported: the source carried `obj_id: SampleRetail-Apparel-LH-58435d2b` and the converted document carried nothing. Mutation-checked. 928 tests pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Not every already-aggregating formula makes the surfacing column's
`aggregation` inert, and the converter treated them as if it did.
Established by domain review and confirmed against a live cluster:
sum ( ... ) column aggregation is a NO-OP
group_sum / group_average ( ... ) NO-OP -- these behave like ordinary formulas
sum ( group_aggregate ( ... ) ) NO-OP -- wrapped, so the outer call rules
group_aggregate ( ... ) BARE USED -- like a raw column, ThoughtSpot may
apply the column's aggregation to it
The code grouped all four as "the documented no-op", naming
`group_aggregate ( ... )` in that comment explicitly, and discarded the value
with nothing logged -- deliberately, since warning on the genuinely inert
shapes would train readers to ignore the issue log. So the one shape where the
property is load-bearing lost it silently: a changed answer, not a changed
spelling.
Ossie's metric expression has nowhere to put it, so it is preserved verbatim in
the stash and restored, rather than discarded or folded into the expression --
folding would change what the formula means, since wrapping a bare
`group_aggregate` is exactly what makes the property inert.
Also recorded, because it was nearly acted on as a defect: `sum([SALES])` with
`aggregation: SUM` is genuinely a no-op. A converted model carrying both was
imported into a live cluster and returned numbers identical to its source. An
earlier reading of a `NESTED_AGGREGATE_NOT_SUPPORTED` error as a converter
defect was wrong -- the nesting was in the QUERY, which wrapped an
already-aggregated formula in SUM().
Mutation-checked. 932 tests pass.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…m hidden
A model's `formulas[]` may hold entries no `columns[]` entry references. Those
are HIDDEN in the ThoughtSpot UI, and the usual reason to write one is that
another formula uses it -- a date-parameter model is the common case, where
`_startDate`/`_endDate` compute a window the visible formulas then apply.
The converter walks `columns[]`, so a formula no column surfaces was never
visited and was dropped outright. Even the unattributed-formula stash could not
catch them: that only holds formulas that WERE visited and could not be
attributed to a dataset.
On the real model that surfaced this, 32 of 41 formulas were lost, and the 9
visible ones referencing them came back with dangling `[formula__startDate]`
references. Round-tripping it now:
dangling references 4 -> NONE
reverse-leg ERRORs 4 -> none (the run exited 1 before)
surfacing columns 45 -> 45 unchanged
They are stashed with their `id` -- that is what the surviving references name,
and re-minting an id from the display name is exactly how the references came
to point at nothing -- and restored as `formulas[]` entries with NO surfacing
column. Giving them one, as the unattributed-formula path deliberately does,
would make a helper the modeller kept private visible to users.
Mutation-checked. 936 tests pass.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Measured across the 30-real-model corpus: 1,473 warnings and errors before, 622 after. None of the difference is a check being weakened -- both codes fired on paths where nothing is wrong, which is exactly how an issue log stops being read, a risk this package's own comments name elsewhere. TS-FIELD-DB-COLUMN-NAME-ASSUMED, 593 -> 92. It said "assumed" about something the document establishes: the forward direction stashes `db_column_name` ONLY when the warehouse name differs from the display name, so its ABSENCE means they agreed. For a field this converter produced that is a recorded fact, not a guess. It remains a real assumption -- and still reports -- for a field with no ThoughtSpot stash at all, where nothing ever recorded the relationship. TS-EXPR-UNRESOLVED, WARNING -> INFO (346 occurrences). A formula may reference any column of a joined table, including one the model does not surface as a column of its own; that is ordinary modelling, and there is then no Ossie field for a portable ANSI_SQL sibling to name. Nothing is lost -- the THOUGHTSPOT dialect entry carries the expression verbatim -- and it is the same fact TS-EXPR-THOUGHTSPOT-ONLY already records at INFO, so the WARNING was inconsistent as well as noisy. The message now says what is and is not affected. Re-running the corpus also confirms the unsurfaced-formula fix: models losing structure 1 -> 0, and 30/30 still pass upstream's validator. 936 tests pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Your two reviews prompted a much wider pass. Seventeen commits since, and the most useful Round-tripped 30 real customer models (161 tables, 1,471 columns, 202 formulas, 146 joins), Then imported one back into the cluster and compared the numbers. A three-table joined That exercise found four defects that five rounds of adversarial review and 900+ tests had all
Also fixed since your review: a formula cross-reference rebinding to a different formula; Two things worth saying plainly. Several of these defects were asserted by our own tests — CI needs approving again on the new head. |
…gate
The exemplar gate added earlier catches a baked-in NUMBER or QUOTED STRING in a
passthrough template. It cannot see a baked-in AGGREGATE, because that is a bare
keyword. Five rows were in that state:
DISTINCT aggregate modifier SUM(DISTINCT {0})
DENSE_RANK() OVER (...) ... order by sum({0}) desc
NTILE(n) OVER (...) ... ORDER BY SUM({0})
CUME_DIST() OVER (...) ... ORDER BY SUM({0})
Window aggregation -- AGG(expr) SUM({0}) OVER (...)
In each the aggregate is illustrative, exactly as NTILE's literal 4 is: the
specification orders by whatever the caller chose. The generated document
presented `SUM` as the mapping, with no marker, for a row whose own spec_name
says `AGG(expr)`. Two of these rows already SAID so in their prose note -- the
Window aggregation note calls its frame "an exemplar, the same convention as
NTILE's literal 4" -- while the machine-readable declaration the document is
built from did not, so the note and the table disagreed.
NTILE also shows why a non-empty declaration is not enough on its own: it
declared `n`, satisfied the literal gate, and left its SUM unmentioned. The new
check therefore requires that some declared exemplar NAMES an aggregate, not
merely that the tuple is non-empty.
LAG/LEAD, separately: both declared `default` as an exemplar literal while their
template had no default argument at all -- a declaration describing something
absent rather than something illustrative. The template now shows the third
argument, so the declaration describes the template.
`baked_literals`, `baked_aggregates` and `declares_an_aggregate_exemplar` are
now module-level in `_types.py`, so the construction-time gate and the two
catalog-wide tests apply one rule rather than three copies of it.
Verified by mutation, both halves, since the guard lives in two places:
undeclaring NTILE's aggregate raises at catalog construction; with the gate
neutralised as well, the new test still fails on it.
No emitted output changes: no exemplar row is reachable from the conversion
path, because `_match_ansi_call` matches only single-argument `NAME(expr)` keys
and `COUNT(DISTINCT expr)`. These rows are the reference document's content.
937 tests pass.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The README opens its expression section with four bare numbers -- "all 146 constructs ... 108 with a native equivalent, 37 as a `sql_*_op` pass-through ... and 1 (`EXISTS_IN()`)". Nothing derived them from the catalog, so adding or reclassifying a row left the sentence quietly wrong. They are correct today (146 = 108 + 37 + 1, checked); this stops them drifting. Each count is asserted PRESENT before it is compared, so rewording the sentence fails the test rather than silently leaving the counts unchecked -- the fail-open shape a guard like this otherwise takes. Mutation-checked both ways: changing 108 to 109 fails on the value, replacing "108" with "one hundred and eight" fails on the absence. 938 tests pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`formulas[].id` has four sources sharing one namespace: preserved from a
field/metric stash, preserved from the model-scope unsurfaced-formula stash,
minted from a display name, and minted for an unattributed formula. The opening
sweep that reserves preserved ids before any minting read only the first. The
unsurfaced ids were therefore never reserved -- `_allocate_formula_id` returns
early for a preserved id and never adds it, and the unsurfaced loop runs after
both minting loops -- so a minted id could take one.
Reproduced, not reasoned about. A document carrying a hidden helper
`formula_revenue` plus a field named "Revenue" (which mints exactly that slug):
emitted formula ids: ['formula_revenue', 'formula_revenue']
collision issue raised? False
Two `formulas[]` entries, one id, no issue, exit 0. Every `[formula_revenue]`
reference in the model is then ambiguous, and ThoughtSpot parses an ambiguous
bracket reference as search tokens rather than failing -- so it imports and
means something else. This is the exact failure `_allocate_formula_id` exists to
prevent; it was simply not being told about one of the four sources.
Reachable by hand-editing the Ossie document, which is the point of a portable
format. A pure round trip does not hit it: there every id is preserved from the
one source document, where they were already unique.
After: the minted id moves to `formula_revenue_2`, the PRESERVED id keeps
`formula_revenue` (it is the identity the surviving cross-references name), and
TS-MODEL-FORMULA-ID-COLLISION is raised.
Found by an audit of what this package's comments claim against what its code
does -- the stale comment ("all THREE sources") and the defect were the same
mistake written down twice.
Mutation-checked: reverting the reservation fails all three new tests. A second,
belt-and-braces `taken.add` in the preserved branch was written first and then
removed -- reverting it alone changed nothing, so it was redundant with the
sweep and would have shipped as an uncovered line.
941 tests pass.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
An audit for tests whose every assertion sits inside a loop, verified by
breaking the thing each one guards. Six were vacuous; the mutation each one
survived is recorded beside it.
The serious one: `test_the_surfacing_column_follows_the_renamed_id` walked
`columns[]` and skipped entries with no `formula_id`. Deleting the lockstep
assignment outright -- so every RENAMED formula's column lost the link that
this project's own TML invariants call compulsory -- left the FULL SUITE green,
936 passed, nothing else caught it either. It now iterates `formulas[]` and
requires each to produce its surfacing column, so an absent key fails.
`test_shipped_references` accumulated eight globs into one list and asserted
only that the total was non-empty -- which `tests/**/*.py` guarantees by
matching the test module itself, so the guard could not fire at all. Renaming
`docs/` passed; moving `src/`, `tools/`, `docs/`, `README.md` and
`pyproject.toml` together also passed, with the non-test shipped surface empty.
Each required pattern is now asserted to match.
`test_every_stash_key_constant_is_a_real_constants_attribute` was written
against exactly this ("a typo in the regex would otherwise silently check
nothing") and had it: breaking the regex made the scan return nothing and the
test passed, while three siblings failed instead.
`test_every_shadows_derivable_key_has_a_witness_constant...` enforced the
witness rule on nothing when no key was SHADOWS_DERIVABLE. Reclassifying them
all failed only the generated-docs check -- so following that failure's own
remedy (regenerate) reached a green suite with the rule gone.
`test_stays_inside_the_output_directory` is a path-traversal guard that proved
nothing when its input vanished: making `dump_document_set` skip
traversal-shaped names instead of sanitising them passed all six malicious
cases. It now asserts both documents are emitted first.
`test_every_referencing_join_carries_the_compulsory_with_field` had three `or
[]` defaults in a row, so emitting no joins at all passed. Now counted.
Two in `test_datatypes.py` walked `OSSIE_DATATYPES`; emptying it passed both.
Every one re-checked against the mutation that defeated it. No production
behaviour changes here -- the surfacing-column defect the first mutation
simulates does not exist in the code, it was simply untested.
941 tests pass.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…uments use
`docs/expression-mapping.md` used four brace notations and explained none of
them: 138 `{0}`-style positional slots, five `{*}` variadic tails, six escaped
`{{`/`}}` pairs, and a dozen literal `{ [attr] }` ThoughtSpot sets. The last
three matter most because they LOOK alike and do unrelated jobs -- `{{ [T::date] }}`
renders as `{ [T::date] }`, which is not a placeholder at all -- and a reader had
no way to tell them apart. `docs/reverse-inventory.md` had the same gap for its
61 positional slots. The meanings existed only in source comments the reader of
a generated document never sees.
Both documents now open with a "Reading the notation" table, which also covers
the two non-brace conventions: the `per-... — see note` dispatch cells and the
`**example only**` exemplar marker.
The legend is DERIVED from the rendered body and applied centrally in
`generate_all()`, not declared per document. So it lists exactly the notations
that document uses -- `datatype-map.md` and `vendor-payload.md` use none and
correctly get no legend -- and a document that grows its first `{0}` gets the
row without anyone remembering.
The test asserts both directions, plus that the detectors matched something at
all. That vacuity guard earned itself immediately: the first version of this
test PASSED when the `{*}` row's key was renamed, because the detector half and
the row half drifted apart while each stayed internally consistent. The two are
now checked against each other where both are in hand, and that mutation fails.
942 tests pass.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…rongly
An audit of every checkable claim in the package's comments and docstrings
against the code beside it. Thirteen were wrong. No behaviour changes; the code
was right in all thirteen and the prose was not.
The one most likely to mislead: `catalog.py` states the trigonometry conversion
directions BACKWARDS -- "SIN/COS/TAN convert degrees->radians on the way in
(`* 180 / pi`)" when `* 180 / pi` is radians->degrees, and the file's own
DEGREES/RADIANS rows two hundred lines later say exactly that. The comment even
warns the two are "easy to transpose by mistake".
Counts that had drifted:
- "the ten `sql_*_op` names" (x3 in reverse.py) -- eleven are registered; the
ten matches the `Variant` enum, which has no `sql_date_op`.
- "one of the two places this converter uses a witness copy" -- five.
- `_AGGREGATE_CALL_NAMES` "plus `group_aggregate`" -- plus nine `group_*`
names. This is the same "there is exactly one such construct" error that the
comment fourteen lines above it records as a fixed bug.
- `tml.py`'s "Four invariants live here so no caller has to carry them" --
three are enforced here; the block-scalar rule is only SUPPORTED here and is
applied by `ossie_to_thoughtspot._maybe_block_scalar`, so it is precisely the
one a caller can still get wrong.
Enumerations that name things they do not cover:
- `emit.py`'s PARTITION BY guard listed eight rows; four carry PARTITION BY in
a template. The OVER row is a dispatch string, CUME_DIST has none, and
RANK/PERCENT_RANK are DIRECT rows the guard never sees -- their fallbacks
live in prose.
- `_forward_call_names` claims "every ThoughtSpot call name the FORWARD
catalog renders"; its regex is anchored at the template start and excludes
digits, so it misses `rank_percentile`, `asin`/`acos`/`atan`, `log10` and
every nested call. 50 of them. Harmless for its one consumer, and the
docstring now says what it actually returns.
Claims contradicted by a later change:
- Two docstrings say `_DisplayNameAllocator` folds the way they do. It folds on
a plain casefold and never calls `identifiers.normalise` -- deliberately, and
its own class docstring says so. Stale since the commit that stopped this
converter inventing collisions ThoughtSpot has not got.
- `tml_to_ossie.convert` says the resolver needs "every ATTRIBUTE column's
identifier"; the index deliberately stores membership only, as the function
building it documents at length.
- `stash.py` names the CLI's handler as `(ConversionError, OSError)`; it is
three-wide.
- `keys.py` cited `validation/validate.py:159-165`, which is an unrelated
list-duplicate helper. Now named by function, not line, since that is what
drifted.
- `identifiers.Allocator`'s docstring credits its case-insensitivity to the
`.casefold()` in `allocate`. Every candidate is already lowercase by then
(`normalise` lowercases), so that call is a no-op on this path; `normalise`
is what delivers the guarantee.
942 tests pass; the generated reference documents are byte-identical, as
comment-only edits should leave them.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…satisfied The remainder of the fail-open sweep. These were lower priority than the six in cc5db0e because a sibling test happened to fail on the same mutation -- but "another test noticed" is not the same as this test working, and the sibling can be deleted or narrowed later without anyone seeing the cover disappear. `test_every_table_or_sql_view_document_is_present` compares two sets, and `set() == set()` holds, so a fixture with no table documents satisfied it and the two sibling tests that walk the same collection. Pinned once for all three. `test_column_type_is_never_a_bare_root_key` reads columns through a helper that ends `or []`, so a model emitting no columns passed without checking one. Not changed, deliberately: the ~16 tests whose loop iterable is a literal list written inline in the test body. Those can only go empty if someone edits the assertion out by hand, which no guard can prevent, and the six catalog `test_classifications` tests already have a sibling pinning an exact row count. 942 tests pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
@djwaldo amazing work! Thanks for the complete pass! I don't have ThoughtSpot cluster and I'm not ThoughtSpot expert as you are 😄 I just checked the code. Let me do another pass, but I propose to merge quickly (we can always improve the converter later). Thanks again! |
| if table_doc is None: | ||
| log.add( | ||
| code="TS-DATASET-TABLE-MISSING", | ||
| severity=Severity.WARNING, |
There was a problem hiding this comment.
Could this resolve the physical column to its Ossie field identifier before emitting the ANSI_SQL sibling?
For example, if the Ossie field is total_revenue but its warehouse column is NET_AMOUNT, this emits orders.NET_AMOUNT. Ossie's portable expression language resolves dataset.field against logical fields, so consumers may not be able to evaluate this reference.
We could emit the logical field name, or omit the sibling when that mapping cannot be made safely.
| if existing: | ||
| print(f"Error: {_refuse_overwrite(existing)}", file=sys.stderr) | ||
| return 1 | ||
|
|
||
| try: | ||
| output_path.parent.mkdir(parents=True, exist_ok=True) | ||
| output_path.write_text(_yaml.dump(result.model), encoding="utf-8") | ||
| _write_issues(result.issues, issues_path) | ||
| except OSError as e: | ||
| print(f"Error: {e}", file=sys.stderr) | ||
| return 1 | ||
|
|
There was a problem hiding this comment.
This only checks whether targets already exists: it doesn't detect duplicate paths among the generated files and --issues.
If --issues is equal to a new output path, the conversion writes the document and then overwrites it with issue JSON.
I think (as a follow-up) we should reject colliding paths before writing. The same problem exists in to-ossie.
There was a problem hiding this comment.
Thanks for this one — I checked it against current main and it's already covered. That's my fault for not saying so when the fix landed.
Your comment on the same thing a day earlier (cli.py:207) prompted fce4b92, which added a collision check across every path a run will write — the converted output, each generated to-tml file, and --issues — evaluated before anything is written. Both shapes you describe refuse:
$ ossie-thoughtspot to-ossie tests/fixtures/minimal/*.tml -o x.yaml --issues x.yaml
Error: refusing to write the same path twice in one run: .../x.yaml.
--issues must name a different file from the converted output.
exit=1
$ ossie-thoughtspot to-tml m.yaml -o t/ --issues t/customers.table.tml
Error: refusing to write the same path twice in one run: .../t/customers.table.tml.
--issues must name a different file from the converted output.
exit=1
Nothing is written in either case — the output directory is left empty rather than half-populated — and to-ossie carries the same guard, so the symmetry you asked about holds.
I suspect the line you were reading is the separate pre-existing-target check, which does only what you describe; the collision check is a different guard that runs first. If those read as one thing, I'm glad to add a comment saying so.
Your other follow-up was real, though: the ANSI_SQL sibling naming warehouse columns is #459, with a fix in #460. It turned out to be 211 of 612 references across 20 of 31 real models — the qualifier was the Ossie dataset name while the column half was the warehouse column, so the pair resolved in neither namespace. Thanks for catching it, and for the review and merge.
jbonofre
left a comment
There was a problem hiding this comment.
I left a couple of comments as follow-up, but this converter is a great work.
Thanks again for your contribution! Great one!
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.A note on relationship orientation
Ossie's
Relationshiphas no cardinality field — direction carries it, withfromdocumentedas "the logical dataset on the many side" and
toas "the one side". ThoughtSpot's joins carryan explicit
cardinality, includingONE_TO_MANY, which has no Ossie representation in thedirection TML declares it.
The converter therefore swaps the endpoints for a
ONE_TO_MANYjoin, so the emittedrelationship is always many-side-to-one-side as the specification requires;
MANY_TO_ONEandONE_TO_ONEpass through unchanged. Both TML spellings of the same relationship consequentlyproduce the same Ossie relationship, and the vendor extension records the swap so the return
trip reproduces the original TML join — same direction, same target, same cardinality.
Worth flagging for reviewers:
validation/validate.pydoes not catch an invertedrelationship, because its key-coverage check skips a dataset that declares no keys. An earlier
revision of this branch emitted the un-swapped form and validated clean.
Note on history
81 commits, conventional-commit style. Happy to squash to a smaller set of logical commits if that is preferred for review.