Cleanup Terms query code

[thePanz]

No description provided.

[ruflin]

Do we need this?

[thePanz]

I would say: yes :)
It is an helper function to directly create a Terms lookup query, without first creating the Terms query, and then assign the lookup settings.

WDYT?

[ruflin]

I wonder if the constructor should then allow this? ;-)

But I'm fine going with it.

[thePanz]

I'd say that the constructor covers the "base" case, while a specific method can configure the "lookup" settings.
Having an helper doing it in one single go, helps in this direction :)

Saying it here as, if we agree, this could be the standard for other query classes too. wdyt?

[ruflin]

Isn't this the same as:

$terms = (new Terms($field))->setTermsLookup($index, $id, $path);

I did not test if the above works.

What I worry is that we add methods which do not necessarily simplify the code on the user end.

[thePanz]

Yes, the above is the same of Terms::buildTermsLookup($field, $index, $id, $path)

[ruflin]

Do you think it is still needed? Or could we put the above for example as a doc comment for the constructor?

[thePanz]

I agree, I removed the helper function.
I guess looking at the Terms methods it is clear how to use the setTermsLookup() :)

See: #1767

[mfn]

@thePanz @ruflin

Is string[] really the only correct type?

Wouldn't be string[]|int[] be equally correct (and all the other references in this class)?

For ElasticSearch, AFAIK, it doesn't matter if I send a number as string or an actual int, both equally work well.

I'm asking because in the process of the upgrade, using static analyzers, now complains passing integers to these methods but from my experience technically this isn't strictly required?

Thank you

[ruflin]

I haven't tested it but I think you are correct @mfn That both will work. I assume even a double could be passed in and just works? Could you test it?

[mfn]

So I did some testing with the terms query directly in the Kibana console and my findings are that string, int and float (i.e. the JavaScript equivalent) work equally well.

E.g. if I've field of type long and I use it in a simple query like this:

GET …/_search
{
    "query": {
      "terms": {
        "some_field": [
          1234
        ]
      }
   }
}

Whether I use "1234", 1234 or 1234.0 did not make any difference.

Note that the same applies to term query too, I tested this:

GET …/_search
{
    "query": {
      "term": {
        "some_field": {
          "value":  1234
        }
      }
   }
}

(Ok realized the Term query class does not use these specific type hints, so not affected by this.)

---

I think since fields can have the type double, yes, I guess float is also appropriate to include (though not sure how useful a term search for this would be compared to e.g. ranges, but who knows).

Do you think this is good enough warranted a phpdoc change here, i.e. to string[]|int[]|float[]? Annoyingly long btw :)

[ruflin]

@mfn Yes! Can you open a PR?

[mfn]

Yes! Continuing in #1872