-
Notifications
You must be signed in to change notification settings - Fork 608
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
fix(mysql): catch PyMySQL OperationalError exception #7919
Conversation
ACTION NEEDED Ibis follows the Conventional Commits specification for release automation. The PR title and description are used as the merge commit message. Please update your PR title and description to match the specification. |
@maxibor Thanks for the PR! Why isn't catching sqlalchemy's |
Seems like these are 2 different exceptions. The pymysql exception was anyway not caught in the current version of Ibis. |
Makes sense! It's not clear to me whether we need both exceptions. Can you describe your MySQL setup?
|
Just copy pasting from #7918 mySQL 5.5.5 |
This is a SQLAlchemy exception, and the Does this PR address the issue for you? |
Yes, that's why I opened it ;) |
Ah, got it. This probably because of the use of a raw cursor and since we're testing against a system that has this defined, the exception is never raised. Hm, I wonder if there's a way we can test this. |
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.
This code path will change once we finish porting backends to the new sqlglot compilers, and we won't need to catch the sqlalchemy exception (just the pymysql
one).
That said, this fixes a user-facing issue and isn't easily testable so I'm going to merge 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.
excuse me, maybe you should use SET @@session.time_zone = '+00:00'
, MySQL/MariaDB can not recognize UTC
by default, only if table mysql.time_zone
is not empty.
Description of changes
When trying to connect to a mySQL backend, the following error can appear:
This is due to an exception generated by PyMySQL.
The exception, formerly generated by SQLAlchemy was already caught by Ibis since #6010, this PR adds a catching of the PyMySQL exception.
Issues closed
Resolves #7918