fix(thoughtspot): guard wrongly-typed TML values from bare tracebacks

[OGsiji]

Summary

Refs #469. A hand-edited ThoughtSpot TML document can carry a value of the wrong Python type. During to-ossie conversion those escaped as bare TypeError/AttributeError, breaking the converter's own "never a bare traceback" contract (the CLI only catches ConversionError/OSError/UnicodeDecodeError).

#482 covered the column and model name cases and explicitly listed the remaining to-ossie scalar reads as untouched. This PR adds a targeted type guard at each of those read boundaries, mirroring the surrounding degrade-and-continue pattern (log.add(code="TS-*", severity=WARNING, ...) then skip or fall back) rather than a broad except:

Wrongly-typed value	Was	Now
aggregation as a list/dict	TypeError (membership test on unhashable)	TS-METRIC-AGGREGATION-UNKNOWN, treated as NONE
formula expr wrong type (field)	TypeError in the formula parser	TS-FIELD-FORMULA-INVALID, field skipped
formula expr wrong type (metric)	TypeError in the formula parser	TS-METRIC-FORMULA-INVALID, metric skipped
physical data_type wrong type	TypeError (unhashable in datatypes.to_ossie)	TS-{KIND}-DATATYPE-UNMAPPED, no datatype emitted
db_column_name wrong type	TypeError in identifiers.normalise	read as absent (falls back like a missing name)
join on wrong type	AttributeError on .strip()	TS-JOIN-MALFORMED, join skipped

Tests

New tests/test_tml_to_ossie_typed_values.py covers all six cases. Verified they fail with bare tracebacks on main and pass with the guards. Full ThoughtSpot suite: 977 passed on Python 3.10 and 3.12 (971 existing + 6 new).

Scope / follow-up

Deliberately scoped to the to-ossie scalar reads #482 enumerated. The remaining #469 items — the wrong container type for columns/model_tables/properties/join destination, and the entire to-tml stash + schema-invalid direction — are a separate class and left for a follow-up PR.

[jbonofre]

This makes convert_field return None, but convert() then stashes the same value. The unattributed-formula block only checks "expr" in formula entry before copying formula_entry["expr"] into unattributed_formulas, and the unsurfaced-formulas loop does the same for a formula no column references, with no issue logged at all.

End to end, a formulas[] entry with expr: 42, expr: [ORDERS::Region] or an empty expr: gives to-ossie exit 0, and to-tml on that output then fails with a bare TypeError. With expr: 2024-01-01 (parsed as a YAML date), to-ossie itself fails in json.dumps when writing the stash.

Could the check move to where the formulas dict is built? Reporting a non-string expr once there and removing the key would send all four readers down the existing "entry has no expr" paths, and the two copies of this guard in convert_field and convert_metric would no longer be needed.

[jbonofre]

db_column_name has other readers that bypass this helper, so a non-string value is not really read as absent:

• resolve() interpolates it into the ANSI_SQL expression. db_column_name: 42 emits ORDERS.42 and a list emits ORDERS.['REGION'], with exit 0 and no issue. A missing name returns None there and suppresses the ANSI sibling.
• _physical_column_stash copies it into the field stash. 42 is stashed without a report, and a YAML date fails in json.dumps.

The first two read from physical_columns_by_prefix, which is built by _normalized_physical_columns, so sanitizing in _normalize_physical_column would cover both in one place. It would also be worth logging an issue there, since the value is currently dropped without any report.

[jbonofre]

is not None also catches falsy non-strings that used to mean "no condition". on: [], on: {}, on: false and on: 0 reported TS-JOIN-NO-CONDITION on main and now report TS-JOIN-MALFORMED. If that change is not intended:

Suggested change

	if on_expression is not None and not isinstance(on_expression, str):
	if on_expression and not isinstance(on_expression, str):

If it is intended, could you add a test that pins it?

[jbonofre]

This branch returns (None, None, False), so the join is dropped, the same as the TS-JOIN-NO-CONDITION path below. The existing TS-JOIN-MALFORMED path does the opposite: it keeps the join in unrepresentable_joins and its message says so. With this change one code means both "preserved" and "discarded", and the comment above ("the same degrade-and-skip the parse failure gets") does not match what the parse failure does.

