fix(datatype): convert from sqlglot VARCHAR(MAX) correctly#11202
fix(datatype): convert from sqlglot VARCHAR(MAX) correctly#11202cpcloud merged 1 commit intoibis-project:mainfrom
Conversation
|
Ah, it looks like there is already |
4c8283b to
32c1adc
Compare
|
eh, the roundtrip test doesn't actually catch the bug, since ibis.String goes to VARCHAR(max), but the bug only appears when using NVARCHAR(max). So at this point I didn't actually add a test, it didn't seem worth it. Maybe could add test only to the mssql backend tests? Another implementation we could do here is to hoist the MSSQL implementation, with the safety check for "MAX" length, up to the superclass SqlglotType. Then there will less code, but the needs of the different backends will get mixed with each other. This implementation is more explicit and granular and extensible. |
|
@cpcloud did this one fall through the cracks, does it want to be merged? I see you sometimes approve PRs but not merge them, and sometimes approve and then immediately merge, and I'm wondering if there is a pattern or reason to it? |
|
I think this one just fell through the cracks! |
Fixes #11191
I'd like to add a test for this. I think checking for roundtripping ibis.string -> sqglot.DataType -> ibis.string across all backends sounds like the best approach.