Make LinkabelElementSet non-optional in FilterSpecResolution #1176
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. |
| specs = self.resolved_linkable_element_set.specs | ||
| if len(specs) == 0: | ||
| return None | ||
| elif len(specs) == 1: | ||
| return specs[0] | ||
| else: | ||
| raise RuntimeError(f"Found {len(specs)} in {self.resolved_linkable_element_set}") | ||
| raise ValueError( |
There was a problem hiding this comment.
Curious why we keep it typed as a LinkableElementSet instead of just a LinkableElement if there can only be one?
There was a problem hiding this comment.
This method is typed to return only one spec (or None) because that's historically what we've needed.
The LinkableElementSet we keep in the dataclass is there because the set maps the ElementPathKey (which is convertible to a LinkableInstanceSpec) to a collection of LinkableElement objects. This allows us to get the complete set of SemanticModel inputs for a given spec - like if a given Entity is sourced from multiple Semantic Models in the final joined query, that should be reflected here.
So we don't strictly need the LinkableElementSet, we can probably get away with extracting the spec and collection of LinkableElement objects, but the set is a little more convenient.
I think this utility becomes a little more obvious at a property access level with the remaining changes in the stack, but it might be worth an update to the dataclass docstring.
The LinkableElementSet object is always present in the extant initializer calls for FilterSpecResolution. This change allows us to acces it without having to handle the Optional type everywhere, and prevents future drift into mayhem as we inspect this object for things like predicate pushdown evaluation.
tlento
force-pushed
the
non-optional-linkable-set
branch
from
c40e6b1 to
7c580e1
Compare
The LinkableElementSet object is always present in the extant
initializer calls for FilterSpecResolution. This change allows
us to acces it without having to handle the Optional type everywhere,
and prevents future drift into mayhem as we inspect this object
for things like predicate pushdown evaluation.