Bug fix: use FULL OUTER JOIN for dimension-only queries
#863
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
changed the title
| @@ -80,49 +80,7 @@ class DataflowRecipe: | |||
| @property | |||
| def join_targets(self) -> List[JoinDescription]: | |||
| """Joins to be made to source node.""" | |||
| join_targets = [] | |||
| for join_recipe in self.join_linkable_instances_recipes: | |||
There was a problem hiding this comment.
Moved this logic to JoinLinkableInstancesRecipe, which seemed cleaner & more appropriate.
| @@ -71,12 +72,36 @@ class JoinLinkableInstancesRecipe: | |||
| @property | |||
| def join_description(self) -> JoinDescription: | |||
There was a problem hiding this comment.
Prior to this PR, this attribute was not used.
courtneyholcomb
changed the title
FULL OUTER JOIN for dimension-only queries
courtneyholcomb
requested review from
tlento,
plypaul and
WilliamDee
and removed request for
tlento and
plypaul
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
github-actions
bot
removed
the
Run Tests With Other SQL Engines
| @@ -71,12 +72,36 @@ class JoinLinkableInstancesRecipe: | |||
| @property | |||
| def join_description(self) -> JoinDescription: | |||
| @@ -306,6 +336,7 @@ def evaluate_node( | |||
| self, | |||
| start_node: BaseOutput, | |||
| required_linkable_specs: Sequence[LinkableInstanceSpec], | |||
| default_join_type: SqlJoinType, | |||
There was a problem hiding this comment.
Why is this default_join_type? Does it get overridden somehow? Or should it be called join_type here as well?
There was a problem hiding this comment.
This was in preparation for my next PR - we'll need to use CROSS JOIN when querying metric_time with other dimensions (but no metrics).
There was a problem hiding this comment.
... which will override the default join type for dimension-only queries.
| right_source_alias=right_data_set.alias, | ||
| column_equality_descriptions=[], | ||
| join_type=join_description.join_type, | ||
| additional_on_conditions=validity_conditions, |
There was a problem hiding this comment.
Do we want validity_conditions here? Or do we just want to do the full CROSS_JOIN operation and not worry about validity windows?
I guess maybe we want validity windows, but that implicitly requires metric_time.
There was a problem hiding this comment.
I'll confess I don't know anything about the validity conditions, but that behavior was not changed in this PR!
|
Also if you remember please squash and merge this one - I read commit-wise at first but the final PR should be cleaner in the local git history. Thank you! |
|
Makes sense, ship it! |
Description
Realized that we were using
LEFT OUTER JOINfor all queries usingJoinToBaseOutputNode. This made sense for metric queries, but not for dimension-only queries. When querying dimensions, we should use aFULL OUTER JOINsince we want to see all possible combinations of dimension values (including nulls). With theLEFT OUTER JOINwe were arbitrarily choosing one dimension's node as the source node and potentially eliminating values from all other dimensions.