You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ossie.apache.org by "djwaldo (via GitHub)" <gi...@apache.org> on 2026/09/25 01:47:36 UTC
[PR] fix(thoughtspot): compose a portable sibling for metrics over MEASURE columns [ossie]
djwaldo opened a new pull request, #463:
URL: https://github.com/apache/ossie/pull/463
Fixes #462. **Stacked on #460** — that PR is the first commit here; the diff against it is the metric change alone.
## Summary
Metrics came out `THOUGHTSPOT`-only — **476 of 476** across 31 real models — and every vendor converter drops a metric with no portable dialect, so a ThoughtSpot-derived document lost all of its business logic on the way anywhere.
Most of the machinery already worked. `_compose_aggregate_entries` composes the sibling and is unit-tested; it never fired on a real model because the resolver indexed **ATTRIBUTE** columns only, and a metric aggregates a **MEASURE** column. Every such reference resolved to nothing via `TS-EXPR-UNRESOLVED`, which is INFO and therefore quiet.
The index is now two sets, because merging them is a known defect:
| | |
|---|---|
| `fields` | ATTRIBUTE only — which column became a *field*. The unsurfaced-columns stash keys on this. A metric preserves no `column_id`, so a column referenced only by a metric must still be captured as unsurfaced, or `build_table` omits it while `build_model` still references it. An earlier revision widened this set and produced exactly that dangling reference — the comment recording it is what stopped me repeating it. |
| `referenceable` | ATTRIBUTE **and** MEASURE — which column a portable expression may name. What `resolve` gates on. |
**No double-aggregation.** The load-bearing/no-op rule already lives upstream of this path. Verified across the corpus: all 348 composed expressions contain exactly one aggregate call, and 336 cross-check as agreeing with their `THOUGHTSPOT` sibling on aggregate *and* dataset (the remaining 12 are shapes the spot-check regex does not model; inspected by hand).
## Identifier quoting — found while verifying, fixed here because this change made it worse
A ThoughtSpot display name is not automatically a valid SQL identifier. `SUM(cargo.Custom Clearance Time (min))` and `SUM(HV: STORES.LATITUDE)` are both rejected by sqlglot. Per `core-spec/expression_language.md`, identifiers follow ANSI SQL naming and the Ossie quote character is the double quote — so a non-regular identifier is now quoted, embedded quotes doubled, and **only when needed** (the spec notes regular identifiers compare case-insensitively while quoted ones compare verbatim).
Against Apache's own validator, **with sqlglot present**:
| | passed | failed | `[SQL]` findings |
|---|---|---|---|
| baseline (#460 only) | 29 | 2 | 7 |
| metric fix, no quoting | 26 | **5** | **30** |
| metric fix + quoting | **31** | **0** | **0** |
The metric fix alone regressed the validator. With quoting it is better than before, clearing 7 pre-existing findings as well as the 23 it would have added.
## `sqlglot` joins the dev group
None of the above was visible without it. `validation/validate.py` prints `sqlglot not installed, skipping SQL validation` and then `Validation PASSED` — a skipped check reported as a pass. Every "validator passed" figure measured during this work was a validator with its SQL half switched off. The new parse test is `importorskip`-guarded like its siblings, but the dependency means it actually runs.
## Test plan
- [x] **951 tests pass** (was 945), nothing skipped
- [x] 31-model corpus: portable metrics **0 → 348**, unresolvable references 0, no structure lost, no regression
- [x] Real validator (sqlglot present): **31/31, 0 SQL findings**
- [x] Mutation-checked: neutralising the quoting helper fails 11 tests, including the parse test and upstream schema validation
- [x] tpcds fixture regenerated — diff is 2 lines, both the new sibling
The remaining 128 metrics (27%) are formula-shaped and need real expression translation; `expressions/reverse.py` is the inventory for that and stays a follow-up.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ossie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(thoughtspot): portable expressions — a field's must not be dataset-qualified, a metric must exist [ossie]
Posted by "djwaldo (via GitHub)" <gi...@apache.org>.
djwaldo commented on PR #463:
URL: https://github.com/apache/ossie/pull/463#issuecomment-5826598576
@jbonofre both of your follow-up comments on #364 are addressed here, and the first one
turned out to be bigger than either of us thought.
**Your ANSI_SQL comment was right.** A field's portable expression named the warehouse
column, qualified by the Ossie dataset name -- two different namespaces, so it resolved in
neither. Measured across 31 real ThoughtSpot Models: **211 of 612 references named nothing
the document declares**, in 20 of 31 models. What settled the fix was checking what the
other converters do: databricks writes `l_linenumber` for a field named `line_number`,
nvidia writes `name` for `customer_name`. The physical column was never the problem, the
qualifier was.
**Verifying that turned up a second one.** Metrics carried a `THOUGHTSPOT` dialect only --
**476 of 476** -- and every vendor converter skips a metric with no portable dialect, so a
converted model lost all of its business logic on the way to any vendor. The machinery was
already there and unit-tested; it never fired because the resolver indexed ATTRIBUTE
columns and a metric aggregates a MEASURE column.
| | before | after |
|---|---|---|
| references naming no declared field | 211 / 612 | **0** |
| metrics with a portable dialect | 0 / 476 | **348 / 476** |
The remaining 128 metrics are formula-shaped and need real expression translation --
`expressions/reverse.py` is the inventory for that, and it stays a follow-up rather than
something I'd rush into this.
Your other comment, on `cli.py` colliding output paths, was already fixed the day before
you wrote it -- I replied inline with the repro rather than changing anything.
**One ask: the workflow run needs approving.** No CI has run on this PR. 951 tests pass
locally and the 31-model corpus is clean, but none of that is visible to you without it.
---
One thing worth knowing beyond this PR, because it is not converter-specific.
`validation/validate.py` prints
[SQL] Warning: sqlglot not installed, skipping SQL validation
Validation PASSED: model.ossie.yaml
A skipped check reported as a pass. Every "validator passed" figure I measured during this
work was a validator with its SQL half switched off -- and with sqlglot installed, this
converter's output was failing 2 of 31 documents before these commits, which I had not
seen. I have added sqlglot to this converter's dev group so its own tests parse what they
emit. Happy to raise it as a separate issue if you think the validator should exit
non-zero, or at least not say PASSED, when it has skipped a whole class of check.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ossie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(thoughtspot): portable expressions — unqualify a field's, quote what needs quoting [ossie]
Posted by "djwaldo (via GitHub)" <gi...@apache.org>.
djwaldo commented on PR #463:
URL: https://github.com/apache/ossie/pull/463#issuecomment-6004415306
Thanks @jbonofre — the reserved-word catch was a real defect and the fixture had been failing `validation/validate.py` in a committed state. All four points are addressed in `b544e4f`.
**1. Reserved words.** Added, with one change to your suggestion worth flagging. I built the set from **two** sources, because measuring showed neither alone is enough:
- The words **sqlglot actually rejects** as a bare column reference — it is the parser the validator runs. That turned up 16 your list did not have: `ALTER`, `GRANT`, `LATERAL`, `QUALIFY`, `RETURNING`, `REVOKE`, `ROLLBACK` and others.
- The **ANSI reserved words sqlglot tolerates**. `SELECT order` parses under sqlglot and is rejected by Snowflake and PostgreSQL alike, so the validator passing proves nothing about a warehouse running it. That is 25 words only your list had — `ORDER`, `GROUP`, `TABLE`, `USER`, `NULL`.
I stopped short of the full ANSI:2016 list (~450 words) deliberately: it reserves `YEAR`, `MONTH` and `VALUE`, which are ordinary ThoughtSpot column names, and quoting is not free — the spec makes a quoted identifier match verbatim where a regular one is case-insensitive, so a needlessly quoted `"Amount"` stops matching a warehouse column stored as `AMOUNT`. There is a test pinning that those stay bare.
**2. The fixture** is now `expression: '"on"'`, and `validation/validate.py` passes on both fixture sets.
**3. SQL validation in the test run** — agreed, and this is the part that mattered most. The fixture test checked `ossie-schema.json` and stopped, so a SQL-invalid expression sat in a committed fixture with 936 tests green. It now imports `validation/validate.py` by path and runs `validate_sql`, rather than reimplementing the check — a local copy would be a second account of the gate, free to agree with the converter while the gate disagrees.
One wrinkle worth recording: `validate_sql` *returns* a warning instead of raising when sqlglot is missing, so a plain assertion on emptiness would pass on the one input that proves nothing. The test asserts the skip warning is absent **before** asserting no errors. "Validation PASSED" is also what the validator prints when it validated nothing, which is how this survived.
**4. The reserved-keyword test** is there, parametrised over `on`, `where`, `ORDER`, `Select`, `qualify` — case-mixed included, since reserved words match case-insensitively — and each asserts both the quoting and that `sqlglot.parse_one` accepts it.
---
Two things turned up while verifying, both in code this PR touches:
**A reverse-direction asymmetry.** `_sql_identifier` doubles an embedded quote, so a column displayed `Size (")` is emitted `"Size ("")"` — and the reverse `_BARE_IDENTIFIER_RE` matched `"[^"]{1,128}"`, which rejects a doubled quote. The regex turned away exactly the strings this converter writes, so the field was not recognised as physical on the way back and was dropped as untranslatable. Fixed both ends.
**The first test feeding a foreign vendor dialect through `build_table`.** With no THOUGHTSPOT dialect and no stash, the physical column can only come from the field's own `expression` — which is what #486 settles it to mean, and what the code already does. But every test on that branch used `ANSI_SQL`, so the dialect-agnostic path was correct by construction and unproven. `DATABRICKS` is the real case, since that is the shape a TML → Ossie → Metric View → Ossie → TML trip hands back.
---
**Verification.** 968 tests pass (936 before). Each of the three fixes was reverted in turn against the suite and every one is caught. Across the 31-model corpus the output is **byte-identical** before and after — no real column in it is a reserved word, so the corpus would never have found this and only the TPC-DS fixture did, which is the argument for the fixture carrying awkward cases.
On sequencing: I see #523 and ossie-converters#1. Happy to re-raise this on `ossie-converters` if the move lands first — just say which you'd prefer. CI still shows no runs on this head.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ossie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(thoughtspot): portable expressions — unqualify a field's, quote what needs quoting [ossie]
Posted by "jbonofre (via GitHub)" <gi...@apache.org>.
jbonofre commented on code in PR #463:
URL: https://github.com/apache/ossie/pull/463#discussion_r4133453941
##########
converters/thoughtspot/pyproject.toml:
##########
@@ -27,6 +27,12 @@ dev = [
# so its absence never fails the suite -- the package's only *runtime* dependency
# stays PyYAML. Same dev-group placement as converters/nvidia's pyproject.toml.
"jsonschema>=4.26.0",
+ # Dev/test-only: the parser Apache's own `validation/validate.py` uses, so a
+ # test can assert an emitted portable expression actually PARSES. Without it
+ # that check silently skipped -- and so did the validator's, which reports
+ # "Validation PASSED" while printing "sqlglot not installed, skipping SQL
+ # validation". Guarded by pytest.importorskip, same discipline as the two below.
+ "sqlglot>=25.0",
Review Comment:
Adding `sqlglot` to the dev group is a great improvement.
As a follow-up in `tests/test_fixtures.py`, `test_expected_output_validates_against_the_upstream_schema` currently only validates against `ossie-schema.json` and doesn't run SQL validation. If we also invoked `validate_sql` (or `validation/validate.py`) there when `sqlglot` is available, `pytest` would catch SQL parse errors in fixtures (like the `store.on` issue) directly during test runs.
##########
converters/thoughtspot/src/ossie_thoughtspot/tml_to_ossie.py:
##########
@@ -139,9 +140,37 @@
from .tml import DocumentSet
+
+#: A regular ANSI SQL identifier: letter or underscore, then letters, digits or
+#: underscores. Anything else has to be double-quoted to survive a SQL parser.
+_REGULAR_IDENTIFIER = re.compile(r"^[A-Za-z_][A-Za-z0-9_]*$")
Review Comment:
`_REGULAR_IDENTIFIER` doesn't check for SQL reserved keywords. Per ANSI SQL (and `core-spec/expression_language.md`), regular identifiers cannot be reserved words.
When a bare field column happens to be a SQL keyword like `on` (which is present in the TPC-DS fixture below), `_sql_identifier` leaves it unquoted as `on`. A query like `SELECT on` then fails SQL parsing.
Could we check `name.upper()` against a set of SQL reserved keywords here?
```
_SQL_RESERVED = frozenset({
"ALL", "AND", "ANY", "AS", "ASC", "BETWEEN", "BY", "CASE", "CHECK",
"CREATE", "CROSS", "DEFAULT", "DESC", "DISTINCT", "DROP", "ELSE",
"END", "EXCEPT", "EXISTS", "FALSE", "FOR", "FROM", "FULL", "GROUP",
"HAVING", "IN", "INNER", "INSERT", "INTERSECT", "INTO", "IS", "JOIN",
"LEFT", "LIKE", "LIMIT", "NOT", "NULL", "OFFSET", "ON", "OR", "ORDER",
"OUTER", "PRIMARY", "RIGHT", "SELECT", "SOME", "TABLE", "THEN", "TRUE",
"UNION", "UNIQUE", "USER", "USING", "WHEN", "WHERE", "WITH",
})
```
And in `_sql_identifier`:
```
if _REGULAR_IDENTIFIER.match(name) and name.upper() not in _SQL_RESERVED:
return name
return '"' + name.replace('"', '""') + '"'
```
##########
converters/thoughtspot/src/ossie_thoughtspot/tml_to_ossie.py:
##########
@@ -1091,8 +1186,36 @@ def convert_metric(
severity=Severity.WARNING,
message=(
f"column {display_name!r}'s expression already contains an "
- f"aggregate; the column-level aggregation {aggregation_raw!r} "
- f"was ignored to avoid double-aggregating"
+ f"aggregate; "
+ + (
+ f"ThoughtSpot's default aggregation is not applied"
+ if aggregation_is_default
+ else f"the column-level aggregation {aggregation_raw!r} was ignored"
+ )
+ + " to avoid double-aggregating"
+ ),
+ object_ref=object_ref,
+ )
+ metric_shape = METRIC_SHAPE_FORMULA
+ dialects = expression_entries(
+ expr, resolve, log, object_ref=object_ref, kind="metric"
+ )
+ elif aggregation_is_default and formula.find_formula_refs(expr):
+ # Scalar-looking, but it references other formulas whose own
+ # expressions may aggregate. ThoughtSpot's default would resolve
+ # against the real definitions; this converter cannot, so it
+ # declines rather than guessing a wrapper around them.
+ log.add(
+ code="TS-METRIC-AGGREGATION-DEFAULT-UNRESOLVED",
Review Comment:
Great catch and handling here. Declining to compose `sum(...)` around cross-referencing formulas and warning via `TS-METRIC-AGGREGATION-DEFAULT-UNRESOLVED` avoids silent double-aggregation bugs (`sum([formula_a] / [formula_b])`).
##########
converters/thoughtspot/tests/fixtures/tpcds/expected.ossie.yaml:
##########
@@ -389,7 +389,7 @@ datasets:
- dialect: THOUGHTSPOT
expression: '[store::on]'
- dialect: ANSI_SQL
- expression: store.on
+ expression: 'on'
Review Comment:
This fixture currently fails `validation/validate.py` when `sqlglot` is installed:
```
python validation/validate.py converters/thoughtspot/tests/fixtures/tpcds/expected.ossie.yaml
```
Output:
```
Validation FAILED with 1 error(s):
[SQL] Field 'store.on' in model 'tpcds_retail_model' (ANSI_SQL): Invalid expression / Unexpected token. Line 1, Col: 9.
```
Once `_sql_identifier` quotes reserved keywords, this should be `expression: '"on"'`.
##########
converters/thoughtspot/tests/test_tml_to_ossie_fields.py:
##########
@@ -592,3 +592,123 @@ def test_a_present_but_unmapped_data_type_logs_exactly_one_issue_naming_it(self)
issues = log.as_dicts()
assert len(issues) == 1
assert "GEOGRAPHY" in issues[0]["message"]
+
+
+class TestPortableReferenceScope:
+ """A field's portable expression is bare; a metric's keeps its dataset.
+
+ The asymmetry is scope, and it is what every sibling converter does. A
+ field belongs to one dataset, whose `source` already names the warehouse
+ table, so `l_linenumber` (databricks) or `name` (nvidia) is unambiguous on
+ its own. A metric is model-scoped and may reference any dataset, so
+ `SUM(amount)` stops being unambiguous the moment two datasets have an
+ `amount`; nvidia qualifies for that reason (`SUM(orders.subtotal)`).
+
+ Qualifying a FIELD was not just redundant, it was unresolvable: the
+ qualifier is the Ossie dataset name (ThoughtSpot's Table-object name, which
+ need not be the warehouse table) while the column half is the warehouse
+ column, so `Dim_Customer.Customer_Name` was neither runnable SQL nor a
+ resolvable logical reference. 211 of 612 references across 31 real models
+ were in that state, every one of them on a field.
+ """
+
+ def test_a_field_reference_carries_the_bare_warehouse_column(self):
+ out = expression_entries(
+ "[ORDERS::Amount]", _resolve, IssueLog(), object_ref="f", kind="field"
+ )
+ portable = next(e for e in out if e["dialect"] == "ANSI_SQL")
+ assert portable["expression"] == "amount", (
+ "a field's portable expression must not carry a dataset qualifier"
+ )
+
+ def test_a_metric_reference_keeps_its_dataset_qualifier(self):
+ out = expression_entries(
+ "[ORDERS::Amount]", _resolve, IssueLog(), object_ref="m", kind="metric"
+ )
+ portable = next(e for e in out if e["dialect"] == "ANSI_SQL")
+ assert portable["expression"] == "orders.amount", (
+ "a metric is model-scoped, so its reference must stay qualified"
+ )
+
+ def test_the_two_kinds_genuinely_differ(self):
+ # Guards the asymmetry itself: making both branches emit the same thing
+ # would satisfy either test above on its own.
+ as_field = expression_entries(
+ "[ORDERS::Amount]", _resolve, IssueLog(), object_ref="f", kind="field"
+ )
+ as_metric = expression_entries(
+ "[ORDERS::Amount]", _resolve, IssueLog(), object_ref="m", kind="metric"
+ )
+ field_expr = next(e["expression"] for e in as_field if e["dialect"] == "ANSI_SQL")
+ metric_expr = next(e["expression"] for e in as_metric if e["dialect"] == "ANSI_SQL")
+ assert field_expr != metric_expr, (
+ f"field and metric both emitted {field_expr!r}; the scope distinction is gone"
+ )
+
+
+class TestPortableIdentifierQuoting:
+ """A ThoughtSpot display name is not automatically a valid SQL identifier.
+
+ Columns and tables carry spaces, colons, percent signs and parentheses
+ freely. Emitted raw they do not merely look wrong, they do not parse:
+ `SUM(cargo.Custom Clearance Time (min))` and `SUM(HV: STORES.LATITUDE)` are
+ both rejected by sqlglot -- the parser Apache's own validator uses. Across
+ 31 real models this was 5 documents failing validation outright and 30 SQL
+ findings; quoting takes both to zero.
+
+ Quoted only WHEN NEEDED, which matters semantically rather than
+ cosmetically: the specification notes regular identifiers are compared
+ case-insensitively while quoted ones are compared verbatim, so quoting a
+ name that does not need it changes how a consumer matches it.
+ """
+
+ @staticmethod
+ def _resolver(dataset, column):
+ return lambda _t, _c: (dataset, column)
+
+ def test_a_name_needing_quotes_is_quoted(self):
+ out = expression_entries(
+ "[T::C]", self._resolver("cargo", "Custom Clearance Time (min)"),
+ IssueLog(), object_ref="m", kind="metric",
+ )
+ portable = next(e["expression"] for e in out if e["dialect"] == "ANSI_SQL")
+ assert portable == 'cargo."Custom Clearance Time (min)"', portable
+
+ def test_a_dataset_name_needing_quotes_is_quoted_too(self):
+ out = expression_entries(
+ "[T::C]", self._resolver("HV: DIM_STORES", "LATITUDE"),
+ IssueLog(), object_ref="m", kind="metric",
+ )
+ portable = next(e["expression"] for e in out if e["dialect"] == "ANSI_SQL")
+ assert portable == '"HV: DIM_STORES".LATITUDE', portable
+
+ def test_a_regular_identifier_is_left_bare(self):
+ # Not cosmetic: quoting changes case-matching semantics, so a name that
+ # does not need quotes must not get them.
+ out = expression_entries(
+ "[T::C]", self._resolver("orders", "amount"),
+ IssueLog(), object_ref="m", kind="metric",
+ )
+ portable = next(e["expression"] for e in out if e["dialect"] == "ANSI_SQL")
+ assert portable == "orders.amount", portable
+ assert '"' not in portable
+
+ def test_an_embedded_double_quote_is_doubled(self):
+ out = expression_entries(
+ "[T::C]", self._resolver("orders", 'He said "hi"'),
+ IssueLog(), object_ref="f", kind="field",
+ )
+ portable = next(e["expression"] for e in out if e["dialect"] == "ANSI_SQL")
+ assert portable == '"He said ""hi"""', portable
+
+ def test_the_quoted_output_actually_parses_as_sql(self):
+ # The point of the exercise. Skipped rather than silently passing when
+ # sqlglot is absent -- the validator's own "not installed" path reports
+ # PASSED, which is how this defect survived unseen.
+ sqlglot = pytest.importorskip("sqlglot")
Review Comment:
Could we add a test case here for a field whose warehouse column is a SQL reserved keyword (e.g. `orders.on` or `orders.where`) asserting that:
1. It is quoted as `"on"`.
2. `sqlglot.parse_on(f"SELECT {portable}")` parses successfully.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ossie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(thoughtspot): portable expressions — unqualify a field's, quote what needs quoting [ossie]
Posted by "jbonofre (via GitHub)" <gi...@apache.org>.
jbonofre merged PR #463:
URL: https://github.com/apache/ossie/pull/463
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ossie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(thoughtspot): portable expressions — unqualify a field's, quote what needs quoting [ossie]
Posted by "djwaldo (via GitHub)" <gi...@apache.org>.
djwaldo commented on PR #463:
URL: https://github.com/apache/ossie/pull/463#issuecomment-5828704175
## Withdrawing the metric half of this PR
An independent adversarial review found that the metric change was wrong, and I have removed it. This PR is now the field fix (#459) plus identifier quoting. **#462 stays open**; I no longer think the fix I proposed there is the right one.
**What was wrong.** The metric commit emitted `SUM(<dataset>.<warehouse column>)` for a metric over a MEASURE column. A MEASURE column never becomes an Ossie field, so **348 of 348** emitted references named a column nothing in the document declares — which is the *same* namespace defect this PR's first commit fixes for fields. I reproduced it independently: 340 of 340 by my own count.
**Why it matters more than it sounds.** The reviewer ran every Ossie-to-vendor converter in the tree against the output. They split three ways:
| How `orders.amount` is read | Converters |
|---|---|
| As the field name | Honeydew, Power BI, OrionBelt, Sigma, Snowflake |
| As the warehouse column (intended) | Databricks, dbt, NVIDIA |
| Field name first, then column | Omni |
On a name collision the first group sums the wrong column. Power BI emits `BLANK()` for all 476 measures; OrionBelt marks 348 as not queryable. **Before the change these metrics had no portable expression at all — a loud, known loss. I replaced it with a quiet wrong answer, which is worse.**
I had justified the dataset qualifier by pointing at nvidia's `SUM(orders.subtotal)`. That was reasoning from one example where I should have run the consumers.
**What I missed, and why.** My own round-trip harness measures unresolvable references by iterating a dataset's `fields` only — it never looks at metrics. It reported `0/0` while the true figure was 348/348. Four separate instruments I built during this work turned out to be measuring nothing; this was the fourth.
**Where #462 goes now.** The right fix is a design question rather than a patch: either MEASURE columns are declared as Ossie fields and metrics reference them by field name, or metrics keep no portable expression. That is the same namespace question the spec leaves open — `spec.md:104` says metrics are "aggregate expressions on **fields**", `expression_language.md:98` gives the shape `dataset.field` — and it affects every converter, not just this one. I will raise it on dev@ with the consumer evidence rather than decide it here.
The review also found several pre-existing defects in the merged converter, which I am filing separately.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ossie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org