-
Notifications
You must be signed in to change notification settings - Fork 97
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Cache linkable elements #1229
Cache linkable elements #1229
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
kind: Under the Hood | ||
body: Cache functions related to parsing group by options in order to improve query | ||
performance. | ||
time: 2024-05-23T15:51:48.789152-07:00 | ||
custom: | ||
Author: courtneyholcomb | ||
Issue: "1229" |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,6 @@ | ||
from __future__ import annotations | ||
|
||
import functools | ||
import logging | ||
import time | ||
from typing import Dict, FrozenSet, Optional, Sequence, Set | ||
|
@@ -44,15 +45,16 @@ def __init__(self, semantic_manifest: SemanticManifest, semantic_model_lookup: S | |
max_entity_links=MAX_JOIN_HOPS, | ||
) | ||
|
||
@functools.lru_cache | ||
def linkable_elements_for_measure( | ||
self, | ||
measure_reference: MeasureReference, | ||
with_any_of: Optional[Set[LinkableElementProperty]] = None, | ||
without_any_of: Optional[Set[LinkableElementProperty]] = None, | ||
with_any_of: Optional[FrozenSet[LinkableElementProperty]] = None, | ||
without_any_of: Optional[FrozenSet[LinkableElementProperty]] = None, | ||
Comment on lines
+52
to
+53
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I kind of wondered why these weren't frozenset to begin with. |
||
) -> LinkableElementSet: | ||
"""Return the set of linkable elements reachable from a given measure.""" | ||
frozen_with_any_of = LinkableElementProperty.all_properties() if with_any_of is None else frozenset(with_any_of) | ||
frozen_without_any_of = frozenset() if without_any_of is None else frozenset(without_any_of) | ||
frozen_with_any_of = LinkableElementProperty.all_properties() if with_any_of is None else with_any_of | ||
frozen_without_any_of = frozenset() if without_any_of is None else without_any_of | ||
|
||
start_time = time.time() | ||
linkable_element_set = self._linkable_spec_resolver.get_linkable_element_set_for_measure( | ||
|
@@ -66,20 +68,22 @@ def linkable_elements_for_measure( | |
|
||
return linkable_element_set | ||
|
||
@functools.lru_cache | ||
def linkable_elements_for_no_metrics_query( | ||
self, | ||
with_any_of: Optional[Set[LinkableElementProperty]] = None, | ||
without_any_of: Optional[Set[LinkableElementProperty]] = None, | ||
with_any_of: Optional[FrozenSet[LinkableElementProperty]] = None, | ||
without_any_of: Optional[FrozenSet[LinkableElementProperty]] = None, | ||
) -> LinkableElementSet: | ||
"""Return the reachable linkable elements for a dimension values query with no metrics.""" | ||
frozen_with_any_of = LinkableElementProperty.all_properties() if with_any_of is None else frozenset(with_any_of) | ||
frozen_without_any_of = frozenset() if without_any_of is None else frozenset(without_any_of) | ||
frozen_with_any_of = LinkableElementProperty.all_properties() if with_any_of is None else with_any_of | ||
frozen_without_any_of = frozenset() if without_any_of is None else without_any_of | ||
|
||
return self._linkable_spec_resolver.get_linkable_elements_for_distinct_values_query( | ||
with_any_of=frozen_with_any_of, | ||
without_any_of=frozen_without_any_of, | ||
) | ||
|
||
@functools.lru_cache | ||
def linkable_elements_for_metrics( | ||
self, | ||
metric_references: Sequence[MetricReference], | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oof Sequence isn't counted as immutable. Should we update this to Tuple or does the typechecker catch it if we pass in a non-hashable type? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The type checker catches it! It fails if the type isn't hashable. |
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would love to cache this function too, but that change would be a bit more involved since base class (
LinkableElementSet
) relies heavily on dictionaries, which are not hashable.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we just store
Sequence[Tuple[ElementPathKey, Sequence[LinkableDimension]]]
or whatever instead of a dict? We go to some lengths to avoid mutation of these things anyway, might as well make it official.We shouldn't do it in this PR, anyway, that sounds like a fairly involved change given the need to update callsites and all of those merge functions and whatnot.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah that could work!