[4.0] Make the tuple length of com_finder configurable #20384

Hackwar · 2018-05-13T21:44:48Z

[UPDATE] After the initial discussion, I changed this to make the tuple length of com_finder be configurable. This should make this less controversial.

Original text for the old PR

Finder is indexing not only every word, but also every combination of 2- and 3-word-tuples in the text to be indexed. This results in gigantic storage consumption by the index and a performance decrease when indexing of about 20%. At the same time, it only marginally improves the search results. When removing this feature, we are sacrificing the possibility to search for specific phrases of up to 3 terms (like "Joomla is great") for a reduction in necessary storage space of about 2/3. (and the performance increase in indexing)

How to test

Install 4.0-dev and go to the backend of Smart Search. Enable the Smart Search Plugin and start the indexing process. Notice that it takes a long time (preferably measure the time with a stopwatch). Go to the frontend and open the Smart Search menu item. Search for something and notice the results.
Now apply these changes and go into the backend of Smart Search. Clear the index and rebuild the index, again checking with a stopwatch. You should see a 20% performance increase here. Now go to the frontend and do the same search. You should get the same result.
Do a search for a specific phrase, wrapping the phrase in quotes. See that this does not work anymore.

Please also see #20185 for further improvements for com_finder.

I want to thank cloudaccess.net for supporting these changes. Without their generous help, this wouldn't be possible.

chrisdavenport · 2018-05-13T21:55:58Z

I'd be opposed to this change as it is removing a feature that is required on some sites. A better solution would be to make the number of words per tuple a configurable parameter of the indexer (default to 1 for new installations).

Hackwar · 2018-05-13T22:22:35Z

Making this configurable would mean yet another option and quite a bit more code for something that 98% out there don't need. At the same time I think that smart search is not and can not be the ultimate site search engine, simply because we are writing a CMS and not a search engine. This component is a feature for all those sites out there that need a slightly better search than com_search, but if you need something really sophisticated, use a pro tool like Elastic Search.

So I'm advocating bit less complexity here. Right now this component is so complex that no one really worked out the issues in the last 5 years. Don't force this to stay abandonware.

Hackwar · 2018-05-21T08:19:29Z

Just to list all current PRs for Finder/Smart Search:

[4.0] Removing artificial sharding from com_finder #20185 [4.0] Removing artificial sharding from com_finder
[4.0] Make the tuple length of com_finder configurable #20384 [4.0] Removing terms tuples from com_finder
[4.0] Using language specific tokeniser and stemmer for com_finder #20391 [4.0] Using language specific tokeniser and stemmer for com_finder

laoneo · 2018-05-28T08:51:36Z

I'm here with @chrisdavenport. This is a feature I like. Perhaps we can think of a new option to enable/disable tuples. But I would not remove the functionality. I'm sure with some use of the showon property, we can make to options more lightweight.

Hackwar · 2018-06-03T18:20:23Z

I've now made this configurable.

…4finder_termsphrases

brianteeman · 2018-06-03T18:21:48Z

administrator/components/com_finder/helpers/indexer/helper.php

@@ -168,33 +174,28 @@ public static function tokenize($input, $lang, $phrase = false)
 				$tokens[] = new FinderIndexerToken($terms[$i], $lang);
 			}

-			// Create two and three word phrase tokens from the individual words.
-			for ($i = 0, $n = count($tokens); $i < $n; $i++)
+			// Create multi-word phrases tokens from the individual words.


phrase not phrases

brianteeman · 2018-06-03T18:24:12Z

administrator/language/en-GB/en-GB.com_finder.ini

@@ -40,6 +40,7 @@ COM_FINDER_CONFIG_STEMMER_FR="French Only"
 COM_FINDER_CONFIG_STEMMER_LABEL="Select Language Stemmer"
 COM_FINDER_CONFIG_STEMMER_PORTER_EN="English Only"
 COM_FINDER_CONFIG_STEMMER_SNOWBALL="Snowball"
+COM_FINDER_CONFIG_TUPLECOUNT_LABEL="Length indexed tuples"


Really hard for me to say i this label is correct or not as I dont understand it

And I have no idea how to phrase this short and elegantly. This is the setting how Joomla should index the text. If you set this to 1, then each word is just indexed separately. If you set it to something > 1, it also indexes every tuple of length 2 to $tuplecount.

Practical example: Text is

Joomla is a good CMS for everything

Then Joomla would index the following tuples for a setting of 3:

Joomla

is

a

good

CMS

for

everything

Joomla is

