You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@lucene.apache.org by GitBox <gi...@apache.org> on 2022/03/17 00:13:05 UTC
[GitHub] [lucene] Yuti-G opened a new pull request #751: LUCENE-10467: Throws IllegalArgumentException for getAllDims and getTopChildren if topN <= 0
Yuti-G opened a new pull request #751:
URL: https://github.com/apache/lucene/pull/751
# Description
Currently, there are different behaviors from subclass that implements `getAllDims` and `getTopChildren` when the requested `topN` is invalid (`topN <= 0`). Some overridden implementations throw a `NullPointerException` and others throw an `IllegalArgumentException`.
This change aligns the behaviors across all overridden implementations of `getAllDims` and `getTopChildren` by throwing an `IllegalArgumentException` and exiting the function earlier if the parameter `topN` is invalid.
# Solution
Added a condition checking if `topN <= 0` in the beginning of both functions. If `topN` is invalid, the function will be terminated by throwing an `IllegalArgumentException`.
# Tests
Added new testing, `getAllDims(0)` and `getTopChildren(0, dim)`, for all subclass that implements `getAllDims` and `getTopChildren`.
# Checklist
Please review the following and check all that apply:
- [X] I have reviewed the guidelines for [How to Contribute](https://wiki.apache.org/lucene/HowToContribute) and my code conforms to the standards described there to the best of my ability.
- [X] I have created a Jira issue and added the issue ID to my pull request title.
- [X] I have given Lucene maintainers [access](https://help.github.com/en/articles/allowing-changes-to-a-pull-request-branch-created-from-a-fork) to contribute to my PR branch. (optional but recommended)
- [X] I have developed this patch against the `main` branch.
- [X] I have run `./gradlew check`.
- [X] I have added tests for my changes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] Yuti-G commented on a change in pull request #751: LUCENE-10467: Throws IllegalArgumentException for getAllDims and getTopChildren if topN <= 0
Posted by GitBox <gi...@apache.org>.
Yuti-G commented on a change in pull request #751:
URL: https://github.com/apache/lucene/pull/751#discussion_r835713023
##########
File path: lucene/facet/src/java/org/apache/lucene/facet/LongValueFacetCounts.java
##########
@@ -348,6 +348,9 @@ private void increment(long value) {
@Override
public FacetResult getTopChildren(int topN, String dim, String... path) {
+ if (topN <= 0) {
Review comment:
Hi @gsmiller, thanks for your feedback! Can you elaborate more on what does open a spin-off issue mean in Lucene? I just added @lucene.experimental in the new commit. If the community thinks this may not be an improvement in the long-term, I can close the PR for now and if users have this use case in the future, they can still see the history in Jira issue. Thanks!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] gsmiller commented on a change in pull request #751: LUCENE-10467: Throws IllegalArgumentException for getAllDims and getTopChildren if topN <= 0
Posted by GitBox <gi...@apache.org>.
gsmiller commented on a change in pull request #751:
URL: https://github.com/apache/lucene/pull/751#discussion_r836685698
##########
File path: lucene/facet/src/java/org/apache/lucene/facet/LongValueFacetCounts.java
##########
@@ -348,6 +348,9 @@ private void increment(long value) {
@Override
public FacetResult getTopChildren(int topN, String dim, String... path) {
+ if (topN <= 0) {
Review comment:
By "spin-off issue," I just meant open a new Jira issue to capture the future work instead of tackling it now. The future work being seeing if there's a way to enforce this precondition check in a parent class common to our various implementations to avoid the copy/paste check. I think this PR should move forward. It's good progress!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] mikemccand commented on a change in pull request #751: LUCENE-10467: Throws IllegalArgumentException for getAllDims and getTopChildren if topN <= 0
Posted by GitBox <gi...@apache.org>.
mikemccand commented on a change in pull request #751:
URL: https://github.com/apache/lucene/pull/751#discussion_r830102401
##########
File path: lucene/facet/src/java/org/apache/lucene/facet/LongValueFacetCounts.java
##########
@@ -348,6 +348,9 @@ private void increment(long value) {
@Override
public FacetResult getTopChildren(int topN, String dim, String... path) {
+ if (topN <= 0) {
Review comment:
Could we maybe factor out a `static` package-private helper method `validateTopN` that would do this check and throw the exception, and call that from all these places?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] gsmiller commented on a change in pull request #751: LUCENE-10467: Throws IllegalArgumentException for getAllDims and getTopChildren if topN <= 0
Posted by GitBox <gi...@apache.org>.
gsmiller commented on a change in pull request #751:
URL: https://github.com/apache/lucene/pull/751#discussion_r835436710
##########
File path: lucene/facet/src/java/org/apache/lucene/facet/LongValueFacetCounts.java
##########
@@ -348,6 +348,9 @@ private void increment(long value) {
@Override
public FacetResult getTopChildren(int topN, String dim, String... path) {
+ if (topN <= 0) {
Review comment:
It also occurs to me that this is somewhat cumbersome to require every sub-class to do this validation if they want to. I don't think we have to solve it here, but could you open a spin-off issue to capture the idea of potentially moving this validation into a common parent class so each concrete implementation doesn't need to do this check?
Along those lines, if we do introduce a `protected` method here, can you please mark it `@lucene.experimental` with a note that it may not exist in future versions of Lucene? I'm not sure we actually want to support this as part of our API surface long-term. Once we add this, it's possible that user-implemented sub-classes could start relying on it, and I'd like to be able to remove it easily without jumping through back-compat hoops.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] Yuti-G commented on a change in pull request #751: LUCENE-10467: Throws IllegalArgumentException for getAllDims and getTopChildren if topN <= 0
Posted by GitBox <gi...@apache.org>.
Yuti-G commented on a change in pull request #751:
URL: https://github.com/apache/lucene/pull/751#discussion_r830443213
##########
File path: lucene/facet/src/java/org/apache/lucene/facet/LongValueFacetCounts.java
##########
@@ -348,6 +348,9 @@ private void increment(long value) {
@Override
public FacetResult getTopChildren(int topN, String dim, String... path) {
+ if (topN <= 0) {
Review comment:
Hi @mikemccand, thank you so much for reviewing my PR!
I tried to factor out a package-private helper method in Facets, but there are subclasses under the subpackage of facet, e.g., `package org.apache.lucene.facet.taxonomy` that cannot call this method. And therefore, I made the method protected in order to be called in all these places in my new commit. Please let me know how you think. Thanks!
```
public abstract class Facets {
protected static void validateTopN(int topN) {
if (topN <= 0) {
throw new IllegalArgumentException("topN must be > 0 (got: " + topN + ")");
}
}
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] Yuti-G commented on a change in pull request #751: LUCENE-10467: Throws IllegalArgumentException for getAllDims and getTopChildren if topN <= 0
Posted by GitBox <gi...@apache.org>.
Yuti-G commented on a change in pull request #751:
URL: https://github.com/apache/lucene/pull/751#discussion_r830443213
##########
File path: lucene/facet/src/java/org/apache/lucene/facet/LongValueFacetCounts.java
##########
@@ -348,6 +348,9 @@ private void increment(long value) {
@Override
public FacetResult getTopChildren(int topN, String dim, String... path) {
+ if (topN <= 0) {
Review comment:
Hi @mikemccand, thank you so much for reviewing my PR!
I tried to factor out a package-private helper method in Facets, but there are subclasses under the subpackage of facet, e.g., `package org.apache.lucene.facet.taxonomy` that cannot call this method. And therefore, I made the method protected in order to be called in all these places in my new commit. Please let me know how you think. Thanks!
```
public abstract class Facets {
...
protected static void validateTopN(int topN) {
if (topN <= 0) {
throw new IllegalArgumentException("topN must be > 0 (got: " + topN + ")");
}
}
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] Yuti-G commented on a change in pull request #751: LUCENE-10467: Throws IllegalArgumentException for getAllDims and getTopChildren if topN <= 0
Posted by GitBox <gi...@apache.org>.
Yuti-G commented on a change in pull request #751:
URL: https://github.com/apache/lucene/pull/751#discussion_r830443213
##########
File path: lucene/facet/src/java/org/apache/lucene/facet/LongValueFacetCounts.java
##########
@@ -348,6 +348,9 @@ private void increment(long value) {
@Override
public FacetResult getTopChildren(int topN, String dim, String... path) {
+ if (topN <= 0) {
Review comment:
Hi @mikemccand, thank you so much for reviewing my PR!
I tried to factor out a package-private helper method in Facets, but there are subclasses under the subpackage of facet, e.g., `package org.apache.lucene.facet.taxonomy;` that cannot call this method. And therefore, I made the method protected in order to be called in all these places in my new commit. Please let me know how you think. Thanks!
```
protected static void validateTopN(int topN) {
if (topN <= 0) {
throw new IllegalArgumentException("topN must be > 0 (got: " + topN + ")");
}
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org