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/12/25 07:35:20 UTC
[GitHub] [lucene] twosom opened a new pull request, #12040: Minor refactoring and cleanup to BooleanQuery code
twosom opened a new pull request, #12040:
URL: https://github.com/apache/lucene/pull/12040
### Description
some cleanups (typos, use Java17 feature , improving readability)
<!--
If this is your first contribution to Lucene, please make sure you have reviewed the contribution guide.
https://github.com/apache/lucene/blob/main/CONTRIBUTING.md
-->
--
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] twosom commented on a diff in pull request #12040: Minor refactoring and cleanup to BooleanQuery code
Posted by GitBox <gi...@apache.org>.
twosom commented on code in PR #12040:
URL: https://github.com/apache/lucene/pull/12040#discussion_r1057624186
##########
lucene/core/src/java/org/apache/lucene/search/BooleanQuery.java:
##########
@@ -408,19 +393,15 @@ public Query rewrite(IndexSearcher indexSearcher) throws IOException {
if (shouldClauses.size() != clauseSets.get(Occur.SHOULD).size()) {
BooleanQuery.Builder builder =
new BooleanQuery.Builder().setMinimumNumberShouldMatch(minimumNumberShouldMatch);
- for (Map.Entry<Query, Double> entry : shouldClauses.entrySet()) {
- Query query = entry.getKey();
- float boost = entry.getValue().floatValue();
- if (boost != 1f) {
- query = new BoostQuery(query, boost);
- }
- builder.add(query, Occur.SHOULD);
- }
- for (BooleanClause clause : clauses) {
- if (clause.getOccur() != Occur.SHOULD) {
- builder.add(clause);
- }
- }
+ shouldClauses.forEach(
+ (query, value) -> {
+ float boost = value.floatValue();
+ if (boost != 1f) {
+ query = new BoostQuery(query, boost);
+ }
+ builder.add(query, Occur.SHOULD);
+ });
+ clauses.stream().filter(clause -> clause.getOccur() != Occur.SHOULD).forEach(builder::add);
Review Comment:
and this stream too.
`clauses.stream().filter(BooleanClause::isNotShould).forEach(builder::add);`
--
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] twosom commented on a diff in pull request #12040: Minor refactoring and cleanup to BooleanQuery code
Posted by GitBox <gi...@apache.org>.
twosom commented on code in PR #12040:
URL: https://github.com/apache/lucene/pull/12040#discussion_r1057622205
##########
lucene/core/src/java/org/apache/lucene/search/BooleanQuery.java:
##########
@@ -346,21 +338,15 @@ public Query rewrite(IndexSearcher indexSearcher) throws IOException {
if (clauseSets.get(Occur.FILTER).size() > 0) {
final Set<Query> filters = new HashSet<>(clauseSets.get(Occur.FILTER));
boolean modified = false;
- if (filters.size() > 1 || clauseSets.get(Occur.MUST).isEmpty() == false) {
+ if (filters.size() > 1 || !clauseSets.get(Occur.MUST).isEmpty()) {
modified = filters.remove(new MatchAllDocsQuery());
}
modified |= filters.removeAll(clauseSets.get(Occur.MUST));
if (modified) {
BooleanQuery.Builder builder = new BooleanQuery.Builder();
builder.setMinimumNumberShouldMatch(getMinimumNumberShouldMatch());
- for (BooleanClause clause : clauses) {
- if (clause.getOccur() != Occur.FILTER) {
- builder.add(clause);
- }
- }
- for (Query filter : filters) {
- builder.add(filter, Occur.FILTER);
- }
+ clauses.stream().filter(clause -> clause.getOccur() != Occur.FILTER).forEach(builder::add);
+ filters.forEach(filter -> builder.add(filter, Occur.FILTER));
Review Comment:
What do you think about change this streams to like `caluses.stream().filter(BooleanClause::isNotFilter).forEach(builder::add);`
--
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] twosom closed pull request #12040: Minor refactoring and cleanup to BooleanQuery code
Posted by "twosom (via GitHub)" <gi...@apache.org>.
twosom closed pull request #12040: Minor refactoring and cleanup to BooleanQuery code
URL: https://github.com/apache/lucene/pull/12040
--
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] jpountz commented on a diff in pull request #12040: Minor refactoring and cleanup to BooleanQuery code
Posted by GitBox <gi...@apache.org>.
jpountz commented on code in PR #12040:
URL: https://github.com/apache/lucene/pull/12040#discussion_r1057563242
##########
lucene/core/src/java/org/apache/lucene/search/BooleanQuery.java:
##########
@@ -204,11 +204,11 @@ BooleanQuery rewriteNoScoring(IndexSearcher indexSearcher) throws IOException {
for (BooleanClause clause : clauses) {
Query query = clause.getQuery();
Query rewritten = new ConstantScoreQuery(query).rewrite(indexSearcher);
- if (rewritten instanceof ConstantScoreQuery) {
- rewritten = ((ConstantScoreQuery) rewritten).getQuery();
+ if (rewritten instanceof ConstantScoreQuery constantScoreQuery) {
+ rewritten = constantScoreQuery.getQuery();
}
BooleanClause.Occur occur = clause.getOccur();
- if (occur == Occur.SHOULD && keepShould == false) {
+ if (occur == Occur.SHOULD && !keepShould) {
Review Comment:
we use `== false` intentionally across the codebase for better readability, can you keep it and other places in this class too?
##########
lucene/core/src/java/org/apache/lucene/search/BooleanQuery.java:
##########
@@ -346,21 +338,15 @@ public Query rewrite(IndexSearcher indexSearcher) throws IOException {
if (clauseSets.get(Occur.FILTER).size() > 0) {
final Set<Query> filters = new HashSet<>(clauseSets.get(Occur.FILTER));
boolean modified = false;
- if (filters.size() > 1 || clauseSets.get(Occur.MUST).isEmpty() == false) {
+ if (filters.size() > 1 || !clauseSets.get(Occur.MUST).isEmpty()) {
modified = filters.remove(new MatchAllDocsQuery());
}
modified |= filters.removeAll(clauseSets.get(Occur.MUST));
if (modified) {
BooleanQuery.Builder builder = new BooleanQuery.Builder();
builder.setMinimumNumberShouldMatch(getMinimumNumberShouldMatch());
- for (BooleanClause clause : clauses) {
- if (clause.getOccur() != Occur.FILTER) {
- builder.add(clause);
- }
- }
- for (Query filter : filters) {
- builder.add(filter, Occur.FILTER);
- }
+ clauses.stream().filter(clause -> clause.getOccur() != Occur.FILTER).forEach(builder::add);
+ filters.forEach(filter -> builder.add(filter, Occur.FILTER));
Review Comment:
I think using streams above to compute `clauseCount` is fine as it makes the code less verbose while remaining easy to read. I'm less sure about this change: it's shorter, but it also reads harder for me.
##########
lucene/core/src/java/org/apache/lucene/search/BooleanQuery.java:
##########
@@ -408,19 +393,15 @@ public Query rewrite(IndexSearcher indexSearcher) throws IOException {
if (shouldClauses.size() != clauseSets.get(Occur.SHOULD).size()) {
BooleanQuery.Builder builder =
new BooleanQuery.Builder().setMinimumNumberShouldMatch(minimumNumberShouldMatch);
- for (Map.Entry<Query, Double> entry : shouldClauses.entrySet()) {
- Query query = entry.getKey();
- float boost = entry.getValue().floatValue();
- if (boost != 1f) {
- query = new BoostQuery(query, boost);
- }
- builder.add(query, Occur.SHOULD);
- }
- for (BooleanClause clause : clauses) {
- if (clause.getOccur() != Occur.SHOULD) {
- builder.add(clause);
- }
- }
+ shouldClauses.forEach(
+ (query, value) -> {
+ float boost = value.floatValue();
+ if (boost != 1f) {
+ query = new BoostQuery(query, boost);
+ }
+ builder.add(query, Occur.SHOULD);
+ });
+ clauses.stream().filter(clause -> clause.getOccur() != Occur.SHOULD).forEach(builder::add);
Review Comment:
Likewise here, streams don't make the code easier to read?
--
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] twosom commented on a diff in pull request #12040: Minor refactoring and cleanup to BooleanQuery code
Posted by GitBox <gi...@apache.org>.
twosom commented on code in PR #12040:
URL: https://github.com/apache/lucene/pull/12040#discussion_r1057623506
##########
lucene/core/src/java/org/apache/lucene/search/BooleanQuery.java:
##########
@@ -346,21 +338,15 @@ public Query rewrite(IndexSearcher indexSearcher) throws IOException {
if (clauseSets.get(Occur.FILTER).size() > 0) {
final Set<Query> filters = new HashSet<>(clauseSets.get(Occur.FILTER));
boolean modified = false;
- if (filters.size() > 1 || clauseSets.get(Occur.MUST).isEmpty() == false) {
+ if (filters.size() > 1 || !clauseSets.get(Occur.MUST).isEmpty()) {
modified = filters.remove(new MatchAllDocsQuery());
}
modified |= filters.removeAll(clauseSets.get(Occur.MUST));
if (modified) {
BooleanQuery.Builder builder = new BooleanQuery.Builder();
builder.setMinimumNumberShouldMatch(getMinimumNumberShouldMatch());
- for (BooleanClause clause : clauses) {
- if (clause.getOccur() != Occur.FILTER) {
- builder.add(clause);
- }
- }
- for (Query filter : filters) {
- builder.add(filter, Occur.FILTER);
- }
+ clauses.stream().filter(clause -> clause.getOccur() != Occur.FILTER).forEach(builder::add);
+ filters.forEach(filter -> builder.add(filter, Occur.FILTER));
Review Comment:
and method isNotFilter would be like
```
public boolean isNotFilter() {
return occur == Occur.FILTER;
}
```
##########
lucene/core/src/java/org/apache/lucene/search/BooleanQuery.java:
##########
@@ -346,21 +338,15 @@ public Query rewrite(IndexSearcher indexSearcher) throws IOException {
if (clauseSets.get(Occur.FILTER).size() > 0) {
final Set<Query> filters = new HashSet<>(clauseSets.get(Occur.FILTER));
boolean modified = false;
- if (filters.size() > 1 || clauseSets.get(Occur.MUST).isEmpty() == false) {
+ if (filters.size() > 1 || !clauseSets.get(Occur.MUST).isEmpty()) {
modified = filters.remove(new MatchAllDocsQuery());
}
modified |= filters.removeAll(clauseSets.get(Occur.MUST));
if (modified) {
BooleanQuery.Builder builder = new BooleanQuery.Builder();
builder.setMinimumNumberShouldMatch(getMinimumNumberShouldMatch());
- for (BooleanClause clause : clauses) {
- if (clause.getOccur() != Occur.FILTER) {
- builder.add(clause);
- }
- }
- for (Query filter : filters) {
- builder.add(filter, Occur.FILTER);
- }
+ clauses.stream().filter(clause -> clause.getOccur() != Occur.FILTER).forEach(builder::add);
+ filters.forEach(filter -> builder.add(filter, Occur.FILTER));
Review Comment:
and method `isNotFilter` would be like
```
public boolean isNotFilter() {
return occur == Occur.FILTER;
}
```
--
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] twosom commented on a diff in pull request #12040: Minor refactoring and cleanup to BooleanQuery code
Posted by GitBox <gi...@apache.org>.
twosom commented on code in PR #12040:
URL: https://github.com/apache/lucene/pull/12040#discussion_r1057623506
##########
lucene/core/src/java/org/apache/lucene/search/BooleanQuery.java:
##########
@@ -346,21 +338,15 @@ public Query rewrite(IndexSearcher indexSearcher) throws IOException {
if (clauseSets.get(Occur.FILTER).size() > 0) {
final Set<Query> filters = new HashSet<>(clauseSets.get(Occur.FILTER));
boolean modified = false;
- if (filters.size() > 1 || clauseSets.get(Occur.MUST).isEmpty() == false) {
+ if (filters.size() > 1 || !clauseSets.get(Occur.MUST).isEmpty()) {
modified = filters.remove(new MatchAllDocsQuery());
}
modified |= filters.removeAll(clauseSets.get(Occur.MUST));
if (modified) {
BooleanQuery.Builder builder = new BooleanQuery.Builder();
builder.setMinimumNumberShouldMatch(getMinimumNumberShouldMatch());
- for (BooleanClause clause : clauses) {
- if (clause.getOccur() != Occur.FILTER) {
- builder.add(clause);
- }
- }
- for (Query filter : filters) {
- builder.add(filter, Occur.FILTER);
- }
+ clauses.stream().filter(clause -> clause.getOccur() != Occur.FILTER).forEach(builder::add);
+ filters.forEach(filter -> builder.add(filter, Occur.FILTER));
Review Comment:
and method isNotFilter will be like
```
public boolean isNotFilter() {
return occur == Occur.FILTER;
}
```
--
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] twosom commented on a diff in pull request #12040: Minor refactoring and cleanup to BooleanQuery code
Posted by GitBox <gi...@apache.org>.
twosom commented on code in PR #12040:
URL: https://github.com/apache/lucene/pull/12040#discussion_r1057574839
##########
lucene/core/src/java/org/apache/lucene/search/BooleanQuery.java:
##########
@@ -204,11 +204,11 @@ BooleanQuery rewriteNoScoring(IndexSearcher indexSearcher) throws IOException {
for (BooleanClause clause : clauses) {
Query query = clause.getQuery();
Query rewritten = new ConstantScoreQuery(query).rewrite(indexSearcher);
- if (rewritten instanceof ConstantScoreQuery) {
- rewritten = ((ConstantScoreQuery) rewritten).getQuery();
+ if (rewritten instanceof ConstantScoreQuery constantScoreQuery) {
+ rewritten = constantScoreQuery.getQuery();
}
BooleanClause.Occur occur = clause.getOccur();
- if (occur == Occur.SHOULD && keepShould == false) {
+ if (occur == Occur.SHOULD && !keepShould) {
Review Comment:
Thanks to reply.
I'll keep `== false`😊
--
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