fix: two usability issues with sqlalchemy #354
Merged
+400
−27
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Askir
force-pushed
the
jascha/sqlalchemy-fixes
branch
from
1de28b4 to
38c7a93
Compare
Askir
force-pushed
the
jascha/sqlalchemy-fixes
branch
2 times, most recently
from
bef36f7 to
f1eabb6
Compare
adolsalamanca
approved these changes
Jan 10, 2025
Comment on lines
152
to
164
| def test_can_build_select(): | ||
| """ | ||
| This is a very minimal test case that failed when doing | ||
| some development with the extension. | ||
| The nature of the vectorizer_relationship being a descriptor messes | ||
| with sqlalchemys relationship resolution. | ||
| It was previously using `backref` to propagate the parent field, | ||
| which is resolved later and an immediate access | ||
| to build queries like this would fail. | ||
| """ | ||
| select(Document.content_embeddings).options( | ||
| joinedload(Document.content_embeddings.parent) # type: ignore | ||
| ) # type: ignore |
There was a problem hiding this comment.
I guess it's intended not to have assertions here, to just verify it builds?
There was a problem hiding this comment.
yes, it was throwing an exception before 😅
cevian
reviewed
Jan 10, 2025
Askir
changed the title
Askir
force-pushed
the
jascha/sqlalchemy-fixes
branch
from
f1eabb6 to
beafcc3
Compare
|
@cevian yes the select test covers this too, because the same file has two models with |
Askir
changed the title
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I am fixing two quite annoying issues with the SQLAlchemy integration in this PR:
It wasn't possible to have the same field name configured on two different entities. So you can't have
content_embeddingstwice, because the name was colliding in the Mapper configuration. So I changed the dynamically generated Model name to contain the parent Model.The way we were configuring the relationship into the other direction via
backrefwas somewhat lazily evaluated and is also no longer recommended by SQLAlchemy. It's considered "legacy". I changed it with some back and forth to be simply two relationships in both directions and added aparent_kwargsfield that allows to configure the relationship into the reverse direction.