Skip to content
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.

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

Cleanup TokenizedPhraseQueryNode code #13041

Merged
merged 6 commits into from
Feb 15, 2024

Conversation

sabi0
Copy link
Contributor

@sabi0 sabi0 commented Jan 28, 2024

  • harmonize implementations of the methods working with children
  • save getChildren() in a variable and re-use it
  • use more efficient isEmpty() instead of size() == 0
  • fix a typo in the tag name: tokenized_t_phrase => tokenizedphrase
  • remove unnecessary toString() call
  • remove trailing comment referring to another class (copy paste? renaming that did not update the comment?)

return null;

} else {
return ((FieldableNode) children.get(0)).getField();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

According to the setField(CharSequence) method below children are not necessarily FieldableNode.
I.e. this line might throw ClassCastException?

Copy link
Contributor

Choose a reason for hiding this comment

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

I see your point. Maybe it's worth adding an assertion?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the two methods should be aligned.
If one has an assert (assumes all the children are FieldableNode) then the other should do so too.
Of they both should silently ignore children of other types.

I would probably go with "silently ignore children of other types" as the least surprising option.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think having the two methods aligned, the way they are in your latest commit, makes sense.

Copy link

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 Feb 13, 2024
Copy link
Contributor

@stefanvodita stefanvodita left a comment

Choose a reason for hiding this comment

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

The clean-up looks good. Sorry it took so long to get a review.

return null;

} else {
return ((FieldableNode) children.get(0)).getField();
Copy link
Contributor

Choose a reason for hiding this comment

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

I see your point. Maybe it's worth adding an assertion?

@github-actions github-actions bot removed the Stale label Feb 14, 2024
@@ -258,6 +258,8 @@ Improvements
Tests are running with random byte order to ensure that the order does not affect correctness
of code. Native order was enabled for LZ4 compression. (Uwe Schindler)

* GITHUB#13041: TokenizedPhraseQueryNode code cleanup (Dmitry Cherniachenko)
Copy link
Contributor

Choose a reason for hiding this comment

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

This would have to move up to version 9.11 because 9.10 is already in the release process.

@stefanvodita stefanvodita merged commit f24b1de into apache:main Feb 15, 2024
4 checks passed
@sabi0 sabi0 deleted the TokenizedPhraseQueryNode branch February 15, 2024 10:13
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