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

feat: Native PG array type #9

Merged
merged 9 commits into from Feb 6, 2022
Merged

feat: Native PG array type #9

merged 9 commits into from Feb 6, 2022

Conversation

etimberg
Copy link
Collaborator

@etimberg etimberg commented Aug 8, 2021

No description provided.

README.md Outdated Show resolved Hide resolved
@mcataford mcataford self-assigned this Oct 26, 2021
@etimberg etimberg force-pushed the array-types branch 2 times, most recently from e45bef5 to 0d31d88 Compare October 26, 2021 18:57
@maitham
Copy link

maitham commented Nov 22, 2021

Can I help at all in getting this merged?

@etimberg
Copy link
Collaborator Author

@maitham I think the big thing this needs now is to figure out how to get contains and contained_by queries working. I briefly got some tests in but they are currently failing. I will push those up now and we can go from there

@etimberg
Copy link
Collaborator Author

I made some progress on debugging these tests and trying to make this work. Basically it boils down to needing to call .contains() on the column and then passing in the correct parameters.

The challenge is that when Ormar compiles statements, it uses literal_binds=True. https://github.com/collerek/ormar/blob/master/ormar/queryset/actions/filter_action.py#L192

Unfortunately, SQLAlchemy cannot render literals for arrays. A simple test to show this is:

from sqlalchemy import Column, Integer
from sqlalchemy.dialects import postgresql

numbers = Column(postgresql.ARRAY(Integer))
clause = numbers.contains([11])
print(clause.compile(compile_kwargs={"literal_binds": True},))  # Causes a NotImplementedError

@collerek is there a reason ormar uses literal_binds=True? Trying to generate strings for PG arrays seems like it will be complicated and error prone.

@etimberg
Copy link
Collaborator Author

Following up on literal_binds=True, SQLAlchemy does not recommend this

@collerek
Copy link

Following up on literal_binds=True, SQLAlchemy does not recommend this

Will check that, to be honest I don't remember why it uses literal binds 🙈

@etimberg
Copy link
Collaborator Author

Thanks for taking a look @collerek! I was digging through it as well. It looks like it was introduced in collerek/ormar#87. It appears to be related to column aliases. I was actually able to make it work with arrays by simply commenting out the call to _compile_clause in filter_action.py but I suspect that breaks a lot of things elsewhere (I haven't tried the ormar test suite with this change).

The good news is that I haven't been able to inject something via a string filter. sqlalchemy.text may be preventing a lot of things.

@collerek
Copy link

collerek commented Jan 6, 2022

There is a PR that removes the literal binds, currently blocked by a bug in encode\databases that was patched but not yet released. The bug refers only to sqlite so you can probably check with the PR branch as should already work for postgres.

@etimberg
Copy link
Collaborator Author

etimberg commented Jan 6, 2022

Amazing, thank you @collerek!

]


for (method_name, method) in FIELD_ACCESSOR_MAP:
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@collerek not sure how you feel about this patching of the FieldAccessor class and the FILTER_OPERATIONS and METHODS_TO_OPERATORS dictionaries. I'm open to other suggestions if there's a better way to implement this.

@etimberg etimberg merged commit ea9162c into main Feb 6, 2022
@etimberg etimberg deleted the array-types branch February 6, 2022 17:46
@tophat-opensource-bot
Copy link
Collaborator

🎉 This PR is included in version 2.1.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request released
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants