Skip to content

ThoughtSpot converter: an absent aggregation is treated as NONE in both directions, but ThoughtSpot's default is SUM #467

Description

@djwaldo

Found by an independent adversarial review, and reproduced. Pre-existing in the merged converter (#364).

The converter treats a missing aggregation key as NONE. ThoughtSpot documents the default as SUM. The premise is wrong in both directions, and the two halves compound.

Read side — convert_metric uses properties.get("aggregation", "NONE")

A MEASURE column with no aggregation key:

- name: Absent Agg
  column_id: orders::amount
  properties: {column_type: MEASURE}

emits a metric with THOUGHTSPOT expression [orders::amount] and no aggregate at all. ThoughtSpot would sum it. Exit 0, no issue for this metric, validator PASSES.

No corpus model omits aggregation, so real exports do not surface this — hand-authored and minimal TML does.

Write side — aggregation: NONE is dropped

This one loses data on a plain round trip. A formula with an explicit NONE:

- {name: Ratio none, formula_id: f1, properties: {column_type: MEASURE, aggregation: NONE}}
- {id: f1, name: Ratio none, expr: "[orders::amount] / [orders::qty]"}

to-ossie then to-tml returns:

{name: Ratio none, formula_id: f1, properties: {column_type: MEASURE}}

The aggregation: NONE is gone. ThoughtSpot applies its documented default, so a per-row ratio is now summed — the model reports the sum of ratios where it used to report the ratio. Exit 0 both directions; the only issue is INFO TS-EXPR-THOUGHTSPOT-ONLY. Verified independently.

The same happens to a column_id MEASURE with explicit NONE.

Cause

_build_metric in ossie_to_thoughtspot.py emits an aggregation only when the shape or the stash supplies one, or when the expression has an outer aggregate. It carries the same "absent means NONE" premise as convert_metric, so an explicit NONE and an absent key become indistinguishable.

Note for whoever fixes this

The test suite currently enforces the wrong premise. The reviewer mutated the code to treat an absent aggregation as SUM and four tests failed. One of them, test_a_bare_group_aggregate_without_one_gains_none, only asserts an absent value stays absent, which is fair round-trip fidelity — the other three encode the premise itself and would need to change with the fix.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions