You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ossie.apache.org by "eisber (via GitHub)" <gi...@apache.org> on 2026/09/21 09:24:57 UTC
[PR] fix(dbt): preserve aggregate expression semantics [ossie]
eisber opened a new pull request, #437:
URL: https://github.com/apache/ossie/pull/437
## Summary
Fixes the dbt converter's complex-expression fallback so it does not encode an already aggregated Ossie metric as both a MetricFlow `expr` and a second `agg`.
- Decomposes a parser-proven root SQL aggregate into MetricFlow's aggregation operator and an aggregate-free scalar expression. For example, `SUM(CASE WHEN ... THEN amount ELSE 0 END)` becomes `agg=sum` plus `expr=CASE WHEN ... THEN amount ELSE 0 END`.
- Removes the guessed-`SUM` fallback. Composite, nested, non-aggregate, unsupported-modifier, and non-SQL metric expressions are omitted with `UNSUPPORTED_METRIC_EXPRESSION` instead of silently changing semantics.
- Surfaces Ossie-to-MSI conversion issues in the CLI and documents the supported behavior.
Before:
```text
INPUT SUM(CASE WHEN ... THEN amount ELSE 0 END)
MSI agg=sum; expr=SUM(CASE WHEN ... THEN amount ELSE 0 END)
OUTPUT SUM(SUM(CASE WHEN ... THEN amount ELSE 0 END))
issues [] / []
```
After:
```text
INPUT SUM(CASE WHEN ... THEN amount ELSE 0 END)
MSI agg=sum; expr=CASE WHEN ... THEN amount ELSE 0 END
OUTPUT SUM(CASE WHEN ... THEN amount ELSE 0 END)
issues [] / []
```
The invariant is that a MetricFlow SIMPLE metric has exactly one aggregation boundary: `expr` is scalar and `agg` supplies the aggregation.
## Related Issues
Fixes #430
## Checklist
### Specification
- [x] No specification change is required; this corrects converter behavior against the existing field/metric expression distinction
- [x] Breaking changes are called out: unsupported fallback metrics are now reported and omitted rather than emitted with invented `SUM` semantics
### Ontology
- [x] No ontology changes are required
### Converters
- [x] dbt converter logic is updated
- [x] Positive, negative, CLI, and round-trip tests are included
### Validation
- [x] No core validation change is required
### Documentation
- [x] The dbt converter README documents the updated conversion behavior
- [x] No contribution-process change is required
### Examples
- [x] No spec construct or example-model change is required
### Tests
- [x] Full dbt suite passes: 112 tests, 5 snapshots
- [x] New functionality and unsupported cases are covered
### Compliance
- [x] Existing ASF-licensed files only; no new source files
- [x] No third-party dependencies added
## AI disclosure
Per the ASF Generative Tooling Guidance, this contribution was prepared with AI assistance. I reviewed the converter semantics, implementation, tests, and generated diff, and verified the results by running the full dbt suite locally.
--
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(dbt): preserve aggregate expression semantics [ossie]
Posted by "eisber (via GitHub)" <gi...@apache.org>.
eisber commented on code in PR #437:
URL: https://github.com/apache/ossie/pull/437#discussion_r4184086974
##########
converters/dbt/src/ossie_dbt/expression_utils.py:
##########
@@ -32,17 +32,24 @@ def _col_name(node: exp.Expression) -> str:
"""Return the bare (unqualified) column name from a sqlglot expression node."""
if isinstance(node, exp.Column):
return node.name
- rendered = node.sql()
- return _strip_qualifier(rendered)
+ return node.sql()
+
+
+def _is_scalar_expression(node: exp.Expression) -> bool:
Review Comment:
Addressed. Scalar validation is now an explicit AST node allowlist. `Anonymous` nodes are not allowed because sqlglot cannot distinguish custom scalar functions from UDAFs, and query/subquery nodes are also absent. Added regressions for both `SUM(MY_UDAF(amount))` and `SUM((SELECT x FROM other_table LIMIT 1))`; both are reported as unsupported.
--
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(dbt): preserve aggregate expression semantics [ossie]
Posted by "eisber (via GitHub)" <gi...@apache.org>.
eisber commented on code in PR #437:
URL: https://github.com/apache/ossie/pull/437#discussion_r4184090550
##########
converters/dbt/src/ossie_dbt/ossie_to_msi.py:
##########
@@ -406,10 +406,24 @@ def _find_dataset_for_col(
def _get_expression(self, ossie_expr: OssieExpression) -> str:
"""Return the expression string for the preferred dialect (fallback: first available)."""
+ dialect_expr = self._get_dialect_expression(ossie_expr)
+ return dialect_expr.expression if dialect_expr is not None else ""
+
+ def _get_dialect_expression(
+ self, ossie_expr: OssieExpression
+ ) -> Optional[OssieDialectExpression]:
+ """Return the expression for the preferred dialect (fallback: first available)."""
for dialect_expr in ossie_expr.dialects:
if dialect_expr.dialect is self._dialect:
- return dialect_expr.expression
- return ossie_expr.dialects[0].expression if ossie_expr.dialects else ""
+ return dialect_expr
+ return ossie_expr.dialects[0] if ossie_expr.dialects else None
+
+ _SQL_DIALECTS = {
Review Comment:
Addressed while rebasing onto current main. The converter retains the current preference order: configured dialect, then OSSIE_SQL_2026, then positional fallback. OSSIE_SQL_2026 is explicitly included in the SQL dialect set, and the portable-dialect selection tests pass.
--
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(dbt): preserve aggregate expression semantics [ossie]
Posted by "MrBlack1995 (via GitHub)" <gi...@apache.org>.
MrBlack1995 commented on PR #437:
URL: https://github.com/apache/ossie/pull/437#issuecomment-6307873085
Thanks @eisber, anchoring the safe scalar set to the REQUIRED categories in `expression_language.md` is exactly what I hoped for, and the date arithmetic, `CONCAT`, `POWER`/`EXP` and `CAST` cases now come through. The import side in #584 will follow the same boundary when it grows past single-column aggregates, so the two converters stay consistent.
--
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(dbt): preserve aggregate expression semantics [ossie]
Posted by "jbonofre (via GitHub)" <gi...@apache.org>.
jbonofre commented on code in PR #437:
URL: https://github.com/apache/ossie/pull/437#discussion_r4194861312
##########
converters/dbt/src/ossie_dbt/ossie_to_msi.py:
##########
@@ -282,14 +289,30 @@ def _convert_metrics(self, ossie_sm: OssieSemanticModel) -> Tuple[List[PydanticM
metrics: List[PydanticMetric] = []
issues: List[ConverterIssue] = []
for metric in ossie_sm.metrics or []:
- expr_str = self._get_expression(metric.expression)
+ dialect_expr = self._get_dialect_expression(metric.expression)
Review Comment:
The dialect gate uses a hardcoded `_SQL_DIALECTS` set. `_get_dialect_expression` picks the converter's own dialect first, so if it is constructed with a dialect outside this set, every metric is dropped. The same happens for any other `OssieDialect` value outside the set, even though sqlglot could parse it.
Could we derive the check from the dialects sqlglot supports, or fall through to the next dialect when the preferred one isn't in the set?
##########
converters/dbt/src/ossie_dbt/ossie_to_msi.py:
##########
@@ -419,20 +415,45 @@ def _find_dataset_for_col(
# Check for a dataset qualifier in the raw expression (e.g. "orders.amount").
# Parse column references instead of splitting the rendered inner expression,
# which may be a compound CASE expression for SUM_BOOLEAN metrics.
- dataset_qualifier = _get_dataset_qualifier(raw_expr_str)
- if dataset_qualifier:
- return dataset_qualifier
+ qualifiers = _get_dataset_qualifiers(raw_expr_str)
Review Comment:
The new "must identify exactly one dataset" check compares `bare_col` against field names. For compound scalar expressions and SUM_BOOLEAN conditions, `bare_col` is the whole rendered expressions (e.g. `CASE WHEN status = 'paid' THEN amount ELSE 0 END`), so it never matches a field name.
With more than one dataset, an unqualified compound metric such as `SUM(price * qty)` or `SUM(CASE WHEN status='paid' THEN 1 ELSE 0 END)` is now dropped as `UNSUPPORTED_METRIC_EXPRESSION`, even when all the columns belong to one dataset. Before this change these fell back to `datasets[0]`.
Could we resolve the dataset from the columns referenced in the expression, e.g. by reusing `_get_dataset_qualifiers`? A test with a multi-dataset document and an unqualified compound expression would also help.
##########
converters/dbt/src/ossie_dbt/expression_utils.py:
##########
@@ -48,6 +48,69 @@ def _col_name(node: exp.Expression) -> str:
return node.transform(_unqualify_column).sql()
+_SAFE_SCALAR_NODE_TYPES = {
Review Comment:
This allow-list is quite narrow. It doesn't include `exp.Concat`, `exp.Interval`, `exp.Tuple`, `Cast` variants, date functions like `DateDiff`/`DateAdded`, `Pow`/`Sqrt`/`Exp`, or `Replace`. Metrics like `SUM(DATEDIFF(...))` or `AVG(POWER(x, 2))` converted before and are now dropped.
Could we widen the list for these common cases, or document the intended scope? A few tests showing which expressions are supported would help.
##########
converters/dbt/src/ossie_dbt/ossie_to_msi.py:
##########
@@ -419,20 +415,45 @@ def _find_dataset_for_col(
# Check for a dataset qualifier in the raw expression (e.g. "orders.amount").
# Parse column references instead of splitting the rendered inner expression,
# which may be a compound CASE expression for SUM_BOOLEAN metrics.
- dataset_qualifier = _get_dataset_qualifier(raw_expr_str)
- if dataset_qualifier:
- return dataset_qualifier
+ qualifiers = _get_dataset_qualifiers(raw_expr_str)
Review Comment:
By the way, a qualifier that isn't a dataset name or a unique last segment new raises. That covers aliases and differently cased qualifiers like `o.amount`. Previously the qualifier was passed through as the semantic model name. That was dubious, but it did not fail.
For single-dataset documents, could we resolve to the only dataset when the qualifier doesn't match? That would avoid regression there.
--
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(dbt): preserve aggregate expression semantics [ossie]
Posted by "MrBlack1995 (via GitHub)" <gi...@apache.org>.
MrBlack1995 commented on PR #437:
URL: https://github.com/apache/ossie/pull/437#issuecomment-6172948397
Thanks for this, I came to it from the import side and the core move here is exactly right: one aggregation boundary, with `expr` scalar and `agg` supplying the aggregation, and a refusal rather than an invented `SUM` when the expression cannot be decomposed safely. I am building the same refuse-by-default decomposition for the Microsoft/DAX direction in #584, so it is good to see the dbt converter land on the same contract. Dropping the guessed-`SUM` fallback in favour of a diagnosed `UNSUPPORTED_METRIC_EXPRESSION` is the right trade: a diagnosed drop is far easier to measure and recover than a silent `SUM(SUM(...))`, which is precisely the fidelity distinction the measurement work in #576 is trying to capture.
On the one open point about the scalar allowlist being narrow (the `SUM(DATEDIFF(...))` and `AVG(POWER(x, 2))` regressions): it may be worth anchoring the allowlist to the spec's REQUIRED core in `core-spec/expression_language.md` rather than growing it case by case. The functions flagged as dropped are all in that REQUIRED set already, `DATEDIFF`/`DATEADD` (Date Arithmetic), `CONCAT` (String Manipulation), `POWER`/`EXP` (Advanced Math) and `CAST`, so deriving the safe scalar nodes from the REQUIRED categories would fix those exact cases and keep the boundary principled: everything in the spec's "Not Supported in Expressions" list stays refused, everything in REQUIRED is admitted.
The reason I raise it here rather than just as a dbt detail is that this question, which scalar nodes are safe as the body of an aggregate, is converter independent. The microsoft engine in #584 needs the same answer, and so does any other converter decomposing aggregates. If each converter grows its own allowlist, we get exactly these divergent regressions. Anchoring all of them to the REQUIRED core keeps them consistent, and I am happy to help line the two up.
Either way this is a clear improvement over the double-aggregation behaviour. Thanks for pushing on the refuse-rather-than-guess direction.
--
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(dbt): preserve aggregate expression semantics [ossie]
Posted by "jbonofre (via GitHub)" <gi...@apache.org>.
jbonofre commented on code in PR #437:
URL: https://github.com/apache/ossie/pull/437#discussion_r4062449776
##########
converters/dbt/src/ossie_dbt/expression_utils.py:
##########
@@ -32,17 +32,24 @@ def _col_name(node: exp.Expression) -> str:
"""Return the bare (unqualified) column name from a sqlglot expression node."""
if isinstance(node, exp.Column):
return node.name
- rendered = node.sql()
- return _strip_qualifier(rendered)
+ return node.sql()
+
+
+def _is_scalar_expression(node: exp.Expression) -> bool:
Review Comment:
This classifies "scalar" by blocklisting `exp.AggFunc`/`exp.Window` rather than allowlisting known-safe node types. Any aggregate sqlglot doesn't specifically model (`LISTAGG`, vendor/custom UDAFs) parses as generic `exp.Anonymous` and gets misclassified as scalar, which silently reintroduces the double-aggregation bug this PR is fixing.
Maybe also worth covering scalar subqueries (e.g. `SUM((SELECT x FROM other_table LIMIT 1))`) pass this check too and get embedded verbatim as `expr`.
I suggest inverting to an explicit allowlist of node types known to be safe as a scalar expression body, rather than trying to enumerate everything unsafe.
--
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(dbt): preserve aggregate expression semantics [ossie]
Posted by "christianeu-db (via GitHub)" <gi...@apache.org>.
christianeu-db commented on PR #437:
URL: https://github.com/apache/ossie/pull/437#issuecomment-6333976188
@eisber the converters have moved to their own dedicated repository (https://github.com/apache/ossie-converters). Do you mind moving (creating) the PR on the new repo? Thanks!
--
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(dbt): preserve aggregate expression semantics [ossie]
Posted by "RyutoYoda (via GitHub)" <gi...@apache.org>.
RyutoYoda commented on code in PR #437:
URL: https://github.com/apache/ossie/pull/437#discussion_r4100315970
##########
converters/dbt/src/ossie_dbt/ossie_to_msi.py:
##########
@@ -406,10 +406,24 @@ def _find_dataset_for_col(
def _get_expression(self, ossie_expr: OssieExpression) -> str:
"""Return the expression string for the preferred dialect (fallback: first available)."""
+ dialect_expr = self._get_dialect_expression(ossie_expr)
+ return dialect_expr.expression if dialect_expr is not None else ""
+
+ def _get_dialect_expression(
+ self, ossie_expr: OssieExpression
+ ) -> Optional[OssieDialectExpression]:
+ """Return the expression for the preferred dialect (fallback: first available)."""
for dialect_expr in ossie_expr.dialects:
if dialect_expr.dialect is self._dialect:
- return dialect_expr.expression
- return ossie_expr.dialects[0].expression if ossie_expr.dialects else ""
+ return dialect_expr
+ return ossie_expr.dialects[0] if ossie_expr.dialects else None
+
+ _SQL_DIALECTS = {
Review Comment:
`OSSIE_SQL_2026` is missing from this set, which turns a currently-working case into a dropped metric.
On `main` today, a metric whose expression is written only in `OSSIE_SQL_2026` converts fine, because `_get_expression()` falls back to `dialects[0]`. I checked with the CLI:
```yaml
metrics:
- name: revenue
expression:
dialects:
- dialect: OSSIE_SQL_2026
expression: SUM(orders.amount)
```
`main`: one metric, `type_params.expr = amount`. With this PR, `_get_dialect_expression()` returns that entry, `dialect in self._SQL_DIALECTS` is false, and the metric is dropped with `UNSUPPORTED_METRIC_EXPRESSION`.
The non-SQL dialects (`MDX`, `MAQL`, `DAX`, `TABLEAU`, `SIGMA`, `THOUGHTSPOT`) are rightly excluded, but `OSSIE_SQL_2026` is the spec's own portable SQL, based on ANSI SQL:2003 Core (#439, #440), and `expression_language.md` asks every implementation to support the Ossie dialect. #443, #446 and #447 added it to the equivalent allowlists in orionbelt, databricks and snowflake for this exact reason, so dbt would be the one converter left out.
Suggested fix is one line:
```suggestion
_SQL_DIALECTS = {
OssieDialect.ANSI_SQL,
OssieDialect.OSSIE_SQL_2026,
OssieDialect.BIGQUERY,
OssieDialect.DATABRICKS,
OssieDialect.SNOWFLAKE,
}
```
Unrelated to this point, but worth flagging since it is the same function: #464 changes `_get_expression()` on `main` so that `OSSIE_SQL_2026` is preferred over a vendor dialect rather than resolved by array position (#461). That PR and this one touch the same lines, so whichever lands second will need a rebase — happy for that to be mine.
--
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(dbt): preserve aggregate expression semantics [ossie]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #437:
URL: https://github.com/apache/ossie/pull/437#discussion_r4060814709
##########
converters/dbt/src/ossie_dbt/expression_utils.py:
##########
@@ -71,21 +78,21 @@ def _extract_agg_info(expression: str) -> Optional[Tuple[AggregationType, str, O
and default.name == "0"
and isinstance(ifs[0].args.get("true"), exp.Literal)
and ifs[0].args["true"].name == "1"
+ and _is_scalar_expression(case)
):
return AggregationType.SUM_BOOLEAN, ifs[0].this.sql(), None, False
- return None
- # SUM(col)
- if isinstance(tree, exp.Sum):
+ # SUM(scalar_expr)
+ if isinstance(tree, exp.Sum) and _is_scalar_expression(tree.this):
return AggregationType.SUM, _col_name(tree.this), None, False
Review Comment:
This now accepts compound scalar expressions without checking that all referenced columns belong to one semantic model. For example, `SUM(orders.amount + refunds.amount)` passes `_is_scalar_expression`, but `_get_dataset_qualifier` returns no qualifier for two datasets and `_find_dataset_for_col` then falls back to `datasets[0]`, emitting a SIMPLE metric whose `semantic_model` is `orders` while `expr` still references `refunds.amount`. That manifest cannot faithfully represent the expression; reject multi-dataset/ambiguous references with `UNSUPPORTED_METRIC_EXPRESSION` instead of selecting the first dataset.
--
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(dbt): preserve aggregate expression semantics [ossie]
Posted by "eisber (via GitHub)" <gi...@apache.org>.
eisber commented on code in PR #437:
URL: https://github.com/apache/ossie/pull/437#discussion_r4184084516
##########
converters/dbt/src/ossie_dbt/expression_utils.py:
##########
@@ -71,21 +78,21 @@ def _extract_agg_info(expression: str) -> Optional[Tuple[AggregationType, str, O
and default.name == "0"
and isinstance(ifs[0].args.get("true"), exp.Literal)
and ifs[0].args["true"].name == "1"
+ and _is_scalar_expression(case)
):
return AggregationType.SUM_BOOLEAN, ifs[0].this.sql(), None, False
- return None
- # SUM(col)
- if isinstance(tree, exp.Sum):
+ # SUM(scalar_expr)
+ if isinstance(tree, exp.Sum) and _is_scalar_expression(tree.this):
return AggregationType.SUM, _col_name(tree.this), None, False
Review Comment:
Addressed. Dataset qualifiers are now collected as a set before binding the SIMPLE metric. More than one qualifier, an unknown qualifier, or ambiguous unqualified ownership raises the unsupported path instead of falling back to the first dataset. Added the exact `SUM(orders.amount + refunds.amount)` regression case, which now emits `UNSUPPORTED_METRIC_EXPRESSION` and no metric.
--
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(dbt): preserve aggregate expression semantics [ossie]
Posted by "christianeu-db (via GitHub)" <gi...@apache.org>.
christianeu-db closed pull request #437: fix(dbt): preserve aggregate expression semantics
URL: https://github.com/apache/ossie/pull/437
--
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(dbt): preserve aggregate expression semantics [ossie]
Posted by "eisber (via GitHub)" <gi...@apache.org>.
eisber commented on PR #437:
URL: https://github.com/apache/ossie/pull/437#issuecomment-6341936151
This change was moved to the converters repository in https://github.com/apache/ossie-converters/pull/52, which has now been merged. The replacement PR preserves the fix and review corrections from this PR.
--
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