Fix incorrect cumulative metric output when start-time and end-time are used #871
Conversation
This file contains 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
Cumulative metric queries are very difficult to reason about, especially when we do automated date sliding to include all of the necessary input data. This commit adds some output tests, which can be compared against the input data config files for fct_bookings and fct_revenue for correctness.
When users submit CLI queries with the --start-time and --end-time parameters, MetricFlow will automatically adjust the time constraint window to include all relevant cumulative metric input. Unfortunately, we got over-zealous with fencepost adjustments and introduced an off-by-one error that meant cumulative metric queries would only be correct when the specified granularity was DAILY. We never caught this issue because the error in the rendered SQL is subtle, and all of our test input data on the integration tests happened to fit within the artificially shortened constraint windows in all of the cases where we were testing non-daily granularity queries. This change updates the integration test configurations to ensure boundary conditions will be caught in the future. In addition, the fix alters the values of the query output snapshot tests, which, when compared with the relevant input test data, will reveal the nature of the original bug and the effects of this fix.
tlento
commented
Nov 15, 2023
| @@ -80,7 +80,7 @@ def adjust_time_constraint_for_cumulative_metric( | |||
| ) -> TimeRangeConstraint: | |||
| """Given a time constraint for the overall query, adjust it to cover the time range for this metric.""" | |||
| if granularity is not None: | |||
| return self._adjust_time_constraint_start_by_window(granularity, count - 1) | |||
| return self._adjust_time_constraint_start_by_window(granularity, count) | |||
There was a problem hiding this comment.
This is the actual fix.
| @@ -6,7 +6,7 @@ integration_test: | |||
| metrics: ["trailing_2_months_revenue"] | |||
| group_bys: ["metric_time"] | |||
| order_bys: ["metric_time"] | |||
| time_constraint: ["2020-01-05", "2021-01-04"] | |||
| time_constraint: ["2020-03-05", "2021-01-04"] | |||
There was a problem hiding this comment.
I left these tests in place for a couple of reasons, but one is to illustrate how the constraint adjustments need to be applied here vs in the where clause.
A number of these were producing correct results on inspection of input data only because the input data didn't start until within the too-short filter window.
| 1 2021-01-04 2000 | ||
| 2 2021-01-05 2000 | ||
| 3 2021-01-06 2000 | ||
| 0 2021-01-03 3000 |
There was a problem hiding this comment.
These updated numbers are correct, while the old ones were missing inputs. See https://github.com/dbt-labs/metricflow/blob/main/metricflow/test/fixtures/source_table_snapshots/simple_model/fct_revenue.yaml#L15-L18
2 tasks
courtneyholcomb
approved these changes
Nov 15, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
When users submit CLI queries with the --start-time and --end-time
parameters, MetricFlow will automatically adjust the time constraint
window to include all relevant cumulative metric input.
Unfortunately, we got over-zealous with fencepost adjustments and
introduced an off-by-one error that meant cumulative metric
queries would only be correct when the specified granularity was
DAILY.
We never caught this issue because the error in the rendered SQL is
subtle, and all of our test input data on the integration tests
happened to fit within the artificially shortened constraint windows
in all of the cases where we were testing non-daily granularity
queries.
This change updates the integration test configurations to ensure
boundary conditions will be caught in the future. In addition, the
fix alters the values of the query output snapshot tests, which,
when compared with the relevant input test data, will reveal the
nature of the original bug and the effects of this fix.