Consume new cumulative_type_params fields
#1293
Merged
+198
−242
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
courtneyholcomb
force-pushed
the
court/cumulative-type-params
branch
4 times, most recently
from
cbb1d71 to
6fa4f5e
Compare
This will ensure that any window or grain_to_date set in the type_params fields will be copied into the corresponding cumulative_type_params fields.
Left some with the old structure to ensure the transformation is working as expected.
courtneyholcomb
force-pushed
the
court/cumulative-type-params
branch
from
225b8ba to
ee1b43c
Compare
courtneyholcomb
added
the
Run Tests With Other SQL Engines
courtneyholcomb
had a problem deploying
to
DW_INTEGRATION_TESTS
GitHub Actions
Failure
courtneyholcomb
had a problem deploying
to
DW_INTEGRATION_TESTS
GitHub Actions
Failure
courtneyholcomb
had a problem deploying
to
DW_INTEGRATION_TESTS
GitHub Actions
Failure
courtneyholcomb
had a problem deploying
to
DW_INTEGRATION_TESTS
GitHub Actions
Failure
tlento
reviewed
Jun 20, 2024
metricflow-semantics/extra-hatch-configuration/requirements.txt
Outdated
Show resolved
Hide resolved
| @@ -25,6 +26,7 @@ def parse_manifest_from_dbt_generated_manifest(manifest_json_string: str) -> Pyd | |||
| # The serialized object in the dbt project does not have all transformations applied to it at | |||
| # this time, which causes failures with input measure resolution. | |||
| # TODO: remove this transform call once the upstream changes are integrated into our dependency tree | |||
| # TODO: align rules between DSI, here, and MFS (if possible!) | |||
There was a problem hiding this comment.
Oh wow. These were all supposed to be removed ages ago but I think we just always applied them. I don't know if we can stop now, I guess we'll see.
We might want to move this to the CLI package in the future.
| FROM ( | ||
| -- Join Self Over Time Range | ||
| -- Pass Only Elements: ['txn_revenue', 'metric_time__week', 'metric_time__day'] | ||
| -- Aggregate Measures | ||
| -- Compute Metrics via Expressions | ||
| SELECT | ||
| subq_12.metric_time__day AS metric_time__day |
There was a problem hiding this comment.
Wait, this doesn't look right. We're now doing the initial aggregation against WEEK instead of DAY, and then taking the average of those grouped by week, which is effectively a no-op. Is that expected?
There was a problem hiding this comment.
Shoot, you're right. I wonder if the column pruner got to it - investigating 🧐
There was a problem hiding this comment.
Definitely seems like an optimizer issue since the non-optimized SQL looks right.
There was a problem hiding this comment.
@tlento fixed this bug in the latest 3 commits.
tlento
reviewed
Jun 20, 2024
…roup by Paul had a TODO up for this already, but this appears to be the first circumstance that made handling it necessary.
courtneyholcomb
added
Run Tests With Other SQL Engines
courtneyholcomb
temporarily deployed
to
DW_INTEGRATION_TESTS
GitHub Actions
Inactive
courtneyholcomb
temporarily deployed
to
DW_INTEGRATION_TESTS
GitHub Actions
Inactive
courtneyholcomb
temporarily deployed
to
DW_INTEGRATION_TESTS
GitHub Actions
Inactive
courtneyholcomb
temporarily deployed
to
DW_INTEGRATION_TESTS
GitHub Actions
Inactive
github-actions
bot
removed
the
Run Tests With Other SQL Engines
tlento
approved these changes
Jun 20, 2024
| @@ -8,23 +8,19 @@ FROM ( | |||
| metric_time__week | |||
| , t2mr | |||
| FROM ( | |||
| -- Compute Metrics via Expressions | |||
There was a problem hiding this comment.
Oh that's weird, but I guess it makes sense?
There was a problem hiding this comment.
Yeah I think they just get optimized in a different way with this change. But not an incorrect way...
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.
Completes SL-2413
Consume new
cumulative_type_paramsfields (windowandgrain_to_date) instead oftype_paramscumulative fields. Eventually we will retire thetype_paramsfields altogether. For now, we have a transformation that copies the fields fromtype_paramstocumulative_type_params, if not yet set. This ensures that we only need to consume the new fields here.This is the last step needed to enable non-default granularities for cumulative metrics in MF 🥳