[4.0] Removing artificial sharding from com_finder #20185
Conversation
|
As there is administrator/components/com_admin/sql/updates/mysql/4.0.0-2018-04-14.sql |
|
I would need help with the postgres stuff. I don't have postgres on my system and would need someone to provide such a file and test that part. |
|
@alikon Could you help me with adding the right migration script for postgres? |
|
yes i'll do it |
|
Thank you! |
|
Will test. Please let me know, apart from the indexing speed (and obvious code corrections), what I can expect, as a multilingual user, from this PR? |
|
At this moment, there is no change in terms of multilingual users. In order to keep these changes testable, I'm restricting my changes to one thing at a time. So this only removes the sharding and gets it all working again in 4.0. The language changes are coming in the next few steps. |
|
made a pr Hackwar#22 |
the update sql for postgresql
|
Merged |
|
I was unable to replicate the error that you mentioned before applying this PR After applying the PR the indexer did not work at all |
|
I have tested this item 🔴 unsuccessfully on 7716cd6 This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/20185. |
|
The current 4.0-dev branch fails in displaying the frontend when opening the smart search component and having debug mode enabled with the following error message: I have updated the branch with the latest changes from 4.0-dev and checked it again. It does work. Could it be that you did not apply the database changes, that this introduces? I would be happy if you could test this again. |
|
@brianteeman Did you see my last message? Could you check your test again, especially with the database changes? |
|
Sorry with JAB and JDFR it slipped my radar - thanks for the reminder I will put it on the todo for tomorrow |
|
Just to list all current PRs for Finder/Smart Search:
|
|
@Hackwar no change from my previous report |
|
Can you see what the ajax call from the indexer returns as error? |
|
Yes please one pr per task. |
|
Thanks Chris for the explanation. |
…4finder_sharding # Conflicts: # components/com_finder/Model/SearchModel.php
Hackwar
force-pushed
the
j4finder_sharding
branch
from
d9cb32f
to
c80b975
Compare
|
I've reverted the changes of the router and cleaned up the commits. |
| @@ -9,6 +9,8 @@ | |||
|
|
|||
| defined('_JEXEC') or die; | |||
|
|
|||
| JLoader::register('FinderHelperLanguage', JPATH_ADMINISTRATOR . '/components/com_finder/helpers/language.php'); | |||
There was a problem hiding this comment.
Yes, otherwise you get a fatal error when using the advanced filters. And please don't ask me to put all these single line fixes to make com_finder run in 4.0 into separate PRs....
|
Would be good if we can get another round of testing and then I think it is good to merge. |
|
We have 2 successfull tests. Why do we need another round of tests now? |
|
I have tested this item ✅ successfully on c80b975 This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/20185. |
| DROP TABLE #__finder_links_termsd; | ||
| DROP TABLE #__finder_links_termse; | ||
| DROP TABLE #__finder_links_termsf; | ||
|
|
There was a problem hiding this comment.
Changed CHANGE COLUMN to CHANGE, dropped AFTER ... and be on same line to make it work.
ALTER TABLE `#__finder_terms` CHANGE `language` `language` CHAR(7) NOT NULL DEFAULT '';
ALTER TABLE `#__finder_terms_common` CHANGE `language` `language` CHAR(7) NOT NULL DEFAULT '';
ALTER TABLE `#__finder_tokens` CHANGE `language` `language` CHAR(7) NOT NULL DEFAULT '';
ALTER TABLE `#__finder_tokens_aggregate` DROP COLUMN `map_suffix`;
ALTER TABLE `#__finder_tokens_aggregate` CHANGE `language` `language` CHAR(7) NOT NULL DEFAULT '';
ALTER TABLE `#__finder_links` CHANGE `language` `language` CHAR(7) NOT NULL DEFAULT '';
| COLLATE='utf8mb4_general_ci' | ||
| ENGINE=InnoDB | ||
| ; | ||
| DROP TABLE #__finder_links_terms0; |
There was a problem hiding this comment.
Tables are not dropped and I don't know why.
There was a problem hiding this comment.
I did this operation manually, and work ok
There was a problem hiding this comment.
Yes, but not via Extensions > Manage > Database > Fix
There was a problem hiding this comment.
Iirc you can't do a drop that way
|
@Hackwar can you please add the suggested changes to make the DB fixing work and then we are good to go here. |
|
there is no reason to make database>fix work |
|
Ok, misinterpreted that comment. What about the suggestion from @Quy? |
|
I'm hesitant to change the SQL again, mainly because I don't see how what is supposed to work how and more importantly because there are quite a few additional changes on the way for the finder tables. At one point we will have to make one PR that combines all these changes into one SQL file. Can we tackle that issue then? (There are 12 open PRs for finder from me, 4 of which have DB changes) |
|
Does the upgrade script work without the recommendations from @Quy? |
|
With fresh install, it works. |
|
This has database schema changes. Patch tester can't deal with database schema changes (you can apply the patch and run |
|
Just to clarify @chrisdavenport s comment here: With the sharding removed, he had a performance improvement of 44%, not a performance reduction. |
|
Thank you! |
|
Thankyou for being so patient with this! Sorry it took so long to deal with! |
* Decoupling highlighter plugin from com_finder * Codestyle * Codestyle * Version comment * Fixing bug due to #20185 * Using url for display and route for route-URL * Cleaning up finder plugins and adding cleanURL attribute in model * Making OutputFilter::stringJSSafe() utf8 aware
Finder has an artificial sharding, which splits up the links between terms and content items onto 16 seperate tables. This PR removes that sharding. It makes the code extremely complex and bloats it unnecessarily and it also slows down the whole finder system. Indexing the original sample data of Joomla 4.0 with the current finder indexer takes about 2:40 minutes on my system. With these changes, we are already down to 1:30 minute.
This PR also fixes a few other bugs that prevented com_finder from working in Joomla 4.0 right now and I standardised all the language fields to be 7 characters long. While I plan to provide more PRs with further improvements to make com_finder more usable, this PR is in itself complete and can be merged like this.
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. See that it throws errors and doesn't work right now.
Apply this PR and clear the index in the backend. Start the re-indexing and notice that it finishes quicker. Go to the frontend and see that that menu item no longer throws errors and that all features work like expected.
When typing in a phrase and then pressing enter, sending the request to the server, a message "error" is displayed before the page is reloaded both before and after applying the patch. I'm still looking for the source of that issue.
I want to thank cloudaccess.net for supporting these changes. Without their generous help, this wouldn't be possible.