Bug fix: source node evaluation #801
Conversation
|
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. |
courtneyholcomb
force-pushed
the
court/source-node-sort
branch
from
87f82b8 to
ea5072c
Compare
courtneyholcomb
force-pushed
the
court/source-node-sort
branch
from
cc64a47 to
06d08ce
Compare
There was a problem hiding this comment.
Honestly, I'm not sure how you figured this out, most impressive. It makes sense to me, though! You might want to get @plypaul to take a quick look just to make sure I'm not missing something, this is an incredibly dense and unruly part of the codebase.
If this does go through I think it might be time to remove this whole DefaultCost visitor (in a separate PR, of course), as I believe it has outlived its usefulness.
| @@ -553,6 +553,7 @@ def _find_measure_recipe( | |||
| # this is going to be the lowest cost solution. | |||
| if len(evaluation.join_recipes) == 0: | |||
| logger.info("Not evaluating other nodes since we found one that doesn't require joins") | |||
| break | |||
| # All source nodes cost the same. Find evaluation with lowest number of joins. | ||
| node_with_lowest_cost_plan = min( | ||
| node_to_evaluation, key=lambda node: len(node_to_evaluation[node].join_recipes) | ||
| ) | ||
| evaluation = node_to_evaluation[node_with_lowest_cost_plan] | ||
| logger.info( | ||
| "Lowest cost node is:\n" | ||
| "Lowest cost plan is:\n" | ||
| + pformat_big_objects( | ||
| lowest_cost_node=dataflow_dag_as_text(node_with_lowest_cost), | ||
| node=dataflow_dag_as_text(node_with_lowest_cost_plan), | ||
| evaluation=evaluation, | ||
| cost=cost_function.calculate_cost(node_with_lowest_cost), | ||
| joins=len(node_to_evaluation[node_with_lowest_cost_plan].join_recipes), |
There was a problem hiding this comment.
Nice find!
We have Yet Another Inscrutable Visitor doing this complex cost calculation, but it can only go from leaf node to root node, not the other way around. Since measure nodes are currently ONLY source nodes the cost comparison is pointless.
Using minimum join count is probably the right answer here. Later on it'll matter more what the user requests, because eventually the measure nodes could be on the right. In theory, this kind of graph walk cost computation will be useful then, but my worry about this block of logic was always that it the results might diverge as the join layout changes. However, since we have now committed to always picking the shortest join paths I think this gets us closer to where we need to be.
|
@courtneyholcomb It would be preferable to encapsulate all costing using a centralized class, but that might be a more involved fix. If this is blocking you, we could merge this and do a follow up later. |
|
@plypaul at the moment we don't do any costing at all. I say we just remove _sort_by_suitability and all of the DataflowPlanCost stuff and drop the pretense. If we find a use for a more involved centralized cost computation setup we can add one then. |
@tlento we do use |
@plypaul |
I should learn to read, missed the tuple return in there..... |
Yeah, fine to remove as well actually. The original use case was for the internal caching implementation, but that's no longer relevant. |
Description
Fixes a bug in how we determine which source node to use. Previously, we were choosing a plan based on source node cost. Source nodes can only be
ReadSqlSourceNodeorMetricTimeDimensionTransformNode, so they all haveDefaultCost(num_joins=0, num_aggregations=0). That means we were just arbitrarily choosing a source node that could satisfy the query. Here, the logic changes to use the number of joins in each node'sLinkableInstanceSatisfiabilityEvaluation, so we choose the dataflow plan with the lowest number of joins.Note that this fixes a bug that came up in the process of enabling querying dimensions without metrics.
Also: we have a log that says
Not evaluating other nodes since we found one that doesn't require joins, but we don't stop evaluating nodes at that point. I added a break to the loop to fix that, too.