SAP / sqlalchemy-hana

SQLAlchemy Dialect for SAP HANA
Apache License 2.0
128 stars 56 forks source link

JSON Support for Alembic #263

Closed Zahlii closed 4 months ago

Zahlii commented 5 months ago

Render JSON data type as NCLOB column. I am testing whether or not this automatically works also for SQLALCHEMY updates to JSON fields.

kasium commented 5 months ago

Can you please

Zahlii commented 5 months ago

Current behavior is that if your sqlalchemy model includes a JSON column and we are using alembic, trying to run a migration that adds a JSON columns results in Compiler <sqlalchemy_hana.dialect.HANATypeCompiler object at 0x14340ddd0> can't render element of type JSON

kasium commented 5 months ago

Thanks a lot, also for the contribution. Can you let me know which alembic and Sqlalchemy version you use.

I thought Sqlalchemy has some defaults or so regarding JSON

kasium commented 5 months ago

Okay, now we have some test failure with JSON. Can you please check them. Please let me know if you need any support

Zahlii commented 5 months ago

Would need info on how to properly set up the tests locally against a HANA instance, otherwise this will be impossible to look into, as the tests that are executed seem to be sqlalchemy standard tests and as such I need to debug whats going on.

On top of that, only basic JSON functionalities are ever going to be supported because HANA has no inherent notion of JSOn as a type (and hence JSONB based looksups like postgres offers are not available). This means that even if we fix this issue which currently fails all of the added tests, other tests such as test_index_typed_access and test_index_typed_comparison are probably never going to work, so we would need to skip them.

EDIT: Seems I can run them via pytest test_sqlalchemy_dialect_suite.py --dburi hana://...

kasium commented 5 months ago

@Zahlii do you have a HANA instance to work with? If yes, please check which tests make sense and which not. I'm fine ignoring broken ones (see existing tests how to do that), but we should try to implement as much functionality as possible

Zahlii commented 5 months ago

I currently only have access to an HDI container, but checking with our global account admin if we can get a user that can create schemas, as currently using HDI fails as we cannot create a random test schema inside of it with default user/pass

kasium commented 5 months ago

@Zahlii if it's fine for you, I can also take over this part here. since you already contributed a lot 😄

Zahlii commented 5 months ago

I managed to reproduce it now with some tweaks, however for me the issue seems to be independent of JSON: The test runs the following create:

CREATE TABLE data_table ( id INTEGER NOT NULL, name NVARCHAR(30) NOT NULL, data NCLOB NOT NULL, nulldata NCLOB, PRIMARY KEY (id) )

And then attempts the following INSERT

FAILED [ 33%]INSERT INTO data_table (name, data, nulldata) VALUES (?, ?, ?)

Which obviously cannot work, as the ID field is not filled...

kasium commented 5 months ago

Good finding. Let me have a look!

kasium commented 5 months ago

Open TODOs