-
Notifications
You must be signed in to change notification settings - Fork 109
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-1645316: Modularize skip lqb conditions and add no db check #2451
base: main
Are you sure you want to change the base?
SNOW-1645316: Modularize skip lqb conditions and add no db check #2451
Conversation
self, | ||
) -> Optional[SkipLargeQueryBreakdownCategory]: | ||
"""Method to check if the optimization should be skipped based on the session state.""" | ||
if self.session.get_current_database() is None: |
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.
how about schema? when there no current schema, that won't work also
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 tried to see if it is possible to get no schema. Snowflake automatically picks PUBLIC
so I could create a scenario where there is no schema.
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.
added a check anyways.
@@ -123,14 +123,14 @@ def __init__( | |||
self.complexity_score_upper_bound = complexity_bounds[1] | |||
|
|||
def apply(self) -> List[LogicalPlan]: | |||
if is_active_transaction(self.session): | |||
if reason := self._should_skip_optimization_for_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.
let's just do
reason = self._should_skip_optimization_for_session()
if reason is not None:
xxx
easier for me to understand
def _should_skip_optimization_for_session( | ||
self, | ||
) -> Optional[SkipLargeQueryBreakdownCategory]: | ||
"""Method to check if the optimization should be skipped based on the session state.""" |
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.
add comment about the return value
Which Jira issue is this PR addressing? Make sure that there is an accompanying issue to your PR.
Fixes SNOW-1645316
Fill out the following pre-review checklist:
Please describe how your code solves the related issue.
i) Move skip large query breakdown into two functions:
_should_skip_optimization_for_session
_should_skip_optimization_for_root
ii) Add a condition to skip large query breakdown when no database exists