Xapian-based catalog search #460

veloman-yunkan · 2021-03-07T09:15:50Z

Fixes #106

Depends on kiwix/kiwix-build#477. Must be merged after #459.

Changes in functionality:

Previously partial matches were implicitly supported. Currently this must be achieved by the usage of an explicit wildcard (as demonstrated by the change in the test/library.cpp unit test).

Notes:

No stemming is performed on the query, since the language assumed for the query is not known for sure. Stemming can be applied if the lang query parameter is used.
Translation of ISO 639-3 language codes to languages supported by Xapian is done only for a subset of languages. This is trivial to fix (though conceptually it's less trivial than it may seem, due to such thing as macrolanguages).

veloman-yunkan · 2021-03-07T09:27:32Z

CI fails because of the said dependence on kiwix/kiwix-build#477

codecov · 2021-03-07T16:40:45Z

Codecov Report

Merging #460 (20b487d) into master (f7c867f) will increase coverage by 0.35%.
The diff coverage is 91.30%.

@@            Coverage Diff             @@
##           master     #460      +/-   ##
==========================================
+ Coverage   62.65%   63.00%   +0.35%     
==========================================
  Files          50       50              
  Lines        3433     3490      +57     
  Branches     1736     1773      +37     
==========================================
+ Hits         2151     2199      +48     
- Misses       1280     1289       +9     
  Partials        2        2

Impacted Files	Coverage Δ
src/library.cpp	`67.49% <91.04%> (+3.31%)`	⬆️
include/library.h	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f7c867f...20b487d. Read the comment docs.

mgautierfr

Why not using xapian to do the full query (size, language, creator, name, local, remote, ...) ?

As kiwix-lib now use xapian we must also update meson.

On libzim we use icu (https://github.com/openzim/libzim/blob/master/src/writer/xapianIndexer.cpp#L39-L40) to translate between different ISO language code.

mgautierfr · 2021-03-09T09:54:48Z

src/library.cpp

+  const auto flags = Xapian::QueryParser::FLAG_PHRASE
+                   | Xapian::QueryParser::FLAG_BOOLEAN
+                   | Xapian::QueryParser::FLAG_LOVEHATE
+                   | Xapian::QueryParser::FLAG_WILDCARD;


Why not using FLAG_PARTIAL ?
It would allow the user to not enter the end wildcard.

It only makes sense if the search endpoint is going to be used for suggestions. @kelson42 What's your opinion on that?

The end point is for now not used for completion/suggestions but as a "normal" search. Not sure if we have a lot to loose to add FLAG_PARTIAL but the default choice seems for me, for the moment, to keep it aside. @mgautierfr Maybe you have arguments to use it here?

This is used in kiwix-desktop to search in the library (local or remote). Search updated "in live" (no need to validate the search).
It seems legit that if the user search for "wiki" he gets result for all ("wikipedia", "wikispecies", ...)
If we don't, he will have no result until he enters a full word.

I am reluctant to use the partial search mode in the backend since there is no indication of where the editing/cursor location in the text is. Though typically text is typed left to right (at least in the languages I am concerned with), it is also possible that the cursor is moved to another location and the text is edited in the middle. It is the front end (which is responsible for the editing of the text search string) that can add a hint about the cursor location in the form of a wildcard.

I am reluctant to use the partial search mode in the backend
It is the front end [...] than can add a hint [...] in form of a wildcard

I globally agree with that.

However, as the documentation of FLAG_PARTIAL said, the wildcard should be added only in some situation (not a phrase, no a binary search, not ...). It could be pretty difficult to detect in the frontend (and we don't want to have all frontend implement it).

Maybe we could add a "boolean flag" in the api telling if we want a partial search or not and let xapian add it (if it is needed).

Maybe we could add a "boolean flag" in the api telling if we want a partial search or not and let xapian add it (if it is needed).

Done this way.

veloman-yunkan · 2021-03-09T11:58:04Z

Why not using xapian to do the full query (size, language, creator, name, local, remote, ...) ?

That was my intention too, but I chose to stick to what #106 asks for. I would like to keep this PR small and enhance the Xapian based catalog search in a follow-up PR. We will have to define how to handle tags.

kelson42 · 2021-03-09T12:24:34Z

Why not using xapian to do the full query (size, language, creator, name, local, remote, ...) ?

That was my intention too, but I chose to stick to what #106 asks for. I would like to keep this PR small and enhance the Xapian based catalog search in a follow-up PR. We will have to define how to handle tags.

Seems legit

kelson42 · 2021-03-09T12:25:21Z

As kiwix-lib now use xapian we must also update meson.

I believe this is rollbacking a commit made a few months ago ?!

veloman-yunkan · 2021-03-09T15:16:25Z

Reminding that this PR is based on #459. It would be better to review and merge that one first.

veloman-yunkan · 2021-03-10T12:02:45Z

As kiwix-lib now use xapian we must also update meson.

Can't we rely on the required indirect xapian dependency brought by libzim?

https://github.com/kiwix/kiwix-lib/blob/f7608c378ee2d1250ce225c0aaefe2452299d481/meson.build#L46-L49

mgautierfr · 2021-03-10T13:18:28Z

Can't we rely on the required indirect xapian dependency brought by libzim?

No.
It works in our CI as we install the "dev" part of all ours dependencies (including xapian) in our dependency archive
But in another build system (rpm/dev packages), if we don't declare kiwix-lib depending of xapian, only the libzim-dev and xapian (among other packages) will be present. We will not have xapian-dev installed (and so, no header nor .so) (And if we build statically, xapian will be a private dependency of libzim. So we will not have the xapian static archive to link with).

mgautierfr

Just a small (style) remark on the new commits. I will not block for that.

I still "Request Changes" as we need to update our dependencies to add xapian.

src/library.cpp

meson.build

kelson42 · 2021-03-17T11:39:54Z

@mgautierfr @veloman-yunkan Probably time now to rebase ad complete the review process of this PR.

veloman-yunkan · 2021-03-17T12:29:21Z

@mgautierfr @veloman-yunkan Probably time now to rebase ad complete the review process of this PR.

I am done with my part

1. Get the subset of books matching the q (title/description) parameter of the search 2. Filter out books not matching the other parameters of the search. Stage 1. currently works in the old way, but will be replaced by Xapian based search in subsequent commits.

The search text in the catalog query is interpreted as partial by default, but partial query mode can be disabled in C++. The latter possibility is not exposed via the /catalog/search kiwix-serve endpoint, though.

Language code is converted from ISO 639-3 to ISO 639 (which is understood by Xapian) via ICU. The previous approach via an explicit map had its advantages since Xapian has more than one stemmer implementations for some languages (selectable via Xapian-specific identifiers). This commit relies on the defaults associated with the ISO 639 language codes.

veloman-yunkan · 2021-03-17T19:14:08Z

Didn't kiwix/kiwix-build#479 have to be merged before this one?

kelson42 · 2021-03-17T20:23:03Z

@veloman-yunkan I have merged it

This should have been done back in PR #460

veloman-yunkan mentioned this pull request Mar 7, 2021

Enable inmemory backend in xapian kiwix/kiwix-build#477

Merged

veloman-yunkan requested a review from mgautierfr March 7, 2021 09:26

veloman-yunkan force-pushed the xapian_based_catalog_search branch from 029f5d4 to 72c566c Compare March 7, 2021 16:43

mgautierfr requested changes Mar 9, 2021

View reviewed changes

kelson42 closed this Mar 9, 2021

mgautierfr reopened this Mar 9, 2021

veloman-yunkan force-pushed the xapian_based_catalog_search branch from 72c566c to b020798 Compare March 10, 2021 11:52

veloman-yunkan requested a review from mgautierfr March 10, 2021 12:54

mgautierfr requested changes Mar 10, 2021

View reviewed changes

src/library.cpp Show resolved Hide resolved

veloman-yunkan requested a review from mgautierfr March 10, 2021 13:51

mgautierfr requested changes Mar 10, 2021

View reviewed changes

meson.build Show resolved Hide resolved

veloman-yunkan force-pushed the xapian_based_catalog_search branch from e3f8b36 to d68dcfd Compare March 10, 2021 13:57

veloman-yunkan force-pushed the opds_category_support branch 2 times, most recently from 39842b2 to b7b0bdb Compare March 17, 2021 10:20

veloman-yunkan force-pushed the xapian_based_catalog_search branch from d68dcfd to 3d34078 Compare March 17, 2021 10:20

Base automatically changed from opds_category_support to master March 17, 2021 11:37

kelson42 linked an issue Mar 17, 2021 that may be closed by this pull request

[library] Use Xapian for library (fulltext) search in title/description #106

Closed

veloman-yunkan requested a review from mgautierfr March 17, 2021 12:57

veloman-yunkan added 2 commits March 17, 2021 14:32

Library::BookIdCollection typedef

db06b2c

veloman-yunkan added 8 commits March 17, 2021 14:32

Library::getBooksByTitleOrDescription()

a17fc0e

Initial version of Xapian-based catalog search

a599fb3

Added some diversity to test/data/library.xml

9e887ca

LibraryServerTest.catalog_prefix_search

6b600a1

LibraryServerTest.catalog_search_with_word_exclusion

47c67a4

Support for partial queries in catalog search

09233bf

The search text in the catalog query is interpreted as partial by default, but partial query mode can be disabled in C++. The latter possibility is not exposed via the /catalog/search kiwix-serve endpoint, though.

Added Xapian as direct dependency

20b487d

mgautierfr force-pushed the xapian_based_catalog_search branch from 3d34078 to 20b487d Compare March 17, 2021 13:32

mgautierfr approved these changes Mar 17, 2021

View reviewed changes

mgautierfr merged commit baed447 into master Mar 17, 2021

mgautierfr deleted the xapian_based_catalog_search branch March 17, 2021 13:45

This was referenced Mar 19, 2021

Unable to compile anymore Kiwix-Desktop kiwix/kiwix-desktop#615

Closed

We must not add xapian include in public headers. #469

Closed

This was referenced Apr 9, 2021

[library] Use Xapian for library search on all fields #484

Closed

Library::removeBookById() updates the reader list and the search DB #485

Merged

veloman-yunkan added a commit that referenced this pull request Apr 11, 2021

Dropped unused functions from Filter's private API

02e7172

This should have been done back in PR #460

veloman-yunkan added a commit that referenced this pull request Apr 11, 2021

Dropped unused functions from Filter's private API

3abe3f9

This should have been done back in PR #460

veloman-yunkan added a commit that referenced this pull request Apr 13, 2021

Dropped unused functions from Filter's private API

ff6e0f6

This should have been done back in PR #460

veloman-yunkan added a commit that referenced this pull request Apr 16, 2021

Dropped unused functions from Filter's private API

08387cf

This should have been done back in PR #460

veloman-yunkan added a commit that referenced this pull request Apr 27, 2021

Dropped unused functions from Filter's private API

2d76f83

This should have been done back in PR #460

veloman-yunkan mentioned this pull request Sep 18, 2024

kiwix-serve raises 500 error on Or search term #1104

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Xapian-based catalog search #460

Xapian-based catalog search #460

veloman-yunkan commented Mar 7, 2021 •

edited

Loading

veloman-yunkan commented Mar 7, 2021

codecov bot commented Mar 7, 2021 •

edited

Loading

mgautierfr left a comment

mgautierfr Mar 9, 2021

veloman-yunkan Mar 9, 2021

kelson42 Mar 9, 2021

mgautierfr Mar 9, 2021

veloman-yunkan Mar 9, 2021

mgautierfr Mar 9, 2021

veloman-yunkan Mar 10, 2021

veloman-yunkan commented Mar 9, 2021

kelson42 commented Mar 9, 2021

kelson42 commented Mar 9, 2021

veloman-yunkan commented Mar 9, 2021

veloman-yunkan commented Mar 10, 2021

mgautierfr commented Mar 10, 2021

mgautierfr left a comment

kelson42 commented Mar 17, 2021

veloman-yunkan commented Mar 17, 2021

veloman-yunkan commented Mar 17, 2021

kelson42 commented Mar 17, 2021

Xapian-based catalog search #460

Xapian-based catalog search #460

Conversation

veloman-yunkan commented Mar 7, 2021 • edited Loading

veloman-yunkan commented Mar 7, 2021

codecov bot commented Mar 7, 2021 • edited Loading

Codecov Report

mgautierfr left a comment

Choose a reason for hiding this comment

mgautierfr Mar 9, 2021

Choose a reason for hiding this comment

veloman-yunkan Mar 9, 2021

Choose a reason for hiding this comment

kelson42 Mar 9, 2021

Choose a reason for hiding this comment

mgautierfr Mar 9, 2021

Choose a reason for hiding this comment

veloman-yunkan Mar 9, 2021

Choose a reason for hiding this comment

mgautierfr Mar 9, 2021

Choose a reason for hiding this comment

veloman-yunkan Mar 10, 2021

Choose a reason for hiding this comment

veloman-yunkan commented Mar 9, 2021

kelson42 commented Mar 9, 2021

kelson42 commented Mar 9, 2021

veloman-yunkan commented Mar 9, 2021

veloman-yunkan commented Mar 10, 2021

mgautierfr commented Mar 10, 2021

mgautierfr left a comment

Choose a reason for hiding this comment

kelson42 commented Mar 17, 2021

veloman-yunkan commented Mar 17, 2021

veloman-yunkan commented Mar 17, 2021

kelson42 commented Mar 17, 2021

veloman-yunkan commented Mar 7, 2021 •

edited

Loading

codecov bot commented Mar 7, 2021 •

edited

Loading