-
Notifications
You must be signed in to change notification settings - Fork 58
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
[NADE][Bugfix] Use resolved app warehouse for regionless query #1292
Conversation
5fc8649
to
58fb01d
Compare
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.
Can you also audit other potential gaps we might have missed -- like post-deploy scripts or package scripts? We should use this new use_warehouse
primitive everywhere it makes sense.
assert ( | ||
"Could not use warehouse MockWarehouse. Object does not exist, or operation cannot be performed." | ||
in err.value.msg | ||
) |
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.
This error message is now different because instead of calling use warehouse
directly which uses our generic_error_handler
, it calls the method self.use
provided by the CLI which has its own error handling.
d8a4638
to
0664030
Compare
if new_wh is not None: | ||
new_wh_id = to_identifier(new_wh) | ||
|
||
if prev_wh is None and new_wh 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.
this logic is a bit too complicated.
-
Why do we need to stay with prev_wh if new_wh is not defined? is it a valid use case that new_wh is None (especially when dealing with snow app run)? Shouldn't we just error?
-
too many combinations. I suggest to simplify as follows if possible:
a) if new_wh is None, error out.
b) proceed to switch to new_wh if different from prev_wh.
c) yield
d) if prev_wh was None, do nothing, otherwise, switch back to it.
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.
Some concerns raised in the previous comments:
- It is possible that prev_wh is None, especially in the case when no default warehouse is assigned to a user, and hence
current_warehouse()
may return None. I do not have a way of verifying this as I do not have permissions to set up a new user in an account, but I do believe this is a case we need to account for since the bug ticket was created for a scenario like this when there is no fallback. - It is possible that new_wh is None. In our case,
package_warehouse
orapplication_warehouse
could be None if user has not provided that warehouse in their snowflake.yml, config.toml and there is no default warehouse. In that case, the connection context would return a None for this property. - Given the two above, we do need to tackle their nullity accordingly.
But I am open to hearing if the assumptions in 1 and/or 2 are incorrect and therefore do not need to be handled.
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'm with Michel, I think we can simplify it as suggested. Sent you a way to determine if warehouse is not set.
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.
My requests: account for None
in the project model warehouse properties, and simplify the use warehouse context manager as suggested by Michel.
if new_wh is not None: | ||
new_wh_id = to_identifier(new_wh) | ||
|
||
if prev_wh is None and new_wh 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.
I'm with Michel, I think we can simplify it as suggested. Sent you a way to determine if warehouse is not set.
0664030
to
5d9c35d
Compare
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.
LGTM
Pre-review checklist
Changes description
As part of fetching the URL where the Native App is live, the CLI needs to execute a SQL query which requires the use of a warehouse. The
application.warehouse
property in the manager class resolves this to either be the app warehouse if found, or the one derived from the connection. Since this URL is pertaining to the app instance, we have decided to execute this query by only activating app warehouse.