-
Notifications
You must be signed in to change notification settings - Fork 0
Add indexAllProperties config #272
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
base: master
Are you sure you want to change the base?
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #272 +/- ##
============================================
+ Coverage 73.37% 73.70% +0.33%
- Complexity 399 401 +2
============================================
Files 47 47
Lines 1292 1297 +5
============================================
+ Hits 948 956 +8
+ Misses 344 341 -3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| private function getPropertiesToIndex( ItemId $itemType, StatementList $statements ): array { | ||
| if ( $this->config->indexAllProperties ) { | ||
| // TODO: is this sufficient, or do we need to end up explicitly indexing properties not present as empty? | ||
| $properties = $statements->getPropertyIds(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As per https://github.com/ProfessionalWiki/WikibaseFacetedSearch/pull/51/files#r1901363923, it seems we should explicitly tell elastic "there is no value" for all properties not present in the statement list, to avoid keeping data for removed statements in the index.
So we need to replace this "IDs of all properties present in the statement list" with "IDs of all properties in the wiki"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@malberts makes sense to you?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That sounds correct. And also relates to defining all properties in #272 (comment)
3fb4652 to
c25f7be
Compare
| <p> | ||
| Indexing values for all properties is useful | ||
| when you want to be able to run structured queries for properties not shown in the UI. It is also | ||
| useful to avoid having to rebuild the search index whenever you add a new property to the UI. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add a new property to the UI.
I think this can be misunderstood. I get this refers to the faceted search UI, but I can see how this might look like it's referring to the general UI (including Wikibase).
Adding an existing property (already indexed) in the faceted search config works automatically. But when a new property is added on the wiki and the config, then the index must still be rebuilt.
The hook for defining the fields to be indexed is separate from the hook returning the data.
|
Does this code actually work as intended? (I did not check manually.) I just realised that I didn't mention
My understanding of the code in this PR is that it will index all defined facet properties on all items, but if a property is not defined as a facet then it will not get indexed. I believe we need to return all the wiki properties there. (Unfortunately, an integration test for this is outstanding: #256) |
|
I did not test this yet and did not look at |
Fixes #271