-
Notifications
You must be signed in to change notification settings - Fork 118
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
SNOW-1869388 add memoization to to_selectable #2815
Changes from 5 commits
0810940
a2ca9f3
16fa0e3
d7b4d30
a0aa297
3dca857
fcefd64
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 |
---|---|---|
|
@@ -68,16 +68,29 @@ def __init__( | |
# order of when the with query block is visited. The order is important to make sure the dependency | ||
# between the CTE definition is satisfied. | ||
self.resolved_with_query_block: Dict[str, Query] = {} | ||
# This is a memoization dict for storing the selectable for a SnowflakePlan when to_selectable | ||
# method is called with the same SnowflakePlan. This is used to de-duplicate nodes created during | ||
# compilation process | ||
self._to_selectable_memo_dict = {} | ||
|
||
def to_selectable(self, plan: LogicalPlan) -> Selectable: | ||
"""Given a LogicalPlan, convert it to a Selectable.""" | ||
if isinstance(plan, Selectable): | ||
return plan | ||
|
||
snowflake_plan = self.resolve(plan) | ||
selectable = SelectSnowflakePlan(snowflake_plan, analyzer=self) | ||
selectable._is_valid_for_replacement = snowflake_plan._is_valid_for_replacement | ||
return selectable | ||
with self.session._lock: | ||
if ( | ||
isinstance(plan, SnowflakePlan) | ||
and plan._uuid in self._to_selectable_memo_dict | ||
): | ||
return self._to_selectable_memo_dict[plan._uuid] | ||
snowflake_plan = self.resolve(plan) | ||
selectable = SelectSnowflakePlan(snowflake_plan, analyzer=self) | ||
selectable._is_valid_for_replacement = ( | ||
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. in fact, it should always be true in query generator instead of propogating from the snowlfake plan |
||
snowflake_plan._is_valid_for_replacement | ||
) | ||
self._to_selectable_memo_dict[snowflake_plan._uuid] = selectable | ||
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. we probably should remember the original logical plan id instead of the resolved plan id, because the different snowflake plan might be created for the same orignal plan node 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. Yea it's true |
||
return selectable | ||
|
||
def generate_queries( | ||
self, logical_plans: List[LogicalPlan] | ||
|
Original file line number | Diff line number | Diff line change | ||
---|---|---|---|---|
|
@@ -79,6 +79,7 @@ def mock_session(mock_analyzer) -> Session: | |||
fake_session = mock.create_autospec(Session) | ||||
fake_session._cte_optimization_enabled = False | ||||
fake_session._analyzer = mock_analyzer | ||||
fake_session._lock = mock.MagicMock() | ||||
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. Can we have a more descriptive name to this lock -- similar to 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. this lock is actually part of
|
||||
fake_session._plan_lock = mock.MagicMock() | ||||
mock_analyzer.session = fake_session | ||||
return fake_session |
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.
why do we need a lock here, but not at other place in query_generator?
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.
We don't need it. query generator is not shared between threads so we should be good. That's why it is not added in other places.