Follow-Ups From Ambiguous Group-By-Item Resolution Stack #946
Conversation
| @@ -31,12 +31,12 @@ | |||
| QueryGroupByItemResolutionNode, | |||
| ) | |||
| from metricflow.query.group_by_item.resolution_path import MetricFlowQueryResolutionPath | |||
| from metricflow.query.issues.group_by_item_resolver.invalid_use_of_date_part import MetricExcludesDatePartIssue | |||
There was a problem hiding this comment.
Nice, that's much better, thanks.
| ) or any(spec_resolution.issue_set.has_errors for spec_resolution in self.spec_resolutions) | ||
|
|
||
| @property | ||
| def has_issues(self) -> bool: # noqa: D |
There was a problem hiding this comment.
Some of these things have issues.....
Sorry, it's almost vacation.
There was a problem hiding this comment.
To clarify, you have a name proposal?
| issue_set=MetricFlowQueryResolutionIssueSet.empty_instance(), | ||
| if any(resolution.issue_set.has_errors for resolution in resolutions) or len(non_parsable_resolutions) > 0: | ||
| return FilterSpecResolutionLookUp( | ||
| spec_resolutions=tuple(resolution for resolution in resolutions if resolution.issue_set.has_errors), |
There was a problem hiding this comment.
Ok, this is filtering to only the ones with errors now. I assume that is to remove "good" resolutions from the output, correct?
There was a problem hiding this comment.
Yeah, I removed the good ones since it made the snapshots smaller and if there are any errors, we wouldn't proceed with the query anyway.
| dimension_call_parameter_sets: List[DimensionCallParameterSet] = [] | ||
| time_dimension_call_parameter_sets: List[TimeDimensionCallParameterSet] = [] | ||
| entity_call_parameter_sets: List[EntityCallParameterSet] = [] | ||
|
|
||
| # FilterCallParameterSets needs an update. | ||
| for filter_call_parameter_sets in filter_call_parameter_sets_sequence: | ||
| for dimension_call_parameter_set in filter_call_parameter_sets.dimension_call_parameter_sets: | ||
| if dimension_call_parameter_set not in dimension_call_parameter_sets: | ||
| dimension_call_parameter_sets.append(dimension_call_parameter_set) | ||
| for time_dimension_call_parameter_set in filter_call_parameter_sets.time_dimension_call_parameter_sets: | ||
| if time_dimension_call_parameter_set not in time_dimension_call_parameter_sets: | ||
| time_dimension_call_parameter_sets.append(time_dimension_call_parameter_set) | ||
| for entity_call_parameter_set in filter_call_parameter_sets.entity_call_parameter_sets: | ||
| if entity_call_parameter_set not in entity_call_parameter_sets: | ||
| entity_call_parameter_sets.append(entity_call_parameter_set) |
There was a problem hiding this comment.
As long as the call parameter sets remain human scale input I guess it's ok, but the advantage of the previous dict-based approach is we don't need to do this separate n^2 deduplication process.
From a readability and "there's a hidden explosive in my codebase" perspective the dict approach is probably preferable as well - these are private methods and we have no use cases for allowing duplicates here.
There was a problem hiding this comment.
I changed to this to track down a testing consistency issue, so partially changed it back. This will get rewritten with the change to FilterCallParameterSets.
plypaul
force-pushed
the
plypaul--70--agbir-follow-up
branch
from
fe9fa04 to
06f2b0f
Compare
PathPrefixable was used in a previous implementation to generate paths, but with DagTraversalPathTracker, it's not needed in some cases. However, there is later use case for error message generation where a signature change would be cleaner.
plypaul
force-pushed
the
plypaul--70--agbir-follow-up
branch
from
06f2b0f to
54a9d04
Compare
Description
This PR addresses some of the feedback in the PR stack for ambiguous group-by-item resolution and includes some re-organization of the associated code (e.g. renames / additional context fields). Please view by commit.