Raise exception by default in expression_to_value #60
Conversation
|
Looking into the utf8 error in the tests 😕 |
|
Okay I guess for now utf8mb4_0900_ai_ci is the by default client charset |
tests/test_query.py
Outdated
| # Sqlglot by-default runs `SET NAMES 'utf8mb4'` if no charset specified, which removes COLLATE settings | ||
| # See https://github.com/sqlalchemy/sqlalchemy/discussions/7858 | ||
| await conn.execute( | ||
| text("SET NAMES 'utf8mb4' COLLATE 'utf8mb4_0900_ai_ci'") |
There was a problem hiding this comment.
this is very nasty :(
setup.py
Outdated
| @@ -20,7 +20,7 @@ | |||
| "dev": [ | |||
| "aiomysql", | |||
| "mypy", | |||
| "mysql-connector-python", | |||
| "mysql-connector-python<9.0.0", | |||
There was a problem hiding this comment.
I believe there is a behavior change for the default collation ("SET NAMES 'utf8mb4' COLLATE 'utf8mb4_0900_ai_ci' was added while making a new connection)
Was trying to fix it in https://github.com/kelsin/mysql-mimic/pull/60/files/cd22920bbe455bc711d6c601d80fc17c40e5a2f9 but failed (it can work in python >= 3.9 but failed in python 3.7, probably because it is using mysql-connector-python 8.0 for python 3.7 but 9.0 for python 3.11
There was a problem hiding this comment.
Another issue combined with the above is sqlalchemy set SET NAMES 'utf8mb4' on connection, which removed that COLLATE which means we need special handling in testing for sqlalchemy.
So, probably limit mysql-connector is the best way to resolve the issue 🫤
There was a problem hiding this comment.
We should avoid limiting mysql-connector. It implies mysql-mimic doesn't work with the latest version of mysql-connector.
Can we just change the expected collation in tests?
Or change the collation when connecting in tests?
https://dev.mysql.com/doc/connector-python/en/connector-python-connectargs.html
https://github.com/kelsin/mysql-mimic/blob/main/tests/conftest.py#L210
And I think you can set connect args in the sqlalchemy URL
There was a problem hiding this comment.
I see, it makes sense!
Going to run the set names/collate query on each of the connection
mysql_mimic/intercept.py
Outdated
| @@ -60,4 +60,4 @@ def expression_to_value(expression: exp.Expression) -> Any: | |||
| return True | |||
| if expression.name == "OFF": | |||
| return False | |||
| return expression.name | |||
| raise ValueError(f"Unexpected expression: {expression}") | |||
There was a problem hiding this comment.
We should raise a https://github.com/kelsin/mysql-mimic/blob/main/mysql_mimic/errors.py#L44
Sometimes I'll go and try to find an appropriate error code here: https://dev.mysql.com/doc/mysql-errors/8.0/en/server-error-reference.html
But other times I give up and just use the default.
In this case, I think MySQL allows query expressions here: https://dev.mysql.com/doc/refman/8.0/en/set-variable.html
So maybe we throw some kind of "not supported yet" error, like this: https://github.com/kelsin/mysql-mimic/blob/main/mysql_mimic/session.py#L521
There was a problem hiding this comment.
Yeah this makes a lot of sense.
I did not go to https://dev.mysql.com/doc/refman/8.0/en/set-variable.html and check expression is allowed
setup.py
Outdated
| @@ -20,7 +20,7 @@ | |||
| "dev": [ | |||
| "aiomysql", | |||
| "mypy", | |||
| "mysql-connector-python", | |||
| "mysql-connector-python<9.0.0", | |||
There was a problem hiding this comment.
We should avoid limiting mysql-connector. It implies mysql-mimic doesn't work with the latest version of mysql-connector.
Can we just change the expected collation in tests?
Or change the collation when connecting in tests?
https://dev.mysql.com/doc/connector-python/en/connector-python-connectargs.html
https://github.com/kelsin/mysql-mimic/blob/main/tests/conftest.py#L210
And I think you can set connect args in the sqlalchemy URL
xiaoyu-meng-mxy
force-pushed
the
xy--tweak
branch
from
84a5951 to
b8d6ec1
Compare
The Error we saw is here
The user passes in an sqglot.exp.In expression and by default it returns the name of that In clause. This silently fallback is hard to debug so we want to raise an exception instead.
Automatically convert the expression to string instead of return the name also seems not good because I guess the assumption here is a variable can only be int, float or str. If we end up parsed out an expression, it's something wrong.
@barakalon