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

Form filter: filter autocomplete list based on previous applied filters #5045

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

nboisteault
Copy link
Member

@nboisteault nboisteault commented Dec 2, 2024

This PR changes the behaviour of the form filter. Previously, all unique values from the text field (with the autocompletion activated) were downloaded once at startup. Now, every time the user types letters, a new request is triggered with the given filter and the LIKE and fetches all corresponding data.

Funded by VSB

simplescreenrecorder-2024-12-03_10.51.09.mp4

@github-actions github-actions bot added this to the 3.10.0 milestone Dec 2, 2024
@nboisteault nboisteault force-pushed the form_filter_dynamic_autocomplete branch from ad438a9 to 6f17feb Compare December 3, 2024 08:58
@nboisteault nboisteault marked this pull request as ready for review December 3, 2024 08:58
@nboisteault nboisteault added filter forms run end2end If the PR must run end2end tests or not backport release_3_9 labels Dec 3, 2024
@nboisteault nboisteault added the sponsored development This development has been funded label Dec 3, 2024
@nboisteault nboisteault modified the milestones: 3.10.0, 3.9.0 Dec 3, 2024
@nboisteault nboisteault self-assigned this Dec 3, 2024
@Gustry
Copy link
Member

Gustry commented Dec 3, 2024

Nice addition.

Related to form_filter, is there any chance we can remove the manual test about the "form filter" at the same time, as you moved from Cypress to Playwright ? https://github.com/3liz/lizmap-web-client/blob/master/tests/qgis-projects/tests/form_filter.md (maybe with the help of echoproxy ?)

@github-actions github-actions bot modified the milestones: 3.9.0, 3.10.0 Dec 3, 2024
@nboisteault
Copy link
Member Author

Nice addition.

Related to form_filter, is there any chance we can remove the manual test about the "form filter" at the same time, as you moved from Cypress to Playwright ? https://github.com/3liz/lizmap-web-client/blob/master/tests/qgis-projects/tests/form_filter.md (maybe with the help of echoproxy ?)

Done bae3440

Copy link
Member

@Gustry Gustry left a comment

Choose a reason for hiding this comment

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

Thanks for the removal of the manual test ... :)

tests/end2end/playwright/form-filter.spec.js Outdated Show resolved Hide resolved
@mdouchin
Copy link
Collaborator

mdouchin commented Dec 3, 2024

This PR changes the behaviour of the form filter. Previously, all unique values from the text field (with the autocompletion activated) were downloaded once at startup. Now, every time the user types letters, a new request is triggered with the given filter and the LIKE and fetches all corresponding data.

It is more dynamic and better than before, but I think we should test it against a big dataset to evaluate the performance. LIKE clauses are known to have poor performances. We could promote the use of the pg_trgm PostgreSQL extension (and add index as described here https://docs.lizmap.com/next/en/publish/configuration/spatial_search.html#optimisation)

@nboisteault
Copy link
Member Author

This PR changes the behaviour of the form filter. Previously, all unique values from the text field (with the autocompletion activated) were downloaded once at startup. Now, every time the user types letters, a new request is triggered with the given filter and the LIKE and fetches all corresponding data.

It is more dynamic and better than before, but I think we should test it against a big dataset to evaluate the performance. LIKE clauses are known to have poor performances. We could promote the use of the pg_trgm PostgreSQL extension (and add index as described here https://docs.lizmap.com/next/en/publish/configuration/spatial_search.html#optimisation)

I've tested with 1 100 000 lines inserted with:

INSERT INTO tests_projects.form_filter (label, geom)
SELECT
 md5(random()::text),
 null
FROM generate_series(1, 1000000);

Request made to get values when typing 'abc' returns in 800ms. 'abcd' or 'abcde' in 300ms.

@mdouchin
Copy link
Collaborator

mdouchin commented Dec 3, 2024

Request made to get values when typing 'abc' returns in 800ms. 'abcd' or 'abcde' in 300ms.

great. Have you tried by using the following index

CREATE EXTENSION IF NOT EXISTS pg_trgm;
DROP INDEX IF EXISTS test_index;
CREATE INDEX test_index ON tests_projects.form_filter USING GIN (label gin_trgm_ops);

@nboisteault
Copy link
Member Author

Request made to get values when typing 'abc' returns in 800ms. 'abcd' or 'abcde' in 300ms.

great. Have you tried by using the following index

CREATE EXTENSION IF NOT EXISTS pg_trgm;
DROP INDEX IF EXISTS test_index;
CREATE INDEX test_index ON tests_projects.form_filter USING GIN (label gin_trgm_ops);

Now request made to get values when typing 'abc' returns in 500ms. 'abcd' or 'abcde' in 100ms.

if (!globalThis['filterConfigData'].filter) {
sdata.filter = autocompleteFilter;
} else {
sdata.filter = globalThis['filterConfigData'].filter + ' AND ' + autocompleteFilter;
Copy link
Collaborator

Choose a reason for hiding this comment

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

The generated filter can contain OR terms. We should use parenthesis to ensure the correct logic will be used. Spaces are used around them to please QGIS :

Suggested change
sdata.filter = globalThis['filterConfigData'].filter + ' AND ' + autocompleteFilter;
sdata.filter = ' ( ' + globalThis['filterConfigData'].filter + ' ) AND ' + autocompleteFilter;

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport release_3_9 filter forms run end2end If the PR must run end2end tests or not sponsored development This development has been funded
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants