Clean up object and module organization #225
Merged
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
|
Current dependencies on/for this PR:
This comment was auto-generated by Graphite. |
We occasionally need to convert WhereClauseConstraint objects to SpecWhereClauseConstraint objects. For historical reasons this logic was held in the MetricFlowQueryParser, but keeping it there does not make a lot of organizational sense. Worse, it sets up potential circular dependencies, as it forces the query parser to depend on our semantic containers, and the dataflow plan builder to depend on the query parser. Here we split this helper into its own independent class. We cannot move it onto either the source or target classes because of other concerns around possible circular dependencies, which we have documented in the new module.
The AggregationType enum was included with the Measure model object because that was where it was first needed, but its utility is quite a bit broader than as a model enumeration. Since its current location leads to potential circular imports, and additional aggregation properties (such as additiveness) might be coming in the future, we move it to a root aggregation_properties module.
Specs are meant to be relatively mutable constructs that can change depending on query constraints, while model objects are meant to be consistent to the user defined metric model. Therefore, we split the active dataflow state operations into a separate NonAdditiveDimensionSpec class, and populate its properties from the NonAdditiveDimensionParams model object.
This nomenclature is a little confusing, so we name all of the variables pointing at NonAdditiveDimensionSpec accordingly.
tlento
force-pushed
the
add-semantic-container-protocols
branch
from
78ffcdb
to
c5eb2f9
Compare
tlento
force-pushed
the
streamline-dependencies
branch
from
9234a81
to
3da103b
Compare
WilliamDee
approved these changes
Aug 29, 2022
| @@ -0,0 +1,39 @@ | |||
| from __future__ import annotations | |||
There was a problem hiding this comment.
what if we had like a metricflow/model/properties/ directory that stores these? I'm not aware of the best practices, but personally I like how we have most things organized in their respective directories rather than being in the root directory.
There was a problem hiding this comment.
I think that'd be fine, although I pulled these to root because I think of them as being on the level of specs and suchlike. To be honest I thought about moving those into a sub-directory as well, but decided not to get too carried away, so I'm open to some simple file move reorganization PRs.
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.
We have a number of slightly wonky organizational artifacts in our
codebase which have been cropping up as circular imports of late.
Most recently, issues with conversion between WhereClauseConstraint
and SpecWhereClauseConstraint are blocking update and merge on
#153 , but that's not the only issue we've been encountering.
This PR does the following in order to tidy things up and bring us
a bit closer to the layout described in #214: