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/02/15 21:05:17 UTC
[GitHub] [lucene] gsmiller commented on a change in pull request #678: LUCENE-10398: Add static method for getting Terms from LeafReader
gsmiller commented on a change in pull request #678:
URL: https://github.com/apache/lucene/pull/678#discussion_r807228024
##########
File path: lucene/core/src/java/org/apache/lucene/index/Terms.java
##########
@@ -124,6 +124,69 @@ protected BytesRef nextSeekTerm(BytesRef term) throws IOException {
/** Zero-length array of {@link Terms}. */
public static final Terms[] EMPTY_ARRAY = new Terms[0];
+ /** An empty {@link Terms} which returns no terms */
+ public static Terms emptyTerms() {
Review comment:
What about defining this once and exposing it through a static variable much like `TermsEnum.EMPTY` (e.g., `Terms.EMPTY`)? This looks perfectly safe to share/reuse to me, so we can avoid the overhead of instantiating over-and-over.
##########
File path: lucene/core/src/java/org/apache/lucene/document/FeatureQuery.java
##########
@@ -111,12 +111,9 @@ public Explanation explain(LeafReaderContext context, int doc) throws IOExceptio
@Override
public Scorer scorer(LeafReaderContext context) throws IOException {
- Terms terms = context.reader().terms(fieldName);
- if (terms == null) {
- return null;
- }
+ Terms terms = Terms.terms(context.reader(), fieldName);
TermsEnum termsEnum = terms.iterator();
- if (termsEnum.seekExact(new BytesRef(featureName)) == false) {
+ if (!termsEnum.seekExact(new BytesRef(featureName))) {
Review comment:
As a best-practice, we prefer using `== false` instead of `!` syntax in this code base. Can you please retain this style? Thanks!
##########
File path: lucene/core/src/java/org/apache/lucene/search/TermInSetQuery.java
##########
@@ -244,8 +244,8 @@ public Weight createWeight(IndexSearcher searcher, ScoreMode scoreMode, float bo
@Override
public Matches matches(LeafReaderContext context, int doc) throws IOException {
- Terms terms = context.reader().terms(field);
- if (terms == null || terms.hasPositions() == false) {
+ Terms terms = Terms.terms(context.reader(), field);
+ if (!terms.hasPositions()) {
Review comment:
minor: Another place to use `== false` instead of `!` please
##########
File path: lucene/core/src/java/org/apache/lucene/index/Terms.java
##########
@@ -124,6 +124,69 @@ protected BytesRef nextSeekTerm(BytesRef term) throws IOException {
/** Zero-length array of {@link Terms}. */
public static final Terms[] EMPTY_ARRAY = new Terms[0];
+ /** An empty {@link Terms} which returns no terms */
+ public static Terms emptyTerms() {
Review comment:
Also, since we don't actually need this outside of the `Terms` class right now, I think I'd also prefer we make access `private`. We can always open up access if a use-case demands it.
##########
File path: lucene/queries/src/java/org/apache/lucene/queries/function/IndexReaderFunctions.java
##########
@@ -212,10 +212,10 @@ private TermFreqDoubleValuesSource(Term term) {
@Override
public DoubleValues getValues(LeafReaderContext ctx, DoubleValues scores) throws IOException {
- Terms terms = ctx.reader().terms(term.field());
- TermsEnum te = terms == null ? null : terms.iterator();
+ Terms terms = Terms.terms(ctx.reader(), term.field());
+ TermsEnum te = terms.iterator();
- if (te == null || te.seekExact(term.bytes()) == false) {
+ if (!te.seekExact(term.bytes())) {
Review comment:
minor: Just another place to use `== false` instead of `!` please.
##########
File path: lucene/core/src/java/org/apache/lucene/index/Terms.java
##########
@@ -124,6 +124,69 @@ protected BytesRef nextSeekTerm(BytesRef term) throws IOException {
/** Zero-length array of {@link Terms}. */
public static final Terms[] EMPTY_ARRAY = new Terms[0];
+ /** An empty {@link Terms} which returns no terms */
+ public static Terms emptyTerms() {
+ return new Terms() {
+ @Override
+ public TermsEnum iterator() throws IOException {
+ return TermsEnum.EMPTY;
+ }
+
+ @Override
+ public long size() throws IOException {
+ return 0;
+ }
+
+ @Override
+ public long getSumTotalTermFreq() throws IOException {
+ return 0;
+ }
+
+ @Override
+ public long getSumDocFreq() throws IOException {
+ return 0;
+ }
+
+ @Override
+ public int getDocCount() throws IOException {
+ return 0;
+ }
+
+ @Override
+ public boolean hasFreqs() {
+ return false;
+ }
+
+ @Override
+ public boolean hasOffsets() {
+ return false;
+ }
+
+ @Override
+ public boolean hasPositions() {
+ return false;
+ }
+
+ @Override
+ public boolean hasPayloads() {
+ return false;
+ }
+ };
+ }
+
+ /**
+ * Returns the {@link Terms} index for this field, or {@link #emptyTerms} if it has none.
+ * @return terms instance, or an empty instance if {@code field} does not exist in this reader
+ * @throws IOException if an I/O error occurs.
+ */
+ public static Terms terms(LeafReader reader, String field) throws IOException {
Review comment:
What about a name like `getTerms`? That's a bit more consistent with how `DocValues` names its static factory methods (e.g., `getNumeric`).
minor: Also, could you move this right under the constructor or to the very bottom so it's not right in the middle of the instance methods? I'd prefer putting it at the top but don't feel super strongly about that.
--
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