Skip to content

Use Arrays.compareUnsigned in IDVersionSegmentTermsEnum and OrdsSegmentTermsEnum. #13782

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
vsop-479 wants to merge 8 commits into
base: main
Choose a base branch
from
Open

Use Arrays.compareUnsigned in IDVersionSegmentTermsEnum and OrdsSegmentTermsEnum. #13782

vsop-479 wants to merge 8 commits into from
+246 −395

Conversation

@vsop-479
Copy link
Contributor

@vsop-479 vsop-479 commented Sep 13, 2024

Description

Description
This change similars to #13252.

@vsop-479
Copy link
Contributor Author

vsop-479 commented Sep 13, 2024

@uschindler
Please take a look when you get a chance.

jpountz
jpountz approved these changes Sep 13, 2024
View reviewed changes
@vsop-479
Copy link
Contributor Author

vsop-479 commented Sep 13, 2024

@jpountz
I think this check failure relats to 5045d3c .

@jpountz
Copy link
Contributor

jpountz commented Sep 13, 2024

Indeed it likely does, can you merge main back into your branch?

uschindler
uschindler approved these changes Sep 13, 2024
View reviewed changes
Copy link
Contributor

@uschindler uschindler left a comment

Choose a reason for hiding this comment

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

Hi,
We had others like this in former PRs (e.g., #13252). As this old code looks like copy pasted over and over, can we find other occurrences using some regex searches?

@uschindler
Copy link
Contributor

uschindler commented Sep 13, 2024

Backport or not?

@jpountz
Copy link
Contributor

jpountz commented Sep 13, 2024

I would backport as it looks pretty safe, though I doubt we have anyone using this postings format, so it likely doesn't matter much in practice.

@vsop-479
Copy link
Contributor Author

vsop-479 commented Sep 13, 2024 •
edited
Loading

can you merge main back into your branch?

Merged.

can we find other occurrences using some regex searches?

I searched code with final int targetUptoMid = targetUpto which these iterating compare start with.
There are only two places remains in org.apache.lucene.backward_codecs.lucene40.blocktree.SegmentTermsEnum now, since this is a bakward class, I am not sure we should modify it.

@uschindler
Copy link
Contributor

uschindler commented Sep 13, 2024

I would backport as it looks pretty safe, though I doubt we have anyone using this postings format, so it likely doesn't matter much in practice.

The we should move the changes entry. Do we have a corresponding 9.x section already?

@vsop-479
Copy link
Contributor Author

vsop-479 commented Sep 13, 2024

The we should move the changes entry. Do we have a corresponding 9.x section already?

We already have a similar change entry for #13252 under 9.11.0, so we should move this change entry?

@uschindler
Copy link
Contributor

uschindler commented Sep 13, 2024

The we should move the changes entry. Do we have a corresponding 9.x section already?

We already have a similar change entry for #13252 under 9.11.0, so we should move this change entry?

Yes, let's merge them an list both PRs.

@vsop-479
Copy link
Contributor Author

vsop-479 commented Sep 13, 2024

Ok, I will do it.

@vsop-479
Copy link
Contributor Author

vsop-479 commented Sep 14, 2024

let's merge them an list both PRs.

I merged them into one entry.

@vsop-479
Copy link
Contributor Author

vsop-479 commented Sep 14, 2024 •
edited
Loading

can we find other occurrences using some regex searches?

Hmm, I also find some suffix's loop comparing in IDVersionSegmentTermsEnumFrame and OrdsSegmentTermsEnumFrame, similar to SegmentTermsEnumFrame, these code maybe could replaced by Arrays#compareUnsigned too.

But, before that I find we attempt to load sub block when scanning a leaf block in IDVersionSegmentTermsEnumFrame#scanToTermLeaf: #13786.

@vsop-479
Copy link
Contributor Author

vsop-479 commented Sep 18, 2024

can we find other occurrences using some regex searches?

I replaced loop compare suffixe with Arrays#compareUnsigned in IDVersionSegmentTermsEnumFrame and OrdsSegmentTermsEnumFrame.

@vsop-479
Copy link
Contributor Author

vsop-479 commented Sep 23, 2024

I replaced loop compare suffixe with Arrays#compareUnsigned in IDVersionSegmentTermsEnumFrame and OrdsSegmentTermsEnumFrame.

@jpountz It seems you implemented the same thing in SegmentTermsEnumFrame. Please take a look when you get a chance.

@github-actions
Copy link
Contributor

github-actions bot commented Oct 8, 2024

This PR has not had activity in the past 2 weeks, labeling it as stale. If the PR is waiting for review, notify the [email protected] list. Thank you for your contribution!

@github-actions github-actions bot added the Stale label Oct 8, 2024
@vsop-479 vsop-479 requested a review from jpountz October 10, 2024 02:35
@github-actions github-actions bot removed the Stale label Oct 11, 2024
@github-actions
Copy link
Contributor

github-actions bot commented Oct 25, 2024

This PR has not had activity in the past 2 weeks, labeling it as stale. If the PR is waiting for review, notify the [email protected] list. Thank you for your contribution!

@github-actions github-actions bot added the Stale label Oct 25, 2024
@vsop-479
Copy link
Contributor Author

vsop-479 commented Nov 13, 2024

Hello @jpountz , Please take a look when you get a chance.

@github-actions github-actions bot removed the Stale label Nov 14, 2024
@github-actions
Copy link
Contributor

github-actions bot commented Nov 28, 2024

This PR has not had activity in the past 2 weeks, labeling it as stale. If the PR is waiting for review, notify the [email protected] list. Thank you for your contribution!

@github-actions github-actions bot added the Stale label Nov 28, 2024
@vsop-479 vsop-479 requested a review from uschindler March 11, 2025 07:28
@github-actions github-actions bot removed the Stale label Mar 12, 2025
@github-actions
Copy link
Contributor

github-actions bot commented Mar 26, 2025

This PR has not had activity in the past 2 weeks, labeling it as stale. If the PR is waiting for review, notify the [email protected] list. Thank you for your contribution!

@github-actions github-actions bot added the Stale label Mar 26, 2025
@github-actions github-actions bot removed the Stale label Oct 1, 2025
@github-actions
Copy link
Contributor

github-actions bot commented Oct 16, 2025

This PR has not had activity in the past 2 weeks, labeling it as stale. If the PR is waiting for review, notify the [email protected] list. Thank you for your contribution!

@github-actions github-actions bot added the Stale label Oct 16, 2025
@vsop-479
Copy link
Contributor Author

vsop-479 commented Jan 5, 2026

The we should move the changes entry. Do we have a corresponding 9.x section already?

We already have a similar change entry for #13252 under 9.11.0, so we should move this change entry?

Yes, let's merge them an list both PRs.

@uschindler Should I split merged change entry, we are on 10.x now.

@github-actions github-actions bot removed the Stale label Jan 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jpountz jpountz Awaiting requested review from jpountz

@uschindler uschindler Awaiting requested review from uschindler

Assignees

No one assigned

Labels

module:sandbox

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@vsop-479 @jpountz @uschindler