Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Introduce native facet mappings #2071

Merged
merged 9 commits into from
Apr 30, 2021
Merged

Introduce native facet mappings #2071

merged 9 commits into from
Apr 30, 2021

Conversation

moraleida
Copy link
Contributor

@moraleida moraleida commented Feb 4, 2021 •
edited
Loading

Description of the Change

Fixes #2078

Introduces a new keyword field for mapping terms within posts. The new field holds the entire term object in json form and is accessible at terms.{$taxonomy}.facet

The idea is that instead of aggregating via terms.{$taxonomy}.slug and getting only a subset of the data, we can run aggregations using the new facet key and get all the information needed.

The stored post object contains terms in this format:

"terms" : {
            "category" : [
              {
                "parent" : 0,
                "name" : "Events",
                "term_taxonomy_id" : 30,
                "term_order" : 0,
                "term_id" : 30,
                "facet" : """{"term_id":30,"slug":"events","name":"Events","parent":0,"term_taxonomy_id":30,"term_order":0}""",
                "slug" : "events"
              },
              {
                "parent" : 30,
                "name" : "Films",
                "term_taxonomy_id" : 406,
                "term_order" : 0,
                "term_id" : 406,
                "facet" : """{"term_id":406,"slug":"films","name":"Films","parent":30,"term_taxonomy_id":406,"term_order":0}""",
                "slug" : "films"
              }
            ]
          }

Aggregations can then be requested using the facet field

      "aggs": {
        "category": {
          "terms": {
            "size": 10000,
            "field": "terms.category.facet"
          }
        }
      }

And return the entire JSON object:

  "aggregations" : {
    "terms" : {
      "doc_count" : 19,
      "category" : {
        "doc_count_error_upper_bound" : 0,
        "sum_other_doc_count" : 0,
        "buckets" : [
          {
            "key" : """{"term_id":30,"slug":"events","name":"Events","parent":0,"term_taxonomy_id":30,"term_order":0}""",
            "doc_count" : 1
          },
          {
            "key" : """{"term_id":406,"slug":"films","name":"Films","parent":30,"term_taxonomy_id":406,"term_order":0}""",
            "doc_count" : 1
          }
        ]
      }
    }
  },

Alternate Designs

Benefits

Possible Drawbacks

Verification Process

Checklist:

  • I have read the CONTRIBUTING document.
  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my change.
  • All new and existing tests passed.

Applicable Issues

Changelog Entry

@moraleida moraleida mentioned this pull request Feb 11, 2021
Closed
@moraleida moraleida changed the title POC: introduce native facet mappings Introduce native facet mappings Feb 15, 2021
@moraleida moraleida marked this pull request as ready for review February 17, 2021 20:27
@felipeelia felipeelia self-requested a review March 5, 2021 15:31
felipeelia
felipeelia previously approved these changes Mar 5, 2021
View reviewed changes
@felipeelia
Copy link
Member

I've tested this and it works fine. Two things we'll need to do after we merge:

  1. Add a @since to the new filter
  2. Add the new version as needs reindex or explain to the users that they might need to reindex before using the new filter.

moraleida
moraleida commented Mar 9, 2021
View reviewed changes
includes/classes/Indexable/Post/Post.php
@@ -429,6 +429,9 @@ private function prepare_terms( $post ) {
'term_taxonomy_id' => $term->term_taxonomy_id,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@felipeelia Can you check if there a reason why we only collect the term_taxonomy_id here and not when getting parent terms in line 462 below? I have checked annotations and this difference has been around since tt_ids were introduced in #840

I've now bumped into an issue in hierarchical taxonomies in which the new native facet is stored twice because of this. It's stored once with term_taxonomy_id (created here) and once without it (created in get_parent_terms() below, whenever a child term is stored).

Adding term_taxonomy_id to parent terms fixes the issue, but I wanted to check if there is any reason why it didn't have that in the first place.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it was just something we missed until now, @moraleida.

@brandwaffle can we aim to have this merged in 3.6.0?

@felipeelia felipeelia added this to the 3.6.0 milestone Mar 19, 2021
@Rahmon Rahmon requested a review from felipeelia March 22, 2021 18:18
@Rahmon Rahmon assigned Rahmon and felipeelia and unassigned Rahmon Apr 22, 2021
@brandwaffle brandwaffle merged commit cdd5362 into 10up:develop Apr 30, 2021
@adiloztaser adiloztaser mentioned this pull request Jul 12, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Rahmon Rahmon Rahmon approved these changes

@felipeelia felipeelia Awaiting requested review from felipeelia

Assignees

@felipeelia felipeelia

Labels
None yet
Projects
None yet
Milestone
3.6.0
Development

Successfully merging this pull request may close these issues.

Retrieve complete term information when using aggregations
4 participants
@moraleida @felipeelia @Rahmon @brandwaffle