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

WIP Nested type aggregations #4645

Closed
wants to merge 4 commits into from

Conversation

Filirom1
Copy link
Contributor

This is a work in progress, I am trying to implement a solution for #1084.

@elasticsearch-release
Copy link

Jenkins standing by to test this. If you aren't a maintainer, you can ignore this comment. Someone with commit access, please review this and clear it for Jenkins to run; then say 'jenkins, test it'.

@Filirom1
Copy link
Contributor Author

Right now, nested queries are sent correctly but not displayed.

nested-aggs

Could you help me please

@rashidkpc
Copy link
Contributor

Is implementing this as an entirely separate aggregation the best way to go about this? It makes it look like you're splitting the rows twice. Seems like it makes more sense to add a "Nested" checkbox to the Advanced setting of any aggregation, which when selected gives you path input.

@Filirom1
Copy link
Contributor Author

The best way, i am not so sure.
The easiest way for me, yes 😋

Kidding asside, the ElasticSearch query corresponding to this Kibana example is composed of 3 aggregations. This is the reason why I do the implementation this way. Each aggregation is mapped to a Kibana row.

Here is the ElasticSearch query I wanted to play with Kibana:

   {
     "query":{
       "nested":{
         "path":"Product",
         "query":{
           "match":{
             "Product.Name":"httpd"
           }
         }
       }
     },
     "aggs":{
       "product":{
         "nested":{
           "path":"Product"
         },
         "aggs":{
           "httpd":{
             "filter":{
               "term":{
                 "Product.Name":"httpd"
               }
             },
             "aggs":{
               "versions":{
                 "terms":{
                   "field":"Product.Version.raw",
                   "size":5
                 }
               }
             }
           }
         }
       }
     }
   }

@rashidkpc
Copy link
Contributor

Sure, but when we actually render the chart/table/whatever, it wouldn't, and shouldn't, have an extra row fore the nested agg.

@Filirom1
Copy link
Contributor Author

nested-aggs

I had something working with tables (d4e97bb), but hierarchical data will be more complex to implement.

There is references to vis.aggs.bySchemaGroup.buckets that in my case has a nested bucket.

But there is also references to resp.aggregations[firstAgg.id] (https://github.com/elastic/kibana/blob/master/src/ui/public/agg_response/hierarchical/build_hierarchical_data.js#L53) that in all cases will point to the nested aggregation.

From this point of view, I think that having a nested bucket will help.

@Filirom1
Copy link
Contributor Author

And here we are with hierarchical data:
nested-aggs

@ppadovani
Copy link

I implemented this same fix, I believe, several weeks ago. However, I do not believe this is the correct approach as it uses up one of the aggregations in the stack of available aggregations and can lead to incorrect display results.

I am working on just adding a new text field to always show up on an aggregation type, that optionally allows the user to enter the nested path. If they have set a nested path it will inject the nested aggregation into the aggregation chain, and strip it out on the response.

I'll post something back when/if I get it working.

@Filirom1
Copy link
Contributor Author

If you get something working, I would be interested to see the implementation, in particular the response transformation.

@Filirom1
Copy link
Contributor Author

I have just added the reverse_nested bucket, and array columns are going crazy. I was so close...

nested-aggs

@ppadovani
Copy link

My team and I are working on this and hope to maybe have something to show by the end of the week.

@Filirom1
Copy link
Contributor Author

Great, I am so eager to see it

@ppadovani
Copy link

We have it partially working... we still have some loose ends to wrap up... Here is a screen shot... I will post here with a patch as soon as we have it done. This patch will also include being able to enter a custom elastic json for the query. @SpaceManiac gets most of the credit for the work done. Edit: We will be working to generate a pull request, and I'll post a link to that pull request as soon as it is available.
screen shot 2015-08-20 at 2 09 54 pm

@Filirom1
Copy link
Contributor Author

Seems really great !

@louisdw
Copy link

louisdw commented Aug 21, 2015

This looks great! Can't wait to try it out.

On 21 August 2015 at 08:49, Romain notifications@github.com wrote:

Seems really great !


Reply to this email directly or view it on GitHub
#4645 (comment).

Louis de Wet
Principal Engineer
Relentless Software
+44 1273 727200

Registered in England and Wales
One Air Street, Brighton, BN1 3FB
Company No. 4941247

@siedwards
Copy link

Great work guys!

@ppadovani
Copy link

We have it completed and are waiting on our legal to give the go ahead to generate a pull request. Once again I want to praise @SpaceManiac for all of his hard work on this. To give everyone a taste here is an additional screen shot of test data.. each visualization uses nested and/or reverse nested aggregations. As soon as I have approval, I will generate a pull request and link to it here.

screen shot 2015-08-21 at 11 22 28 am

@ppadovani
Copy link

Done and a pull request created here:
#4806

@rashidkpc
Copy link
Contributor

Closing in favor of #4806

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants