Highlighting Support #93
Conversation
There was a problem hiding this comment.
Thanks for the contribution! The implementation approach looks good to me. Can you add a test and also add documentation for the new feature (https://github.com/ngageoint/elasticgeo/blob/master/gs-web-elasticsearch/doc/index.rst)?
|
Yeah I will put together a unit test and documentation. |
|
Can you rebase this on the latest master branch when you get a chance? |
dclemenzi
force-pushed
the
91-highlights
branch
from
8739481
to
01bdbdb
Compare
|
Sorry other priorities have come up and I have not gotten back to the unit tests that you request. Also I have been rethinking how the aggregation and highlighting were implemented. Not sure passing database proprietary JSON to the query as a hint is the best approach. Been thinking about adopting a custom function such as highlight(PropertyName) or aggregate(PropertyName, ...) so the interface will be common across multiple database implementations. The FilterToElastic would of course need to translate the function. Maybe there are other solutions already available in the geotool libraries, have not really looked. Thoughts? |
dclemenzi
force-pushed
the
91-highlights
branch
from
01bdbdb
to
7906bcd
Compare
|
Adding as a custom function sounds like an interesting idea but I haven't looked at it either. The use of viewparams was modeled after what's done in SolrDataStore. Let me know whether you want to continue with this PR or hold it while you explore the possible alternate implementation. |
|
We have used custom functions before to handle other edge cases so I know it would work. Just not sure if its the correct solution. I am sure others have run into the same issue using geotool APIs so I would not be surprised if there was not some approach already within geotools. When I get around to implementing something more generic (highlighting and aggregations) it would probably be best to continue supporting the hints (maybe as deprecated) to avoid breaking anyone depending on it. We are still using version 19.3 of geotools so we are not using the master branch so merging this has no impact on us. Its up to you whether you want to accept this implementation or wait for a more generic approach. Can't say when I will get back to this. |
|
Sounds good. Thanks for the rebase. |
#91 Implemented support for highlights