facets: integrate combined_subjects / fix nested subject faceting #1625
Conversation
fenekku
commented
Nov 29, 2023
•
fenekku
commented
⚠️ depends on facets: provide CombinedTermsFacet to fix #798 [+] invenio-records-resources#549 being merged and released- closes Subjects are improperly aggregated #798
- closes subject_nested facet issue invenio-app-rdm#1529 (at least I was able to experience the absence of that problem 😅 )
fenekku
force-pushed
the
798_faceting_fix_records_resources_integration
branch
3 times, most recently
from
63e78b0 to
83ccf45
Compare
| # subject_combined does require a pre-existing change to indexed documents, | ||
| # so it's unclear if a direct replacement is right. | ||
| # Keeping it around until v13 might be better. On the flipside it is an incorrect | ||
| # facet... |
There was a problem hiding this comment.
Yeah, I don't know what we want to do here:
- do we replace
"subject_nested": {"facet": facets.subject_nested}by"subject_nested": {"facet": facets.subject_combined}directly (or even in facets.py changesubject_nested)? - or do we do as here which is a more gradual transition (but leaves people with the issue if they don't make the switch)?
- other takes?
invenio_rdm_records/records/api.py
Outdated
| @@ -339,7 +341,7 @@ class RDMDraft(CommonFieldsMixin, Draft): | |||
|
|
|||
| model_cls = models.RDMDraftMetadata | |||
|
|
|||
| index = IndexField("rdmrecords-drafts-draft-v6.0.0", search_alias="rdmrecords") | |||
| index = IndexField("rdmrecords-drafts-draft-v6.1.0", search_alias="rdmrecords") | |||
There was a problem hiding this comment.
Going for "6.1.0" instead of 7.0.0 because it only adds to the corresponding jsonschema 6.0.0 . And since Zenodo probably uses 6.0.0 now, we can't just edit that version.
There was a problem hiding this comment.
The mappings names need to align with the jsonschemas so we'll either need a new jsonschema or simply add it to existing v6.0.0. I just did a reindex (on the search cluster) of all of records/drafts zenodo today, so it's fairly ok - about 15min.
There was a problem hiding this comment.
👍 Switched back to using 6.0.0, so it will require a re-indexing of Zenodo, but the mapping version will align exactly to the jsonschema version now.
| from invenio_records.dumpers import SearchDumperExt | ||
|
|
||
|
|
||
| SPLITCHAR = "::" # explict better than implicit |
There was a problem hiding this comment.
Maybe a little bit weird, but I don't want to rely on defaults to make sure that both CombinedTermsFacet and CombinedSubjectsDumperExt use the same splitchar (which should really be called splitstr, but other Facets use splitchar, so I kept with it). It's better to be explicit.
| def deprecated_subject_nested(): | ||
| """Deprecated NestedTermsFacet. | ||
|
|
||
| Will warn until this is completely removed. |
There was a problem hiding this comment.
Yeah, not ideal, but if we go the "soft" transition route of letting subject_nested around, (so that re-indexing can be done on administrator's schedule), having a deprecation warning is a good way to alert the administrator. It's unfortunate that it will alert even after the switch is made, but there isn't really an alternative (unless there is no deprecation warning) until it's removed.
fenekku
force-pushed
the
798_faceting_fix_records_resources_integration
branch
from
83ccf45 to
ca25cfa
Compare
|
This PR was automatically marked as stale. |
fenekku
force-pushed
the
798_faceting_fix_records_resources_integration
branch
from
ca25cfa to
dceb433
Compare
fenekku
force-pushed
the
798_faceting_fix_records_resources_integration
branch
from
dceb433 to
eeda5fb
Compare
