From 43ee86a3d060d05a23251d2f5a31e0b03d850acd Mon Sep 17 00:00:00 2001 From: Munendra S N Date: Sat, 21 Sep 2019 11:46:11 +0530 Subject: [PATCH] SOLR-13272: add support for arbitrary ranges in JSON Range faceting In some cases, the gap might need to be different for different ranges. To support such cases, add support to specify arbitrary ranges. --- solr/CHANGES.txt | 3 + .../apache/solr/request/IntervalFacets.java | 2 +- .../apache/solr/search/facet/FacetRange.java | 279 ++++++++++++++++-- .../solr/search/facet/FacetRequest.java | 10 +- .../search/facet/RangeFacetCloudTest.java | 164 ++++++++-- .../search/facet/TestJsonFacetRefinement.java | 36 ++- .../solr/search/facet/TestJsonFacets.java | 252 +++++++++++++++- 7 files changed, 695 insertions(+), 51 deletions(-) diff --git a/solr/CHANGES.txt b/solr/CHANGES.txt index 9537dfbae05..9e1d0cd5d13 100644 --- a/solr/CHANGES.txt +++ b/solr/CHANGES.txt @@ -136,6 +136,9 @@ New Features * SOLR-13734: JWTAuthPlugin now supports multiple IdP issuers through configuring a new 'issuers' configuration key. Access tokens issued and signed by any of the configured issuers will be validated (janhoy) +* SOLR-13272: Add support for arbitrary ranges in JSON facet's Range facets. + (Apoorv Bhawsar, Munendra S N, Mikhail Khludnev, Ishan Chattopadhyaya, Jan Høydahl) + Improvements ---------------------- diff --git a/solr/core/src/java/org/apache/solr/request/IntervalFacets.java b/solr/core/src/java/org/apache/solr/request/IntervalFacets.java index 6e492e70034..188c07a61cd 100644 --- a/solr/core/src/java/org/apache/solr/request/IntervalFacets.java +++ b/solr/core/src/java/org/apache/solr/request/IntervalFacets.java @@ -558,7 +558,7 @@ public class IntervalFacets implements Iterable { } else if (intervalStr.charAt(lastNdx) == ']') { endOpen = false; } else { - throw new SyntaxError("Invalid end character " + intervalStr.charAt(0) + " in facet interval " + intervalStr); + throw new SyntaxError("Invalid end character " + intervalStr.charAt(lastNdx) + " in facet interval " + intervalStr); } StringBuilder startStr = new StringBuilder(lastNdx); diff --git a/solr/core/src/java/org/apache/solr/search/facet/FacetRange.java b/solr/core/src/java/org/apache/solr/search/facet/FacetRange.java index d7925193491..b5f152188a0 100644 --- a/solr/core/src/java/org/apache/solr/search/facet/FacetRange.java +++ b/solr/core/src/java/org/apache/solr/search/facet/FacetRange.java @@ -38,6 +38,7 @@ import org.apache.solr.schema.SchemaField; import org.apache.solr.schema.TrieDateField; import org.apache.solr.schema.TrieField; import org.apache.solr.search.DocSet; +import org.apache.solr.search.SyntaxError; import org.apache.solr.search.facet.SlotAcc.SlotContext; import org.apache.solr.util.DateMathParser; @@ -50,6 +51,7 @@ public class FacetRange extends FacetRequestSorted { Object start; Object end; Object gap; + Object ranges; boolean hardend = false; EnumSet include; EnumSet others; @@ -72,11 +74,15 @@ public class FacetRange extends FacetRequestSorted { @Override public Map getFacetDescription() { - Map descr = new HashMap(); + Map descr = new HashMap<>(); descr.put("field", field); - descr.put("start", start); - descr.put("end", end); - descr.put("gap", gap); + if (ranges != null) { + descr.put("ranges", ranges); + } else { + descr.put("start", start); + descr.put("end", end); + descr.put("gap", gap); + } return descr; } @@ -95,7 +101,8 @@ class FacetRangeProcessor extends FacetProcessor { final Comparable start; final Comparable end; final String gap; - + final Object ranges; + /** Build by {@link #createRangeList} if and only if needed for basic faceting */ List rangeList; /** Build by {@link #createRangeList} if and only if needed for basic faceting */ @@ -120,11 +127,22 @@ class FacetRangeProcessor extends FacetProcessor { include = freq.include; sf = fcontext.searcher.getSchema().getField(freq.field); calc = getCalcForField(sf); - start = calc.getValue(freq.start.toString()); - end = calc.getValue(freq.end.toString()); - gap = freq.gap.toString(); + if (freq.ranges != null && (freq.start != null || freq.end != null || freq.gap != null)) { + throw new SolrException(SolrException.ErrorCode.BAD_REQUEST, + "Cannot set gap/start/end and ranges params together"); + } + if (freq.ranges != null) { + ranges = freq.ranges; + start = null; + end = null; + gap = null; + } else { + start = calc.getValue(freq.start.toString()); + end = calc.getValue(freq.end.toString()); + gap = freq.gap.toString(); + ranges = null; + } - // Under the normal mincount=0, each shard will need to return 0 counts since we don't calculate buckets at the top level. // If mincount>0 then we could *potentially* set our sub mincount to 1... // ...but that would require sorting the buckets (by their val) at the top level @@ -245,7 +263,12 @@ class FacetRangeProcessor extends FacetProcessor { Comparable low = start; Comparable loop_end = this.end; - + + if (ranges != null) { + rangeList.addAll(parseRanges(ranges)); + return; + } + while (low.compareTo(end) < 0) { Comparable high = calc.addGap(low, gap); if (end.compareTo(high) < 0) { @@ -263,14 +286,14 @@ class FacetRangeProcessor extends FacetProcessor { if (high.compareTo(low) == 0) { throw new SolrException (SolrException.ErrorCode.BAD_REQUEST, - "range facet infinite loop: gap is either zero, or too small relative start/end and caused underflow: " + low + " + " + gap + " = " + high ); + "range facet infinite loop: gap is either zero, or too small relative start/end and caused underflow: " + low + " + " + gap + " = " + high); } - boolean incLower =(include.contains(FacetRangeInclude.LOWER) || - (include.contains(FacetRangeInclude.EDGE) && 0 == low.compareTo(start))); + boolean incLower = (include.contains(FacetRangeInclude.LOWER) || + (include.contains(FacetRangeInclude.EDGE) && 0 == low.compareTo(start))); boolean incUpper = (include.contains(FacetRangeInclude.UPPER) || - (include.contains(FacetRangeInclude.EDGE) && 0 == high.compareTo(end))); - + (include.contains(FacetRangeInclude.EDGE) && 0 == high.compareTo(end))); + Range range = new Range(calc.buildRangeLabel(low), low, high, incLower, incUpper); rangeList.add( range ); @@ -299,8 +322,203 @@ class FacetRangeProcessor extends FacetProcessor { actual_end = null; } } - - + + /** + * Parses the given list of maps and returns list of Ranges + * + * @param input - list of map containing the ranges + * @return list of {@link Range} + */ + private List parseRanges(Object input) { + if (!(input instanceof List)) { + throw new SolrException(SolrException.ErrorCode.BAD_REQUEST, + "Expected List for ranges but got " + input.getClass().getSimpleName() + " = " + input + ); + } + List intervals = (List) input; + List ranges = new ArrayList<>(); + for (Object obj : intervals) { + if (!(obj instanceof Map)) { + throw new SolrException(SolrException.ErrorCode.BAD_REQUEST, + "Expected Map for range but got " + obj.getClass().getSimpleName() + " = " + obj); + } + Range range; + Map interval = (Map) obj; + if (interval.containsKey("range")) { + range = getRangeByOldFormat(interval); + } else { + range = getRangeByNewFormat(interval); + } + ranges.add(range); + } + return ranges; + } + + private boolean getBoolean(Map args, String paramName, boolean defVal) { + Object o = args.get(paramName); + if (o == null) { + return defVal; + } + // TODO: should we be more flexible and accept things like "true" (strings)? + // Perhaps wait until the use case comes up. + if (!(o instanceof Boolean)) { + throw new SolrException(SolrException.ErrorCode.BAD_REQUEST, + "Expected boolean type for param '"+paramName + "' but got " + o.getClass().getSimpleName() + " = " + o); + } + + return (Boolean)o; + } + + private String getString(Map args, String paramName, boolean required) { + Object o = args.get(paramName); + if (o == null) { + if (required) { + throw new SolrException(SolrException.ErrorCode.BAD_REQUEST, + "Missing required parameter '" + paramName + "' for " + args); + } + return null; + } + if (!(o instanceof String)) { + throw new SolrException(SolrException.ErrorCode.BAD_REQUEST, + "Expected string type for param '"+paramName + "' but got " + o.getClass().getSimpleName() + " = " + o); + } + + return (String)o; + } + + /** + * Parses the range given in format {from:val1, to:val2, inclusive_to:true} + * and returns the {@link Range} + * + * @param rangeMap Map containing the range info + * @return {@link Range} + */ + private Range getRangeByNewFormat(Map rangeMap) { + Object fromObj = rangeMap.get("from"); + Object toObj = rangeMap.get("to"); + + String fromStr = fromObj == null? "*" : fromObj.toString(); + String toStr = toObj == null? "*": toObj.toString(); + boolean includeUpper = getBoolean(rangeMap, "inclusive_to", false); + boolean includeLower = getBoolean(rangeMap, "inclusive_from", true); + + Object key = rangeMap.get("key"); + // if (key == null) { + // key = (includeLower? "[": "(") + fromStr + "," + toStr + (includeUpper? "]": ")"); + // } + // using the default key as custom key won't work with refine + // refine would need both low and high values + key = (includeLower? "[": "(") + fromStr + "," + toStr + (includeUpper? "]": ")"); + + Comparable from = getComparableFromString(fromStr); + Comparable to = getComparableFromString(toStr); + if (from != null && to != null && from.compareTo(to) > 0) { + // allowing from and to be same + throw new SolrException(SolrException.ErrorCode.BAD_REQUEST, "'from' is higher than 'to' in range for key: " + key); + } + + return new Range(key, from, to, includeLower, includeUpper); + } + + /** + * Parses the range string from the map and Returns {@link Range} + * + * @param range map containing the interval + * @return {@link Range} + */ + private Range getRangeByOldFormat(Map range) { + String key = getString(range, "key", false); + String rangeStr = getString(range, "range", true); + try { + return parseRangeFromString(key, rangeStr); + } catch (SyntaxError e) { + throw new SolrException(SolrException.ErrorCode.BAD_REQUEST, e); + } + } + + /** + * Parses the given string and returns Range. + * This is adopted from {@link org.apache.solr.request.IntervalFacets} + * + * @param key The name of range which would be used as {@link Range}'s label + * @param rangeStr The string containing the Range + * @return {@link Range} + */ + private Range parseRangeFromString(String key, String rangeStr) throws SyntaxError { + rangeStr = rangeStr.trim(); + if (rangeStr.isEmpty()) { + throw new SyntaxError("empty facet range"); + } + + boolean includeLower = true, includeUpper = true; + Comparable start = null, end = null; + if (rangeStr.charAt(0) == '(') { + includeLower = false; + } else if (rangeStr.charAt(0) != '[') { + throw new SyntaxError( "Invalid start character " + rangeStr.charAt(0) + " in facet range " + rangeStr); + } + + final int lastNdx = rangeStr.length() - 1; + if (rangeStr.charAt(lastNdx) == ')') { + includeUpper = false; + } else if (rangeStr.charAt(lastNdx) != ']') { + throw new SyntaxError("Invalid end character " + rangeStr.charAt(lastNdx) + " in facet range " + rangeStr); + } + + StringBuilder startStr = new StringBuilder(lastNdx); + int i = unescape(rangeStr, 1, lastNdx, startStr); + if (i == lastNdx) { + if (rangeStr.charAt(lastNdx - 1) == ',') { + throw new SyntaxError("Empty range limit"); + } + throw new SyntaxError("Missing unescaped comma separating range ends in " + rangeStr); + } + start = getComparableFromString(startStr.toString()); + + StringBuilder endStr = new StringBuilder(lastNdx); + i = unescape(rangeStr, i, lastNdx, endStr); + if (i != lastNdx) { + throw new SyntaxError("Extra unescaped comma at index " + i + " in range " + rangeStr); + } + end = getComparableFromString(endStr.toString()); + + if (start != null && end != null && start.compareTo(end) > 0) { + throw new SolrException(SolrException.ErrorCode.BAD_REQUEST, "'start' is higher than 'end' in range for key: " + rangeStr); + } + + // not using custom key as it won't work with refine + // refine would need both low and high values + return new Range(rangeStr, start, end, includeLower, includeUpper); + } + + /* Fill in sb with a string from i to the first unescaped comma, or n. + Return the index past the unescaped comma, or n if no unescaped comma exists */ + private int unescape(String s, int i, int n, StringBuilder sb) throws SyntaxError { + for (; i < n; ++i) { + char c = s.charAt(i); + if (c == '\\') { + ++i; + if (i < n) { + c = s.charAt(i); + } else { + throw new SyntaxError("Unfinished escape at index " + i + " in facet range " + s); + } + } else if (c == ',') { + return i + 1; + } + sb.append(c); + } + return n; + } + + private Comparable getComparableFromString(String value) { + value = value.trim(); + if ("*".equals(value)) { + return null; + } + return calc.getValue(value); + } + private SimpleOrderedMap getRangeCountsIndexed() throws IOException { int slotCount = rangeList.size() + otherList.size(); @@ -341,7 +559,7 @@ class FacetRangeProcessor extends FacetProcessor { addStats(bucket, rangeList.size() + idx); doSubs(bucket, rangeList.size() + idx); } - + if (null != actual_end) { res.add(FacetRange.ACTUAL_END_JSON_KEY, calc.formatValue(actual_end)); } @@ -404,7 +622,7 @@ class FacetRangeProcessor extends FacetProcessor { } /** - * Given the low value for a bucket, generates the appropraite "label" object to use. + * Given the low value for a bucket, generates the appropriate "label" object to use. * By default return the low object unmodified. */ public Object buildRangeLabel(Comparable low) { @@ -471,7 +689,7 @@ class FacetRangeProcessor extends FacetProcessor { /** * Adds the String gap param to a low Range endpoint value to determine - * the corrisponding high Range endpoint value, throwing + * the corresponding high Range endpoint value, throwing * a useful exception if not possible. */ public final Comparable addGap(Comparable value, String gap) { @@ -485,7 +703,7 @@ class FacetRangeProcessor extends FacetProcessor { } /** * Adds the String gap param to a low Range endpoint value to determine - * the corrisponding high Range endpoint value. + * the corresponding high Range endpoint value. * Can throw a low level format exception as needed. */ protected abstract Comparable parseAndAddGap(Comparable value, String gap) @@ -695,7 +913,7 @@ class FacetRangeProcessor extends FacetProcessor { // But range faceting does *NOT* use the "leaves" and "partial" syntax // // If/When range facet becomes more like field facet in it's ability to sort and limit the "range buckets" - // FacetRangeProcessor and FacetFieldProcessor should prbably be refactored to share more code. + // FacetRangeProcessor and FacetFieldProcessor should probably be refactored to share more code. boolean skipThisFacet = (fcontext.flags & SKIP_FACET) != 0; @@ -722,7 +940,7 @@ class FacetRangeProcessor extends FacetProcessor { { // refine the special "other" buckets - // NOTE: we're re-useing this variable for each special we look for... + // NOTE: we're re-using this variable for each special we look for... Map specialFacetInfo; specialFacetInfo = (Map) fcontext.facetInfo.get(FacetRangeOther.BEFORE.toString()); @@ -784,7 +1002,20 @@ class FacetRangeProcessor extends FacetProcessor { private SimpleOrderedMap refineBucket(Object bucketVal, boolean skip, Map facetInfo) throws IOException { - Comparable low = calc.getValue(bucketVal.toString()); + String val = bucketVal.toString(); + if (ranges != null) { + try { + Range range = parseRangeFromString(val, val); + final SimpleOrderedMap bucket = refineRange(range, skip, facetInfo); + bucket.add("val", range.label); + return bucket; + } catch (SyntaxError e) { + // execution won't reach here as ranges are already validated + throw new SolrException(SolrException.ErrorCode.BAD_REQUEST, e); + } + } + + Comparable low = calc.getValue(val); Comparable high = calc.addGap(low, gap); Comparable max_end = end; if (end.compareTo(high) < 0) { diff --git a/solr/core/src/java/org/apache/solr/search/facet/FacetRequest.java b/solr/core/src/java/org/apache/solr/search/facet/FacetRequest.java index 566be2e5331..3d79a8abc73 100644 --- a/solr/core/src/java/org/apache/solr/search/facet/FacetRequest.java +++ b/solr/core/src/java/org/apache/solr/search/facet/FacetRequest.java @@ -1057,10 +1057,12 @@ class FacetRangeParser extends FacetParser { Map m = (Map) arg; facet.field = getString(m, "field", null); + facet.ranges = getVal(m, "ranges", false); - facet.start = getVal(m, "start", true); - facet.end = getVal(m, "end", true); - facet.gap = getVal(m, "gap", true); + boolean required = facet.ranges == null; + facet.start = getVal(m, "start", required); + facet.end = getVal(m, "end", required); + facet.gap = getVal(m, "gap", required); facet.hardend = getBoolean(m, "hardend", facet.hardend); facet.mincount = getLong(m, "mincount", 0); @@ -1069,7 +1071,7 @@ class FacetRangeParser extends FacetParser { List list = getStringList(m, "include", false); String[] includeList = null; if (list != null) { - includeList = (String[])list.toArray(new String[list.size()]); + includeList = list.toArray(new String[list.size()]); } facet.include = FacetParams.FacetRangeInclude.parseParam( includeList ); facet.others = EnumSet.noneOf(FacetParams.FacetRangeOther.class); diff --git a/solr/core/src/test/org/apache/solr/search/facet/RangeFacetCloudTest.java b/solr/core/src/test/org/apache/solr/search/facet/RangeFacetCloudTest.java index b11ff3ec9bb..77663dfeead 100644 --- a/solr/core/src/test/org/apache/solr/search/facet/RangeFacetCloudTest.java +++ b/solr/core/src/test/org/apache/solr/search/facet/RangeFacetCloudTest.java @@ -48,7 +48,7 @@ import org.junit.BeforeClass; * Builds a random index of a few simple fields, maintaining an in-memory model of the expected * doc counts so that we can verify the results of range facets w/ nested field facets that need refinement. * - * The focus here is on stressing the casees where the document values fall directonly on the + * The focus here is on stressing the cases where the document values fall direct only on the * range boundaries, and how the various "include" options affects refinement. */ public class RangeFacetCloudTest extends SolrCloudTestCase { @@ -63,8 +63,7 @@ public class RangeFacetCloudTest extends SolrCloudTestCase { private static final int NUM_RANGE_VALUES = 6; private static final int TERM_VALUES_RANDOMIZER = 100; - // TODO: add 'count asc' once SOLR-12343 is fixed - private static final List SORTS = Arrays.asList("count desc", "index asc", "index desc"); + private static final List SORTS = Arrays.asList("count desc", "count asc", "index asc", "index desc"); private static final List> OTHERS = buildListOfFacetRangeOtherOptions(); private static final List BEFORE_AFTER_BETWEEN @@ -136,20 +135,20 @@ public class RangeFacetCloudTest extends SolrCloudTestCase { ("q", "*:*", "rows", "0", "json.facet", // exclude a single low value from our ranges "{ foo:{ type:range, field:"+INT_FIELD+" start:1, end:5, gap:1"+otherStr+include+subFacet+" } }"); - + final QueryResponse rsp = cluster.getSolrClient().query(solrQuery); try { final NamedList foo = ((NamedList>)rsp.getResponse().get("facets")).get("foo"); final List> buckets = (List>) foo.get("buckets"); - + assertEquals("num buckets", 4, buckets.size()); for (int i = 0; i < 4; i++) { int expectedVal = i+1; assertBucket("bucket#" + i, expectedVal, modelVals(expectedVal), subFacetLimit, buckets.get(i)); } - + assertBeforeAfterBetween(other, modelVals(0), modelVals(5), modelVals(1,4), subFacetLimit, foo); - + } catch (AssertionError|RuntimeException ae) { throw new AssertionError(solrQuery.toString() + " -> " + rsp.toString() + " ===> " + ae.getMessage(), ae); } @@ -157,7 +156,7 @@ public class RangeFacetCloudTest extends SolrCloudTestCase { } } } - + public void testInclude_Lower_Gap2() throws Exception { for (boolean doSubFacet : Arrays.asList(false, true)) { final Integer subFacetLimit = pickSubFacetLimit(doSubFacet); @@ -538,10 +537,6 @@ public class RangeFacetCloudTest extends SolrCloudTestCase { } catch (AssertionError|RuntimeException ae) { throw new AssertionError(solrQuery.toString() + " -> " + rsp.toString() + " ===> " + ae.getMessage(), ae); } - - - - } } } @@ -582,6 +577,137 @@ public class RangeFacetCloudTest extends SolrCloudTestCase { } } + public void testRangeWithInterval() throws Exception { + for (boolean doSubFacet : Arrays.asList(false, true)) { + final Integer subFacetLimit = pickSubFacetLimit(doSubFacet); + final CharSequence subFacet = makeSubFacet(subFacetLimit); + for (boolean incUpper : Arrays.asList(false, true)) { + String incUpperStr = ",inclusive_to:"+incUpper; + final SolrQuery solrQuery = new SolrQuery + ("q", "*:*", "rows", "0", "json.facet", + "{ foo:{ type:range, field:" + INT_FIELD + " ranges:[{from:1, to:2"+ incUpperStr+ "}," + + "{from:2, to:3"+ incUpperStr +"},{from:3, to:4"+ incUpperStr +"},{from:4, to:5"+ incUpperStr+"}]" + + subFacet + " } }"); + + final QueryResponse rsp = cluster.getSolrClient().query(solrQuery); + try { + final NamedList foo = ((NamedList>) rsp.getResponse().get("facets")).get("foo"); + final List> buckets = (List>) foo.get("buckets"); + + assertEquals("num buckets", 4, buckets.size()); + for (int i = 0; i < 4; i++) { + String expectedVal = "[" + (i + 1) + "," + (i + 2) + (incUpper? "]": ")"); + ModelRange modelVals = incUpper? modelVals(i+1, i+2) : modelVals(i+1); + assertBucket("bucket#" + i, expectedVal, modelVals, subFacetLimit, buckets.get(i)); + } + } catch (AssertionError | RuntimeException ae) { + throw new AssertionError(solrQuery.toString() + " -> " + rsp.toString() + " ===> " + ae.getMessage(), ae); + } + } + } + } + + public void testRangeWithOldIntervalFormat() throws Exception { + for (boolean doSubFacet : Arrays.asList(false, true)) { + final Integer subFacetLimit = pickSubFacetLimit(doSubFacet); + final CharSequence subFacet = makeSubFacet(subFacetLimit); + for (boolean incUpper : Arrays.asList(false, true)) { + String incUpperStr = incUpper? "]\"":")\""; + final SolrQuery solrQuery = new SolrQuery + ("q", "*:*", "rows", "0", "json.facet", + "{ foo:{ type:range, field:" + INT_FIELD + " ranges:[{range:\"[1,2"+ incUpperStr+ "}," + + "{range:\"[2,3"+ incUpperStr +"},{range:\"[3,4"+ incUpperStr +"},{range:\"[4,5"+ incUpperStr+"}]" + + subFacet + " } }"); + + final QueryResponse rsp = cluster.getSolrClient().query(solrQuery); + try { + final NamedList foo = ((NamedList>) rsp.getResponse().get("facets")).get("foo"); + final List> buckets = (List>) foo.get("buckets"); + + assertEquals("num buckets", 4, buckets.size()); + for (int i = 0; i < 4; i++) { + String expectedVal = "[" + (i + 1) + "," + (i + 2) + (incUpper? "]": ")"); + ModelRange modelVals = incUpper? modelVals(i+1, i+2) : modelVals(i+1); + assertBucket("bucket#" + i, expectedVal, modelVals, subFacetLimit, buckets.get(i)); + } + } catch (AssertionError | RuntimeException ae) { + throw new AssertionError(solrQuery.toString() + " -> " + rsp.toString() + " ===> " + ae.getMessage(), ae); + } + } + } + } + + public void testIntervalWithMincount() throws Exception { + for (boolean doSubFacet : Arrays.asList(false, true)) { + final Integer subFacetLimit = pickSubFacetLimit(doSubFacet); + final CharSequence subFacet = makeSubFacet(subFacetLimit); + + long mincount_to_use = -1; + Object expected_mincount_bucket_val = null; + + // without mincount + SolrQuery solrQuery = new SolrQuery( + "q", "*:*", "rows", "0", "json.facet", + "{ foo:{ type:range, field:" + INT_FIELD + " ranges:[{from:1, to:3},{from:3, to:5}]" + + subFacet + " } }" + ); + + QueryResponse rsp = cluster.getSolrClient().query(solrQuery); + try { + final NamedList foo = ((NamedList>)rsp.getResponse().get("facets")).get("foo"); + final List> buckets = (List>) foo.get("buckets"); + + assertEquals("num buckets", 2, buckets.size()); + + // upper is not included + assertBucket("bucket#0", "[1,3)", modelVals(1,2), subFacetLimit, buckets.get(0)); + assertBucket("bucket#1", "[3,5)", modelVals(3,4), subFacetLimit, buckets.get(1)); + + // if we've made it this far, then our buckets match the model + // now use our buckets to pick a mincount to use based on the MIN(+1) count seen + long count0 = ((Number)buckets.get(0).get("count")).longValue(); + long count1 = ((Number)buckets.get(1).get("count")).longValue(); + + mincount_to_use = 1 + Math.min(count0, count1); + if (count0 > count1) { + expected_mincount_bucket_val = buckets.get(0).get("val"); + } else if (count1 > count0) { + expected_mincount_bucket_val = buckets.get(1).get("val"); + } + + } catch (AssertionError|RuntimeException ae) { + throw new AssertionError(solrQuery.toString() + " -> " + rsp.toString() + " ===> " + ae.getMessage(), ae); + } + + // with mincount + solrQuery = new SolrQuery( + "q", "*:*", "rows", "0", "json.facet", + "{ foo:{ type:range, field:" + INT_FIELD + " ranges:[{from:1, to:3},{from:3, to:5}]" + + ",mincount:" + mincount_to_use + subFacet + " } }" + ); + + rsp = cluster.getSolrClient().query(solrQuery); + try { + final NamedList foo = ((NamedList>)rsp.getResponse().get("facets")).get("foo"); + final List> buckets = (List>) foo.get("buckets"); + + if (null == expected_mincount_bucket_val) { + assertEquals("num buckets", 0, buckets.size()); + } else { + assertEquals("num buckets", 1, buckets.size()); + final Object actualBucket = buckets.get(0); + if (expected_mincount_bucket_val.equals("[1,3)")) { + assertBucket("bucket#0(0)", "[1,3)", modelVals(1,2), subFacetLimit, actualBucket); + } else { + assertBucket("bucket#0(1)", "[3,5)", modelVals(3,4), subFacetLimit, actualBucket); + } + } + } catch (AssertionError|RuntimeException ae) { + throw new AssertionError(solrQuery.toString() + " -> " + rsp.toString() + " ===> " + ae.getMessage(), ae); + } + } + } + /** * Helper method for validating a single 'bucket' from a Range facet. * @@ -592,7 +718,7 @@ public class RangeFacetCloudTest extends SolrCloudTestCase { * @param actualBucket the actual bucket returned from a query for all assertions to be conducted against. */ private static void assertBucket(final String label, - final Integer expectedVal, + final Object expectedVal, final ModelRange expectedRangeValues, final Integer subFacetLimitUsed, final Object actualBucket) { @@ -614,7 +740,7 @@ public class RangeFacetCloudTest extends SolrCloudTestCase { expectedCount += RANGE_MODEL[i]; toMerge.add(TERM_MODEL[i]); } - + assertEqualsHACK("count", expectedCount, bucket.get("count")); // merge the maps of our range values by summing the (int) values on key collisions @@ -650,7 +776,7 @@ public class RangeFacetCloudTest extends SolrCloudTestCase { } /** - * A convinience method for calling {@link #assertBucket} on the before/after/between buckets + * A convenience method for calling {@link #assertBucket} on the before/after/between buckets * of a facet result, based on the {@link FacetRangeOther} specified for this facet. * * @see #assertBucket @@ -686,7 +812,7 @@ public class RangeFacetCloudTest extends SolrCloudTestCase { private static final class ModelRange { public final int lower; public final int upper; - /** Don't use, use the convinience methods */ + /** Don't use, use the convenience methods */ public ModelRange(int lower, int upper) { if (lower < 0 || upper < 0) { assert(lower < 0 && upper < lower); @@ -771,13 +897,13 @@ public class RangeFacetCloudTest extends SolrCloudTestCase { String val = other.toString(); if (random().nextBoolean()) { // two valid syntaxes to randomize between: - // - a JSON list of items (conviniently the default toString of EnumSet), - // - a single quoted string containing the comma seperated list + // - a JSON list of items (conveniently the default toString of EnumSet), + // - a single quoted string containing the comma separated list val = val.replaceAll("\\[|\\]","'"); // HACK: work around SOLR-12539... // - // when sending a single string containing a comma seperated list of values, JSON Facets 'other' + // when sending a single string containing a comma separated list of values, JSON Facets 'other' // parsing can't handle any leading (or trailing?) whitespace val = val.replaceAll("\\s",""); } diff --git a/solr/core/src/test/org/apache/solr/search/facet/TestJsonFacetRefinement.java b/solr/core/src/test/org/apache/solr/search/facet/TestJsonFacetRefinement.java index 40dabea228b..461611cecb2 100644 --- a/solr/core/src/test/org/apache/solr/search/facet/TestJsonFacetRefinement.java +++ b/solr/core/src/test/org/apache/solr/search/facet/TestJsonFacetRefinement.java @@ -211,7 +211,7 @@ public class TestJsonFacetRefinement extends SolrTestCaseHS { null, null ); - + // same test, but nested in a terms facet doTestRefine("{top:{type:terms, field:Afield, facet:{x : {type:terms, field:X, limit:2, refine:true} } } }", "{top: {buckets:[{val:'A', count:2, x:{buckets:[{val:x1, count:5},{val:x2, count:3}], more:true} } ] } }", @@ -290,7 +290,39 @@ public class TestJsonFacetRefinement extends SolrTestCaseHS { // refinement... null, null); - + + // same test, but nested in range facet with ranges + doTestRefine("{top:{type:range, field:R, ranges:[{from:0, to:1}], facet:{x : {type:terms, field:X, limit:2, refine:true} } } }", + "{top: {buckets:[{val:\"[0,1)\", count:2, x:{buckets:[{val:x1, count:5},{val:x2, count:3}],more:true} } ] } }", + "{top: {buckets:[{val:\"[0,1)\", count:1, x:{buckets:[{val:x2, count:4},{val:x3, count:2}],more:true} } ] } }", + null, + "=={top: {" + + "_s:[ [\"[0,1)\" , {x:{_l:[x1]}} ] ]" + + " } " + + "}" + ); + + doTestRefine("{top:{type:range, field:R, ranges:[{from:\"*\", to:1}], facet:{x : {type:terms, field:X, limit:2, refine:true} } } }", + "{top: {buckets:[{val:\"[*,1)\", count:2, x:{buckets:[{val:x1, count:5},{val:x2, count:3}],more:true} } ] } }", + "{top: {buckets:[{val:\"[*,1)\", count:1, x:{buckets:[{val:x2, count:4},{val:x3, count:2}],more:true} } ] } }", + null, + "=={top: {" + + "_s:[ [\"[*,1)\" , {x:{_l:[x1]}} ] ]" + + " } " + + "}" + ); + + // a range facet w/o any sub facets shouldn't require any refinement + // other and include ignored for ranges + doTestRefine("{top:{type:range, other:all, field:R, ranges:[{from:0, to:2},{from:2, to:3}] } }" + + // phase #1 + "{top: {buckets:[{val:\"[0,2)\", count:2}, {val:\"[2,3)\", count:2}]," + + " } }", + "{top: {buckets:[{val:\"[0,2)\", count:2}, {val:\"[2,3)\", count:19}]," + + " } }", + // refinement... + null, + null); // for testing partial _p, we need a partial facet within a partial facet doTestRefine("{top:{type:terms, field:Afield, refine:true, limit:1, facet:{x : {type:terms, field:X, limit:1, refine:true} } } }", 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 b70c8ddd7b2..8658c2cc8ab 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 @@ -295,7 +295,7 @@ public class TestJsonFacets extends SolrTestCaseHS { ); } } - + /** * whitebox sanity checks that a shard request range facet that returns "between" or "after" * will cause the correct "actual_end" to be returned @@ -3207,6 +3207,256 @@ public class TestJsonFacets extends SolrTestCaseHS { } + @Test + public void testRangeFacetWithRanges() throws Exception { + Client client = Client.localClient(); + client.deleteByQuery("*:*", null); + indexSimple(client); + + final SolrParams p = params("q", "*:*", "rows", "0"); + // with lower and upper include + client.testJQ(params(p, "json.facet" + , "{price:{type : range,field : num_i, ranges:[{range:\" [-5,7] \"}]}}"), + "facets=={count:6, price:{buckets:[{val:\"[-5,7]\",count:5}]}}"); + + // with lower include and upper exclude + client.testJQ(params(p, "json.facet" + , "{price:{type : range,field : num_i,ranges:[{range:\"[-5,7)\"}]}}"), + "facets=={count:6, price:{buckets:[{val:\"[-5,7)\",count:4}]}}"); + + // with lower exclude and upper include + client.testJQ(params(p, "json.facet" + , "{price:{type : range,field : num_i,ranges:[{range:\"(-5,7]\"}]}}"), + "facets=={count:6, price:{buckets:[{val:\"(-5,7]\",count:3}]}}"); + + // with lower and upper exclude + client.testJQ(params(p, "json.facet" + , "{price:{type : range,field : num_i,ranges:[{range:\"(-5,7)\"}]}}"), + "facets=={count:6, price:{buckets:[{val:\"(-5,7)\",count:2}]}}"); + + // with other and include, they are not supported + // but wouldn't throw any error as they are not consumed + client.testJQ(params(p, "json.facet" + , "{price:{type : range,field : num_i,ranges:[{range:\"(-5,7)\"}],include:\"lower\",other:[\"after\"]}}"), + "facets=={count:6, price:{buckets:[{val:\"(-5,7)\",count:2}]}}"); + + // with mincount>0 + client.testJQ( + params(p, "json.facet", "{price:{type : range,field : num_i,mincount:3," + + "ranges:[{range:\"(-5,7)\"},{range:\"(-5,7]\"}]}}" + ), + "facets=={count:6, price:{buckets:[{val:\"(-5,7]\",count:3}]}}"); + + // with multiple ranges + client.testJQ( + params(p, "json.facet", "{price:{type : range,field : num_i," + + "ranges:[{range:\"(-5,7)\"},{range:\"(-5,7]\"}]}}" + ), + "facets=={count:6, price:{buckets:[{val:\"(-5,7)\",count:2},{val:\"(-5,7]\",count:3}]}}"); + + // with * as one of the values + client.testJQ(params(p, "json.facet" + , "{price:{type : range,field : num_i,ranges:[{range:\"(*,10]\"}]}}"), + "facets=={count:6, price:{buckets:[{val:\"(*,10]\",count:5}]}}"); + client.testJQ(params(p, "json.facet" + , "{price:{type : range,field : num_i,ranges:[{range:\"[-5,*)\"}]}}"), + "facets=={count:6, price:{buckets:[{val:\"[-5,*)\",count:5}]}}"); + client.testJQ(params(p, "json.facet" + , "{price:{type : range,field : num_i,ranges:[{range:\"[*,*]\"}]}}"), + "facets=={count:6, price:{buckets:[{val:\"[*,*]\",count:5}]}}"); + } + + @Test + public void testRangeFacetWithRangesInNewFormat() throws Exception { + Client client = Client.localClient(); + client.deleteByQuery("*:*", null); + indexSimple(client); + SolrParams p = params("q", "*:*", "rows", "0"); + + //case without inclusive params + client.testJQ(params(p, "json.facet" + , "{price:{type : range,field : num_i,ranges:[{from:-5, to:7}]}}"), + "facets=={count:6, price:{buckets:[{val:\"[-5,7)\",count:4}]}}"); + + //case without key param and to included + client.testJQ(params(p, "json.facet" + , "{price:{type : range,field : num_i,ranges:[{from:-5, to:7,inclusive_from:true ,inclusive_to:true}]}}"), + "facets=={count:6, price:{buckets:[{val:\"[-5,7]\",count:5}]}}"); + + //case with all params + client.testJQ(params(p, "json.facet" + , "{price:{type : range,field : num_i,ranges:[{from:-5, to:7,inclusive_from:true ,inclusive_to:true}]}}"), + "facets=={count:6, price:{buckets:[{val:\"[-5,7]\",count:5}]}}"); + + // from and to excluded + client.testJQ(params(p, "json.facet" + , "{price:{type : range,field : num_i,ranges:[{from:-5, to:7,inclusive_from:false ,inclusive_to:false}]}}"), + "facets=={count:6, price:{buckets:[{val:\"(-5,7)\",count:2}]}}"); + + // from excluded and to included + client.testJQ(params(p, "json.facet" + , "{price:{type : range,field : num_i,ranges:[{from:-5, to:7,inclusive_from:false ,inclusive_to:true}]}}"), + "facets=={count:6, price:{buckets:[{val:\"(-5,7]\",count:3}]}}"); + + // multiple ranges + client.testJQ(params(p, "json.facet" + , "{price:{type : range,field : num_i,include:[\"lower\"], outer:\"before\"," + + "ranges:[{from:-5, to:7,inclusive_from:false ,inclusive_to:true},{from:-5, to:7,inclusive_from:false ,inclusive_to:false}]}}"), + "facets=={count:6, price:{buckets:[{val:\"(-5,7]\",count:3},{val:\"(-5,7)\",count:2}]}}"); + + // with mincount>0 + client.testJQ(params(p, "json.facet" + , "{price:{type : range,field : num_i,mincount:3" + + "ranges:[{from:-5, to:7,inclusive_from:false ,inclusive_to:true},{from:-5, to:7,inclusive_from:false ,inclusive_to:false}]}}"), + "facets=={count:6, price:{buckets:[{val:\"(-5,7]\",count:3}]}}"); + + // mix of old and new formats + client.testJQ(params(p, "json.facet" + , "{price:{type : range,field : num_i," + + "ranges:[{from:-5, to:7,inclusive_from:false ,inclusive_to:true},{range:\"(-5,7)\"}]}}"), + "facets=={count:6, price:{buckets:[{val:\"(-5,7]\",count:3},{val:\"(-5,7)\",count:2}]}}"); + + // from==to + client.testJQ(params(p, "json.facet" + , "{price:{type : range,field : num_i,ranges:[{from:-5, to:-5,inclusive_from:false ,inclusive_to:true}]}}"), + "facets=={count:6, price:{buckets:[{val:\"(-5,-5]\",count:0}]}}"); + client.testJQ(params(p, "json.facet" + , "{price:{type : range,field : num_i,ranges:[{from:-5, to:-5,inclusive_from:false ,inclusive_to:false}]}}"), + "facets=={count:6, price:{buckets:[{val:\"(-5,-5)\",count:0}]}}"); + client.testJQ(params(p, "json.facet" + , "{price:{type : range,field : num_i,ranges:[{from:-5, to:-5,inclusive_from:true ,inclusive_to:false}]}}"), + "facets=={count:6, price:{buckets:[{val:\"[-5,-5)\",count:0}]}}"); + client.testJQ(params(p, "json.facet" + , "{price:{type : range,field : num_i,ranges:[{from:-5, to:-5,inclusive_from:true ,inclusive_to:true}]}}"), + "facets=={count:6, price:{buckets:[{val:\"[-5,-5]\",count:2}]}}"); + + // with * as one of the values + client.testJQ(params(p, "json.facet" + , "{price:{type : range,field : num_i,ranges:[{from:\"*\", to:10,inclusive_from:false ,inclusive_to:true}]}}"), + "facets=={count:6, price:{buckets:[{val:\"(*,10]\",count:5}]}}"); + client.testJQ(params(p, "json.facet" + , "{price:{type : range,field : num_i,ranges:[{from:-5, to:\"*\",inclusive_from:true ,inclusive_to:false}]}}"), + "facets=={count:6, price:{buckets:[{val:\"[-5,*)\",count:5}]}}"); + client.testJQ(params(p, "json.facet" + , "{price:{type : range,field : num_i,ranges:[{from:-5,inclusive_from:true ,inclusive_to:false}]}}"), + "facets=={count:6, price:{buckets:[{val:\"[-5,*)\",count:5}]}}"); + client.testJQ(params(p, "json.facet" + , "{price:{type : range,field : num_i,ranges:[{from:\"*\", to:\"*\",inclusive_from:true ,inclusive_to:false}]}}"), + "facets=={count:6, price:{buckets:[{val:\"[*,*)\",count:5}]}}"); + client.testJQ(params(p, "json.facet" + , "{price:{type : range,field : num_i,ranges:[{inclusive_from:true ,inclusive_to:false}]}}"), + "facets=={count:6, price:{buckets:[{val:\"[*,*)\",count:5}]}}"); + } + + @Test + public void testRangeFacetsErrorCases() throws Exception { + Client client = Client.localClient(); + client.deleteByQuery("*:*", null); + indexSimple(client); + + SolrParams params = params("q", "*:*", "rows", "0"); + + // invalid format for ranges + SolrException ex = expectThrows(SolrException.class, + () -> h.query(req(params, "json.facet", "{price:{type :range, field : num_i,start:-10,end:10,gap:2," + + "ranges:[{key:\"0-200\", to:200}]}}")) + ); + assertEquals(SolrException.ErrorCode.BAD_REQUEST.code, ex.code()); + assertEquals("Cannot set gap/start/end and ranges params together", ex.getMessage()); + + ex = expectThrows(SolrException.class, + () -> h.query(req(params, "json.facet", "{price:{type :range, field : num_i," + + "ranges:bleh}}")) + ); + assertEquals(SolrException.ErrorCode.BAD_REQUEST.code, ex.code()); + assertTrue(ex.getMessage().contains("Expected List for ranges but got String")); + + ex = expectThrows(SolrException.class, + () -> h.query(req(params, "json.facet", "{price:{type :range, field : num_i," + + "ranges:[bleh]}}")) + ); + assertEquals(SolrException.ErrorCode.BAD_REQUEST.code, ex.code()); + assertTrue(ex.getMessage().contains("Expected Map for range but got String")); + + ex = expectThrows(SolrException.class, + () -> h.query(req(params, "json.facet", "{price:{type :range, field : num_i," + + "ranges:[{from:0, to:200, inclusive_to:bleh}]}}")) + ); + assertEquals(SolrException.ErrorCode.BAD_REQUEST.code, ex.code()); + assertTrue(ex.getMessage().contains("Expected boolean type for param 'inclusive_to' but got String")); + + ex = expectThrows(SolrException.class, + () -> h.query(req(params, "json.facet", "{price:{type :range, field : num_i," + + "ranges:[{from:0, to:200, inclusive_from:bleh}]}}")) + ); + assertEquals(SolrException.ErrorCode.BAD_REQUEST.code, ex.code()); + assertTrue(ex.getMessage().contains("Expected boolean type for param 'inclusive_from' but got String")); + + ex = expectThrows(SolrException.class, + () -> h.query(req(params, "json.facet", "{price:{type :range, field : num_i," + + "ranges:[{from:bleh, to:200}]}}")) + ); + assertEquals(SolrException.ErrorCode.BAD_REQUEST.code, ex.code()); + assertEquals("Can't parse value bleh for field: num_i", ex.getMessage()); + + ex = expectThrows(SolrException.class, + () -> h.query(req(params, "json.facet", "{price:{type :range, field : num_i," + + "ranges:[{from:0, to:bleh}]}}")) + ); + assertEquals(SolrException.ErrorCode.BAD_REQUEST.code, ex.code()); + assertEquals("Can't parse value bleh for field: num_i", ex.getMessage()); + + ex = expectThrows(SolrException.class, + () -> h.query(req(params, "json.facet", "{price:{type :range, field : num_i," + + "ranges:[{from:200, to:0}]}}")) + ); + assertEquals(SolrException.ErrorCode.BAD_REQUEST.code, ex.code()); + assertEquals("'from' is higher than 'to' in range for key: [200,0)", ex.getMessage()); + + // with old format + ex = expectThrows(SolrException.class, + () -> h.query(req(params, "json.facet", "{price:{type :range, field : num_i," + + "ranges:[{range:\"\"}]}}")) + ); + assertEquals(SolrException.ErrorCode.BAD_REQUEST.code, ex.code()); + assertTrue(ex.getMessage().contains("empty facet range")); + + ex = expectThrows(SolrException.class, + () -> h.query(req(params, "json.facet", "{price:{type :range, field : num_i," + + "ranges:[{range:\"bl\"}]}}")) + ); + assertEquals(SolrException.ErrorCode.BAD_REQUEST.code, ex.code()); + assertTrue(ex.getMessage().contains("Invalid start character b in facet range bl")); + + ex = expectThrows(SolrException.class, + () -> h.query(req(params, "json.facet", "{price:{type :range, field : num_i," + + "ranges:[{range:\"(bl\"}]}}")) + ); + assertEquals(SolrException.ErrorCode.BAD_REQUEST.code, ex.code()); + assertTrue(ex.getMessage().contains("Invalid end character l in facet range (bl")); + + ex = expectThrows(SolrException.class, + () -> h.query(req(params, "json.facet", "{price:{type :range, field : num_i," + + "ranges:[{range:\"(bleh,12)\"}]}}")) + ); + assertEquals(SolrException.ErrorCode.BAD_REQUEST.code, ex.code()); + assertEquals("Can't parse value bleh for field: num_i", ex.getMessage()); + + ex = expectThrows(SolrException.class, + () -> h.query(req(params, "json.facet", "{price:{type :range, field : num_i," + + "ranges:[{range:\"(12,bleh)\"}]}}")) + ); + assertEquals(SolrException.ErrorCode.BAD_REQUEST.code, ex.code()); + assertEquals("Can't parse value bleh for field: num_i", ex.getMessage()); + + ex = expectThrows(SolrException.class, + () -> h.query(req(params, "json.facet", "{price:{type :range, field : num_i," + + "ranges:[{range:\"(200,12)\"}]}}")) + ); + assertEquals(SolrException.ErrorCode.BAD_REQUEST.code, ex.code()); + assertEquals("'start' is higher than 'end' in range for key: (200,12)", ex.getMessage()); + } + @Test public void testOtherErrorCases() throws Exception { Client client = Client.localClient();