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

Fixes and tests for metadata labels #8765

Merged
merged 6 commits into from Nov 29, 2021

Conversation

rtibbles
Copy link
Member

@rtibbles rtibbles commented Nov 22, 2021

Summary

  • Adds extensive testing for searching by metadata labels and the available labels generation code
  • This showed that the available metadata labels code was not quite doing what it ought
  • Changes how the available metadata labels code produces the available metadata labels to make the tests pass and also improve performance
  • Uses a Bit wise OR in Postgres to determine which labels are present
  • Implements a Bit wise OR shim for SQLite that functions identically to the postgres Bit wise OR, except that the maximum size of bits must be specified in advance
  • Uses a bit wise OR to do the label aggregation in a single query without subqueries, which significantly improves the performance, and gives full test compliance
  • Limits the data returned for Channels and Languages in the available label metadata to just the ids (reduces query times for these two specific items by roughly 100x for each)
  • Bootstraps full language data into the learn page explicitly to compensate for only the ids (to compensate for the full data no longer coming from the available labels)

Fixes #8737

Reviewer guidance

Does filtering seem to produce the correct remaining labels?
When testing I changed this line to if True: https://github.com/learningequality/kolibri/blob/release-v0.15.x/kolibri/core/content/utils/search.py#L91 to repeatably execute the queries that are being optimized here.


Testing checklist

  • Contributor has fully tested the PR manually
  • If there are any front-end changes, before/after screenshots are included
  • Critical user journeys are covered by Gherkin stories
  • Critical and brittle code paths are covered by unit tests

PR process

  • PR has the correct target branch and milestone
  • PR has 'needs review' or 'work-in-progress' label
  • If PR is ready for review, a reviewer has been added. (Don't use 'Assignees')
  • If this is an important user-facing change, PR or related issue has a 'changelog' label
  • If this includes an internal dependency change, a link to the diff is provided

Reviewer checklist

  • Automated test coverage is satisfactory
  • PR is fully functional
  • PR has been tested for accessibility regressions
  • External dependency files were updated if necessary (yarn and pip)
  • Documentation is updated
  • Contributor is in AUTHORS.md

@rtibbles rtibbles added the TODO: needs review Waiting for review label Nov 22, 2021
@rtibbles rtibbles added this to the 0.15.0 milestone Nov 22, 2021
@pcenov
Copy link
Member

pcenov commented Nov 23, 2021

Tested in this build and the 500 error is fixed now plus filtering is fully functional.

Copy link
Member

@bjester bjester left a comment

Choose a reason for hiding this comment

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

Couple comments, but looks good!

kolibri/core/content/utils/search.py Show resolved Hide resolved
kolibri/core/content/utils/search.py Outdated Show resolved Hide resolved
Copy link
Member

@jredrejo jredrejo left a comment

Choose a reason for hiding this comment

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

I have tested it as much as I have been able (in sqlite only) and it has worked perfectly.
Also, seeing how pytest kolibri/core/content/test/test_search.py results in
371 passed in 63.65 seconds it's quite impressive.
Great work!

kolibri/core/content/utils/search.py Show resolved Hide resolved
@rtibbles rtibbles merged commit 9abb866 into learningequality:release-v0.15.x Nov 29, 2021
@rtibbles rtibbles deleted the metadata_labels branch November 29, 2021 17:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
TODO: needs review Waiting for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Search - Getting a 500 error after having applied too many filters
4 participants