Fix incorrect handling of missing values in ToParentBlockJoinSortField (#15548) by gmarouli · Pull Request #15727 · apache/lucene

gmarouli · 2026-02-19T09:40:26Z

Description

Problem

ToParentBlockJoinSortField did not handle missing values for child documents. When a parent had a mix of children with a value and children without, the children without values were silently ignored from the min/max selection. This led to incorrect sort ordering -- the missing children should participate in the comparison using a configurable missing value, consistent with how the missing value in SortField works.

Additionally, the parent-level missingValue which was not configurable after #15483, is reinstated and configurable via the constructor.

Plan

Add support for missingValue configuration for the parent-level in ToParentBlockJoinSortField making it again configurable.
Rename the ambiguous reverse and order flags in the constructor of ToParentBlockJoinSortField.
Add support for missingValue configuration for the child-level in ToParentBlockJoinSortField and extend ToParentDocValues and BlockJoinSelector to detect children with missing values and incorporate the configurable missing value into the min/max aggregation, with proper handling for all supported types.

Fixes #15548

romseygeek

This looks great @gmarouli, thank you! I left some comments, but this all heading in the right direction.

lucene/join/src/java/org/apache/lucene/search/join/ToParentDocValues.java

romseygeek · 2026-02-20T09:31:15Z

lucene/join/src/java/org/apache/lucene/search/join/ToParentBlockJoinSortField.java

+  }
+
+  private static void validate(Type type, Object childMissingValue) {
+    switch (type) {


Nit: we can use switch expressions here (ie, case STRING -> validateMissingValue())

Done, I was a bit reluctant to add the comment // $CASES-OMITTED$ because I thought it might be recommended, but I did find other similar places in the code that it was used so it might be ok, right?

lucene/join/src/java/org/apache/lucene/search/join/ToParentDocValues.java

romseygeek · 2026-02-20T09:39:30Z

lucene/join/src/java/org/apache/lucene/search/join/ToParentDocValues.java

+    int prevParentDocID = parents.prevSetBit(docID - 1);
+    hasChildWithMissingValue = childrenWithValuesCount < getTotalChildrenCount(prevParentDocID);
+    if (hasChildWithMissingValue && missingWithChildWithoutValue) {
+      docID = nextDoc();


Can we avoid recursion here?

Yes, I used a loop with the seen flag in the condition. I changed the code, so seen is set to true only when we are certain that the parent doc id will be returned.

It used to be set as long as we found a child doc, so I added a dedicated local flag for that and that allowed us to use the seen flag in the new loop.

lucene/join/src/java/org/apache/lucene/search/join/ToParentDocValues.java

gmarouli · 2026-02-20T13:13:04Z

This looks great @gmarouli, thank you! I left some comments, but this all heading in the right direction.

Thank you for all the guidance @romseygeek . I followed up to the comments and I am open to further changes if you see fit.

romseygeek

Thanks @gmarouli! I left another comment.

romseygeek · 2026-02-20T15:06:41Z

lucene/join/src/java/org/apache/lucene/search/join/ToParentDocValues.java

-    int nextParentDocID = parents.nextSetBit(childWithValues.docID());
-    collector.reset();
-    seen = true;
+      int nextParentDocID = parents.nextSetBit(childWithValues.docID());


I think we need to check for NO_MORE_DOCS here now, given that it's possible that we iterate through parent docs until there are none left?

Worth adding a test that exercises this scenario.

Thank you for spotting it! It should have been in the loop. Fixed and test added, I added a new one dedicated to nextDoc because of its complexity, but if you think it would be better to add it to the existing one I can do it too.

romseygeek · 2026-02-23T09:37:59Z

lucene/join/src/java/org/apache/lucene/search/join/ToParentDocValues.java

+   * targetParentDocID}.
+   *
+   * <p>A parent document is considered to have a value when at least one of its child documents has
+   * a value. When {@code skipParentsWithMissingChildValues} is {@code true}, the parent is


Reading this again, I'm not sure it's quite what we want? We need to return a top-level missing value when a parent has no child documents with sort values, rather than when it has some missing ones.

We need to return a top-level missing value when a parent has no child documents with sort values, rather than when it has some missing ones.

I agree and this is what we do for the numeric doc values. But with binary doc values, we do not have a default value, we only have the flag, if missing values go first or last, so the missing value ends up missing in the parent too.

This why I approached it like this, if a parent has children with missing values and the children with a missing value is meant to "win" (this is why we say check if the sorting is reverse or not and if it's sortingMissingLast) then the parent is treated like it did not having children.

Am I missing some nuance here? (I wouldn't be surprised).

Oh, right I see what you mean. I think it still needs to return true from advanceExact() though? And then return an appropriate value from ord().

Ah, I see. What would be an appropriate value for ord(), my initial thought is -1 since any positive number could be confused with an actual ordinal, right?

TermOrdValComparator has this snippet:

if (sortMissingLast) { missingOrd = Integer.MAX_VALUE; } else { missingOrd = -1; }

I see, so we could use the same approach based on the child sortMissingLast. If parent and child have different reverse or sortMissingLast will be able to differentiate between a parent without children and a parent with a child with a missing value.

Thank you! This was very helpful.

romseygeek

Nearly there, I think! Sorry this is taking so long...

romseygeek · 2026-02-26T14:48:30Z

lucene/join/src/java/org/apache/lucene/search/join/ToParentDocValues.java

@@ -88,6 +97,9 @@ public boolean advanceExact(int targetParentDocID) throws IOException {

    @Override
    public int ordValue() {
+      if (iter.hasChildWithMissingValue() && (reverse == sortMissingLast)) {


I'm not sure this is correct, I think we need to check against the ord if there's a missing value? Like you do for NumDV

romseygeek · 2026-02-26T14:51:39Z

lucene/join/src/test/org/apache/lucene/search/join/TestBlockJoinSelector.java

+    ords[5] = 3;
+    ords[8] = 10;
+
+    final SortedDocValues naturalOrder =


Let's also add a test case for wrap(.... , true, true)

gmarouli added 4 commits February 18, 2026 13:15

Rename reversing sorting order in ToParentBlockJoinSortField

b269016

Add missingValues and tests

f3bba28

Add detection for children with missing values

e781ce7

Implement missingValue in ToParentBlockJoinSortField

c13d987

github-actions bot added the module:join label Feb 19, 2026

gmarouli marked this pull request as draft February 19, 2026 09:49

gmarouli added 2 commits February 19, 2026 12:05

Polishing

c7f27be

Add log in the CHANGES.txt

40593d0

github-actions bot added this to the 11.0.0 milestone Feb 19, 2026

Update CHANGES.txt with PR number

b03918f

gmarouli marked this pull request as ready for review February 19, 2026 11:07

romseygeek requested changes Feb 20, 2026

View reviewed changes

github-actions bot added module:core/index module:core/search module:test-framework module:build-infra labels Feb 20, 2026

gmarouli force-pushed the fix-bug-15548-incorrect-handling-of-missing-value branch from c3f4c8a to b03918f Compare February 20, 2026 12:42

github-actions bot removed module:core/index module:core/search module:test-framework module:build-infra labels Feb 20, 2026

gmarouli added 3 commits February 20, 2026 14:42

Use switch expression

b44c633

Apply review comments and fix incorrect use of seen

4d03431

Add // $CASES-OMITTED$

45325f6

gmarouli requested a review from romseygeek February 20, 2026 13:06

romseygeek requested changes Feb 20, 2026

View reviewed changes

Move NO_MORE_DOCS check in the loop

69a4309

gmarouli requested a review from romseygeek February 22, 2026 18:30

romseygeek reviewed Feb 23, 2026

View reviewed changes

Differentiate between parents with missing some or all kids

9cc92c7

romseygeek requested changes Feb 26, 2026

View reviewed changes

Conversation

gmarouli commented Feb 19, 2026

Description

Uh oh!

romseygeek left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

gmarouli commented Feb 20, 2026

Uh oh!

romseygeek left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

romseygeek left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants