Allow SQLAlchemy types in DataFrame.to_sql dtype#1763
Conversation
Signed-off-by: Sai Asish Y <say.apm35@gmail.com>
loicdiridollou
left a comment
loicdiridollou
left a comment
There was a problem hiding this comment.
See the CI failures, please run poetry run poe test_all for checking the CI locally.
Added some comments in the code to guide you.
| chunksize: int | None = None, | ||
| dtype: DtypeArg | None = None, | ||
| dtype: ( | ||
| DtypeArg | Mapping[Hashable, type[TypeEngine[Any]] | TypeEngine[Any]] | None |
There was a problem hiding this comment.
I think you may need TypeEngineMixin instead.
Please try running tests locally with poetry run poe test_all for the full CI cycle or poetry run poe mypy for just mypy.
There was a problem hiding this comment.
Done in 140b790. TypeEngineMixin alone did not fix the dict case since mypy would not pick a context from a union with two separate Mapping members, so I merged them into a single Mapping and also allowed a bare sqlalchemy type since to_sql applies a scalar to all columns. poe test_all passes locally now.
| "test", | ||
| con=engine, | ||
| dtype={ | ||
| "a": sqlalchemy.types.VARCHAR(16), |
There was a problem hiding this comment.
You added the case for dict[str, TypeEngineMixin], to be complete you should also add for TypeEngineMixin so you test all the cases possible.
There was a problem hiding this comment.
Added scalar checks for both an instance and a class in 140b790.
Signed-off-by: Sai Asish Y <say.apm35@gmail.com>
SAY-5
force-pushed
the
fix-to-sql-sqlalchemy-dtype
branch
from
a71f68e to
140b790
Compare
2 participants
assert_type()to assert the type of any return value)AGENTS.md.DataFrame.to_sqlpasses itsdtypemapping straight through to SQLAlchemy, so the values can be SQLAlchemy types (either the class or an instance likeNVARCHAR(length=1024)), but the stub only acceptedDtypeArg. Widen thedtypeparameter to also allow a column mapping ofTypeEngineclasses/instances.