Improved source node logic for custom granularities #1427
Conversation
courtneyholcomb
changed the title
courtneyholcomb
force-pushed
the
court/custom-grain-where-2
branch
from
50f3ca7 to
dc20356
Compare
courtneyholcomb
force-pushed
the
court/top-of-stack
branch
from
0cc1e9f to
6dcf27a
Compare
courtneyholcomb
force-pushed
the
court/custom-grain-where-2
branch
from
dc20356 to
3feccdc
Compare
courtneyholcomb
force-pushed
the
court/top-of-stack
branch
from
6dcf27a to
b940157
Compare
courtneyholcomb
force-pushed
the
court/custom-grain-where-2
branch
from
06fec60 to
f035679
Compare
courtneyholcomb
force-pushed
the
court/top-of-stack
branch
from
b940157 to
447a7d0
Compare
| # Replace any custom granularities with their base granularities. The custom granularity will be joined in | ||
| # later, since custom granularities cannot be satisfied by source nodes. But we will need the dimension at | ||
| # base granularity from the source node in order to join to the appropriate time spine later. |
| @@ -102,7 +102,7 @@ | |||
|
|
|||
|
|
|||
| @dataclass(frozen=True) | |||
| class DataflowRecipe: | |||
| class SourceNodeRecipe: | |||
| spec.with_base_grain() if isinstance(spec, TimeDimensionSpec) else spec | ||
| for spec in required_linkable_specs.as_tuple | ||
| ] | ||
| required_linkable_specs.replace_custom_granularity_with_base_granularity().as_tuple |
There was a problem hiding this comment.
Ok, this class is fairly complex, so I'm going to give up and just ask - I thought that by moving one of the replace_custom_granularity_with_base_granularity calls to _find_source_node_recipe, we obviated the need to do this again anywhere else. Was that impression wrong?
There was a problem hiding this comment.
You're right, that was the plan! I hadn't realized this use case was outside of that function.
I took a stab at seeing if we could add this spec set as a property to the SourceNodeRecipe to avoid calculating it again here, which would clean things up a bit. It works everywhere except for conversion metrics, which have a separate SourceNodeRecipe that they build. I tried to mess around with that code to make it work but I'm not familiar enough with conversion metrics to make any significant changes without consulting someone so I scrapped it.
Basically the blocker is that we haven't consolidated this code enough to make this simple. I would love to clean up the whole DataflowPlanBuilder to remove redundancies, and I we would certainly be able to clean this piece up then, but I think that's too big a task to block this feature.
|
Please take a look at #1427 (comment) before committing. |
courtneyholcomb
force-pushed
the
court/custom-grain-where-2
branch
from
f035679 to
6f78489
Compare
courtneyholcomb
force-pushed
the
court/top-of-stack
branch
from
447a7d0 to
67ed80c
Compare
The dataflow recipe is essentially the best choice for the source node to select from and how to join any other source nodes required to satisfy all requested specs. The name is confusing, though, since the dataflow PLAN is the entire DAG, whichextends far beyond the source nodes. Rename to source node recipe in order to make it more clear that this part of the dataflow plan is only dealing with source nodes.
Consolidates that logic into _find_source_node_recipe() as much as possible, removes isinstance() checks, and clarifies comments.
courtneyholcomb
force-pushed
the
court/top-of-stack
branch
from
67ed80c to
dd7eb7c
Compare
Based on feedback in #1409, this PR makes some updates to the dataflow plan builder. These are only refactoring changes, there should be no behavioral changes. Changes include renaming some things and simplifying the code related to converting custom granularities to base granularities in source node resolution. Reviewing by commit is recommended - full details in commit descriptions.