SNOW-1869362: Plan plotter improvements #2813
Conversation
sfc-gh-aalam
added
the
NO-CHANGELOG-UPDATES
| os.environ["ENABLE_SNOWPARK_LOGICAL_PLAN_PLOTTING"] = str(enabled) | ||
| os.environ["SNOWPARK_LOGICAL_PLAN_PLOTTING_THRESHOLD"] = str( | ||
| plotting_score_threshold | ||
| ) |
There was a problem hiding this comment.
This might be better done with something like:
with mock.patch.dict(os.environ, {...}):
| try: | ||
| os.environ["ENABLE_SNOWPARK_LOGICAL_PLAN_PLOTTING"] = str(enabled) | ||
| os.environ["SNOWPARK_LOGICAL_PLAN_PLOTTING_THRESHOLD"] = str( | ||
| plotting_score_threshold | ||
| ) | ||
| tmp_dir = tempfile.gettempdir() | ||
|
|
||
| with patch("graphviz.Graph.render") as mock_render: | ||
| large_query_df.collect() |
There was a problem hiding this comment.
Can we perhaps add a comment explaining that the actual complexity for large_query_df falls somewhere between 0 and 10M?
| @@ -381,15 +383,27 @@ def plot_plan_if_enabled(root: LogicalPlan, filename: str) -> None: | |||
| ): | |||
| return | |||
|
|
|||
| if int( | |||
There was a problem hiding this comment.
what is this Plotting threshold used for? seems it is used for restricting the complexity score? maybe call this SNOWPARK_LOGICAL_PLAN_PLOTTING_COMPLEXITY_THRESHOLD to be more clear
| if node is None: | ||
| return "EMPTY_SOURCE_PLAN" # pragma: no cover | ||
| addr = hex(id(node)) | ||
| name = str(type(node)).split(".")[-1].split("'")[0] | ||
| return f"{name}({addr})" | ||
| suffix = "" | ||
| if isinstance(node, SnowflakeCreateTable): |
There was a problem hiding this comment.
add a comment here about what are the different printing used here
| @@ -381,15 +383,27 @@ def plot_plan_if_enabled(root: LogicalPlan, filename: str) -> None: | |||
| ): | |||
| return | |||
|
|
|||
| if int( | |||
| os.environ.get("SNOWPARK_LOGICAL_PLAN_PLOTTING_THRESHOLD", 0) | |||
There was a problem hiding this comment.
let's simply make the default threshold -1, be clear that by default plot out all nodes.
was there a reason about why we want to add this threshold?
There was a problem hiding this comment.
yeah. In my tests, I generally want to plot and debug "big" plans but sometime the plans get overwritten by smaller plan if they are present somewhere. That's why I added this variable. I don't think this is the best way - I'm open to suggestions.
There was a problem hiding this comment.
Can you be more specific about " sometime the plans get overwritten by smaller plan if they are present somewhere"? not quite getting this part, and what information you want to get to help your debugging process?
Which Jira issue is this PR addressing? Make sure that there is an accompanying issue to your PR.
Fixes SNOW-1869362
Fill out the following pre-review checklist:
Please describe how your code solves the related issue.
Made the following improvements with this PR: