SNOW-733235: Returning correct exception when table does not exist

[davidov541]

…ption.

Please answer these questions before submitting your pull requests. Thanks!

1. What GitHub issue is this PR addressing? Make sure that there is an accompanying issue to your PR.

   Fixes SNOW-733235

2. Fill out the following pre-review checklist:

   • I am adding a new automated test(s) to verify correctness of my new code
   • I am adding new logging messages
   • I am adding new credentials
   • I am adding a new dependency

3. Please describe how your code solves the related issue.

   My code detects if a table does not exist in a dictionary before attempting to index into that dictionary during requests to get columns. This prevents KeyErrors from being returned, and allows us to instead return more descriptive and standard errors indicating that the table does not exist.

[github-actions]

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

[davidov541]

I have read the CLA Document and I hereby sign the CLA

[davidov541]

recheck

[sfc-gh-sfan]

@sfc-gh-aling : Do we consider this a behavior change? This falls into the discussion we had yesterday, where we correct a behavior.

[davidov541]

Has there been any update on this? It's a pretty simple fix that enables a pretty powerful feature with Great Expectations (the ability to keep validation results in Snowflake directly for analysis), so the sooner this can get approved and merged, the sooner I can move my client off of using a custom build from my personal repository for this work.

[sfc-gh-sfan]

This looks good to me, I'll let @sfc-gh-aling chime in as well.

[davidov541]

Great, thanks @sfc-gh-sfan . It looks like a few checks failed, but I don't see how they relate directly to my changes. Can you take a look and let me know if there are changes I need to make to make the checks work? The lint failure is below:

RuntimeError: The Poetry configuration is invalid:
- [extras.pipfile_deprecated_finder.2] 'pip-shims<=0.3.4' does not match '^[a-zA-Z-_.0-9]+$'

[sfc-gh-aling]

appreciate the PR! we would love to take it, may I ask you to do us a favor to see if there're any other places where KeyError should not be raised when checking the existence of tables (or other objects you find) and update the code accordingly. I've left comments, please let me know if you have any questions regarding my comments.

for the failure in CI, please try rebasing your branch to the latest main branch.

[sfc-gh-aling]

appreciate the quick response and data collection!
apologize that I might not have the time to take a look this week due to some high priority tasks I have to deal with. I should have time next week to take a closer look at the code where we want to raise proper error.

[davidov541]

appreciate the quick response and data collection! apologize that I might not have the time to take a look this week due to some high priority tasks I have to deal with. I should have time next week to take a closer look at the code where we want to raise proper error.

No worries! I will be travelling for a week starting on Friday, so I'll be spotty myself, but any chance you get to give some feedback and get this resolved once and for all would be greatly appreciated!

[davidov541]

appreciate the quick response and data collection! apologize that I might not have the time to take a look this week due to some high priority tasks I have to deal with. I should have time next week to take a closer look at the code where we want to raise proper error.

Did you get any chance to look into this at all?

[sfc-gh-aling]

thanks @davidov541 , due to our ci limitation, I have created a draft PR for testing: https://github.com/snowflakedb/snowflake-sqlalchemy/pull/392/files

once it passes, let's get your PR merged. thank you so much for the contribution and apologize for my late response!

[sfc-gh-aling]

hey @davidov541, do you mind if I merge the identical test PR instead of this PR? (merge #392 and close this one)

We have some issue with the CI setting -- for now we don't allow credential to be included in a external PR test running so CIs are expected to fail, in the meanwhile we have branch protection rule that requires the checks to pass... it's an issue we need to fix, but to unblock you, I think we can merge the identical test PR.

[davidov541]

hey @davidov541, do you mind if I merge the identical test PR instead of this PR? (merge #392 and close this one)

We have some issue with the CI setting -- for now we don't allow credential to be included in a external PR test running so CIs are expected to fail, in the meanwhile we have branch protection rule that requires the checks to pass... it's an issue we need to fix, but to unblock you, I think we can merge the identical test PR.

I'm fine with you going with that approach if you all need it done.