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.

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

Aggregation: Ability to add metadata #1677

Open
leflings opened this issue Oct 17, 2019 · 6 comments

Comments

@leflings
Copy link
Contributor

@leflings leflings commented Oct 17, 2019

Hi

First of all, thank you for a very nice library, which I've enjoyed using (with/through FOSElasticaBundle)

ElasticSearch has the ability to add metadata to aggregations, a feature I've found to be wanting while making a faceted search solution.

Is it possible to emulate this with the current API (I'm using version 6.1.1)?

If not, could this be added to the Aggregation API? Hopefully in a way where it lands in a 6.X version (the bundle doesn't yet support versions 7.X)

@ruflin

This comment has been minimized.

Copy link
Owner

@ruflin ruflin commented Oct 17, 2019

Just learned a new feature in ES, thanks! My guess would be that setParam could be used on each aggregation to add it. I didn't try it out but as all Aggregations inherit Params, this should work: https://github.com/ruflin/Elastica/blob/master/lib/Elastica/Aggregation/AbstractAggregation.php#L9

Happy to get native support for it, interested to contribute 😇

@leflings

This comment has been minimized.

Copy link
Contributor Author

@leflings leflings commented Oct 18, 2019

I tried setParam but it unfortunately doesn't generate a valid aggregation.

The following code:

$termsAgg = new \Elastica\Aggregation\Terms('titles');
$termsAgg->setField('title');
$termsAgg->setParam('meta', ['color' => 'blue']);

$query = new \Elastica\Query();
$query->addAggregation($termsAgg);

Generates the following query:

{
    "aggs": {
        "titles": {
            "terms": {
                "field": "title",
                "meta": {
                    "color": "blue"
                }
            }
        }
    },
    "query": {
        "match_all": {}
    }
}

Which generates the following response:

{
  "error": {
    "root_cause": [
      {
        "type": "x_content_parse_exception",
        "reason": "[6:17] [terms] unknown field [meta], parser not found"
      }
    ],
    "type": "x_content_parse_exception",
    "reason": "[6:17] [terms] unknown field [meta], parser not found"
  },
  "status": 400
}

The meta object needs to be output "one level higher" / at the same level as the aggregation type. See Structuring Aggregations.

I've locally implemented setMeta(array) and clearMeta() on AbstractAggregation and handled the new field in toArray(). It works just fine. I'll see about making a proper pull request later in the day.

leflings added a commit to leflings/Elastica that referenced this issue Oct 18, 2019
ElasticSearch allows metadata to be set on an aggregation that will be
returned as-is with the aggregation result.

See https://www.elastic.co/guide/en/elasticsearch/reference/6.8/agg-metadata.html

Using `setParam` generates invalid query, hence these additions. See
ruflin#1677
leflings added a commit to leflings/Elastica that referenced this issue Oct 18, 2019
ElasticSearch allows metadata to be set on an aggregation that will be
returned as-is with the aggregation result.

See https://www.elastic.co/guide/en/elasticsearch/reference/6.8/agg-metadata.html

Using `setParam` generates invalid query, hence these additions. See
ruflin#1677
@leflings

This comment has been minimized.

Copy link
Contributor Author

@leflings leflings commented Oct 18, 2019

I've made a pull request (#1678) towards the 6.x branch. If this is the wrong way around and you would rather have a request towards the master branch, that can then be backported, say so. I guess my thinking was that this commit could be "forwardported" :)

Disclaimer: I could not get the docker environment running properly for the tests to complete (neither when trying the same commit rebased to the master branch). It kept waiting on the ElasticSearch server to be ready, which it never became. The lint and check-style steps worked. I've run both tests manually elsewhere, so I'm pretty certain the would pass in a working environment.

@ruflin

This comment has been minimized.

Copy link
Owner

@ruflin ruflin commented Oct 21, 2019

@leflings Thank you! Yes, I would prefer to have it in master first and backport it from there if possible.

leflings added a commit to leflings/Elastica that referenced this issue Oct 21, 2019
ElasticSearch allows metadata to be set on an aggregation that will be
returned as-is with the aggregation result.

See https://www.elastic.co/guide/en/elasticsearch/reference/6.8/agg-metadata.html

Using `setParam` generates invalid query, hence these additions. See
ruflin#1677
@leflings

This comment has been minimized.

Copy link
Contributor Author

@leflings leflings commented Oct 21, 2019

@ruflin Opened a new pull request (#1680) based on master and using existing _rawParams

leflings added a commit to leflings/Elastica that referenced this issue Oct 21, 2019
ElasticSearch allows metadata to be set on an aggregation that will be
returned as-is with the aggregation result.

See https://www.elastic.co/guide/en/elasticsearch/reference/6.8/agg-metadata.html

Using `setParam` generates invalid query, hence these additions. See
ruflin#1677
ruflin added a commit that referenced this issue Oct 22, 2019
ElasticSearch allows metadata to be set on an aggregation that will be
returned as-is with the aggregation result.

See https://www.elastic.co/guide/en/elasticsearch/reference/6.8/agg-metadata.html

Using `setParam` generates invalid query, hence these additions. See
#1677
@ruflin

This comment has been minimized.

Copy link
Owner

@ruflin ruflin commented Oct 22, 2019

Great, thanks. Already merged. Do you want to open the backport PR from there?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.