Could this use a separate code, or at least say in the message that the join is not preserved? If you would rather preserve it, note that _unrepresentable_entry stores on verbatim, so the stash and to-tml would then have to cope with the non-string value. The docstring line "both None when there is no condition at all to report" needs updating either way.

[jbonofre]

These tests call the helpers directly, so none of them checks the contract the module docstring states (no bare traceback from the converter). This one passes on field is None while convert() still stashes the same expr and to-tml fails on the result.

Could you add cases that run a document carrying each wrong-typed value through convert(), and through to-tml for the values that end up in the stash, asserting on the issue log and the emitted document?

Separately, test_non_string_aggregation_falls_back_to_none_and_is_reported only asserts metric is not None, so it does not check the NONE fallback in its name.

[OGsiji]

Thanks, this was a good catch across the board. You're right that guarding at each reader left the values reaching the stash and to-tml. Reworked in 15eda51 to guard at the data boundary instead, and the tests now drive full documents through convert() and ossie_to_thoughtspot.convert().

Formula expr (the stash + the four readers). Moved the check to where the formulas dict is built: a non-string expr is reported once (TS-FORMULA-EXPR-INVALID) and the key is dropped, so convert_field, convert_metric, the unattributed block, and the unsurfaced loop all take their existing "entry has no expr" path. The two per-reader guards are gone. Covers the expr: 42, expr: [..], empty and YAML-date cases, and to-ossie -> to-tml no longer raises.

db_column_name (the other readers). table_lookup wraps the same physical_columns_by_prefix list the resolver and the field stash read, so one sanitizer there covers all three readers (resolver, _physical_column_stash, _physical_db_column_name). A non-string value is dropped to None with a report (TS-COLUMN-DB-NAME-INVALID), so no more ORDERS.42, no silent drop, and no json.dumps crash. The helper-level guard is removed.

Join on falsy values. Took the suggestion: if on_expression and not isinstance(on_expression, str). on: [], on: {}, on: 0, on: false keep their prior TS-JOIN-NO-CONDITION meaning; added a test pinning that.

Join on MALFORMED semantics. You're right that TS-JOIN-MALFORMED means "preserved". The non-string case drops the join, so it now uses a distinct TS-JOIN-CONDITION-INVALID whose message says it is dropped, and the docstring is updated. I kept it dropped rather than preserved, since _unrepresentable_entry stores on verbatim and preserving it would carry the non-string value straight into the stash.

Tests. Rewrote them to run documents through convert() (and to-tml for the stashed expr and db_column_name values), asserting on the issue log and the emitted document, not just a helper return. The aggregation test now asserts the NONE fallback (the metric emits [ORDERS::Amount], no aggregate wrapper).

Full ThoughtSpot suite: 977 passed on Python 3.10 and 3.12.

The container-type cases and the to-tml stash direction from #469 remain a separate follow-up, as noted in the description.

[kayemkim]

The rework reads well, especially moving the formula check to where the dict is built so the four readers need nothing.

I ran the second revision merged onto current main through the thoughtspot workflow on 3.10 to 3.14, 977 passed. I also drove ten wrong-typed shapes through cli.main, to-ossie and then to-tml on its output, on main and on this branch. On main, expr as an int, a list, null or a YAML date, db_column_name as a date, on: 42, aggregation: [SUM] and data_type: [INT64] all end in a bare TypeError or AttributeError, and db_column_name: 42 exits 0 with nothing reported. On this branch each one exits 0 with the expected code and to-tml completes, and on: [] still reports TS-JOIN-NO-CONDITION.

One shape outside the five you scoped, for the follow-up list rather than this PR: a formula id that YAML parses as a date still raises on this branch, from _write_stash_safely into stash.write_stash's json.dumps, because the unsurfaced-formula stash carries the id verbatim.

[jbonofre]

LGTM! Thanks!

Just a note: I would have added an end-to-end test for data_type to test_tml_to_ossie_typed_values.py. We could do that as a follow-up not a big deal.