Expand test coverage for cumulative and derived offset metrics #856
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
These were all copy/pasted before and not really covering the reality of what the tests were doing.
Cumulative metric query rendering currently follows the same pattern as all other metrics, namely, we join -> filter -> aggregate. The problem is, if the filter specifies a restrictive time span, we may apply the filter on the inputs to the aggregations, which will produce incorrect cumulative metric outputs. This will happen in any case where the dimension `metric_time` is not incorporated into the query, as illustrated by the changes to the plan files for test_cumulative_metric_with_time_constraint. For now, we simply illustrate current behavior with the test cases in preparation for adding a validation enhancement to prevent bad queries from being processed. We will expand support for cumulative metric queries beyond metric_time in a follow-up.
Derived metric offsets handle time filters correctly provided the query depends on the `metric_time` dimension. Since the DataflowPlanBuilder has an invariant in its runtime to ensure that metric_time is included in the requested query specs, this is always true in practice. Additional tests for different query patterns may be warranted when we expand support. For the time being, we simply add an illustrative query rendering test for the behavior when filter spans are at odds with the offset window selection.
|
Current dependencies on/for this PR:
This stack of pull requests is managed by Graphite. |
|
Thank you for your pull request! We could not find a changelog entry for this change. For details on how to document a change, see the contributing guide. |
tlento
added
Skip Changelog
Run Tests With Other SQL Engines
tlento
temporarily deployed
to
DW_INTEGRATION_TESTS
GitHub Actions
Inactive
tlento
temporarily deployed
to
DW_INTEGRATION_TESTS
GitHub Actions
Inactive
tlento
temporarily deployed
to
DW_INTEGRATION_TESTS
GitHub Actions
Inactive
tlento
temporarily deployed
to
DW_INTEGRATION_TESTS
GitHub Actions
Inactive
github-actions
bot
removed
the
Run Tests With Other SQL Engines
courtneyholcomb
approved these changes
Nov 8, 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.
Current test coverage for cumulative and derived offset metrics does not include
any tests of how we render queries with filters that do not allow us to do any
expansion on metric_time. This will be necessary as we add support for
detecting metric_time filter expressions and full predicate pushdown, as we will need
a way to verify that our logic is still rendering queries that will produce the correct results.
Worth noting, in this PR, that we currently have a number of rendering scenarios for
cumulative metrics where the queries are incorrect. As these are meant to be blocked by
query validation, I am leaving most of them intact, with the idea that when we properly
support those scenarios the rendering tests will update to reflect the fixes.