fullscale / elastic.js

A JavaScript implementation of the elasticsearch Query DSL
http://docs.fullscale.co/elasticjs/
MIT License
654 stars 163 forks source link

Begin implementation of TopHitsAggregation #68

Closed Sean-Der closed 10 years ago

Sean-Der commented 10 years ago

As mentioned in issue #64 elastic.js currently doesn't have TopHitsAggregation

This doesn't have any tests, but I will add them in the next day or two. I would like to hear if this is the follow proper patterns, and if not what I should fix up before I write tests that depend on this code.

thanks!

ps: @babeya would you mind checking this out and see if it is as you expect?

mattweber commented 10 years ago

This looks great. I did have one comment and I would like to add the remaining options and tests before pulling this in. If you have time to do this you can find all the options here:

https://github.com/elasticsearch/elasticsearch/blob/master/src/main/java/org/elasticsearch/search/aggregations/metrics/tophits/TopHitsParser.java

Thanks!

Sean-Der commented 10 years ago

Hi @mattweber

Thanks for checking this out! I will fix the mixin, add tests and add the remaining options in the next couple of days.

Thanks for looking at it, and elastic.js is awesome really enjoy using it.

ErwanPigneul commented 10 years ago

Hello, I just test it and it's working ! We use this nice library for this project : https://github.com/pagesjaunes/curiosity

Thanks

ErwanPigneul commented 10 years ago

This aggregation is particular since it support per hit feature. I will perhaps try to enrich the agg.

Sean-Der commented 10 years ago

@ErwanPigneul

Hey! Sorry I didn't get to this earlier, I am going to be working on this tonight. Hopefully I will be able to get it all knocked out and and ready for merging tonight.

Have you made any progress? Don't want to duplicate work if not needed.

thanks!

ErwanPigneul commented 10 years ago

Hi Sean

no not yet :)

Sean-Der commented 10 years ago

@ErwanPigneul This should be good to go! If you want to use this in your code base now, the API shouldn't change

@mattweber The last handful of functions should be in a mixin, but because of the differing struct of query/aggregations I didn't see a way to do it that follows an existing pattern in the code base (didn't look super close pretty late) Would you mind looking it over, and if I need to move stuff into a mixin just tell me.

Also, this should have full test coverage for the functions defined by TopHitsAggregation

thanks!

ErwanPigneul commented 10 years ago

Awesome, thanks a lot @Sean-Der !

Sean-Der commented 10 years ago

Hey @mattweber anything I can do to improve this?

thanks

mattweber commented 10 years ago

Thanks @Sean-Der! I have not had a chance to review this yet, hopefully I will be able to get to it this week. I will keep you updated. Thanks!

mattweber commented 10 years ago

PR looks good. I am going to pull it into a local branch run some tests then commit in the next day or so. Thanks again!

arpu commented 10 years ago

hello any idea how i can implemt this with elastic.js ?

curl -XGET "http://localhost:9200/myjdbc/mytype/_search?search_type=count" -d' { "query": { "match": { "_all" : "house" } }, "aggs": { "title": { "terms": { "field": "title.raw" }, "aggs": { "top_tags_hits": { "top_hits": { } }, "top_hit": { "max": { "lang" : "expression", "script": "_score" } } } } } }'

Sean-Der commented 10 years ago

Hi @arpu

This is not meant to be a place to ask for support, it makes it difficult for others when you go off topic. However, I have implemented your query below.

var request = ejs.Request().query(ejs.MatchQuery('_all', 'house'));                                                                                                                                                                            
request = request.agg(ejs.TermsAggregation('title').                                                                                                                                                                                           
                        field('title.raw').                                                                                                                                                                                                    
                        agg(ejs.TopHitsAggregation('top_tags_hits')).                                                                                                                                                                          
                        agg(ejs.MaxAggregation('top_hit').lang('expression').script('_score')));  
{  
   "query":{  
      "match":{  
         "_all":{  
            "query":"house"
         }
      }
   },
   "aggs":{  
      "title":{  
         "terms":{  
            "field":"title.raw"
         },
         "aggs":{  
            "top_tags_hits":{  },
            "top_hit":  {
               "max":{  
                  "lang":"expression",
                  "script":"_score"
               }
         }
      }
   }
}
arpu commented 10 years ago

hello @Sean-Der

sorry for asking this in the pull request, where is the recommended place to ask user questions?

thx a lot for implemend the query!

best regards

mattweber commented 10 years ago

Looks great, merged! Thanks!