From 3c7d948789a93de6a144e22147d67765a190e2fc Mon Sep 17 00:00:00 2001 From: Michael Gibney Date: Fri, 26 Jul 2019 11:50:03 -0400 Subject: [PATCH] SOLR-7798: Robust support for ExpandComponent when used independently of CollapsingPostFilter There are applications of ExpandComponent that intentionally do not involve prior collapsing of results on the expand field, which can lead to an NPE in expand component when expand.field (for matched docs) has fewer unique values than the number of matched docs. This commit refines the approach taken in SOLR-13877, which addressed the same underlying issue. --- solr/CHANGES.txt | 2 +- .../handler/component/ExpandComponent.java | 35 ++++++++++--------- .../component/TestExpandComponent.java | 13 +++++++ 3 files changed, 32 insertions(+), 18 deletions(-) diff --git a/solr/CHANGES.txt b/solr/CHANGES.txt index 4e213d32b458..32fe2c525ca0 100644 --- a/solr/CHANGES.txt +++ b/solr/CHANGES.txt @@ -176,7 +176,7 @@ Bug Fixes * SOLR-12393: Compute score if requested even when expanded docs not sorted by score in ExpandComponent. (David Smiley, Munendra S N) -* SOLR-13877: Fix NPE in expand component when matched docs have fewer unique values. (Munendra S N) +* SOLR-13877, SOLR-7798: Robust support for ExpandComponent when used independently of CollapsingPostFilter. (Jörg Rathlev, Michael Gibney, Munendra S N) * SOLR-13823: Fix ClassCastEx when score is requested with group.query. This also fixes score not being generated for distributed group.query case. (Uwe Jäger, Munendra S N) diff --git a/solr/core/src/java/org/apache/solr/handler/component/ExpandComponent.java b/solr/core/src/java/org/apache/solr/handler/component/ExpandComponent.java index 2a58248bfa92..0440a3e5abeb 100644 --- a/solr/core/src/java/org/apache/solr/handler/component/ExpandComponent.java +++ b/solr/core/src/java/org/apache/solr/handler/component/ExpandComponent.java @@ -281,7 +281,6 @@ public void process(ResponseBuilder rb) throws IOException { currentValues = sortedDocValues[currentContext]; segmentOrdinalMap = ordinalMap.getGlobalOrds(currentContext); } - int count = 0; ordBytes = new IntObjectHashMap<>(); @@ -303,12 +302,12 @@ public void process(ResponseBuilder rb) throws IOException { currentValues.advance(contextDoc); } if (contextDoc == currentValues.docID()) { - int ord = currentValues.ordValue(); - ++count; - BytesRef ref = currentValues.lookupOrd(ord); - ord = (int)segmentOrdinalMap.get(ord); - ordBytes.put(ord, BytesRef.deepCopyOf(ref)); - groupBits.set(ord); + int contextOrd = currentValues.ordValue(); + int ord = (int)segmentOrdinalMap.get(contextOrd); + if (!groupBits.getAndSet(ord)) { + BytesRef ref = currentValues.lookupOrd(contextOrd); + ordBytes.put(ord, BytesRef.deepCopyOf(ref)); + } collapsedSet.add(globalDoc); } } else { @@ -317,22 +316,22 @@ public void process(ResponseBuilder rb) throws IOException { } if (globalDoc == values.docID()) { int ord = values.ordValue(); - ++count; - BytesRef ref = values.lookupOrd(ord); - ordBytes.put(ord, BytesRef.deepCopyOf(ref)); - groupBits.set(ord); + if (!groupBits.getAndSet(ord)) { + BytesRef ref = values.lookupOrd(ord); + ordBytes.put(ord, BytesRef.deepCopyOf(ref)); + } collapsedSet.add(globalDoc); } } } + int count = ordBytes.size(); if(count > 0 && count < 200) { groupQuery = getGroupQuery(field, count, ordBytes); } } else { groupSet = new LongHashSet(docList.size()); NumericDocValues collapseValues = contexts.get(currentContext).reader().getNumericDocValues(field); - int count = 0; for(int i=0; i= nextDocBase) { @@ -353,12 +352,12 @@ public void process(ResponseBuilder rb) throws IOException { value = 0; } if(value != nullValue) { - ++count; groupSet.add(value); collapsedSet.add(globalDoc); } } + int count = groupSet.size(); if(count > 0 && count < 200) { if (fieldType.isPointField()) { groupQuery = getPointGroupQuery(schemaField, count, groupSet); @@ -685,7 +684,8 @@ private Query getGroupQuery(String fname, int size, LongHashSet groupSet) { - List bytesRefs = new ArrayList<>(size); + BytesRef[] bytesRefs = new BytesRef[size]; + int index = -1; BytesRefBuilder term = new BytesRefBuilder(); Iterator it = groupSet.iterator(); @@ -693,7 +693,7 @@ private Query getGroupQuery(String fname, LongCursor cursor = it.next(); String stringVal = numericToString(ft, cursor.value); ft.readableToIndexed(stringVal, term); - bytesRefs.add(term.toBytesRef()); + bytesRefs[++index] = term.toBytesRef(); } return new TermInSetQuery(fname, bytesRefs); @@ -734,11 +734,12 @@ private String numericToString(FieldType fieldType, long val) { private Query getGroupQuery(String fname, int size, IntObjectHashMap ordBytes) { - List bytesRefs = new ArrayList<>(size); + BytesRef[] bytesRefs = new BytesRef[size]; + int index = -1; Iterator>it = ordBytes.iterator(); while (it.hasNext()) { IntObjectCursor cursor = it.next(); - bytesRefs.add(cursor.value); + bytesRefs[++index] = cursor.value; } return new TermInSetQuery(fname, bytesRefs); } diff --git a/solr/core/src/test/org/apache/solr/handler/component/TestExpandComponent.java b/solr/core/src/test/org/apache/solr/handler/component/TestExpandComponent.java index 336d608a32cd..690ad607b946 100644 --- a/solr/core/src/test/org/apache/solr/handler/component/TestExpandComponent.java +++ b/solr/core/src/test/org/apache/solr/handler/component/TestExpandComponent.java @@ -352,6 +352,19 @@ private void _testExpand(String group, String floatAppend, String hint) throws E "/response/lst[@name='expanded']/result[@name='2000.0']/doc[1]/str[@name='id'][.='7']", "count(//*[@name='score'])=0" ); + + // Support expand enabled without previous collapse + assertQ(req("q", "type_s:child", "sort", group+" asc, test_l desc", "defType", "edismax", + "expand", "true", "expand.q", "type_s:parent", "expand.field", group), + "*[count(/response/result/doc)=4]", + "*[count(/response/lst[@name='expanded']/result)=2]", + "/response/result/doc[1]/str[@name='id'][.='7']", + "/response/result/doc[2]/str[@name='id'][.='2']", + "/response/result/doc[3]/str[@name='id'][.='8']", + "/response/result/doc[4]/str[@name='id'][.='6']", + "/response/lst[@name='expanded']/result[@name='1"+floatAppend+"']/doc[1]/str[@name='id'][.='1']", + "/response/lst[@name='expanded']/result[@name='2"+floatAppend+"']/doc[1]/str[@name='id'][.='5']" + ); } @Test