SNOW-1632701: Add uuid to SnowflakePlan #2233
Conversation
sfc-gh-aalam
added
the
NO-CHANGELOG-UPDATES
sfc-gh-aalam
requested review from
sfc-gh-jdu,
sfc-gh-yixie and
sfc-gh-jrose
| @@ -260,6 +260,7 @@ def __init__( | |||
| referenced_ctes.copy() if referenced_ctes else set() | |||
| ) | |||
| self._cumulative_node_complexity: Optional[Dict[PlanNodeCategory, int]] = None | |||
| self._uuid = str(uuid.uuid4()) | |||
There was a problem hiding this comment.
so there is very embracing state here that the snowflake_plan its-self already have and self._id fields, and used as part of the eq comparison, the id is generated based on the query and params, which may not be the exact indicator of whether two plan are the same plan.
I think we can keep both, but let's make sure we we doc it clearly here about the difference. @sfc-gh-jdu i also start question is changing the eq and hash correct in this case? I got the point that we want to simplify the comparison, but that also kind of changes the meaning of the equivalence of two snowflake plan nodes here.
| return self._uuid | ||
|
|
||
| @uuid.setter | ||
| def uuid(self, value: str) -> None: |
There was a problem hiding this comment.
i don't think we should provide a setter function here, i don't think anyone is suppose to change this once produced.
| @@ -502,6 +511,7 @@ def __deepcopy__(self, memodict={}) -> "SnowflakePlan": # noqa: B006 | |||
| copied_plan._is_valid_for_replacement = True | |||
| if copied_source_plan: | |||
| copied_source_plan._is_valid_for_replacement = True | |||
| copied_plan.uuid = self.uuid | |||
There was a problem hiding this comment.
by definition, the copied_plan should have a different uuid here, right? because it is a different node
sfc-gh-aalam
deleted the
aalam-SNOW-1632701-add-a-plan-id-to-statement-params
branch
Which Jira issue is this PR addressing? Make sure that there is an accompanying issue to your PR.
Fixes SNOW-1632701
Fill out the following pre-review checklist:
Please describe how your code solves the related issue.
Associate a
uuidwith eachSnowflakePlanso that we can track all queries that originated from the same plan.