-
-
Notifications
You must be signed in to change notification settings - Fork 338
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
Add records.search
RPC function
#3708
Conversation
@Anish9901 This is ready for review, but I'm marking it as a draft to keep us from merging until #3700 is merged. |
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.
This looks great logic wise @mathemancer, there are some unhandled edge cases that break the function. Please see my specific comments.
result = db_conn.exec_msar_func( | ||
conn, 'search_records_from_table', table_oid, json.dumps(search), limit |
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 think we could avoid this function call when the search list is empty. Assuming frontend already has the records from records.list
.
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.
We could, but I'd rather have the front end do that. I.e., if they want to avoid the call, they should just not send the request until there are letters in the array. For the completeness of this function I'd really rather treat it as a kind of "narrowing filter" concept. And for that, we should return unfiltered results whenever we get an empty search definition.
msar.search_records_from_table( | ||
tab_id oid, | ||
search_ jsonb, | ||
limit_ integer |
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.
For some reason having NULL
for limit_
here doesn't return anything, But works perfectly in list_records_from_table
.
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.
Now it returns all matching records.
search_ jsonb, | ||
limit_ integer |
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.
SELECT msar.search_records_from_table(2254321, '[]'::jsonb, 4);
ERROR: syntax error at or near ">"
Passing an empty json for search_
and a non null value for limit_
also breaks the function and results in a syntax error.
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.
Empty or null values for the search def now return unfiltered results.
db/sql/00_msar.sql
Outdated
msar.get_score_expr(tab_id, search_), | ||
limit_, | ||
concat( | ||
msar.get_score_expr(tab_id, search_), |
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.
Can we consider storing the result for this function into a variable instead of calling it twice?
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 did a quick experiment. The variable idea had no noticeable effect, but setting the get_score_expr
function to STABLE
did. So, I went with that.
Fixes #3707
This adds a
records.search
RPC function.Technical details
The function is documented as usual, and it should be possible to review this with no further explanation. If not, request a documentation improvement.
Screenshots
Checklist
Update index.md
).develop
branch of the repositoryvisible errors.
Developer Certificate of Origin
Developer Certificate of Origin