Skip to content
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

JSON Support for Alembic #263

Open
wants to merge 25 commits into
base: main
Choose a base branch
from
Open

JSON Support for Alembic #263

wants to merge 25 commits into from

Conversation

Zahlii
Copy link

@Zahlii Zahlii commented May 17, 2024

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

@kasium
Copy link
Contributor

kasium commented May 17, 2024

Can you please

  • add a test
  • explain what the current behaviour is

@Zahlii
Copy link
Author

Zahlii commented May 17, 2024

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
Copy link
Contributor

kasium commented May 17, 2024

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

sqlalchemy_hana/dialect.py Outdated Show resolved Hide resolved
sqlalchemy_hana/dialect.py Outdated Show resolved Hide resolved
sqlalchemy_hana/dialect.py Outdated Show resolved Hide resolved
sqlalchemy_hana/dialect.py Outdated Show resolved Hide resolved
Copy link
Contributor

@kasium kasium left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add json_type to test/requirements.py with exclusions.open()

sqlalchemy_hana/dialect.py Outdated Show resolved Hide resolved
@Zahlii Zahlii requested a review from kasium May 21, 2024 14:17
@kasium
Copy link
Contributor

kasium commented May 22, 2024

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

@Zahlii
Copy link
Author

Zahlii commented May 24, 2024

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
Copy link
Contributor

kasium commented May 24, 2024

@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
Copy link
Author

Zahlii commented May 24, 2024

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
Copy link
Contributor

kasium commented May 24, 2024

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

@Zahlii
Copy link
Author

Zahlii commented May 24, 2024

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
Copy link
Contributor

kasium commented May 24, 2024

Good finding. Let me have a look!

@kasium
Copy link
Contributor

kasium commented May 24, 2024

Open TODOs

  • Add a section to the readme explaining what is supported and whats not
  • Overwrite visit_json_getitem_op_binary and visit_json_path_getitem_op_binary in HANAStatementCompiler and raise an error stating that these operations are not supported

pyproject.toml Outdated Show resolved Hide resolved
@kasium kasium self-requested a review June 11, 2024 05:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants