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();