diff --git a/solr/CHANGES.txt b/solr/CHANGES.txt index 813a0b79f07..6834eb56bb2 100644 --- a/solr/CHANGES.txt +++ b/solr/CHANGES.txt @@ -270,6 +270,11 @@ Bug Fixes * SOLR-8449: Fix the core restore functionality to allow restoring multiple times on the same core (Johannes Brucher, Varun Thacker) +* SOLR-8155: JSON Facet API - field faceting on a multi-valued string field without + docValues (i.e. UnInvertedField implementation), but with a prefix or with a sort + other than count, resulted in incorrect results. This has been fixed, and facet.prefix + support for facet.method=uif has been enabled. (Mikhail Khludnev, yonik) + Optimizations ---------------------- * SOLR-7876: Speed up queries and operations that use many terms when timeAllowed has not been diff --git a/solr/core/src/java/org/apache/solr/request/SimpleFacets.java b/solr/core/src/java/org/apache/solr/request/SimpleFacets.java index 2c61f6da2da..a9ce11dcfde 100644 --- a/solr/core/src/java/org/apache/solr/request/SimpleFacets.java +++ b/solr/core/src/java/org/apache/solr/request/SimpleFacets.java @@ -487,17 +487,7 @@ public class SimpleFacets { jsonFacet.put("limit", limit); jsonFacet.put("mincount", mincount); jsonFacet.put("missing", missing); - - if (prefix!=null) { - // presumably it supports single-value, but at least now returns wrong results on multi-value - throw new SolrException ( - SolrException.ErrorCode.BAD_REQUEST, - FacetParams.FACET_PREFIX+"="+prefix+ - " are not supported by "+FacetParams.FACET_METHOD+"="+FacetParams.FACET_METHOD_uif+ - " for field:"+ field - //jsonFacet.put("prefix", prefix); - ); - } + jsonFacet.put("prefix", prefix); jsonFacet.put("numBuckets", params.getFieldBool(field, "numBuckets", false)); jsonFacet.put("allBuckets", params.getFieldBool(field, "allBuckets", false)); jsonFacet.put("method", "uif"); diff --git a/solr/core/src/java/org/apache/solr/search/facet/UnInvertedField.java b/solr/core/src/java/org/apache/solr/search/facet/UnInvertedField.java index 9294792097a..c1613cde680 100644 --- a/solr/core/src/java/org/apache/solr/search/facet/UnInvertedField.java +++ b/solr/core/src/java/org/apache/solr/search/facet/UnInvertedField.java @@ -499,10 +499,11 @@ public class UnInvertedField extends DocTermOrds { if (delta==0) break; tnum += delta - TNUM_OFFSET; int arrIdx = tnum - startTermIndex; - if (arrIdx < 0) continue; - if (arrIdx >= nTerms) break; - countAcc.incrementCount(arrIdx, 1); - processor.collectFirstPhase(segDoc, arrIdx); + if (arrIdx >= 0) { + if (arrIdx >= nTerms) break; + countAcc.incrementCount(arrIdx, 1); + processor.collectFirstPhase(segDoc, arrIdx); + } delta = 0; } code >>>= 8; diff --git a/solr/core/src/test/org/apache/solr/TestRandomDVFaceting.java b/solr/core/src/test/org/apache/solr/TestRandomDVFaceting.java index 7decfce45a4..90c239481bd 100644 --- a/solr/core/src/test/org/apache/solr/TestRandomDVFaceting.java +++ b/solr/core/src/test/org/apache/solr/TestRandomDVFaceting.java @@ -215,9 +215,6 @@ public class TestRandomDVFaceting extends SolrTestCaseJ4 { List methods = multiValued ? multiValuedMethods : singleValuedMethods; List responses = new ArrayList<>(methods.size()); for (String method : methods) { - if (method.equals("uif") && params.get("facet.prefix")!=null) { - continue; // it's not supported there - } if (method.equals("dv")) { params.set("facet.field", "{!key="+facet_field+"}"+facet_field+"_dv"); params.set("facet.method",(String) null); diff --git a/solr/core/src/test/org/apache/solr/request/SimpleFacetsTest.java b/solr/core/src/test/org/apache/solr/request/SimpleFacetsTest.java index 1541a462811..3b99eb88692 100644 --- a/solr/core/src/test/org/apache/solr/request/SimpleFacetsTest.java +++ b/solr/core/src/test/org/apache/solr/request/SimpleFacetsTest.java @@ -512,7 +512,7 @@ public class SimpleFacetsTest extends SolrTestCaseJ4 { params.set("facet.method", method); } for (String prefix : prefixes) { - if (prefix == null || "uif".equals(method)) {// there is no support + if (prefix == null) { params.remove("facet.prefix"); } else { params.set("facet.prefix", prefix); @@ -2016,16 +2016,6 @@ public class SimpleFacetsTest extends SolrTestCaseJ4 { doFacetPrefix("tt_s1", "{!threads=2}", "", "facet.method","fcs"); // specific number of threads } - /** no prefix for uif */ - @Test(expected=RuntimeException.class) - public void testNOFacetPrefixForUif() { - if (random().nextBoolean()) { - doFacetPrefix("tt_s1", null, "", "facet.method", "uif"); - } else { - doFacetPrefix("t_s", null, "", "facet.method", "uif"); - } - } - @Test @Ignore("SOLR-8466 - facet.method=uif ignores facet.contains") public void testFacetContainsUif() { diff --git a/solr/core/src/test/org/apache/solr/request/TestFaceting.java b/solr/core/src/test/org/apache/solr/request/TestFaceting.java index 16a6b1364db..97dcedfe6a0 100644 --- a/solr/core/src/test/org/apache/solr/request/TestFaceting.java +++ b/solr/core/src/test/org/apache/solr/request/TestFaceting.java @@ -529,12 +529,8 @@ public class TestFaceting extends SolrTestCaseJ4 { } private void assertQforUIF(String message, SolrQueryRequest request, String ... tests) { - final String paramString = request.getParamString(); - if (paramString.contains("uif") && paramString.contains("prefix")){ - assertQEx("uif prohibits prefix", "not supported", request, ErrorCode.BAD_REQUEST); - }else{ - assertQ(message,request, tests); - } + // handle any differences for uif here, like skipping unsupported options + assertQ(message,request, tests); } private void add50ocs() { diff --git a/solr/core/src/test/org/apache/solr/search/facet/TestJsonFacets.java b/solr/core/src/test/org/apache/solr/search/facet/TestJsonFacets.java index 7df03f14dcc..83220edc468 100644 --- a/solr/core/src/test/org/apache/solr/search/facet/TestJsonFacets.java +++ b/solr/core/src/test/org/apache/solr/search/facet/TestJsonFacets.java @@ -644,6 +644,27 @@ public class TestJsonFacets extends SolrTestCaseHS { " } " ); + // test prefix on real multi-valued field + client.testJQ(params(p, "q", "*:*" + , "json.facet", "{" + + " f1:{${terms} type:terms, field:${multi_ss}, prefix:A }" + + ",f2:{${terms} type:terms, field:${multi_ss}, prefix:z }" + + ",f3:{${terms} type:terms, field:${multi_ss}, prefix:aa }" + + ",f4:{${terms} type:terms, field:${multi_ss}, prefix:bb }" + + ",f5:{${terms} type:terms, field:${multi_ss}, prefix:a }" + + ",f6:{${terms} type:terms, field:${multi_ss}, prefix:b }" + + "}" + ) + , "facets=={ 'count':6 " + + ",f1:{buckets:[]}" + + ",f2:{buckets:[]}" + + ",f3:{buckets:[]}" + + ",f4:{buckets:[]}" + + ",f5:{buckets:[ {val:a,count:3} ]}" + + ",f6:{buckets:[ {val:b,count:3} ]}" + + " } " + ); + // // missing //