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

[Search] Extend query #19

Open
c3dr0x opened this issue Jun 22, 2018 · 7 comments
Open

[Search] Extend query #19

c3dr0x opened this issue Jun 22, 2018 · 7 comments
Assignees

Comments

@c3dr0x
Copy link
Member

c3dr0x commented Jun 22, 2018

When calling AdvancedQuery method, we shall be able a give an optional parameter giving access to the NEST object to extend query as needed without having to build everything from scratch

@durandx
Copy link
Contributor

durandx commented Jun 27, 2018

As in the previous issue, what is your Use Case ? The goal of Kinetix.Search is to provide an high level abstraction for real case queries. If you need low level query object, maybe we should add a specific use case for search instead of opening the API.

@c3dr0x
Copy link
Member Author

c3dr0x commented Jun 27, 2018

As explained in Thomas' mail:

We need to do some specific querying and computation not available with the way the api work today. For example, compute distances.
Exposing in a callback the Nest object would allow us to do those specific case without having to redo all logic that is already present in the AdvancedSearch method. If you look at issue #20 the fact that a TextFilter is not present could be done with this. Not a production fix for that specific issue I agree, just an example.

But a more low level wrapping offered by Kinetix around Nest object using the Documents and there annotations can still be an improvement. Need to think about it.

@durandx
Copy link
Contributor

durandx commented Jun 27, 2018

Ok, so what was your idea about exposing a Nest object in a callback :

  • exposing directly the client ?
  • exposing the search selector function ?
  • exposing the sort, query, postfilter or agreagation callback ?

Moreover, I prefer to expose a dedicated Method with callback (like a CustomAdvancedQuery) whereas exposing directly an optional parameter that could be wrongly overloaded by a developper :
The goal of the AdvancedQuery method is to stick with the common "Focus Advanced Search" pattern that should be the 95% use case scenario.
This doesn't prevent to mutualize code between the 2 methods.

@durandx durandx self-assigned this Jun 28, 2018
@durandx
Copy link
Contributor

durandx commented Jun 29, 2018

Can you list all the usage you will need in the Search Api ? What is your needs about the distances computation ?

@c3dr0x
Copy link
Member Author

c3dr0x commented Jun 29, 2018

I think exposing the client is great, it will allow us to fallback more easily to it if necessary.

I agree with that exposing a callback in a custom method is better.

My document will have coordinates information about its geographic position. I need to find all documents that are in a range of X km of a search position.

Something like that as filter :

m => m.GeoShapeMultiPolygon(g => 
    g.Field(fi => fi.Polygone)
        .Relation(GeoShapeRelation.Intersects)
        .Coordinates(zones.Geometries.Cast<Polygon>().Select(
            zone =>new[] { zone.ExteriorRing.Coordinates }
                .Concat(zone.InteriorRings.Select(ring => ring.Coordinates)))
                .Select(c => c.Select(d => d.Select(e => new GeoCoordinate(e.Y, e.X)))))),

So if I can access the Nest object I'll be able to add that custom filter.

@durandx
Copy link
Contributor

durandx commented Jul 5, 2018

As discussed, we need to support GeoQueries in Kinetix.Search (and in Vertigo)
We need to co-design this point.
So we need GeoPoint indexing with Radius Search ?

@JabX
Copy link
Contributor

JabX commented Jul 5, 2018

I'm sorry but I fail to see the point of trying to abstract over such a complex area of queries (and indexing) when ElasticSearch (with Nest) already offers everything we might need in a relatively convenient way, provided the client is exposed.

Nest is the defacto standard, we already use it and there are plenty of examples in the docs and everywhere else on the internet about that.

We don't have a lot of manpower to spend on this and the closer we stay to what already exists, works and is widely used, the more we'll be able to work on high-added-value areas. As it stands today, and I'm saying that after having spent quite some time using and improving it, I think that the Search module is way too opinionated and abstracts away a lot of stuff that should at least been opened for extension. Sure, it does the job of providing the advanced search API for Focus applications very well, but it should be allowed to do more than that because projects can have all sorts of other valid uses for ElasticSearch.

I don't think trying to come up with all the solutions before any problem arises is a good use of anyone's time, instead we should be focusing on what projects need and try to find a solution for that together if it hasn't been done before, and see if it is relevant to include in the shared framework or if it's not worth it.

To go back to the original issue of GeoQueries, I don't think we have anything specific to offer about that because the scenarios aren't really predictable like the advanced search is. But there are multiple roadblocks currently preventing projects to use them, which are:

  • No way to define mappings for types that aren't supported by the Search module (only strings, ints, decimals and some scenarios for dates are supported)
  • No way to define custom Json serializers to support types that Nest can't translate on its own (like GIS data types for instance)
  • No way to access the Nest client to write custom queries that can leverage all these advanced types

I have a working prototype of this (along with ES6 support) in the Core version of Kinetix.Search that I use in my project that I'd be more than happy and willing to share, to use as a starting point for further improvements.

EDIT:

I'm not sure if the original discussion was about simply exposing the client or providing a way of customizing the AdvancedQuery with Nest constructs to add custom filters, facets and groupings. I was only speaking about the former, the latter will probably require a significant rewrite of the query builder to accommodate for it. Filtering only might be doable pretty easily thought, but I don't think there is that much value here, the "killer" feature being faceting. They leak everywhere from aggregations to grouping to filters and post filters (to handle multi-selection where applicable), and even if we already have a IFacetHandler interface, the existing implementations are still too complex to be replicated (and they are only covering the most basic cases!) and parts of the logic isn't even there.

If we were to tackle this issue, the answer can't just be about supporting @c3dr0x's specific need because you can be damn sure than someone else will come in a few months with a similar request but not quite, so that the existing API won't cover its problem and we'll have to start over again, making the query builder bigger and bigger until it becomes a giant pile of spaghetti code that no one will ever be able to untangle.

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

No branches or pull requests

3 participants