Remove generic source dataset constructs #768
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
The original implementation of MetricFlow used a generic type annotation for the "Source DataSet" of the dataflow plan. This was in place to support some specific implementations of Transform's proprietary product logic, and as an interface boundary it proved ineffective. Consequently, attempts to revisit that functionality will rely on other approaches. Within MetricFlow, this generic type resolves, to one of two concrete types: the SqlDataSet, or the SemanticModelDataSet, which inherits from SqlDataSet. Since it's far simpler to just use SqlDataSet as the required type than to litter the codebase with generic typehints and concrete type specs in some places but not others, we make that change here. Sadly, this must all be done in one massive commit, because there's no way to get the typechecker to pass by removing this one stage at a time without doing considerably more restructuring around the data set handoffs. This commit was produced via a mixture of mechanical find/replace approaches within VSCode, along with file-by-file manual fixups for cases where leftovers were causing trouble.
In cleaning up our dataset module organization I noticed this helper class was not used anywhere. This removes it.
The other DataSet classes are all in one place, and this is no longer strictly a plan_conversion artifact (indeed, it never was, it just looked that way due to the generics).
|
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. |
plypaul
approved these changes
Sep 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.
The original implementation of MetricFlow used a generic type
annotation for the "Source DataSet" of the dataflow plan. This was
in place to support some specific implementations of Transform's
proprietary product logic, and as an interface boundary it proved
ineffective. Consequently, attempts to revisit that functionality
will rely on other approaches.
Within MetricFlow, this generic type resolves, to one of two concrete types:
the SqlDataSet, or the SemanticModelDataSet, which inherits from
SqlDataSet. Since it's far simpler to just use SqlDataSet as the
required type than to litter the codebase with generic typehints and
concrete type specs in some places but not others, we make that change
along with some associated cleanup. In particular, the SqlDataSet class
rightly belongs with the other DataSet subclasses, rather than in the
plan_conversion package.