Improve robustness of LinkableElementSet operations #1145
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This was referenced Apr 23, 2024
This stack of pull requests is managed by Graphite. Learn more about stacking. |
|
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. |
There are some comments that are a bit outdated and confusing, or at the very least not terribly useful, in the first sections of the LinkableElementSet, so we remove them. This was found in a modification to the LinkableDimension class, as the longer comment makes a reference to it.
Currently the ElementPathKey is agnostic to the type of element it represents. In some cases this will be fairly obvious - if the time granularity is set it is a time dimension. In others we can determine this by inspection - if the name and links resolve to a distinct dimension in a semantic model then it must be a dimension, and similarly for an entity. However, there is the possibility of some ambiguity in rare cases where an entity and dimension might end up with the same identifier/link combinations. This doesn't matter in the current usage of the ElementPathKey, which is always bound to an element subtype, but when we add support for predicate pushdown we'll need to be able to use this key to fetch the specific set of LinkableElement classes, and this means we must be able to easily get a determinstic type. This commit adds the elmeent type annotation we require, and does some additional related work to ensure that time dimensions and dimensions are always classified correctly when the LinkableDimension instance is created.
The LinkableElementSet has some helper methods to assist with certain gnarly operations, but these are largely untested. Since tests are the best documentation of behavior, we add some here. This commit also includes some updates to documentation to clarify behavior under certain common scenarios.
tlento
force-pushed
the
remove-granularity-free-time-dimension-element-path-keys
branch
from
d1a6803 to
8164aaf
Compare
tlento
force-pushed
the
make-linkable-element-set-more-robust
branch
from
9159ec0 to
9ff7465
Compare
Base automatically changed from
remove-granularity-free-time-dimension-element-path-keys
to
main
April 23, 2024 21:19
plypaul
approved these changes
Apr 23, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The LinkableElementSet is a container class for a bunch of metadata-carrying
objects related to available join paths. It keys all of these objects by an
ElementPathKey, and a number of helpers for some fairly complex operations
meant to enable merging or intersecting multiple sets.
The LinkableElementSet operates under a number of underlying assumptions
which are not codified anywhere, and, indeed, there are places in the
codebase that do not conform to these assumptions.
First, the ElementPathKey is assumed to match the type of the elements
associated with it, but there is no mechanism for enforcing this, nor
is there a way to determine the element type directly from the path key
itself. This is an issue because we use the ElementPathKey independently
of the LinkableElements in a number of places.
Second, the ElementPathKey does not bind to an underlying DimensionType,
rather, the distinction between time dimensions and categorical dimensions
is based on whether or not the optional time_granularity property is set
in the ELementPathKey.
Finally, there are inconsistencies in when and whether the helper methods
apply any deduplication logic to the LinkableElement collections assigned
to an ElementPathKey.
This PR updates some of the signatures to include more explicit type information,
adds some validation checks on the dataclasses to ensure expectations are
being met across the class, and adds a test suite illustrating the
current behaviors.