-
Notifications
You must be signed in to change notification settings - Fork 4
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
Feature/ogc 508 replace elastic search by postgres v2 #1357
base: master
Are you sure you want to change the base?
Feature/ogc 508 replace elastic search by postgres v2 #1357
Conversation
This reverts commit 737fb14.
@Daverball I am working on the tests in |
You're relying a lot on implicit type conversions here and hoping that SQLAlchemy is doing the right thing for you. You could try Either way to properly debug this, you would have to look at the final SQL statement SQLAlchemy generates for you. Sometimes you will just have to manually define an |
Also as a side note |
@@ -323,7 +323,9 @@ def generic_search(self) -> list['Searchable']: | |||
query = (query.filter(model.fts_idx.op('@@')(ts_query)) | |||
.add_columns(rank_expression)) | |||
|
|||
results.extend(list(query.all())) | |||
res = list(query.all()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
query.all()
already returns a list so you don't need to convert to a list
.
).json | ||
'/search/suggest?q=89').json | ||
|
||
# postgres |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The same is valid for this test. Any ideas @Daverball?
@@ -152,7 +166,7 @@ def role(self) -> str: | |||
assert self.user is not None | |||
return self.user.role | |||
|
|||
@property | |||
@hybrid_property |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since I converted email
to a hybrid_property
the test test_add_edit_external_attendee
fails with TypeError: 'email' is an invalid keyword argument for CourseAttendee
.
If I revert back the test passed but the search-postgres?q=martin
fails due to the missing hybrid property...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's very possible that setter
has to be defined before the expression
. Although I'm still not sure if the auto-generated __init__
for SQLAlchemy will contain settable hybrid properties, it might not.
But this whole thing seems to be heading in very much the same direction as your first attempt at implementing the indexing. Maybe it would be more robust to store the computed es properties like es_public
in the database at the same time we store the index.
This way we don't have to add all these new potentially subtly incorrect hybrid properties just to use them within a SQL query and are closer to the original elastic search implementation. If we want more of these things to happen online down the road, we can always improve it later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried the setter
before the expression
but it didn't help...
I will try the approach of calculating and storing the properties in a separate column while indexing and use them for searching. |
Please fill in the commit message below and work through the checklist. You can delete parts that are not needed, e.g. the optional description, the link to a ticket or irrelevant options of the checklist.
Commit message
:
TYPE: <Feature|Bugfix>
LINK:
HINT:
Checklist