SOLR-18201: Range Fields support docValues based optimization#4645
SOLR-18201: Range Fields support docValues based optimization#4645ercsonusharma wants to merge 2 commits into
Conversation
github-actions
Bot
added
documentation
gerlowskija
left a comment
gerlowskija
left a comment
There was a problem hiding this comment.
Hey @ercsonusharma - this looks great overall! There are two big conceptual questions worth discussing before getting this in:
- Binary vs. SortedSet DocValues - your PR opts to use Lucene's "Binary" docValue type. This isn't without precedent in Solr, but SortedSet is definitely more common. Did you consider (but discard) the idea of using SortedSet? If so, why did you end up deciding on Binary? Did you do any benchmarking of the different approaches?
- Sorting and Faceting - I notice this PR omits support for docValues. Is that due to a particular technical limitation or hurdle, or more a question of scope/value?
Documenting answers to those questions will be a big help to other reviewers or folks trying to understand this code down the road 👍
Other than that, this code looks pretty far along and hopefully we can get it merged in pretty short order. I've left a number of other smaller comments "in line". Many of these are in the test code. It feels like there's some consolidation possible there, but I may be missing certain nuances between the test files...
Anyway, let me know what you think!
There was a problem hiding this 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?
| * calls {@link #createFieldsFromAllValues(SchemaField, Collection)} once per (multiValued) field | ||
| * instead of {@link #createFields(SchemaField, Object)} per value. | ||
| */ | ||
| public boolean createsFieldsFromAllValues() { |
There was a problem hiding this 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?
| @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 |
There was a problem hiding this comment.
[0] IMO these are worth enabling by default, for two reasons:
- Solr has been moving towards "docValues by default" for awhile (example).
- 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.
|
|
||
| // 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; |
There was a problem hiding this 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.
| } | ||
|
|
||
| return newContainsQuery(field.getName(), singleBoundRange); | ||
| // A single bound is the degenerate range [p,p]; "contains p" is equivalent to |
There was a problem hiding this 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?
| import org.junit.Test; | ||
|
|
||
| /** | ||
| * Integration tests over the {@code schema-numericrange.xml} verifying that the docValues-enabled |
There was a problem hiding this comment.
| * Integration tests over the {@code schema-numericrange.xml} verifying that the docValues-enabled | |
| * Integration tests over {@code schema-numericrange.xml} verifying that the docValues-enabled |
| @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 |
There was a problem hiding this 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?
| import org.junit.Test; | ||
|
|
||
| /** | ||
| * Integration tests for multiValued range fields with docValues (backed by SortedSet docValues). |
There was a problem hiding this 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?
| assertU(commit()); | ||
|
|
||
| // Neither individual range of doc 1 contains [1,15] | ||
| assertSameCount("contains", "[1 TO 15]", "price_range_multi", "price_range_mv_dv", 0); |
There was a problem hiding this 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.
| |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. |
There was a problem hiding this 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?
2 participants
https://issues.apache.org/jira/browse/SOLR-18201
Description
The numeric range field types i.e. IntRangeField, LongRangeField, FloatRangeField, and DoubleRangeField, did not support docValues. This PR adds opt-in, single/multiValued docValues to all four.
The motivation is a query-time optimization. Today a range clause (e.g. my_range:[100 TO 200]) always walks the field's Points (BKD) tree and materializes a bitset of all matching docs, regardless of how selective the rest of the query is. When such a clause is AND-ed with a much more selective clause (myStrField:someSelectiveValue AND my_range:[100 TO 200]), this is wasteful, we materialize a large bitset only to intersect it with a handful of candidates.
Solution
With docValues present, the range clause is wrapped in Lucene's IndexOrDocValuesQuery, which lets the selective clause lead iteration and verifies the range predicate per-candidate from docValues, skipping the BKD materialization entirely.
Also, introducec MultiBinaryRangeDocValuesQuery, backed by BinaryDocValues as SortedSet backing builds a huge term dictionary and every per-doc verify pays a lookupOrd making it more costly than the sequential BKD materialization it was meant to replace. It comes with minor modification in lucene document creation by packing multiValued field's ranges into a single blob.
These field types are new/unreleased, so this is added without back-compat concerns.
Tests