is a

a good

good CMS

CMS for

for everything

Joomla is a

is a good

a good CMS

good CMS for

CMS for everything

Simply label it with
Tuple Length
or
Maximum Tuple Length

??

brianteeman

approve language strings

chrisdavenport · 2018-06-03T21:34:12Z

administrator/language/en-GB/en-GB.com_finder.ini

@@ -40,6 +40,7 @@ COM_FINDER_CONFIG_STEMMER_FR="French Only"
 COM_FINDER_CONFIG_STEMMER_LABEL="Select Language Stemmer"
 COM_FINDER_CONFIG_STEMMER_PORTER_EN="English Only"
 COM_FINDER_CONFIG_STEMMER_SNOWBALL="Snowball"
+COM_FINDER_CONFIG_TUPLECOUNT_LABEL="Tuple Length"


I wonder if "Words Per Phrase" might be a little clearer?

If that's correct then it's better. Although to be honest all the terms here are pretty meaningless. Snowball? Stemmer?

I'll change the options to include an "advanced" switch that hides those options...

infograf768 · 2018-06-04T05:09:22Z

FYI I looked all over for a wording in French for tuple easily understandable (specialists use the word tuple).
Found "Ligne d'enregistrement d'une table relationnelle. " is also a definition for specialists...
Therefore, without a tip to explain what it is about, "Words per phrase" looks like the less bad solution.

Hackwar · 2018-06-12T09:05:40Z

I changed the wording. I would put the change with the advanced option for the configuration in another PR, since there are a few changes in the other PRs for com_finder.

carlitorweb · 2018-06-18T14:39:27Z

WITHOUT THE PR (Index perform)

WITH THE PR (Index perform)

WITHOUT THE PR (Search perform)

WITH THE PR (Search perform)

IMO Sacrifice the ability to search for a specific phrase, if with this we obtain a better performance, worth it. It may be that after we have all these PRs inside, we can find a way to return this ability, or something similar.

carlitorweb · 2018-06-18T14:41:29Z

Also, I notice, while I try search, the focus of the input gone, and I needed to click on the input again to continue writing. Some JS wrong? need check the console. Anyway, this is not related to this PR

carlitorweb · 2018-06-18T14:51:07Z

I have tested this item ✅ successfully on 0e20307

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/20384.}

infograf768 · 2018-07-03T15:01:09Z

With these changes a lot of the "Advanced" tips are not necessary in the frontend form when "tuplecount' is set to 1. String is COM_FINDER_ADVANCED_TIPS

I am speaking of

I suggest therefore to condition their display.

Other than that, this looks fine. Thank you.

…4finder_termsphrases

Hackwar · 2018-07-04T20:30:53Z

I added separate translation strings for each search feature and an intro and outro string.

infograf768 · 2018-07-05T14:02:11Z

will test asap

Hackwar · 2018-07-07T18:13:30Z

@carlitorweb @infograf768 would you have a few minutes to test this again?

Quy · 2018-07-08T03:05:28Z

Installed the sample demo, searched "blog" and not "sample" and got the following error:

