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

LUCENE-10550: Add getAllChildren functionality to facets #914

Merged
merged 5 commits into from
Jun 21, 2022

Conversation

Yuti-G
Copy link
Contributor

@Yuti-G Yuti-G commented May 20, 2022

Description

This new API returns all children for a specified path and allows users to sort in their desired criteria

Solution

  • Added getAllChildren function in the Facets class.
  • Override getAllChildren in subclasses

Tests

Added new testing for all the overridden implementations of getAllChildren in Facets

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 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.

Copy link
Contributor

@gsmiller gsmiller left a comment

Choose a reason for hiding this comment

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

Thanks @Yuti-G! Excited to see this getting implemented. I ran out of time today to look at your approach to testing, but left you some feedback on the main implementation. Thanks again!

@Yuti-G
Copy link
Contributor Author

Yuti-G commented Jun 17, 2022

Thanks @gsmiller for spending time reviewing my PR and leaving the great feedback! I addressed all of the comments except for the one that requires casting List<Integer> to int[] for ordinals to call getBulkPath in the IntTaxonomy and FloatTaxonomy. Please let me know if there is a better way to do it. Thanks!

Copy link
Contributor

@gsmiller gsmiller left a comment

Choose a reason for hiding this comment

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

Thanks! Changes look good to me. Left one small comment on javadoc. Also, could you please add an entry to the CHANGES.txt file? From what I can tell, this should be a fully backwards-compatible change, so we should be able to put it in the 9.3 section and back-port. Please let me know if you have questions on adding the CHANGES entry, or questions about back-porting. Thanks again!

@Yuti-G
Copy link
Contributor Author

Yuti-G commented Jun 21, 2022

Thank you so much for the last check! I added more javadoc and a new entry to the CHANGES.txt. For back-porting, should I wait until this PR is merged and checkout a new branch against the latest branch_9x to cherrypick the merged commit? Thanks again!

@gsmiller
Copy link
Contributor

For back-porting, should I wait until this PR is merged and checkout a new branch against the latest branch_9x to cherrypick the merged commit? Thanks again!

Exactly! Then you can open a PR with that branch against origin/branch_9x (github will automatically select origin/main as the suggested destination so just change that). And just mention that it's a backport PR.

@gsmiller gsmiller merged commit bdcb4b3 into apache:main Jun 21, 2022
@gsmiller
Copy link
Contributor

Merged onto main. Thanks again @Yuti-G! Exciting to see this new functionality available :)

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.

None yet

2 participants