Skip to content

LUCENE-10321: Tweak MultiRangeQuery interval tree creation logic#547

Merged
gsmiller merged 3 commits intoapache:mainfrom
gsmiller:LUCENE-10321-no-min-pullup
Dec 17, 2021
Merged

LUCENE-10321: Tweak MultiRangeQuery interval tree creation logic#547
gsmiller merged 3 commits intoapache:mainfrom
gsmiller:LUCENE-10321-no-min-pullup

Conversation

@gsmiller
Copy link
Contributor

Description

Small tweak that makes the interval tree creation logic in MultiRangeQuery consistent with ComponentTree.

Solution

Just remove the "roll up" of min values since internal node bounding boxes don't need consistent min values.

Tests

All existing tests continue to pass.

Checklist

Please review the following and check all that apply:

  • I have reviewed the guidelines for How to Contribute and my code conforms to the standards described there to the best of my ability.
  • I have created a Jira issue and added the issue ID to my pull request title.
  • I have given Lucene maintainers access to contribute to my PR branch. (optional but recommended)
  • I have developed this patch against the main branch.
  • I have run ./gradlew check.
  • I have added tests for my changes.

@gsmiller gsmiller requested a review from iverase December 16, 2021 15:37
Copy link
Contributor

@iverase iverase left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer if RangeTree does not implement Range any more as it does not follow the contract. What do you think?

hold on - I think I have another thing in mind :)

So I was thinking we can remove this part as well:

Then creating the tree should be pretty cheap.

@gsmiller
Copy link
Contributor Author

I would prefer if RangeTree does not implement Range any more as it does not follow the contract. What do you think?

+1 makes sense

So I was thinking we can remove this part as well:

Yeah, good catch!

I think my new revision addresses both of these suggestions. Thanks for the quick feedback!

Copy link
Contributor

@iverase iverase left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@gsmiller gsmiller merged commit 1e8b94a into apache:main Dec 17, 2021
@gsmiller gsmiller deleted the LUCENE-10321-no-min-pullup branch December 17, 2021 13:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants