SNOW-1819531: propagate referenced ctes to all root nodes #2670
Conversation
sfc-gh-aalam
added
the
NO-CHANGELOG-UPDATES
sfc-gh-aalam
requested review from
sfc-gh-smirzaei,
sfc-gh-jdu and
sfc-gh-yixie
|
Seems like your changes contain some Local Testing changes, please request review from @snowflakedb/local-testing |
|
Seems like your changes contain some Local Testing changes, please request review from @snowflakedb/local-testing |
|
Seems like your changes contain some Local Testing changes, please request review from @snowflakedb/local-testing |
|
Seems like your changes contain some Local Testing changes, please request review from @snowflakedb/local-testing |
sfc-gh-aalam
changed the title
| return True | ||
|
|
||
| if isinstance(node, SnowflakePlan) and node.source_plan is not None: | ||
| return node._is_valid_for_replacement |
There was a problem hiding this comment.
i am kind of confused about why we need this function here?
There was a problem hiding this comment.
Can you also add comment for this function?
There was a problem hiding this comment.
while testing, I noticed that large query breakdown failed with error saying node is not valid for replacement for a SnowflakePlan whose child was a WithQueryBlock. At the time, I though this function would help us catch-up all such cases but on a second thought, it was a bug in repeated subquery elimination. I fixed the bug and removed this function now.
|
Seems like your changes contain some Local Testing changes, please request review from @snowflakedb/local-testing |
| @@ -169,6 +169,7 @@ def do_resolve_with_resolved_children( | |||
| iceberg_config=logical_plan.iceberg_config, | |||
| table_exists=logical_plan.table_exists, | |||
| ) | |||
| resolved_plan.referenced_ctes = resolved_child.referenced_ctes | |||
There was a problem hiding this comment.
For the correction of code generation, we can use the way you have now, but please make sure we comment it out clearly about why things is done in such way
| @@ -306,7 +309,17 @@ def get_snowflake_plan_queries( | |||
|
|
|||
| plan_queries = plan.queries | |||
| post_action_queries = plan.post_actions | |||
| if len(plan.referenced_ctes) > 0: | |||
| if len(plan.referenced_ctes) > 0 and not isinstance( | |||
| plan.source_plan, | |||
There was a problem hiding this comment.
adda a comment here about why are we doing this here
| resolved_with_block._is_valid_for_replacement = True | ||
| resolved_with_block_map[ |
There was a problem hiding this comment.
the optimization may fail. @sfc-gh-aalam maybe it would be good to add a try catch for the optimization stage, and fallback to the original query if the optimization fails.
|
Seems like your changes contain some Local Testing changes, please request review from @snowflakedb/local-testing |
|
Seems like your changes contain some Local Testing changes, please request review from @snowflakedb/local-testing |
|
Seems like your changes contain some Local Testing changes, please request review from @snowflakedb/local-testing |
| selectable = SelectSnowflakePlan(snowflake_plan, analyzer=query_generator) | ||
| selectable._is_valid_for_replacement = snowflake_plan._is_valid_for_replacement | ||
| return selectable |
There was a problem hiding this comment.
before update:
after update:
|
Seems like your changes contain some Local Testing changes, please request review from @snowflakedb/local-testing |
|
Seems like your changes contain some Local Testing changes, please request review from @snowflakedb/local-testing |
|
Seems like your changes contain some Local Testing changes, please request review from @snowflakedb/local-testing |
|
Seems like your changes contain some Local Testing changes, please request review from @snowflakedb/local-testing |
… into aalam-SNOW-1819531-lqb-bug-fixes
|
Seems like your changes contain some Local Testing changes, please request review from @snowflakedb/local-testing |
Which Jira issue is this PR addressing? Make sure that there is an accompanying issue to your PR.
Fixes SNOW-1819531
Fill out the following pre-review checklist:
Please describe how your code solves the related issue.
i) SNOW-1748140: Modify schema_expression to be structured type aware. #2659 re-enable test which is fixed by SNOW-1844090: deprecate propagate referenced ctes #2720
ii) fix a bug where
relaxed_pipeline_breakerproperty forSnowflakePlanandSelectSnowflakePlanwere not read correctlyiii) updated plan plotter to color nodes that are not valid for replacement with red color.
iv) updated plan plotter to print number of reference a node contains.