Re: [hibernate-dev] [Search] ElasticSearch - Progress on faceting

Hi Guillaume, Awesome, great to see you making progress here! I need to take a closer look, but some quick comments inline. @Sanne, this reminds me of that pending PR of mine; Any chance you spend some cycles reviewing and merging it? Guillaume's work is based on it and it'll also serve as base of my own future work. Thanks! 2016-02-04 16:37 GMT+01:00 Guillaume Smet <guillaume.smet(a)gmail.com&gt;: It's a great example of how subjective readability is, because I don't find it necessarily better than the previous version, it's just different :) An actual disadvantage is the higher number of object allocations due to the builders. In the end I don't really care about it that much; how the JSON is created is more a technical detail which we may change at any time. So if you feel strongly about it, feel free to do it. But I would not expose this builder and the usage of GSON through the public API so we actually have the freedom of future changes. +1 very good that you avoided some forth-and-back cycles, that has been bugging me, too. Yes, keep in mind that the route through creating a Lucene Document object is just temporary hack to have a PoC of integrating ES. For HS 6 we'll look into refactoring the required pieces and avoid that intermediary step. Not sure how much sense it makes to spend cycles on this right now, it'll be larger task for sure. That's cool. Actually I had prepared that very thing locally, too, but your impl is even nicer :) I wouldn't use arrays for single values, the upgrade from single value to array if needed seems nicer. Btw. that's one of the cases where going through the Lucene document is limiting us. When having element collections of component types, we cannot assemble the JSON array in such way that's ensured that each array element represents one "row" from the collection. This is because we don't know which of the Lucene fields belong together. That'll change with the larger re-work in 6. Also the case of root.components[0].foo, root.components[0].bar, root.components[1].foo, root.components[1].bar where the array would have to be at the level of "components" and not at the leave level. Couldn't you do the conversion in the ElasticSearchIndexWorkVisitor?