SNOW-701257: add query attribute to Error class #1437
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1437 +/- ##
==========================================
+ Coverage 81.77% 81.83% +0.06%
==========================================
Files 60 60
Lines 8575 8577 +2
Branches 1266 1266
==========================================
+ Hits 7012 7019 +7
+ Misses 1236 1232 -4
+ Partials 327 326 -1
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
|
You might want to port it to sp connector as well. Should be simple. |
src/snowflake/connector/errors.py
Outdated
| else: | ||
| self.msg = f"{self.errno:06d} ({self.sqlstate}): {self.msg}" | ||
| self.msg = f"{self.errno:06d} ({self.sqlstate}): {self.query}: {self.msg}" |
There was a problem hiding this comment.
I am not sure about this section. Should we add query here even when we were not printing sfqid
There was a problem hiding this comment.
I also think we should add sfqid. I don't know why we should hide them at WARN and ERROR level. That means user's production long won't have them.
But @sfc-gh-mkeller once mentioned that this should be regarded as a break change.
src/snowflake/connector/errors.py
Outdated
| @@ -66,9 +68,9 @@ def __init__( | |||
| if self.sqlstate != "n/a": | |||
| if not already_formatted_msg: | |||
| if logger.getEffectiveLevel() in (logging.INFO, logging.DEBUG): | |||
| self.msg = f"{self.errno:06d} ({self.sqlstate}): {self.sfqid}: {self.msg}" | |||
| self.msg = f"{self.errno:06d} ({self.sqlstate}): {self.sfqid}: {self.msg}: {self.query}" | |||
There was a problem hiding this comment.
@sfc-gh-mkeller could I get your input on whether this could be considered behavior change? We want to add query into Errors generated by python connector (more context here) so that query is easily accessible when debugging issues and does not get lost.
There was a problem hiding this comment.
So, I don't think that this would be a behavior change, but I think it should be.
If you look at line 46 super().__init__(msg) you'll see that we call Exceptions's initalizer with the initially given msg, so this later isn't a behavior change. But I'm not sure that right now it works as it should. I verified this with:
❯ python
Python 3.9.16 (main, Dec 7 2022, 10:06:04)
[Clang 14.0.0 (clang-1400.0.29.202)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> from snowflake.connector.errors import Error
/Users/mkeller/snowflakedb/snowflake-connector-python/src/snowflake/connector/options.py:13: UserWarning: Module snowflake was already imported from None, but /Users/mkeller/snowflakedb/snowflake-connector-python/src is being added to sys.path
import pkg_resources
>>> e = Error("this is a test error", errno=111)
>>> e.args
('this is a test error',)
>>> e.msg
'000111: this is a test error'There was a problem hiding this comment.
However; it does affect __str__
>>> str(e)
'000111: this is a test error'
There was a problem hiding this comment.
As a middle ground could we not add the query to msg but have it as a field of Error? For the short-term at least
https://github.com/snowflakedb/Stored-Proc-Python-Connector/pull/87 |
Please answer these questions before submitting your pull requests. Thanks!
What GitHub issue is this PR addressing? Make sure that there is an accompanying issue to your PR.
Fixes #SNOW-701257
Fill out the following pre-review checklist:
Please describe how your code solves the related issue.
Adding query in
ProgrammingErrorso that query information is not lost if logging is unset. Related PR SNOW-701257: optionally disable logging error for sql exceptions snowpark-python#674