dsmiley commented on code in PR #3972:
URL: https://github.com/apache/solr/pull/3972#discussion_r2751719025
##########
solr/core/src/java/org/apache/solr/handler/component/ExpandComponent.java:
##########
@@ -335,7 +339,8 @@ public void process(ResponseBuilder rb) throws IOException {
} else {
groupSet = new LongHashSet(docList.size());
NumericDocValues collapseValues =
- contexts.get(currentContext).reader().getNumericDocValues(field);
+ DocValues.unwrapSingleton(
Review Comment:
why this change? It was simple/direct before.
If we do need to keep this idiom, which I see you repeated again and again,
it'd be good to put this into a single utility call IMO.
##########
solr/core/src/java/org/apache/solr/schema/FieldProperties.java:
##########
@@ -53,6 +53,8 @@ public abstract class FieldProperties {
protected static final int LARGE_FIELD = 0b1000000000000000000;
protected static final int UNINVERTIBLE = 0b10000000000000000000;
+ protected static final int ENHANCED_INDEX = 0b100000000000000000000;
Review Comment:
we should discuss, maybe in JIRA, how to name this / its semantics, to
include a different approach where a user picks the *type* of index instead of
another boolean. There are several options a user might pick from in terms of
which index types exist.
##########
solr/core/src/java/org/apache/solr/schema/LongPointField.java:
##########
@@ -103,27 +104,39 @@ public Object toObject(IndexableField f) {
}
@Override
- protected Query getExactQuery(SchemaField field, String externalVal) {
- return LongPoint.newExactQuery(
- field.getName(), parseLongFromUser(field.getName(), externalVal));
- }
-
- @Override
- public Query getSetQuery(QParser parser, SchemaField field,
Collection<String> externalVal) {
- assert externalVal.size() > 0;
- if (!field.indexed()) {
- return super.getSetQuery(parser, field, externalVal);
- }
- long[] values = new long[externalVal.size()];
- int i = 0;
- for (String val : externalVal) {
- values[i] = parseLongFromUser(field.getName(), val);
- i++;
+ public Query getSetQuery(QParser parser, SchemaField field,
Collection<String> externalVals) {
+ assert externalVals.size() > 0;
+ Query indexQuery = null;
+ long[] values = null;
+ if (field.indexed()) {
+ values = new long[externalVals.size()];
+ int i = 0;
+ for (String val : externalVals) {
+ values[i++] = parseLongFromUser(field.getName(), val);
+ }
+ indexQuery = LongPoint.newSetQuery(field.getName(), values);
}
if (field.hasDocValues()) {
- return LongField.newSetQuery(field.getName(), values);
+ long[] points;
+ if (values != null) {
+ points = values.clone();
+ } else {
+ points = new long[externalVals.size()];
+ int i = 0;
+ for (String val : externalVals) {
+ points[i++] = parseLongFromUser(field.getName(), val);
+ }
+ }
+ Query docValuesQuery =
SortedNumericDocValuesField.newSlowSetQuery(field.getName(), points);
+ if (indexQuery != null) {
+ return new IndexOrDocValuesQuery(indexQuery, docValuesQuery);
Review Comment:
Looks like you're expanding the scope to optimize point fields for using
IndexOrDocValuesQuery ?
##########
solr/core/src/java/org/apache/solr/schema/LongPointField.java:
##########
@@ -103,27 +104,39 @@ public Object toObject(IndexableField f) {
}
@Override
- protected Query getExactQuery(SchemaField field, String externalVal) {
- return LongPoint.newExactQuery(
- field.getName(), parseLongFromUser(field.getName(), externalVal));
- }
-
- @Override
- public Query getSetQuery(QParser parser, SchemaField field,
Collection<String> externalVal) {
- assert externalVal.size() > 0;
- if (!field.indexed()) {
- return super.getSetQuery(parser, field, externalVal);
- }
- long[] values = new long[externalVal.size()];
- int i = 0;
- for (String val : externalVal) {
- values[i] = parseLongFromUser(field.getName(), val);
- i++;
+ public Query getSetQuery(QParser parser, SchemaField field,
Collection<String> externalVals) {
+ assert externalVals.size() > 0;
+ Query indexQuery = null;
+ long[] values = null;
+ if (field.indexed()) {
+ values = new long[externalVals.size()];
+ int i = 0;
+ for (String val : externalVals) {
+ values[i++] = parseLongFromUser(field.getName(), val);
+ }
+ indexQuery = LongPoint.newSetQuery(field.getName(), values);
}
if (field.hasDocValues()) {
- return LongField.newSetQuery(field.getName(), values);
+ long[] points;
+ if (values != null) {
+ points = values.clone();
Review Comment:
is the clone necessary?
##########
solr/core/src/java/org/apache/solr/search/TermsQParserPlugin.java:
##########
@@ -170,8 +171,13 @@ public Query parse() throws SyntaxError {
"Method '%s' not supported in TermsQParser when using
PointFields",
localParams.get(METHOD)));
}
- return ((PointField) ft)
- .getSetQuery(this, req.getSchema().getField(fname),
Arrays.asList(splitVals));
+ if (ft instanceof PointField pointField) {
+ return pointField.getSetQuery(
+ this, req.getSchema().getField(fname),
Arrays.asList(splitVals));
+ } else if (ft instanceof NumericField numericField) {
Review Comment:
this check is nonetheless underneath `if (ft.isPointField())` ~10 lines
above... so will it happen?
##########
solr/core/src/java/org/apache/solr/schema/LongField.java:
##########
@@ -0,0 +1,340 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.solr.schema;
+
+import java.util.Collection;
+import java.util.Collections;
+import java.util.List;
+import org.apache.lucene.document.Field;
+import org.apache.lucene.document.InvertableType;
+import org.apache.lucene.document.LongPoint;
+import org.apache.lucene.document.SortedNumericDocValuesField;
+import org.apache.lucene.document.StoredField;
+import org.apache.lucene.document.StoredValue;
+import org.apache.lucene.index.DocValuesType;
+import org.apache.lucene.index.IndexOptions;
+import org.apache.lucene.index.IndexableField;
+import org.apache.lucene.queries.function.ValueSource;
+import
org.apache.lucene.queries.function.valuesource.MultiValuedLongFieldSource;
+import org.apache.lucene.search.MatchNoDocsQuery;
+import org.apache.lucene.search.Query;
+import org.apache.lucene.search.SortedNumericSelector;
+import org.apache.lucene.util.BytesRef;
+import org.apache.lucene.util.BytesRefBuilder;
+import org.apache.solr.common.SolrException;
+import org.apache.solr.search.QParser;
+import org.apache.solr.uninverting.UninvertingReader.Type;
+
+/**
+ * An {@code NumericField} implementation of a field for {@code Long} values
using {@code
+ * LongPoint}, {@code StringField}, {@code SortedNumericDocValuesField} and
{@code StoredField}.
+ *
+ * @see PointField
+ * @see LongPoint
+ */
+public class LongField extends NumericField implements LongValueFieldType {
+
+ public LongField() {
+ type = NumberType.LONG;
+ }
+
+ @Override
+ public Object toNativeType(Object val) {
+ if (val == null) return null;
+ if (val instanceof Number) return ((Number) val).longValue();
+ if (val instanceof CharSequence) return Long.parseLong(val.toString());
+ return super.toNativeType(val);
+ }
+
+ @Override
+ public Query getPointFieldQuery(QParser parser, SchemaField field, String
value) {
+ return LongPoint.newExactQuery(field.getName(),
parseLongFromUser(field.getName(), value));
+ }
+
+ @Override
+ public Query getDocValuesFieldQuery(QParser parser, SchemaField field,
String value) {
+ return SortedNumericDocValuesField.newSlowExactQuery(
+ field.getName(), parseLongFromUser(field.getName(), value));
+ }
+
+ @Override
+ public Query getPointRangeQuery(
+ QParser parser,
+ SchemaField field,
+ String min,
+ String max,
+ boolean minInclusive,
+ boolean maxInclusive) {
+ long actualMin, actualMax;
+ if (min == null) {
+ actualMin = Long.MIN_VALUE;
+ } else {
+ actualMin = parseLongFromUser(field.getName(), min);
+ if (!minInclusive) {
+ if (actualMin == Long.MAX_VALUE) return new MatchNoDocsQuery();
+ ++actualMin;
+ }
+ }
+ if (max == null) {
+ actualMax = Long.MAX_VALUE;
+ } else {
+ actualMax = parseLongFromUser(field.getName(), max);
+ if (!maxInclusive) {
+ if (actualMax == Long.MIN_VALUE) return new MatchNoDocsQuery();
+ --actualMax;
+ }
+ }
+ return LongPoint.newRangeQuery(field.getName(), actualMin, actualMax);
+ }
+
+ @Override
+ public Query getDocValuesRangeQuery(
+ QParser parser,
+ SchemaField field,
+ String min,
+ String max,
+ boolean minInclusive,
+ boolean maxInclusive) {
+ long actualMin, actualMax;
+ if (min == null) {
+ actualMin = Long.MIN_VALUE;
+ } else {
+ actualMin = parseLongFromUser(field.getName(), min);
+ if (!minInclusive) {
+ if (actualMin == Long.MAX_VALUE) return new MatchNoDocsQuery();
+ ++actualMin;
+ }
+ }
+ if (max == null) {
+ actualMax = Long.MAX_VALUE;
+ } else {
+ actualMax = parseLongFromUser(field.getName(), max);
+ if (!maxInclusive) {
+ if (actualMax == Long.MIN_VALUE) return new MatchNoDocsQuery();
+ --actualMax;
+ }
+ }
+ return SortedNumericDocValuesField.newSlowRangeQuery(field.getName(),
actualMin, actualMax);
+ }
+
+ @Override
+ public Query getPointSetQuery(
+ QParser parser, SchemaField field, Collection<String> externalVals) {
+ long[] values = new long[externalVals.size()];
+ int i = 0;
+ for (String val : externalVals) {
+ values[i++] = parseLongFromUser(field.getName(), val);
+ }
+ return LongPoint.newSetQuery(field.getName(), values);
+ }
+
+ @Override
+ public Query getDocValuesSetQuery(
+ QParser parser, SchemaField field, Collection<String> externalVals) {
+ long[] points = new long[externalVals.size()];
+ int i = 0;
+ for (String val : externalVals) {
+ points[i++] = parseLongFromUser(field.getName(), val);
+ }
+ return SortedNumericDocValuesField.newSlowSetQuery(field.getName(),
points);
+ }
+
+ @Override
+ public Object toObject(SchemaField sf, BytesRef term) {
+ return LongPoint.decodeDimension(term.bytes, term.offset);
+ }
+
+ @Override
+ public Object toObject(IndexableField f) {
+ final StoredValue storedValue = f.storedValue();
+ if (storedValue != null) {
+ return storedValue.getLongValue();
+ }
+ final Number val = f.numericValue();
+ if (val != null) {
+ return val.longValue();
+ } else {
+ throw new AssertionError("Unexpected state. Field: '" + f + "'");
+ }
+ }
+
+ @Override
+ public String storedToReadable(IndexableField f) {
+ return Long.toString(f.storedValue().getLongValue());
+ }
+
+ @Override
+ protected String indexedToReadable(BytesRef indexedForm) {
+ return Long.toString(LongPoint.decodeDimension(indexedForm.bytes,
indexedForm.offset));
+ }
+
+ @Override
+ public void readableToIndexed(CharSequence val, BytesRefBuilder result) {
+ result.grow(Long.BYTES);
+ result.setLength(Long.BYTES);
+ LongPoint.encodeDimension(parseLongFromUser(null, val.toString()),
result.bytes(), 0);
+ }
+
+ @Override
+ public Type getUninversionType(SchemaField sf) {
+ if (sf.multiValued()) {
+ return null;
+ } else {
+ return Type.LONG_POINT;
+ }
+ }
+
+ @Override
+ public ValueSource getValueSource(SchemaField field, QParser qparser) {
+ field.checkFieldCacheSource();
+ return new MultiValuedLongFieldSource(field.getName(),
SortedNumericSelector.Type.MIN);
+ }
+
+ @Override
+ protected ValueSource getSingleValueSource(SortedNumericSelector.Type
choice, SchemaField f) {
+ return new MultiValuedLongFieldSource(f.getName(), choice);
+ }
+
+ @Override
+ public List<IndexableField> createFields(SchemaField sf, Object value) {
+ long longValue =
+ (value instanceof Number) ? ((Number) value).longValue() :
Long.parseLong(value.toString());
+ return Collections.singletonList(
+ new SolrLongField(
+ sf.getName(),
+ longValue,
+ sf.indexed(),
+ sf.enhancedIndex(),
+ sf.hasDocValues(),
+ sf.stored()));
+ }
+
+ @Override
+ public IndexableField createField(SchemaField field, Object value) {
+ long longValue =
+ (value instanceof Number) ? ((Number) value).longValue() :
Long.parseLong(value.toString());
+ return new LongPoint(field.getName(), longValue);
+ }
+
+ @Override
+ protected StoredField getStoredField(SchemaField sf, Object value) {
+ return new StoredField(sf.getName(), (Long) this.toNativeType(value));
+ }
+
+ /**
+ * Wrapper for {@link Long#parseLong(String)} that throws a BAD_REQUEST
error if the input is not
+ * valid
+ *
+ * @param fieldName used in any exception, may be null
+ * @param val string to parse, NPE if null
+ */
+ static long parseLongFromUser(String fieldName, String val) {
+ if (val == null) {
+ throw new NullPointerException(
+ "Invalid input" + (null == fieldName ? "" : " for field " +
fieldName));
+ }
+ try {
+ return Long.parseLong(val);
+ } catch (NumberFormatException e) {
+ String msg = "Invalid Number: " + val + (null == fieldName ? "" : " for
field " + fieldName);
+ throw new SolrException(SolrException.ErrorCode.BAD_REQUEST, msg);
+ }
+ }
+
+ static final class SolrLongField extends Field {
+
+ static org.apache.lucene.document.FieldType getType(
+ boolean rangeIndex, boolean termIndex, boolean docValues, boolean
stored) {
+ org.apache.lucene.document.FieldType type = new
org.apache.lucene.document.FieldType();
+ if (rangeIndex) {
+ type.setDimensions(1, Long.BYTES);
+ }
+ if (termIndex) {
+ type.setIndexOptions(IndexOptions.DOCS);
+ }
+ if (docValues) {
Review Comment:
I suppose there's an implicit assumption here, that SORTED_NUMERIC with only
single values is just as efficient to index & query when it's only populated
with one value. This should be stated.
##########
solr/core/src/java/org/apache/solr/schema/FieldType.java:
##########
@@ -547,10 +552,14 @@ public boolean incrementToken() throws IOException {
return false;
}
- if (isPointField()) {
+ if (FieldType.this instanceof PointField) {
Review Comment:
perhaps this multi-way `if` should be pulled out so the field type can
override it. instanceof checks is a smell that we aren't using
object-orientation that we have right here.
##########
solr/core/src/java/org/apache/solr/handler/component/ExpandComponent.java:
##########
@@ -869,9 +912,17 @@ private static String numericToString(FieldType fieldType,
long val) {
case LONG:
return Long.toString(val);
case FLOAT:
- return Float.toString(Float.intBitsToFloat((int) val));
+ if (fieldType instanceof PointField || fieldType instanceof
TrieField) {
+ return Float.toString(Float.intBitsToFloat((int) val));
+ } else {
+ return Float.toString(NumericUtils.sortableIntToFloat((int) val));
Review Comment:
how did this end up happening?
##########
solr/core/src/java/org/apache/solr/request/IntervalFacets.java:
##########
@@ -267,22 +206,53 @@ private void getCountMultiValuedNumeric() throws
IOException {
final Iterator<LeafReaderContext> ctxIt = leaves.iterator();
LeafReaderContext ctx = null;
- SortedNumericDocValues longs = null;
+ DocIdSetIterator longs = null;
+ IORunnable accumulate = null;
for (DocIterator docsIt = docs.iterator(); docsIt.hasNext(); ) {
final int doc = docsIt.nextDoc();
if (ctx == null || doc >= ctx.docBase + ctx.reader().maxDoc()) {
do {
ctx = ctxIt.next();
} while (ctx == null || doc >= ctx.docBase + ctx.reader().maxDoc());
assert doc >= ctx.docBase;
- longs = DocValues.getSortedNumeric(ctx.reader(), fieldName);
+ final SortedNumericDocValues sortedNumeric =
+ DocValues.getSortedNumeric(ctx.reader(), fieldName);
+ NumericDocValues numeric = DocValues.unwrapSingleton(sortedNumeric);
+ if (numeric != null) {
+ if (!schemaField.multiValued() && !(schemaField.getType() instanceof
NumericField)) {
+ if (schemaField.getType().getNumberType() == NumberType.FLOAT) {
Review Comment:
a little comment on this condition/scenario would be helpful. (XXXX
requires int translation)
##########
solr/core/src/java/org/apache/solr/schema/FieldType.java:
##########
@@ -857,7 +866,7 @@ protected SortField getStringSort(SchemaField field,
boolean reverse) {
* @see #getSortField
*/
protected SortField getNumericSort(SchemaField field, NumberType type,
boolean reverse) {
- if (field.multiValued()) {
+ if (field.multiValued() || this instanceof NumericField) {
MultiValueSelector selector =
field.type.getDefaultMultiValueSelectorForSort(field, reverse);
Review Comment:
perhaps we can just call this always and not check the type? If it will
return null when we don't need it.
##########
solr/core/src/java/org/apache/solr/schema/LongPointField.java:
##########
@@ -103,27 +104,39 @@ public Object toObject(IndexableField f) {
}
@Override
- protected Query getExactQuery(SchemaField field, String externalVal) {
- return LongPoint.newExactQuery(
- field.getName(), parseLongFromUser(field.getName(), externalVal));
- }
-
- @Override
- public Query getSetQuery(QParser parser, SchemaField field,
Collection<String> externalVal) {
- assert externalVal.size() > 0;
- if (!field.indexed()) {
- return super.getSetQuery(parser, field, externalVal);
- }
- long[] values = new long[externalVal.size()];
- int i = 0;
- for (String val : externalVal) {
- values[i] = parseLongFromUser(field.getName(), val);
- i++;
+ public Query getSetQuery(QParser parser, SchemaField field,
Collection<String> externalVals) {
+ assert externalVals.size() > 0;
+ Query indexQuery = null;
+ long[] values = null;
+ if (field.indexed()) {
+ values = new long[externalVals.size()];
+ int i = 0;
+ for (String val : externalVals) {
+ values[i++] = parseLongFromUser(field.getName(), val);
+ }
+ indexQuery = LongPoint.newSetQuery(field.getName(), values);
}
if (field.hasDocValues()) {
- return LongField.newSetQuery(field.getName(), values);
+ long[] points;
+ if (values != null) {
+ points = values.clone();
+ } else {
+ points = new long[externalVals.size()];
+ int i = 0;
+ for (String val : externalVals) {
+ points[i++] = parseLongFromUser(field.getName(), val);
+ }
+ }
+ Query docValuesQuery =
SortedNumericDocValuesField.newSlowSetQuery(field.getName(), points);
+ if (indexQuery != null) {
+ return new IndexOrDocValuesQuery(indexQuery, docValuesQuery);
+ } else {
+ return docValuesQuery;
+ }
+ } else if (indexQuery != null) {
+ return indexQuery;
} else {
- return LongPoint.newSetQuery(field.getName(), values);
+ return super.getSetQuery(parser, field, externalVals);
Review Comment:
I wonder if this scenario is actually valid; could we return null or
MatchNoDocsQuery?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