PHP Notice:  Undefined offset: 0 in C:\xampp\htdocs\joomla-cms-j4finder_termsphrases\administrator\components\com_finder\helpers\indexer\query.php on line 1180
PHP Notice:  Trying to get property 'phrase' of non-object in C:\xampp\htdocs\joomla-cms-j4finder_termsphrases\administrator\components\com_finder\helpers\indexer\query.php on line 1262
PHP Notice:  Trying to get property 'term' of non-object in C:\xampp\htdocs\joomla-cms-j4finder_termsphrases\administrator\components\com_finder\helpers\indexer\query.php on line 1271
PHP Notice:  Trying to get property 'stem' of non-object in C:\xampp\htdocs\joomla-cms-j4finder_termsphrases\administrator\components\com_finder\helpers\indexer\query.php on line 1271
PHP Warning:  Creating default object from empty value in C:\xampp\htdocs\joomla-cms-j4finder_termsphrases\administrator\components\com_finder\helpers\indexer\query.php on line 1280
PHP Notice:  Undefined property: stdClass::$term in C:\xampp\htdocs\joomla-cms-j4finder_termsphrases\administrator\components\com_finder\helpers\indexer\query.php on line 1301
PHP Notice:  Undefined property: stdClass::$phrase in C:\xampp\htdocs\joomla-cms-j4finder_termsphrases\administrator\components\com_finder\helpers\indexer\query.php on line 1302
PHP Notice:  Undefined property: stdClass::$term in C:\xampp\htdocs\joomla-cms-j4finder_termsphrases\administrator\components\com_finder\helpers\indexer\query.php on line 1321
PHP Notice:  Undefined property: stdClass::$term in C:\xampp\htdocs\joomla-cms-j4finder_termsphrases\administrator\components\com_finder\helpers\indexer\query.php on line 1321
PHP Notice:  Undefined property: stdClass::$term in C:\xampp\htdocs\joomla-cms-j4finder_termsphrases\administrator\components\com_finder\helpers\indexer\query.php on line 1321
PHP Notice:  Undefined property: stdClass::$term in C:\xampp\htdocs\joomla-cms-j4finder_termsphrases\administrator\components\com_finder\helpers\indexer\query.php on line 1321
PHP Notice:  Undefined property: stdClass::$term in C:\xampp\htdocs\joomla-cms-j4finder_termsphrases\administrator\components\com_finder\helpers\indexer\query.php on line 1321
PHP Notice:  Undefined property: stdClass::$term in C:\xampp\htdocs\joomla-cms-j4finder_termsphrases\administrator\components\com_finder\helpers\indexer\query.php on line 1321
PHP Notice:  Undefined property: stdClass::$term in C:\xampp\htdocs\joomla-cms-j4finder_termsphrases\administrator\components\com_finder\helpers\indexer\query.php on line 1321
PHP Notice:  Undefined property: stdClass::$term in C:\xampp\htdocs\joomla-cms-j4finder_termsphrases\administrator\components\com_finder\helpers\indexer\query.php on line 1321
PHP Notice:  Undefined property: stdClass::$term in C:\xampp\htdocs\joomla-cms-j4finder_termsphrases\administrator\components\com_finder\helpers\indexer\query.php on line 1321
PHP Notice:  Undefined offset: 0 in C:\xampp\htdocs\joomla-cms-j4finder_termsphrases\administrator\components\com_finder\helpers\indexer\query.php on line 1191
PHP Notice:  Undefined property: stdClass::$term in C:\xampp\htdocs\joomla-cms-j4finder_termsphrases\administrator\components\com_finder\helpers\indexer\query.php on line 458

infograf768 · 2018-07-08T08:59:10Z

@Quy
The format is not
"blog" and not "sample"
but
blog not sample

EDIT: One should be able to also obtain the right results with
"blog" not "sample"
can you test again?

infograf768 · 2018-07-08T09:22:05Z

I have fatal error here before and after patch
[08-Jul-2018 11:02:36 Europe/Berlin] PHP Fatal error: Cannot use Joomla\CMS\Factory as Factory because the name is already in use in /Applications/MAMP/htdocs/installmulti/joomla40/components/com_finder/View/Search/HtmlView.php on line 20

Looking at the file I see that
use Joomla\CMS\Factory;
is used twice
IMHO this needs a specific urgent PR or direct change by maintainers of 4.0 ( @Webdongle @laoneo @mbabker )

infograf768 · 2018-07-08T09:34:20Z

After correcting this locally, all works fine.

infograf768 · 2018-07-08T09:41:19Z

PR for the Fatal Error : #21016

Hackwar · 2018-07-08T10:02:45Z

@Quy This is an interesting error. I just tested this and this is also present in 3.x. Can we merge/test this one without fixing that bug here and now? I will provide another PR to fix this soon. Just want this one merged in...

Hackwar · 2018-07-08T10:03:08Z

@infograf768 thanks for your PR. 😄

infograf768 · 2018-07-08T10:04:28Z

How may using a wrong format be a code error?

Hackwar · 2018-07-08T10:31:42Z

using a "wrong" format should indeed not be a big issue. In worst case you get no result at all. But it should NEVER throw an error like it does right now. @Quy might not get the result that he is looking for, but he definitely should not get the warnings that he got right now. That is an issue that we also have to fix in 3.x and you could consider that a (low-level) security issue. It is an information disclosure.

carlitorweb · 2018-07-08T18:08:54Z

@wilsonge

Can we merge/test this one without fixing that bug here and now? I will provide another PR to fix this soon. Just want this one merged in...

infograf768 · 2018-07-10T05:25:46Z

I have tested this item ✅ successfully on 5f21dc9

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/20384.}

ghost · 2018-07-10T07:08:00Z

@carlitorweb can you please retest?

carlitorweb · 2018-07-10T12:56:13Z

I have tested this item ✅ successfully on 5f21dc9

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/20384.}

