gerlowskija commented on code in PR #4645:
URL: https://github.com/apache/solr/pull/4645#discussion_r3618929998
##########
solr/core/src/java/org/apache/solr/schema/FieldType.java:
##########
@@ -351,6 +351,36 @@ public List<IndexableField> createFields(SchemaField
field, Object value) {
return f == null ? List.of() : List.of(f);
}
+ /**
+ * Whether this type needs all values of a multiValued field together
(rather than one value at a
+ * time) in order to build its indexable fields - for example to pack
several ranges into a single
+ * {@code BinaryDocValues} blob. When {@code true}, {@link
org.apache.solr.update.DocumentBuilder}
+ * calls {@link #createFieldsFromAllValues(SchemaField, Collection)} once
per (multiValued) field
+ * instead of {@link #createFields(SchemaField, Object)} per value.
+ */
+ public boolean createsFieldsFromAllValues() {
Review Comment:
[-0] IMO the code that uses these new methods would be easier to grok if it
started with "should" or "is" or some other question-word that suggests that
the method returns a boolean. Maybe, `shouldBatchProcessFieldValues()` or
something similar? Wdyt?
##########
solr/core/src/java/org/apache/solr/schema/numericrange/AbstractNumericRangeField.java:
##########
@@ -134,13 +141,20 @@ protected Pattern getSingleBoundPattern() {
@Override
protected boolean enableDocValuesByDefault() {
- return false; // Range fields do not support docValues
+ // DocValues are supported for both single and multiValued range fields,
but are opt-in: they
+ // add an extra (binary) docValues field, and range docValues only help
queries that combine
+ // the range clause with a more selective clause, so we don't enable them
by default.
+ return false;
}
@Override
protected void init(IndexSchema schema, Map<String, String> args) {
super.init(schema, args);
+ // Force useDocValuesAsStored off; it otherwise defaults on for schema,
which
+ // would make fl=* responses fail on a docValues-enabled range field.
+ properties &= ~USE_DOCVALUES_AS_STORED;
Review Comment:
[Q] Why are these fields incompatible with useDocValuesAsStored?
If it's a hard restriction we should probably throw an exception or log a
warning or something, rather than just quietly overriding what the user
explicitly requested.
##########
solr/core/src/test/org/apache/solr/search/numericrange/NumericRangeMultiValuedDocValuesTest.java:
##########
@@ -0,0 +1,184 @@
+/*
+ * 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.search.numericrange;
+
+import java.util.ArrayList;
+import java.util.List;
+import org.apache.solr.SolrTestCaseJ4;
+import org.junit.BeforeClass;
+import org.junit.Test;
+
+/**
+ * Integration tests for multiValued range fields with docValues (backed by
SortedSet docValues).
Review Comment:
[Q] I'm not totally clear on the difference between this test-class and
NumericRangeDocValuesTest. Are the test cases different between the two? Or
are they near duplicates with the multivalued flag in the schema types being
the primary difference?
[-1] We're not actually using SortedSet docValues as this comment claims,
are we?
##########
solr/benchmark/src/java/org/apache/solr/bench/search/RangeSearch.java:
##########
Review Comment:
[-0] I think most folks' association with "range search" is using a range
query to search non-range data (i.e. `price_i:[0 TO 100]`.
Wdyt of renaming to be more specific: eg. RangeFieldSearch,
DocValuesRangeFieldSearch...something along those lines?
##########
solr/core/src/java/org/apache/solr/schema/numericrange/AbstractNumericRangeField.java:
##########
@@ -353,7 +453,10 @@ public Query getFieldQuery(QParser parser, SchemaField
field, String externalVal
+ ")");
}
- return newContainsQuery(field.getName(), singleBoundRange);
+ // A single bound is the degenerate range [p,p]; "contains p" is
equivalent to
Review Comment:
[Q] Can you clarify this a bit? I've poked around the `newIntersectsQuery`
and `newContainsQuery` implementations for (e.g.) IntRangeField, and afaict
they both have similar optimizations in terms of how they use
IndexOrDocValuesQuery. Is `newIntersectsQuery` really more optimized than
`newContainsQuery` in some way?
##########
solr/core/src/java/org/apache/solr/schema/numericrange/MultiBinaryRangeDocValuesQuery.java:
##########
@@ -0,0 +1,172 @@
+/*
+ * 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.numericrange;
+
+import java.io.IOException;
+import java.util.Arrays;
+import java.util.Objects;
+import org.apache.lucene.document.RangeFieldQuery;
+import org.apache.lucene.index.BinaryDocValues;
+import org.apache.lucene.index.DocValues;
+import org.apache.lucene.index.LeafReader;
+import org.apache.lucene.index.LeafReaderContext;
+import org.apache.lucene.search.ConstantScoreScorer;
+import org.apache.lucene.search.ConstantScoreWeight;
+import org.apache.lucene.search.IndexSearcher;
+import org.apache.lucene.search.Query;
+import org.apache.lucene.search.QueryVisitor;
+import org.apache.lucene.search.ScoreMode;
+import org.apache.lucene.search.Scorer;
+import org.apache.lucene.search.ScorerSupplier;
+import org.apache.lucene.search.TwoPhaseIterator;
+import org.apache.lucene.search.Weight;
+import org.apache.lucene.util.ArrayUtil;
+import org.apache.lucene.util.ArrayUtil.ByteArrayComparator;
+import org.apache.lucene.util.BytesRef;
+
+/**
+ * A docValues-backed range query over {@link BinaryDocValues} for numeric
range field types
+ * (single- or multiValued).
+ *
+ * <p>A document's range(s) are packed into a single {@code BinaryDocValues}
value as one or more
+ * concatenated fixed-width {@code [min... | max...]} blobs (see {@code
+ * AbstractNumericRangeField.encodePackedValues}). Because every range for a
field is the same width
+ * ({@code stride = 2 * numDims * bytesPerDim}), the count is recovered as
{@code value.length /
+ * stride}.
+ *
+ * <p>Reading the flat per-doc blob avoids the dictionary/ordinal dereference
a SortedSet-backed
+ * docValues range query would pay, so it stays cheap even for
high-cardinality range data. It is a
+ * candidate to push down to Lucene, which currently lacks a multi-valued
binary range docValues
+ * field.
+ */
+final class MultiBinaryRangeDocValuesQuery extends Query {
+
+ private final String field;
+ private final byte[] queryPackedValue;
+ private final int numDims;
+ private final int bytesPerDim;
+ private final RangeFieldQuery.QueryType queryType;
+ private final ByteArrayComparator comparator;
+
+ /**
+ * @param field the field name
+ * @param queryPackedValue the query range, encoded as {@code [min... |
max...]} the same way the
+ * indexed ranges are
+ * @param numDims number of dimensions (1-4)
+ * @param bytesPerDim bytes per dimension value (e.g. {@code Integer.BYTES})
+ * @param queryType the relation to test ({@code INTERSECTS}, {@code
CONTAINS}, {@code WITHIN},
+ * {@code CROSSES})
+ */
+ MultiBinaryRangeDocValuesQuery(
+ String field,
+ byte[] queryPackedValue,
+ int numDims,
+ int bytesPerDim,
+ RangeFieldQuery.QueryType queryType) {
+ this.field = Objects.requireNonNull(field, "field must not be null");
+ this.queryPackedValue =
+ Objects.requireNonNull(queryPackedValue, "queryPackedValue must not be
null");
+ this.numDims = numDims;
+ this.bytesPerDim = bytesPerDim;
+ this.queryType = Objects.requireNonNull(queryType, "queryType must not be
null");
+ this.comparator = ArrayUtil.getUnsignedComparator(bytesPerDim);
+ }
+
+ @Override
+ public Weight createWeight(IndexSearcher searcher, ScoreMode scoreMode,
float boost) {
+ return new ConstantScoreWeight(this, boost) {
+ @Override
+ public ScorerSupplier scorerSupplier(LeafReaderContext context) throws
IOException {
+ LeafReader reader = context.reader();
+ if (reader.getFieldInfos().fieldInfo(field) == null) {
+ return null;
+ }
+ final BinaryDocValues values = DocValues.getBinary(reader, field);
+ final int stride = 2 * numDims * bytesPerDim;
+ final byte[] docPacked = new byte[stride];
+ final TwoPhaseIterator iterator =
+ new TwoPhaseIterator(values) {
+ @Override
+ public boolean matches() throws IOException {
+ // The doc's value is N concatenated stride-byte ranges; match
if ANY satisfies.
+ // Copy each range to a 0-based buffer because
QueryType.matches indexes from 0.
+ BytesRef b = values.binaryValue();
+ for (int off = b.offset; off < b.offset + b.length; off +=
stride) {
+ System.arraycopy(b.bytes, off, docPacked, 0, stride);
+ if (queryType.matches(
+ queryPackedValue, docPacked, numDims, bytesPerDim,
comparator)) {
+ return true;
+ }
+ }
+ return false;
+ }
+
+ @Override
+ public float matchCost() {
+ return stride; // ~ one packed-value comparison per range
+ }
+ };
+
+ Scorer scorer = new ConstantScoreScorer(score(), scoreMode, iterator);
+ return new DefaultScorerSupplier(scorer);
+ }
+
+ @Override
+ public boolean isCacheable(LeafReaderContext ctx) {
+ return DocValues.isCacheable(ctx, field);
+ }
+ };
+ }
+
+ @Override
+ public void visit(QueryVisitor visitor) {
+ if (visitor.acceptField(field)) {
+ visitor.visitLeaf(this);
+ }
+ }
+
+ @Override
+ public String toString(String f) {
+ return getClass().getSimpleName() + "(field=" + field + ", type=" +
queryType + ")";
Review Comment:
[0] Is it worth string-ifying the value being searched for and including it
in the returned string?
##########
solr/core/src/java/org/apache/solr/schema/numericrange/AbstractNumericRangeField.java:
##########
@@ -134,13 +141,20 @@ protected Pattern getSingleBoundPattern() {
@Override
protected boolean enableDocValuesByDefault() {
- return false; // Range fields do not support docValues
+ // DocValues are supported for both single and multiValued range fields,
but are opt-in: they
Review Comment:
[0] IMO these are worth enabling by default, for two reasons:
1. Solr has been moving towards "docValues by default" for awhile
([example](https://solr.apache.org/guide/solr/latest/upgrade-notes/major-changes-in-solr-9.html#schemaversion-upgraded-to-1-7)).
2. Many users would probably have a hard time determining at schema-design
time whether a given field will be used with a filter or not. It's probably
better to give these folks the optimization by default. They can always
reverse-course and reindex if they decide that they don't want to pay the
disk-space penalty.
##########
solr/core/src/test/org/apache/solr/search/numericrange/NumericRangeDocValuesTest.java:
##########
@@ -0,0 +1,272 @@
+/*
+ * 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.search.numericrange;
+
+import java.util.List;
+import org.apache.solr.SolrTestCaseJ4;
+import org.apache.solr.common.SolrException;
+import org.junit.BeforeClass;
+import org.junit.Test;
+
+/**
+ * Integration tests over the {@code schema-numericrange.xml} verifying that
the docValues-enabled
+ * range fields return exactly the same results as their indexed-only
counterparts.
+ *
+ * <p>The docValues-enabled fields wrap their BKD query in a Lucene {@link
+ * org.apache.lucene.search.IndexOrDocValuesQuery}. These tests assert that
this produces identical
+ * matches to the direct BKD query across intersects / contains / single-bound
/ multi-dimensional
+ * queries, inside a selective conjunction, and for the docValues-only
(non-indexed) field.
+ */
+public class NumericRangeDocValuesTest extends SolrTestCaseJ4 {
Review Comment:
[-0] Many of the tests in this class are similar in some ways to the
test-cases already present in `NumericRangeQParserPluginIntTest` and its
siblings.
I wonder if the classes could be combined/merged by making better use of
randomization. i.e. Have each test-case randomly choose whether it'll index
and query DV or non-DV types.
##########
solr/core/src/test/org/apache/solr/schema/numericrange/MultiBinaryRangeDocValuesQueryTest.java:
##########
@@ -0,0 +1,217 @@
+/*
+ * 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.numericrange;
+
+import java.io.IOException;
+import java.util.Arrays;
+import java.util.Collection;
+import org.apache.lucene.document.BinaryDocValuesField;
+import org.apache.lucene.document.Document;
+import org.apache.lucene.document.IntRange;
+import org.apache.lucene.document.IntRangeDocValuesField;
+import org.apache.lucene.document.RangeFieldQuery.QueryType;
+import org.apache.lucene.index.IndexReader;
+import org.apache.lucene.index.LeafReaderContext;
+import org.apache.lucene.search.CollectorManager;
+import org.apache.lucene.search.IndexSearcher;
+import org.apache.lucene.search.Query;
+import org.apache.lucene.search.ScoreMode;
+import org.apache.lucene.search.SimpleCollector;
+import org.apache.lucene.store.Directory;
+import org.apache.lucene.tests.index.RandomIndexWriter;
+import org.apache.lucene.tests.util.TestUtil;
+import org.apache.lucene.util.BytesRef;
+import org.apache.lucene.util.BytesRefBuilder;
+import org.apache.lucene.util.FixedBitSet;
+import org.apache.solr.SolrTestCase;
+import org.junit.Test;
+
+/**
+ * Unit tests for {@link MultiBinaryRangeDocValuesQuery}, the binary
docValues-backed range
+ * verifier.
+ *
+ * <p>Each doc indexes its range(s) both as BKD ({@link IntRange}) and as a
single {@code
+ * BinaryDocValues} blob (all ranges concatenated, as {@code
AbstractNumericRangeField} does),
+ * letting us assert the docValues verifier returns exactly the same documents
as the trusted BKD
+ * query across all four relations - for both single-valued docs (one range)
and multiValued docs
+ * (several ranges in one blob), where "any range matches" (not the union of
ranges) is correct.
+ */
+public class MultiBinaryRangeDocValuesQueryTest extends SolrTestCase {
+
+ private static final String FIELD = "r";
+
+ /** Encodes a 1-D query range the same way indexed ranges are encoded. */
+ private static byte[] encode(int min, int max) {
+ BytesRef b = new IntRangeDocValuesField(FIELD, new int[] {min}, new int[]
{max}).binaryValue();
+ return Arrays.copyOfRange(b.bytes, b.offset, b.offset + b.length);
+ }
+
+ private static Query dv(QueryType type, int min, int max) {
+ return new MultiBinaryRangeDocValuesQuery(FIELD, encode(min, max), 1,
IntRange.BYTES, type);
+ }
+
+ private static Query bkd(QueryType type, int min, int max) {
+ int[] mn = {min};
+ int[] mx = {max};
+ return switch (type) {
+ case INTERSECTS -> IntRange.newIntersectsQuery(FIELD, mn, mx);
+ case CONTAINS -> IntRange.newContainsQuery(FIELD, mn, mx);
+ case WITHIN -> IntRange.newWithinQuery(FIELD, mn, mx);
+ case CROSSES -> IntRange.newCrossesQuery(FIELD, mn, mx);
+ };
+ }
+
+ /**
+ * Adds a document whose (possibly multiple) ranges are indexed as BKD and
as one BinaryDocValues
+ * blob holding every range concatenated (a single-range doc is just the
one-range case).
+ */
+ private static void addDoc(RandomIndexWriter w, int[]... ranges) throws
IOException {
+ Document doc = new Document();
+ BytesRefBuilder blob = new BytesRefBuilder();
+ for (int[] r : ranges) {
+ int[] mn = {r[0]};
+ int[] mx = {r[1]};
+ doc.add(new IntRange(FIELD, mn, mx)); // BKD (ground truth)
+ blob.append(new IntRangeDocValuesField(FIELD, mn, mx).binaryValue()); //
concat into one blob
+ }
+ doc.add(new BinaryDocValuesField(FIELD, blob.toBytesRef()));
+ w.addDocument(doc);
+ }
+
+ private static FixedBitSet matches(IndexSearcher s, Query q) throws
IOException {
+ int maxDoc = Math.max(1, s.getIndexReader().maxDoc());
+ return s.search(
+ q,
+ new CollectorManager<BitSetCollector, FixedBitSet>() {
+ @Override
+ public BitSetCollector newCollector() {
+ return new BitSetCollector(maxDoc);
+ }
+
+ @Override
+ public FixedBitSet reduce(Collection<BitSetCollector> collectors) {
+ FixedBitSet merged = new FixedBitSet(maxDoc);
+ for (BitSetCollector c : collectors) {
+ merged.or(c.bits);
+ }
+ return merged;
+ }
+ });
+ }
+
+ /**
+ * Collects matching doc ids (segment-local -> absolute) into a bitset,
per CollectorManager.
+ */
+ private static final class BitSetCollector extends SimpleCollector {
+ final FixedBitSet bits;
+ int base;
+
+ BitSetCollector(int maxDoc) {
+ this.bits = new FixedBitSet(maxDoc);
+ }
+
+ @Override
+ public void collect(int doc) {
+ bits.set(base + doc);
+ }
+
+ @Override
+ protected void doSetNextReader(LeafReaderContext ctx) {
+ base = ctx.docBase;
+ }
+
+ @Override
+ public ScoreMode scoreMode() {
+ return ScoreMode.COMPLETE_NO_SCORES;
+ }
+ }
+
+ @Test
+ public void testMultiValuedPerRangeSemantics() throws Exception {
Review Comment:
[-0] Most Solr tests, even for relatively "low level" things like Query
implementations, test them by using the available Solr abstractions: create a
Solr collection, index your docs with SolrJ or helper methods, query with SolrJ
or helper methods.
I'd recommend reworking this test to align a bit more closely with that
approach. There's nothing wrong with the direct-Lucene approach you picked
here, but the Solr idioms are more familiar to the devs on this side so it'll
make your test easier for everyone to grok and maintain going forward...
##########
solr/core/src/test-files/solr/collection1/conf/schema-numericrange.xml:
##########
@@ -105,6 +105,24 @@
<!-- 4D DoubleRangeField (tesseract) -->
<field name="double_range_4d" type="doublerange4d" indexed="true"
stored="true"/>
+ <!-- docValues-enabled variants (single-valued) exercising the
IndexOrDocValuesQuery optimization -->
+ <field name="price_range_dv" type="intrange" indexed="true" stored="true"
docValues="true"/>
+ <field name="bbox_dv" type="intrange2d" indexed="true" stored="true"
docValues="true"/>
+ <field name="long_range_dv" type="longrange" indexed="true" stored="true"
docValues="true"/>
+ <field name="float_range_dv" type="floatrange" indexed="true" stored="true"
docValues="true"/>
+ <field name="double_range_dv" type="doublerange" indexed="true"
stored="true" docValues="true"/>
+
+ <!-- docValues-only (not indexed) intrange, exercising the DV-only query
path -->
+ <field name="price_range_dvonly" type="intrange" indexed="false"
stored="false" docValues="true"/>
+
+ <!-- multiValued docValues variants (SortedSet-backed): exercise multiple
ranges per document -->
Review Comment:
[-1] This comment is incorrect right? AFAICT in this PR all of the DV
support relies on the BinaryDocValues type, not SortedSet. Or did I miss
something?
##########
solr/core/src/test/org/apache/solr/search/numericrange/NumericRangeDocValuesTest.java:
##########
@@ -0,0 +1,272 @@
+/*
+ * 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.search.numericrange;
+
+import java.util.List;
+import org.apache.solr.SolrTestCaseJ4;
+import org.apache.solr.common.SolrException;
+import org.junit.BeforeClass;
+import org.junit.Test;
+
+/**
+ * Integration tests over the {@code schema-numericrange.xml} verifying that
the docValues-enabled
Review Comment:
```suggestion
* Integration tests over {@code schema-numericrange.xml} verifying that the
docValues-enabled
```
##########
solr/solr-ref-guide/modules/indexing-guide/pages/field-types-included-with-solr.adoc:
##########
@@ -51,17 +51,17 @@ The
{solr-javadocs}/core/org/apache/solr/schema/package-summary.html[`org.apache
|IntPointField |Integer field (32-bit signed integer). This class encodes int
values using a "Dimensional Points" based data structure that allows for very
efficient searches for specific values, or ranges of values. For single valued
fields, `docValues="true"` must be used to enable sorting.
-|IntRangeField |Stores single or multi-dimensional ranges, using syntax like
`[1 TO 4]` or `[1,2 TO 3,4]`. Up to 4 dimensions are supported.
Dimensionality is specified on new field-types using a `numDimensions`
property, and all values for a particular field must have exactly this number
of dimensions. Field type does not support docValues. Typically queried using
the xref:query-guide:other-parsers.adoc#numeric-range-query-parser[Numeric
Range Query Parser], though the Lucene and other query parsers also support
this field by assuming "contains" semantics for searches.
+|IntRangeField |Stores single or multi-dimensional ranges, using syntax like
`[1 TO 4]` or `[1,2 TO 3,4]`. Up to 4 dimensions are supported.
Dimensionality is specified on new field-types using a `numDimensions`
property, and all values for a particular field must have exactly this number
of dimensions. Field type supports `docValues="true"` (with
`multiValued="true"` or `"false"`) as a query-time filter optimization.
Typically queried using the
xref:query-guide:other-parsers.adoc#numeric-range-query-parser[Numeric Range
Query Parser], though the Lucene and other query parsers also support this
field by assuming "contains" semantics for searches.
Review Comment:
[0] Most readers will assume that sorting+faceting are possible whenever
they see that a field supports docValues. It might be worth calling that out
here?
##########
solr/core/src/test/org/apache/solr/search/numericrange/NumericRangeMultiValuedDocValuesTest.java:
##########
@@ -0,0 +1,184 @@
+/*
+ * 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.search.numericrange;
+
+import java.util.ArrayList;
+import java.util.List;
+import org.apache.solr.SolrTestCaseJ4;
+import org.junit.BeforeClass;
+import org.junit.Test;
+
+/**
+ * Integration tests for multiValued range fields with docValues (backed by
SortedSet docValues).
+ *
+ * <p>A document may hold several ranges. The docValues verifier ({@code
+ * MultiBinaryRangeDocValuesQuery}) matches a document when <em>any</em> of
its ranges satisfies the
+ * relation, the same "any value matches" semantics as multiValued Points
(BKD). Each test indexes
+ * the same ranges into a multiValued BKD-only field ({@code *_multi}) and a
multiValued docValues
+ * field ({@code *_mv_dv}) and asserts they return identical counts, so the
docValues path is
+ * checked against the trusted BKD path.
+ *
+ * <p>That these fields load at all also verifies that {@code multiValued=true
docValues=true} is
+ * now accepted for range field types.
+ */
+public class NumericRangeMultiValuedDocValuesTest extends SolrTestCaseJ4 {
+
+ @BeforeClass
+ public static void beforeClass() throws Exception {
+ initCore("solrconfig.xml", "schema-numericrange.xml");
+ }
+
+ @Override
+ public void setUp() throws Exception {
+ super.setUp();
+ clearIndex();
+ assertU(commit());
+ }
+
+ /** Indexes the given ranges into both a BKD-only and a docValues
multiValued field. */
+ private void addMvDoc(String id, String bkdField, String dvField, String...
ranges) {
+ List<String> fieldsAndValues = new ArrayList<>();
+ fieldsAndValues.add("id");
+ fieldsAndValues.add(id);
+ for (String r : ranges) {
+ fieldsAndValues.add(bkdField);
+ fieldsAndValues.add(r);
+ fieldsAndValues.add(dvField);
+ fieldsAndValues.add(r);
+ }
+ assertU(adoc(fieldsAndValues.toArray(new String[0])));
+ }
+
+ /** Asserts the BKD field and the docValues field return the same (expected)
count. */
+ private void assertSameCount(
+ String criteria, String range, String bkdField, String dvField, int
expected) {
+ for (String f : new String[] {bkdField, dvField}) {
+ assertQ(
+ "criteria=" + criteria + " field=" + f + " " + range,
+ req("q", "{!numericRange criteria=" + criteria + " field=" + f + "}"
+ range),
+ "//result[@numFound='" + expected + "']");
+ }
+ }
+
+ @Test
+ public void testContainsPerRangeNotUnion() {
+ // doc 1: two overlapping ranges whose *union* is [1,20]
+ addMvDoc("1", "price_range_multi", "price_range_mv_dv", "[1 TO 10]", "[5
TO 20]");
+ // doc 2: two disjoint ranges
+ addMvDoc("2", "price_range_multi", "price_range_mv_dv", "[0 TO 5]", "[100
TO 200]");
+ assertU(commit());
+
+ // Neither individual range of doc 1 contains [1,15]
+ assertSameCount("contains", "[1 TO 15]", "price_range_multi",
"price_range_mv_dv", 0);
Review Comment:
[0] 'assertSameCount' might blithely pass even if a totally different set of
documents matched than what you intended. If we're going to keep this test
class around, it might be worth asserting on the specific doc IDs that are
returned.
##########
solr/core/src/test/org/apache/solr/search/numericrange/NumericRangeDocValuesTest.java:
##########
@@ -0,0 +1,272 @@
+/*
+ * 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.search.numericrange;
+
+import java.util.List;
+import org.apache.solr.SolrTestCaseJ4;
+import org.apache.solr.common.SolrException;
+import org.junit.BeforeClass;
+import org.junit.Test;
+
+/**
+ * Integration tests over the {@code schema-numericrange.xml} verifying that
the docValues-enabled
+ * range fields return exactly the same results as their indexed-only
counterparts.
+ *
+ * <p>The docValues-enabled fields wrap their BKD query in a Lucene {@link
+ * org.apache.lucene.search.IndexOrDocValuesQuery}. These tests assert that
this produces identical
+ * matches to the direct BKD query across intersects / contains / single-bound
/ multi-dimensional
+ * queries, inside a selective conjunction, and for the docValues-only
(non-indexed) field.
+ */
+public class NumericRangeDocValuesTest extends SolrTestCaseJ4 {
+
+ @BeforeClass
+ public static void beforeClass() throws Exception {
+ initCore("solrconfig.xml", "schema-numericrange.xml");
+ }
+
+ @Override
+ public void setUp() throws Exception {
+ super.setUp();
+ clearIndex();
+ assertU(commit());
+ }
+
+ /**
+ * Index the same int range into the indexed-only, indexed+docValues, and
docValues-only fields.
+ */
+ private void addIntDoc(String id, String range) {
+ assertU(
+ adoc(
+ "id", id,
+ "price_range", range,
+ "price_range_dv", range,
+ "price_range_dvonly", range));
+ }
+
+ private List<String> get1dIntRangeFieldNames() {
+ return List.of("price_range", "price_range_dv", "price_range_dvonly");
+ }
+
+ @Test
+ public void testIntersectsParity() {
+ addIntDoc("1", "[100 TO 200]");
+ addIntDoc("2", "[150 TO 250]");
+ addIntDoc("3", "[50 TO 80]");
+ assertU(commit());
+
+ // intersects [120 TO 180] -> docs 1 and 2, for every field variant
+ for (String field : get1dIntRangeFieldNames()) {
+ assertQ(
+ "intersects on " + field,
+ req("q", "{!numericRange criteria=intersects field=" + field +
"}[120 TO 180]"),
+ "//result[@numFound='2']",
+ "//result/doc/str[@name='id'][.='1']",
+ "//result/doc/str[@name='id'][.='2']");
+ }
+ }
+
+ @Test
+ public void testContainsParity() {
+ addIntDoc("1", "[100 TO 200]"); // fully contains [120 TO 180]
+ addIntDoc("2", "[130 TO 160]"); // within, does NOT contain
+ addIntDoc("3", "[150 TO 250]"); // partial overlap, does NOT contain
+ assertU(commit());
+
+ for (String field : get1dIntRangeFieldNames()) {
+ assertQ(
+ "contains on " + field,
+ req("q", "{!numericRange criteria=contains field=" + field + "}[120
TO 180]"),
+ "//result[@numFound='1']",
+ "//result/doc/str[@name='id'][.='1']");
+ }
+ }
+
+ @Test
+ public void testStandardSyntaxContainsParity() {
+ addIntDoc("1", "[100 TO 200]"); // contains [120 TO 180]
+ addIntDoc("2", "[130 TO 160]"); // no
+ assertU(commit());
+
+ for (String field : get1dIntRangeFieldNames()) {
+ assertQ(
+ "standard-syntax contains on " + field,
+ req("q", field + ":[120 TO 180]"),
+ "//result[@numFound='1']",
+ "//result/doc/str[@name='id'][.='1']");
+ }
+ }
+
+ @Test
+ public void testSingleBoundParity() {
+ // A single bound (field:150) is a degenerate range [150,150]; contains ==
intersects for a
+ // point.
+ addIntDoc("1", "[100 TO 200]"); // contains 150
+ addIntDoc("2", "[150 TO 150]"); // contains 150
+ addIntDoc("3", "[100 TO 149]"); // no (max below 150)
+ addIntDoc("4", "[151 TO 300]"); // no (min above 150)
+ assertU(commit());
+
+ for (String f : get1dIntRangeFieldNames()) {
+ assertQ(
+ "single-bound on " + f,
+ req("q", f + ":150"),
+ "//result[@numFound='2']",
+ "//result/doc/str[@name='id'][.='1']",
+ "//result/doc/str[@name='id'][.='2']");
+ }
+ }
+
+ @Test
+ public void test2DContainsParity() {
+ assertU(adoc("id", "1", "bbox", "[0,0 TO 10,10]", "bbox_dv", "[0,0 TO
10,10]")); // contains
+ assertU(adoc("id", "2", "bbox", "[4,4 TO 6,6]", "bbox_dv", "[4,4 TO
6,6]")); // does not contain
+ assertU(commit());
+
+ for (String f : new String[] {"bbox", "bbox_dv"}) {
+ assertQ(
+ "2D contains on " + f,
+ req("q", "{!numericRange criteria=contains field=" + f + "}[3,3 TO
7,7]"),
+ "//result[@numFound='1']",
+ "//result/doc/str[@name='id'][.='1']");
+ }
+ }
+
+ @Test
+ public void testSelectiveConjunction() {
+ // Exercises IndexOrDocValuesQuery inside a conjunction: the selective
title clause leads and
+ // the docValues range clause verifies. Must match the equivalent
non-docValues query exactly.
+ assertU(
+ adoc(
+ "id",
+ "1",
+ "title",
+ "match",
+ "price_range",
+ "[100 TO 200]",
+ "price_range_dv",
+ "[100 TO 200]"));
+ assertU(
+ adoc(
+ "id",
+ "2",
+ "title",
+ "match",
+ "price_range",
+ "[300 TO 400]",
+ "price_range_dv",
+ "[300 TO 400]"));
+ assertU(
+ adoc(
+ "id",
+ "3",
+ "title",
+ "other",
+ "price_range",
+ "[100 TO 200]",
+ "price_range_dv",
+ "[100 TO 200]"));
+ assertU(commit());
+
+ for (String f : new String[] {"price_range", "price_range_dv"}) {
+ assertQ(
+ "selective AND contains on " + f,
+ req("q", "+title:match +" + f + ":[120 TO 180]"),
+ "//result[@numFound='1']",
+ "//result/doc/str[@name='id'][.='1']");
+ }
+ }
+
+ @Test
+ public void testLongParity() {
+ assertU(adoc("id", "1", "long_range", "[100 TO 200]", "long_range_dv",
"[100 TO 200]"));
+ assertU(adoc("id", "2", "long_range", "[150 TO 250]", "long_range_dv",
"[150 TO 250]"));
+ assertU(adoc("id", "3", "long_range", "[50 TO 80]", "long_range_dv", "[50
TO 80]"));
+ assertU(commit());
+
+ for (String f : new String[] {"long_range", "long_range_dv"}) {
+ assertQ(
+ "long intersects on " + f,
+ req("q", "{!numericRange criteria=intersects field=" + f + "}[120 TO
180]"),
+ "//result[@numFound='2']");
+ }
+ }
+
+ @Test
+ public void testFloatIntersectsParity() {
+ assertU(adoc("id", "1", "float_range", "[1.0 TO 2.0]", "float_range_dv",
"[1.0 TO 2.0]"));
+ assertU(adoc("id", "2", "float_range", "[1.5 TO 3.0]", "float_range_dv",
"[1.5 TO 3.0]"));
+ assertU(adoc("id", "3", "float_range", "[5.0 TO 6.0]", "float_range_dv",
"[5.0 TO 6.0]"));
+ assertU(commit());
+
+ for (String f : new String[] {"float_range", "float_range_dv"}) {
+ assertQ(
+ "float intersects on " + f,
+ req("q", "{!numericRange criteria=intersects field=" + f + "}[1.2 TO
1.8]"),
+ "//result[@numFound='2']");
+ }
+ }
+
+ @Test
+ public void testDoubleContainsParity() {
+ // doc 1 fully contains [2.0 TO 4.0]; doc 2 is within it (does not contain)
+ assertU(adoc("id", "1", "double_range", "[1.0 TO 5.0]", "double_range_dv",
"[1.0 TO 5.0]"));
+ assertU(adoc("id", "2", "double_range", "[2.5 TO 3.5]", "double_range_dv",
"[2.5 TO 3.5]"));
+ assertU(commit());
+
+ for (String f : new String[] {"double_range", "double_range_dv"}) {
+ assertQ(
+ "double contains on " + f,
+ req("q", "{!numericRange criteria=contains field=" + f + "}[2.0 TO
4.0]"),
+ "//result[@numFound='1']",
+ "//result/doc/str[@name='id'][.='1']");
+ }
+ }
+
+ @Test
+ public void testSortingUnsupportedEvenWithDocValues() {
+ addIntDoc("1", "[100 TO 200]");
+ assertU(commit());
+ // Range fields don't implement sorting: there's no canonical key for
ordering intervals (by
+ // min? max? width? midpoint?), multi-dimensional ranges
(bbox/cube/tesseract) have no sensible
+ // order at all, and the packed BINARY docValues isn't a sortable scalar.
Enabling docValues
+ // does not change that, getSortField still throws.
+ assertQEx(
+ "sorting on a range field must fail even with docValues",
+ "Cannot sort on",
+ req("q", "*:*", "sort", "price_range_dv asc"),
+ SolrException.ErrorCode.BAD_REQUEST);
+ }
+
+ @Test
+ public void testQueryFacetingWithDocValues() {
+ // Range fields support facet.query (count docs matching a range query),
the realistic way to
+ // facet ranges in Solr. (facet.field / value faceting is NOT supported:
range docValues are
Review Comment:
[0] If value faceting isn't supported, then it might be nice to have a test
case similar to what you've written for "sort" above, but for the facet.field
case. Wdyt?
--
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]
