Add max_children limit to nested sort

[erayarslan]

FEATURE BRANCH

• First SortMode added (including nested fields)

related with (#33592)

[elasticmachine]

Pinging @elastic/es-search-aggs

[jimczi]

Thanks @erayarslan , I left some comments regarding the scope of this new mode. It makes sense only when it is applied to select the first child document, not to select a value in a multi valued field since doc_values do not preserve the original order. Can you add a validation in the SortBuilder that checks if this mode is used in conjunction with a nested field ? I also wonder what value should be picked inside the first child document in case it contains multiple values. Should we have a second mode to pick the value ? Or should we accept this mode only if the nested document have a single value in the sort field ?
Also can you add a specific test in FieldSortIT that checks the behavior of this new mode ?

[erayarslan]

Thanks @jimczi , I implemented what you describe.

Currently FIRST mode only for nested fields. It does not include multi valued field.
Can you please take a look?

[jimczi]

Thanks @erayarslan , I wonder if we should decorrelate the MultiValueMode from the selection of the children. With this pr the first mode would select the first value inside the first children that matches. However as explained before first only makes sense for the selection of the children, not the values on each child since they are reordered internally. I wonder if we could add an option in the NestedSortBuilder instead and change the signature of MultiValueMode#pick to:

pick(SortedNumericDoubleValues values, double missingValue, DocIdSetIterator docItr, int startDoc, int endDoc, int maxChildren)

where maxChildren indicates the number of child that we should consider on each parent document. A value of 1 would select the first child document that matches.
The full sort would look like:

"sort" : [
       {
          "offer.price" : {
             "mode" :  "avg",
             "order" : "asc",
             "nested": {
                 "max_children": 1,
                "path": "offer",
                "filter": {
                   "term" : { "offer.color" : "blue" }
                }
             }
          }
       }
    ]

What do you think ?

[erayarslan]

Thank you for clarification @jimczi

Btw sorry for misunderstand. I was focused completely on the FIRST Enum SortMode.
This also works, i think.

So FIRST enum will be deleted and every nested MultiValueMode#pick method will get maxChildren argument? Am i right?

[jimczi]

So FIRST enum well be deleted and every nested MultiValueMode#pick method will get maxChildren argument? Am i right?

Yes if we agree that it's a better solution ;).

[erayarslan]

Hi @jimczi ,

I implemented max_children solution. Can you please take a look?

And I have some concerns.

• DocIdSetIterator iteration working reversed (i can't slice documents with max_children on iteration)
• I don't know how many documents are in DocIdSetIterator. (startDoc, endDoc arguments or DocIdSetIterator#cost() method cannot estimate when filter used)

Because of these;

I used two loop in new implementation.

• First loop for populating documents for slice documents with max_children value. (n times)
• Second loop for SortMode calculations after sliced documents. (max_children times)

Total: n + max_children times

I think FIRST Mode solution better for performance but this method more generic.

What do you think?

[jimczi]

Thanks @erayarslan . I like the max_children option better. I left more comments regarding the implementation.

[erayarslan]

Thanks for review @jimczi ,

I implemented last change request.
Please take a look.

[jimczi]

Thanks for making the changes. I left more comment but I think it's getting close. However I'd expect to see explicit test for this new feature. Can you add one in MultiValueModeTests and maybe one in FieldSortIT ? The tricky part is to document this new feature correctly, we need to come up with a description that is easy to understand and reason about. I'd also like to get some feedbacks from others because we changed the initial implementation to cope with the internals of nested document indexing. @jpountz what do you think of the current approach ? I wrongly assumed that we respect nested document order when indexing but I forgot that we reverse this ordering to ensure that parents always come last. See #33587 (comment) for the context of the discussion and the possible solution that we discussed.

[erayarslan]

I left tests to the last. After everything is finalized.
I think we agreed on this solution and it can solve our business problem too.

Thank you and can you please take a look to the last changes?

[jimczi]

Sorry for the late reply @erayarslan . We discussed internally with @jpountz and we think that we can preserve the order of the nested documents at index time in 6.x. Currently we reverse the order of all nested documents but we just need to put parents after their children so technically we could preserve the order of the children and only reorder the parent. If we implement this the max_children option could work on the initial ordering if we limit it to indices created after this change.
I'll try to implement something in the coming days and I'll ping you when it's ready to see how we can integrate it in the current pr.

[jimczi]

@erayarslan I merged #34225 so indices created in 6.5.0 will preserve the original order of nested documents. Can you merge with master and update this pr to reflect the new logic ? The idea is to throw an exception if max_children is used on indices created before 6.5.0. You should be able to test this in FieldSortBuilder#build where you can check the created version of the current shard with:

if (context.indexVersionCreated().before(Version.V_6_5_0)) {
....
}

[erayarslan]

I fixed integration test @jimczi
Can you trigger ci?

[jimczi]

@elasticmachine test this please

[jimczi]

I merged in master and 6x, thanks for all the iterations @erayarslan !

[erayarslan]

Thank you for all your help. @jimczi