[DBT] Emit OFFSET_SEMANTICS_LOSS when a DERIVED metric input uses offset_window/offset_to_grain - #371
Open
georgea-tk wants to merge 1 commit into
Open
Conversation
…set_window/offset_to_grain MSIToOssieConverter._resolve_derived reads alias, name and filter off each DERIVED metric input but never offset_window or offset_to_grain. A current-vs-offset comparison (e.g. YoY) resolves both inputs to byte-identical SQL, silently producing an always-zero expression with no ConverterIssue raised. Emits OFFSET_SEMANTICS_LOSS, following the existing CUMULATIVE_SEMANTICS_LOSS pattern, whenever a DERIVED metric declares an input with offset_window or offset_to_grain set. This is the minimum fix requested in apache#302: it turns a silent wrong answer into a visible, gateable issue. Representing the offset semantics themselves is a separate spec question, since OSI/Ossie currently has nothing to carry a time offset. Fixes apache#302 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Comment on lines
+113
to
+119
| if metric.type is MetricType.DERIVED and any( | ||
| input_metric.offset_window or input_metric.offset_to_grain | ||
| for input_metric in metric.type_params.metrics or [] | ||
| ): | ||
| issues.append( | ||
| ConverterIssue(issue_type=ConverterIssueType.OFFSET_SEMANTICS_LOSS, element_name=metric.name) | ||
| ) |
Contributor
There was a problem hiding this comment.
I just found https://github.com/apache/ossie/pull/318/changes by @fanruan-tuco, which has some elements that could be adopted. I like the stronger-typed message from this PR and the recursive approach from the other PR, which could capture more cases.
Perhaps, this would be a good opportunity to collaborate & merge the two PRs?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
MSIToOssieConverter._resolve_derivedreadsalias,nameandfilteroff each DERIVED metric input but neveroffset_windoworoffset_to_grain. A current-vs-offset comparison (e.g. a YoY metric with one input at the current period and one offset by a year) resolves both inputs to byte-identical SQL, so the derived expression becomes something like(SUM(x) - SUM(x)) / NULLIF(SUM(x), 0) * 100— always zero, withConverterResult.issuesempty. A plausible-looking wrong number with no signal that anything was dropped.This PR adds the minimum fix requested in #302: a new
OFFSET_SEMANTICS_LOSSConverterIssueType, emitted whenever a DERIVED metric declares an input withoffset_windoworoffset_to_grainset, following the existingCUMULATIVE_SEMANTICS_LOSSpattern. It does not attempt to represent the offset semantics in the emitted expression — OSI/Ossie has no concept of a time offset today, so full representation is a separate spec question, not a converter one.Related Issues
Fixes #302
Checklist
Specification
core-spec/and follow the existing structure — N/A, no spec changeOntology
ontology/are consistent with spec changes — N/AConverters
converters/is updated to reflect spec or ontology changesconverters/dbt/tests/Validation
validation/are updated if the spec changed — N/ADocumentation
docs/is updated to reflect any user-facing changes — N/A, this is an internalConverterIssuesignal, no user-facing doc surfaceCONTRIBUTING.mdis updated if the contribution process changed — N/AExamples
examples/are added or updated for any new spec constructs or converter support — N/ATests
pytest/ CI green) — ran fullconverters/dbtsuite locally, 102 passedoffset_window,offset_to_grain, and a negative case confirming an ordinary DERIVED metric with no offset does not emit the issueCompliance