Coerce time granularity to configured value in all cases #797
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
The time dimension conversion logic inside of our semantic model to data set converter class has expanded to the point where it's difficult to reason through where or how to best make changes. In order to at least keep added layers of logic isolated to the specific task of converting time dimensions, this commit moves that logic into a separate private method.
|
Current dependencies on/for this PR:
This comment was auto-generated 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
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
tlento
temporarily deployed
to
DW_INTEGRATION_TESTS
GitHub Actions
Inactive
2 tasks
MetricFlow's initial time dimension select statement rendering was built on the assumption that the granularity configred for a given time dimension would always match the granularity of the values stored. For example, if a time dimension was defined with granularity DAY, we assumed that the values in the warehouse would always correspond to daily granularity data, and would never be offset from a day boundary. This assumption is flawed for a variety of reasons - yearly or quarterly granularities might use different offset boundaries, users may misconfigure dimensions or allow offset values into the dataset which could be safely corrected, etc. This change updates our rendering to apply the date_trunc operation on the base time dimension column, just as we do for the other, coarser granularities. The one exception is for the base granularity column for validity window time dimensions, which must retain their underlying granularity in order to remain viable as validity window boundaries for any DAILY granularity contexts, as we do not support smaller than daily granularities at this time. Note this may cause queries with start and end time constraints to no longer trigger partition pruning, an issue which will be addressed in subsequent updates.
tlento
force-pushed
the
coerce-date-time-granularities
branch
from
ef76ee3
to
6260b7a
Compare
|
Warehouse test action run: https://github.com/dbt-labs/metricflow/actions/runs/6423205270/job/17441226665?pr=797 |
plypaul
approved these changes
Oct 5, 2023
Base automatically changed from
clean-up-sql-time-interval-math-expression
to
main
October 5, 2023 22:20
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.
MetricFlow's initial time dimension select statement rendering
was built on the assumption that the granularity configred for a given
time dimension would always match the granularity of the values stored.
For example, if a time dimension was defined with granularity DAY, we
assumed that the values in the warehouse would always correspond to
daily granularity data, and would never be offset from a day boundary.
This assumption is flawed for a variety of reasons - yearly or quarterly
granularities might use different offset boundaries, users may
misconfigure dimensions or allow offset values into the dataset which
could be safely corrected, etc.
This change updates our rendering to apply the date_trunc operation on
the base time dimension column, just as we do for the other, coarser
granularities. The one exception is for the base granularity column
for validity window time dimensions, which must retain their underlying
granularity in order to remain viable as validity window boundaries
for any DAILY granularity contexts, as we do not support smaller than
daily granularities at this time.
Note this may cause queries with start and end time constraints to
no longer trigger partition pruning, an issue which will be addressed
in subsequent updates.