ElasticsearchQueryBuilder - support for Objects

[synweap15]

Hello!

As mentioned in #15, I'd like to propose a solution for handling elasticsearch-dsl's Object fields (or ES objects in general). During the flattening of nested objects in Elasticsearch, fields in form of:

    country = Object(properties={
        "id": Integer(),
        "name": Text()
    })

are transformed into a flat structure, as can be seen here: link

Due to the fact, that here:

luqum/luqum/elasticsearch/visitor.py

Lines 211 to 213 in a8002c2

	def _is_nested(self, node):
	if isinstance(node, SearchField) and '.' in node.name:
	return True

is the '.' in node.name check present, parsed queries in form of country.name:Italy will be transformed to a nested query, which will cause hiccups in Elasticsearch.

Now, the country.name seems to be parsed correctly by YACC as dot isn't a special char. It's the check in the mentioned file causes the problem for me here.

What have I done - I've subclassed ElasticsearchQueryBuilder as follows:

class ElasticsearchDotAwareQueryBuilder(ElasticsearchQueryBuilder):
    def _is_nested(self, node):
        for child in node.children:
            if isinstance(child, SearchField):
                return True
            elif self._is_nested(child):
                return True

        return False

And now I have both : available for nested fields, as well as . for flat objects.

I didn't have enough time to dive deeply but I think it shouldn't break other functionalities. My question is - why was the check for dot presence there in the first place - is it just an alternative to :, just not introduced on parsing level but later in the query building process, or am I missing something?

[synweap15]

Okay, now I see that some other modifications are needed to make this work for Nested within Object :)

[alexgarel]

I think it would be to support . as well as : both for nested fields as well as for (sub) Object. The philosophy should be that you explicitly design nested fields thanks to the nested_fields, the rest should be considered object fields.

In this I agree with you that the current _is_nested method is wrong.

That said, I don't know when I will be able to look at it (maybe next week?) but I'd welcome a PR.

[synweap15]

Okay, so basically . was introduced just to be an alternative to : without any special additional, special meaning?

For my dirty approach to work, I also had to alter the _create_nested method, so finally the class looks like this:

class ElasticsearchDotAwareQueryBuilder(ElasticsearchQueryBuilder):
    def _is_nested(self, node):
        for child in node.children:
            if isinstance(child, SearchField):
                return True
            elif self._is_nested(child):
                return True

        return False
    
    def _create_nested(self, node_name, items):
        return self.es_item_factory.build(
            ENested, nested_path=node_name, items=items)

Another thing is that in the nested fields definitions have to be altered to have . in them. Apart from that, I think everything works.

I should find time to submit a PR, just trying to make sure my dirty approach isn't breaking anything important.

[alexgarel]

Hello @synweap15, commit 8738b65 should bring the functionality.

I think I will release a new version after adding a utility to deduce the query builder parameters from the elasticsearch schema (because it's starting to be complex).

It was a big rework in order to be able to manage object field in nested field and any combinations thereof. (It required me some interesting testing on the do and don't of elasticsearch on such scenarii)

Hope you can test it and that you'll enjoy it :-) I let you then close the ticket if it seems ok to you.

[synweap15]

Hey @alexgarel!

I was a little busy with other things lately, have been using the hack I've shown above with success. Going to check your solution next week and report if any problems popped up.

Thanks!

[alexgarel]

@synweap15, time passing, I will close this issue. Feels free to re-open it if you're not happy with the current solution!

[synweap15]

It's still on my todo, but other things keep popping up - will look when I have a chance. Thank you!