From 1c2d66480e8405da84759cdb6f78d18d4489fc5b Mon Sep 17 00:00:00 2001 From: Andrew Prudhomme Date: Wed, 8 Jan 2025 09:21:44 -0800 Subject: [PATCH] Restructure term query validation (#808) --- .../nrtsearch/server/field/AtomFieldDef.java | 16 ++++++++++++++++ .../server/field/BooleanFieldDef.java | 5 ++--- .../server/field/DateTimeFieldDef.java | 12 ++---------- .../nrtsearch/server/field/DoubleFieldDef.java | 6 ++++-- .../nrtsearch/server/field/FloatFieldDef.java | 6 ++++-- .../nrtsearch/server/field/IdFieldDef.java | 2 ++ .../nrtsearch/server/field/IntFieldDef.java | 6 ++++-- .../nrtsearch/server/field/LongFieldDef.java | 6 ++++-- .../server/field/TextBaseFieldDef.java | 2 ++ .../server/field/properties/TermQueryable.java | 18 ------------------ .../server/query/QueryNodeMapper.java | 2 -- .../server/field/DateTimeFieldDefTest.java | 4 ++-- 12 files changed, 42 insertions(+), 43 deletions(-) diff --git a/src/main/java/com/yelp/nrtsearch/server/field/AtomFieldDef.java b/src/main/java/com/yelp/nrtsearch/server/field/AtomFieldDef.java index 8da4656fd..9797f804a 100644 --- a/src/main/java/com/yelp/nrtsearch/server/field/AtomFieldDef.java +++ b/src/main/java/com/yelp/nrtsearch/server/field/AtomFieldDef.java @@ -22,12 +22,15 @@ import com.yelp.nrtsearch.server.grpc.Field; import com.yelp.nrtsearch.server.grpc.RangeQuery; import com.yelp.nrtsearch.server.grpc.SortType; +import java.util.List; +import java.util.stream.Collectors; import org.apache.lucene.analysis.Analyzer; import org.apache.lucene.analysis.core.KeywordAnalyzer; import org.apache.lucene.document.FieldType; import org.apache.lucene.document.SortedSetDocValuesField; import org.apache.lucene.index.DocValuesType; import org.apache.lucene.index.IndexOptions; +import org.apache.lucene.index.Term; import org.apache.lucene.search.Query; import org.apache.lucene.search.SortField; import org.apache.lucene.search.SortedSetSortField; @@ -82,6 +85,19 @@ protected Analyzer parseSearchAnalyzer(Field requestField) { return keywordAnalyzer; } + @Override + public Query getTermQueryFromTextValue(String textValue) { + verifySearchable("Term query"); + return new org.apache.lucene.search.TermQuery(new Term(getName(), textValue)); + } + + @Override + public Query getTermInSetQueryFromTextValues(List textValues) { + verifySearchable("Term in set query"); + List textTerms = textValues.stream().map(BytesRef::new).collect(Collectors.toList()); + return new org.apache.lucene.search.TermInSetQuery(getName(), textTerms); + } + @Override public SortField getSortField(SortType type) { verifyDocValues("Sort field"); diff --git a/src/main/java/com/yelp/nrtsearch/server/field/BooleanFieldDef.java b/src/main/java/com/yelp/nrtsearch/server/field/BooleanFieldDef.java index 481739835..333b5af0a 100644 --- a/src/main/java/com/yelp/nrtsearch/server/field/BooleanFieldDef.java +++ b/src/main/java/com/yelp/nrtsearch/server/field/BooleanFieldDef.java @@ -140,15 +140,14 @@ public String getType() { @Override public Query getTermQueryFromBooleanValue(boolean booleanValue) { + verifySearchable("Term query"); String indexTermValue = booleanValue ? "1" : "0"; return new org.apache.lucene.search.TermQuery(new Term(getName(), indexTermValue)); } @Override public Query getTermQueryFromTextValue(String textValue) { - boolean termValue = parseBooleanOrThrow(textValue); - String indexTermValue = termValue ? "1" : "0"; - return new org.apache.lucene.search.TermQuery(new Term(getName(), indexTermValue)); + return getTermQueryFromBooleanValue(parseBooleanOrThrow(textValue)); } @Override diff --git a/src/main/java/com/yelp/nrtsearch/server/field/DateTimeFieldDef.java b/src/main/java/com/yelp/nrtsearch/server/field/DateTimeFieldDef.java index 1a498a8c9..2469768f4 100644 --- a/src/main/java/com/yelp/nrtsearch/server/field/DateTimeFieldDef.java +++ b/src/main/java/com/yelp/nrtsearch/server/field/DateTimeFieldDef.java @@ -149,18 +149,9 @@ public Query getRangeQuery(RangeQuery rangeQuery) { return new IndexOrDocValuesQuery(pointQuery, dvQuery); } - @Override - public void checkTermQueriesSupported() { - if (!isSearchable() && !hasDocValues()) { - throw new IllegalStateException( - "Field \"" - + getName() - + "\" is not searchable or does not have doc values, which is required for TermQuery / TermInSetQuery"); - } - } - @Override public Query getTermQueryFromLongValue(long longValue) { + verifySearchableOrDocValues("Term query"); Query pointQuery = null; Query dvQuery = null; if (isSearchable()) { @@ -180,6 +171,7 @@ public Query getTermQueryFromLongValue(long longValue) { @Override public Query getTermInSetQueryFromLongValues(List longValues) { + verifySearchableOrDocValues("Term in set query"); Query pointQuery = null; Query dvQuery = null; if (isSearchable()) { diff --git a/src/main/java/com/yelp/nrtsearch/server/field/DoubleFieldDef.java b/src/main/java/com/yelp/nrtsearch/server/field/DoubleFieldDef.java index a11471a15..ca7ccfc8d 100644 --- a/src/main/java/com/yelp/nrtsearch/server/field/DoubleFieldDef.java +++ b/src/main/java/com/yelp/nrtsearch/server/field/DoubleFieldDef.java @@ -150,23 +150,25 @@ private void ensureUpperIsMoreThanLower(RangeQuery rangeQuery, double lower, dou @Override public Query getTermQueryFromDoubleValue(double doubleValue) { + verifySearchable("Term query"); return DoublePoint.newExactQuery(getName(), doubleValue); } @Override public Query getTermInSetQueryFromDoubleValues(List doubleValues) { + verifySearchable("Term in set query"); return DoublePoint.newSetQuery(getName(), doubleValues); } @Override public Query getTermQueryFromTextValue(String textValue) { - return DoublePoint.newExactQuery(getName(), Double.parseDouble(textValue)); + return getTermQueryFromDoubleValue(Double.parseDouble(textValue)); } @Override public Query getTermInSetQueryFromTextValues(List textValues) { List doubleTerms = new ArrayList<>(textValues.size()); textValues.forEach((s) -> doubleTerms.add(Double.parseDouble(s))); - return DoublePoint.newSetQuery(getName(), doubleTerms); + return getTermInSetQueryFromDoubleValues(doubleTerms); } } diff --git a/src/main/java/com/yelp/nrtsearch/server/field/FloatFieldDef.java b/src/main/java/com/yelp/nrtsearch/server/field/FloatFieldDef.java index 5fb3755eb..91835b223 100644 --- a/src/main/java/com/yelp/nrtsearch/server/field/FloatFieldDef.java +++ b/src/main/java/com/yelp/nrtsearch/server/field/FloatFieldDef.java @@ -149,23 +149,25 @@ private void ensureUpperIsMoreThanLower(RangeQuery rangeQuery, float lower, floa @Override public Query getTermQueryFromFloatValue(float floatValue) { + verifySearchable("Term query"); return FloatPoint.newExactQuery(getName(), floatValue); } @Override public Query getTermInSetQueryFromFloatValues(List floatValues) { + verifySearchable("Term in set query"); return FloatPoint.newSetQuery(getName(), floatValues); } @Override public Query getTermQueryFromTextValue(String textValue) { - return FloatPoint.newExactQuery(getName(), Float.parseFloat(textValue)); + return getTermQueryFromFloatValue(Float.parseFloat(textValue)); } @Override public Query getTermInSetQueryFromTextValues(List textValues) { List floatTerms = new ArrayList<>(textValues.size()); textValues.forEach((s) -> floatTerms.add(Float.parseFloat(s))); - return FloatPoint.newSetQuery(getName(), floatTerms); + return getTermInSetQueryFromFloatValues(floatTerms); } } diff --git a/src/main/java/com/yelp/nrtsearch/server/field/IdFieldDef.java b/src/main/java/com/yelp/nrtsearch/server/field/IdFieldDef.java index f5a5fc4f0..2f78dfa31 100644 --- a/src/main/java/com/yelp/nrtsearch/server/field/IdFieldDef.java +++ b/src/main/java/com/yelp/nrtsearch/server/field/IdFieldDef.java @@ -164,11 +164,13 @@ public Term getTerm(Document document) { @Override public Query getTermQueryFromTextValue(String textValue) { + // _ID fields are always searchable return new org.apache.lucene.search.TermQuery(new Term(getName(), textValue)); } @Override public Query getTermInSetQueryFromTextValues(List textValues) { + // _ID fields are always searchable List textTerms = textValues.stream().map(BytesRef::new).collect(Collectors.toList()); return new org.apache.lucene.search.TermInSetQuery(getName(), textTerms); } diff --git a/src/main/java/com/yelp/nrtsearch/server/field/IntFieldDef.java b/src/main/java/com/yelp/nrtsearch/server/field/IntFieldDef.java index 04361116f..498268802 100644 --- a/src/main/java/com/yelp/nrtsearch/server/field/IntFieldDef.java +++ b/src/main/java/com/yelp/nrtsearch/server/field/IntFieldDef.java @@ -143,24 +143,26 @@ private void ensureUpperIsMoreThanLower(RangeQuery rangeQuery, int lower, int up @Override public Query getTermQueryFromIntValue(int intValue) { + verifySearchable("Term query"); return IntPoint.newExactQuery(getName(), intValue); } @Override public Query getTermInSetQueryFromIntValues(List intValues) { + verifySearchable("Term in set query"); return IntPoint.newSetQuery(getName(), intValues); } @Override public Query getTermQueryFromTextValue(String textValue) { - return IntPoint.newExactQuery(getName(), Integer.parseInt(textValue)); + return getTermQueryFromIntValue(Integer.parseInt(textValue)); } @Override public Query getTermInSetQueryFromTextValues(List textValues) { List intTerms = new ArrayList<>(textValues.size()); textValues.forEach((s) -> intTerms.add(Integer.parseInt(s))); - return IntPoint.newSetQuery(getName(), intTerms); + return getTermInSetQueryFromIntValues(intTerms); } @Override diff --git a/src/main/java/com/yelp/nrtsearch/server/field/LongFieldDef.java b/src/main/java/com/yelp/nrtsearch/server/field/LongFieldDef.java index ab52a7293..c22173ab5 100644 --- a/src/main/java/com/yelp/nrtsearch/server/field/LongFieldDef.java +++ b/src/main/java/com/yelp/nrtsearch/server/field/LongFieldDef.java @@ -139,24 +139,26 @@ private void ensureUpperIsMoreThanLower(RangeQuery rangeQuery, long lower, long @Override public Query getTermQueryFromLongValue(long longValue) { + verifySearchable("Term query"); return LongPoint.newExactQuery(getName(), longValue); } @Override public Query getTermInSetQueryFromLongValues(List longValues) { + verifySearchable("Term in set query"); return LongPoint.newSetQuery(getName(), longValues); } @Override public Query getTermQueryFromTextValue(String textValue) { - return LongPoint.newExactQuery(getName(), Long.parseLong(textValue)); + return getTermQueryFromLongValue(Long.parseLong(textValue)); } @Override public Query getTermInSetQueryFromTextValues(List textValues) { List longTerms = new ArrayList<>(textValues.size()); textValues.forEach((s) -> longTerms.add(Long.parseLong(s))); - return LongPoint.newSetQuery(getName(), longTerms); + return getTermInSetQueryFromLongValues(longTerms); } protected Number parseNumberString(String numberString) { diff --git a/src/main/java/com/yelp/nrtsearch/server/field/TextBaseFieldDef.java b/src/main/java/com/yelp/nrtsearch/server/field/TextBaseFieldDef.java index f34b6d17e..6d52b583e 100644 --- a/src/main/java/com/yelp/nrtsearch/server/field/TextBaseFieldDef.java +++ b/src/main/java/com/yelp/nrtsearch/server/field/TextBaseFieldDef.java @@ -287,11 +287,13 @@ private void addFacet(Document document, String value, List paths) { @Override public Query getTermQueryFromTextValue(String textValue) { + verifySearchable("Term query"); return new org.apache.lucene.search.TermQuery(new Term(getName(), textValue)); } @Override public Query getTermInSetQueryFromTextValues(List textValues) { + verifySearchable("Term in set query"); List textTerms = textValues.stream().map(BytesRef::new).collect(Collectors.toList()); return new org.apache.lucene.search.TermInSetQuery(getName(), textTerms); } diff --git a/src/main/java/com/yelp/nrtsearch/server/field/properties/TermQueryable.java b/src/main/java/com/yelp/nrtsearch/server/field/properties/TermQueryable.java index 17555d19c..ddaca23ba 100644 --- a/src/main/java/com/yelp/nrtsearch/server/field/properties/TermQueryable.java +++ b/src/main/java/com/yelp/nrtsearch/server/field/properties/TermQueryable.java @@ -16,7 +16,6 @@ package com.yelp.nrtsearch.server.field.properties; import com.yelp.nrtsearch.server.field.FieldDef; -import com.yelp.nrtsearch.server.field.IndexableFieldDef; import com.yelp.nrtsearch.server.grpc.TermInSetQuery; import com.yelp.nrtsearch.server.grpc.TermQuery; import java.util.List; @@ -261,21 +260,4 @@ default Query getTermInSetQueryFromLongValues(List longValues) { default Query getTermInSetQueryFromTextValues(List textValues) { return null; } - - /** - * Verify that this field supports term/term in set queries. - * - * @throws IllegalStateException if term queries are not supported - */ - default void checkTermQueriesSupported() { - if (!(this instanceof IndexableFieldDef indexableFieldDef)) { - throw new IllegalStateException("Instance is not an IndexableFieldDef"); - } - if (!indexableFieldDef.isSearchable()) { - throw new IllegalStateException( - "Field " - + indexableFieldDef.getName() - + " is not searchable, which is required for TermQuery / TermInSetQuery"); - } - } } diff --git a/src/main/java/com/yelp/nrtsearch/server/query/QueryNodeMapper.java b/src/main/java/com/yelp/nrtsearch/server/query/QueryNodeMapper.java index 4cb440038..3480b315d 100644 --- a/src/main/java/com/yelp/nrtsearch/server/query/QueryNodeMapper.java +++ b/src/main/java/com/yelp/nrtsearch/server/query/QueryNodeMapper.java @@ -297,7 +297,6 @@ private Query getTermQuery(com.yelp.nrtsearch.server.grpc.TermQuery termQuery, I FieldDef fieldDef = state.getFieldOrThrow(fieldName); if (fieldDef instanceof TermQueryable termQueryable) { - termQueryable.checkTermQueriesSupported(); return termQueryable.getTermQuery(termQuery); } @@ -312,7 +311,6 @@ private Query getTermInSetQuery( FieldDef fieldDef = state.getFieldOrThrow(fieldName); if (fieldDef instanceof TermQueryable termQueryable) { - termQueryable.checkTermQueriesSupported(); return termQueryable.getTermInSetQuery(termInSetQuery); } diff --git a/src/test/java/com/yelp/nrtsearch/server/field/DateTimeFieldDefTest.java b/src/test/java/com/yelp/nrtsearch/server/field/DateTimeFieldDefTest.java index 59a5f388c..af6dda9dc 100644 --- a/src/test/java/com/yelp/nrtsearch/server/field/DateTimeFieldDefTest.java +++ b/src/test/java/com/yelp/nrtsearch/server/field/DateTimeFieldDefTest.java @@ -658,7 +658,7 @@ public void testTermQuery_noSearchOrDV() { assertTrue( e.getMessage() .contains( - "Field \"stored_only\" is not searchable or does not have doc values, which is required for TermQuery / TermInSetQuery")); + "Term query requires field to be searchable or have doc values: stored_only")); } } @@ -830,7 +830,7 @@ public void testTermInSetQuery_noSearchOrDV() { assertTrue( e.getMessage() .contains( - "Field \"stored_only\" is not searchable or does not have doc values, which is required for TermQuery / TermInSetQuery")); + "Term in set query requires field to be searchable or have doc values: stored_only")); } }