ghost · 2018-07-10T13:26:52Z

Ready to Commit after two successful tests.

Hackwar · 2018-07-18T18:24:23Z

Thank you!

wilsonge · 2018-07-18T21:21:30Z

Thankyou!

joomla-cms-bot added the PR-4.0-dev label May 13, 2018

Hackwar mentioned this pull request May 21, 2018

[4.0] Removing artificial sharding from com_finder #20185

Merged

This was referenced May 21, 2018

[4.0] Using language specific tokeniser and stemmer for com_finder #20391

Merged

Add "whole word" search option to search's #17751

Closed

Make the tuple length in com_finder configurable

c26f898

Hackwar force-pushed the j4finder_termsphrases branch from 7a5ab32 to c26f898 Compare June 3, 2018 18:16

Hackwar requested review from brianteeman and mbabker as code owners June 3, 2018 18:16

joomla-cms-bot added the Language Change This is for Translators label Jun 3, 2018

Hackwar changed the title ~~[4.0] Removing terms tuples from com_finder~~ [4.0] Make the tuple length of com_finder configurable Jun 3, 2018

Hackwar added 2 commits June 3, 2018 20:21

Codestyle

08657ba

Merge branch '4.0-dev' of https://github.com/joomla/joomla-cms into j…

6323f80

…4finder_termsphrases

brianteeman reviewed Jun 3, 2018

View reviewed changes

Hackwar added 2 commits June 3, 2018 20:24

Comment fix

bc06e55

Changing wording

1d35cec

brianteeman approved these changes Jun 3, 2018

View reviewed changes

chrisdavenport reviewed Jun 3, 2018

View reviewed changes

Hackwar added 2 commits June 12, 2018 09:00

Merge branch '4.0-dev' into j4finder_termsphrases

97fb57b

Changing wording for finder option

0e20307

Hackwar added 2 commits July 4, 2018 22:04

Merge branch '4.0-dev' of https://github.com/joomla/joomla-cms into j…

9487f9c

…4finder_termsphrases

Integrating proposals

5f21dc9

infograf768 mentioned this pull request Jul 8, 2018

[4.0] com_finder: "use Joomla\CMS\Factory;" one call too much #21016

Merged

mbabker approved these changes Jul 10, 2018

View reviewed changes

joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Jul 10, 2018

Merge branch '4.0-dev' into j4finder_termsphrases

b9a96a0

wilsonge merged commit 1c59a62 into joomla:4.0-dev Jul 18, 2018

joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Jul 18, 2018

wilsonge added this to the Joomla 4.0 milestone Jul 18, 2018

Hackwar deleted the j4finder_termsphrases branch October 23, 2020 19:58

[4.0] Make the tuple length of com_finder configurable #20384

[4.0] Make the tuple length of com_finder configurable #20384

Conversation

Hackwar commented May 13, 2018 • edited by joomla-cms-bot Loading

Original text for the old PR

How to test

chrisdavenport commented May 13, 2018

Hackwar commented May 13, 2018

Hackwar commented May 21, 2018

laoneo commented May 28, 2018

Hackwar commented Jun 3, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brianteeman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

infograf768 commented Jun 4, 2018

Hackwar commented Jun 12, 2018

carlitorweb commented Jun 18, 2018

carlitorweb commented Jun 18, 2018 • edited Loading

carlitorweb commented Jun 18, 2018

infograf768 commented Jul 3, 2018

Hackwar commented Jul 4, 2018

infograf768 commented Jul 5, 2018

Hackwar commented Jul 7, 2018

Quy commented Jul 8, 2018 • edited Loading

infograf768 commented Jul 8, 2018 • edited Loading

infograf768 commented Jul 8, 2018

infograf768 commented Jul 8, 2018

infograf768 commented Jul 8, 2018

Hackwar commented Jul 8, 2018

Hackwar commented Jul 8, 2018

infograf768 commented Jul 8, 2018

Hackwar commented Jul 8, 2018

carlitorweb commented Jul 8, 2018

infograf768 commented Jul 10, 2018

ghost commented Jul 10, 2018

carlitorweb commented Jul 10, 2018

ghost commented Jul 10, 2018

Hackwar commented Jul 18, 2018

wilsonge commented Jul 18, 2018

Hackwar commented May 13, 2018 •

edited by joomla-cms-bot

Loading

carlitorweb commented Jun 18, 2018 •

edited

Loading

Quy commented Jul 8, 2018 •

edited

Loading

infograf768 commented Jul 8, 2018 •

edited

Loading