Use correct join node specs for GroupByMetrics
#1193
Conversation
…t pruned We only build them when we know we need them anyway!
… metrics Also updates documentation in this function since I had a hard time following the logic previously.
courtneyholcomb
force-pushed
the
court/gb-metric-test2
branch
from
acf40aa to
2cc544a
Compare
courtneyholcomb
changed the title
GroupByMetrics
courtneyholcomb
added
the
Run Tests With Other SQL Engines
courtneyholcomb
temporarily deployed
to
DW_INTEGRATION_TESTS
GitHub Actions
Inactive
courtneyholcomb
temporarily deployed
to
DW_INTEGRATION_TESTS
GitHub Actions
Inactive
courtneyholcomb
temporarily deployed
to
DW_INTEGRATION_TESTS
GitHub Actions
Inactive
courtneyholcomb
temporarily deployed
to
DW_INTEGRATION_TESTS
GitHub Actions
Inactive
courtneyholcomb
removed
the
Run Tests With Other SQL Engines
| @@ -386,7 +386,7 @@ def _build_conversion_metric_output_node( | |||
| metric_spec=metric_spec, | |||
| aggregated_measures_node=aggregated_measures_node, | |||
| for_group_by_source_node=for_group_by_source_node, | |||
| aggregated_to_elements=queried_linkable_specs.as_reference_set, | |||
| aggregated_to_elements=set(queried_linkable_specs.as_tuple), | |||
There was a problem hiding this comment.
Oh right because time dimensions or joined elements can vary.....
There was a problem hiding this comment.
Yeah, previously we had no need for the full spec but now we do!
| ) | ||
| # Get the specs needed to join onto this node. | ||
| if self.node_to_join.aggregated_to_elements: | ||
| include_specs.extend(self.node_to_join.aggregated_to_elements) |
There was a problem hiding this comment.
Did we want these as-is or did we want to convert them to the LinklessEntitySpec types at this point?
There was a problem hiding this comment.
We want them as-is. The reason they were always LinklessEntitySpecs previously is that we were always joining to source nodes (which would not have joins contained WITHIN them). In this case, we are joining to a node that has joins within it, so its instance specs might have entity links.
| if include_specs_not_found: | ||
| raise RuntimeError( | ||
| f"Include specs {include_specs_not_found} are not in the spec set {instance_set.spec_set} - " | ||
| f"check if this node was constructed correctly." |
There was a problem hiding this comment.
nit: this is an odd follow up. If I think it was constructed correctly and I hit this error, what does that indicate?
There was a problem hiding this comment.
It indicates that you either 1) constructed the instance set correctly or 2) constructed the node incorrectly, since they don't match!
There was a problem hiding this comment.
This was related to this PR because I ran into this error and that's why I needed to change the instance specs built in the JoinDescription. Definitely could have been split into another PR though.
| f"check if this node was constructed correctly." | ||
| ) | ||
| exclude_specs_not_found.append(exclude_spec) | ||
| if exclude_specs_not_found: |
There was a problem hiding this comment.
It is very nice to get all of these at once. 😁
..._filter_rendering.py/SqlQueryPlan/DuckDB/test_multi_hop_inner_join_path__plan0_optimized.sql
Outdated
Show resolved
Hide resolved
Description
In the process of writing rendering tests, I found an issue with the logic used to determine which specs to include in a joined node. This PR primarily updates that logic to handle
GroupByMetricSpecs, plus with some related adjustments along the way.