This is an automated email from the ASF dual-hosted git repository. dweiss pushed a commit to branch jira/solr-13105-toMerge in repository https://gitbox.apache.org/repos/asf/solr.git
commit e054ad740d698097a78a8092790af83cf3012961 Author: Munendra S N <[email protected]> AuthorDate: Sun Nov 29 05:16:35 2020 +0530 SOLR-12559: fix error when multi-val fields are derefernced in JSON aggs This ensures all derefernced fields are not parsed into actual valuesource but parsed into a placeholder value. This works for 1-level of dereferencing --- solr/CHANGES.txt | 3 ++ .../org/apache/solr/search/FunctionQParser.java | 26 ++++++---- .../apache/solr/search/facet/TestJsonFacets.java | 14 +++--- .../search/facet/TestJsonFacetsStatsParsing.java | 56 ++++++++++++---------- 4 files changed, 55 insertions(+), 44 deletions(-) diff --git a/solr/CHANGES.txt b/solr/CHANGES.txt index 79610ad..68886d1 100644 --- a/solr/CHANGES.txt +++ b/solr/CHANGES.txt @@ -322,6 +322,9 @@ Bug Fixes * SOLR-14514: Avoid picking 'stream' method in JSON facet when any of 'allBuckets', 'numBuckets', and 'missing' parameters are enabled (hossman, Munendra S N) +* SOLR-12559: Fix placeholder valuesource 'FIELDNAME' not working with de-referenced values in JSON aggregator parsing. + (hossman, Munendra S N) + Other Changes --------------------- diff --git a/solr/core/src/java/org/apache/solr/search/FunctionQParser.java b/solr/core/src/java/org/apache/solr/search/FunctionQParser.java index 60cbecf..73f6d7a 100644 --- a/solr/core/src/java/org/apache/solr/search/FunctionQParser.java +++ b/solr/core/src/java/org/apache/solr/search/FunctionQParser.java @@ -356,17 +356,23 @@ public class FunctionQParser extends QParser { throw new SyntaxError("Missing param " + param + " while parsing function '" + sp.val + "'"); } - QParser subParser = subQuery(val, "func"); - if (subParser instanceof FunctionQParser) { - ((FunctionQParser)subParser).setParseMultipleSources(true); - } - Query subQuery = subParser.getQuery(); - if (subQuery == null) { - valueSource = new ConstValueSource(0.0f); - } else if (subQuery instanceof FunctionQuery) { - valueSource = ((FunctionQuery) subQuery).getValueSource(); + if ((flags & FLAG_USE_FIELDNAME_SOURCE) != 0 && req.getSchema().getFieldOrNull(val) != null) { + // Don't try to create a ValueSource for the field, just use a placeholder. + // this handles the case like x=myfunc($qq)&qq=something + valueSource = new FieldNameValueSource(val); } else { - valueSource = new QueryValueSource(subQuery, 0.0f); + QParser subParser = subQuery(val, "func"); + if (subParser instanceof FunctionQParser) { + ((FunctionQParser) subParser).setParseMultipleSources(true); + } + Query subQuery = subParser.getQuery(); + if (subQuery == null) { + valueSource = new ConstValueSource(0.0f); + } else if (subQuery instanceof FunctionQuery) { + valueSource = ((FunctionQuery) subQuery).getValueSource(); + } else { + valueSource = new QueryValueSource(subQuery, 0.0f); + } } /*** 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 166968b..179661b 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 @@ -1123,8 +1123,6 @@ public class TestJsonFacets extends SolrTestCaseHS { fieldLists.put("sparse_s", getAlternatives("sparse_s")); fieldLists.put("multi_ss", getAlternatives("multi_ss")); - // TODO: if a field will be used as a function source, we can't use multi-valued types for it (currently) - int maxAlt = 0; for (List<String> fieldList : fieldLists.values()) { maxAlt = Math.max(fieldList.size(), maxAlt); @@ -1923,24 +1921,24 @@ public class TestJsonFacets extends SolrTestCaseHS { ); // stats at top level on multi-valued fields - client.testJQ(params(p, "q", "*:*" + client.testJQ(params(p, "q", "*:*", "myfield", "${multi_ss}" , "json.facet", "{ sum1:'sum(${num_fs})', sumsq1:'sumsq(${num_fs})', avg1:'avg(${num_fs})', mind:'min(${num_fs})', maxd:'max(${num_fs})'" + ", mini:'min(${num_is})', maxi:'max(${num_is})', mins:'min(${multi_ss})', maxs:'max(${multi_ss})'" + ", stddev:'stddev(${num_fs})', variance:'variance(${num_fs})', median:'percentile(${num_fs}, 50)'" + - ", perc:'percentile(${num_fs}, 0,75,100)'" + + ", perc:'percentile(${num_fs}, 0,75,100)', maxss:'max($multi_ss)'" + " }" ) , "facets=={ 'count':6, " + "sum1:0.0, sumsq1:51.5, avg1:0.0, mind:-5.0, maxd:3.0" + ", mini:-5, maxi:3, mins:'a', maxs:'b'" + - ", stddev:2.712405363721075, variance:7.3571428571, median:0.0, perc:[-5.0,2.25,3.0]" + + ", stddev:2.712405363721075, variance:7.3571428571, median:0.0, perc:[-5.0,2.25,3.0], maxss:'b'" + "}" ); // test sorting by multi-valued - client.testJQ(params(p, "q", "*:*" - , "json.facet", "{f1:{terms:{${terms} field:'${cat_s}', sort:'n1 desc', facet:{n1:'avg(${num_is})'} }}" + - " , f2:{terms:{${terms} field:'${cat_s}', sort:'n1 asc', facet:{n1:'avg(${num_is})'} }} }" + client.testJQ(params(p, "q", "*:*", "my_field", "${num_is}" + , "json.facet", "{f1:{terms:{${terms} field:'${cat_s}', sort:'n1 desc', facet:{n1:'avg($my_field)'} }}" + + " , f2:{terms:{${terms} field:'${cat_s}', sort:'n1 asc', facet:{n1:'avg($my_field)'} }} }" ) , "facets=={ 'count':6, " + " f1:{ 'buckets':[{ val:'B', count:3, n1: 0.25}, { val:'A', count:2, n1:0.0}]}" + diff --git a/solr/core/src/test/org/apache/solr/search/facet/TestJsonFacetsStatsParsing.java b/solr/core/src/test/org/apache/solr/search/facet/TestJsonFacetsStatsParsing.java index 24ca10f..ed75cf5 100644 --- a/solr/core/src/test/org/apache/solr/search/facet/TestJsonFacetsStatsParsing.java +++ b/solr/core/src/test/org/apache/solr/search/facet/TestJsonFacetsStatsParsing.java @@ -20,16 +20,17 @@ import java.io.IOException; import java.util.Map; import org.apache.lucene.queries.function.valuesource.IntFieldSource; - import org.apache.solr.SolrTestCaseJ4; import org.apache.solr.common.util.Utils; import org.apache.solr.request.SolrQueryRequest; +import org.apache.solr.search.function.FieldNameValueSource; +import org.hamcrest.MatcherAssert; import org.junit.BeforeClass; +import org.noggit.ObjectBuilder; + import static org.hamcrest.CoreMatchers.not; import static org.hamcrest.core.IsInstanceOf.instanceOf; -import org.noggit.ObjectBuilder; - /** Whitebox test of the various syntaxes for specifying stats in JSON Facets */ public class TestJsonFacetsStatsParsing extends SolrTestCaseJ4 { @@ -56,44 +57,47 @@ public class TestJsonFacetsStatsParsing extends SolrTestCaseJ4 { try (SolrQueryRequest req = req("custom_req_param","foo_i", "overridden_param","xxxxx_i")) { - // NOTE: we don't bother trying to test 'min(foo_i)' because of SOLR-12559 - // ...once that bug is fixed, several assertions below will need to change @SuppressWarnings({"unchecked"}) final FacetRequest fr = FacetRequest.parse (req, (Map<String,Object>) Utils.fromJSONString ("{ " + - " s1:'min(field(\"foo_i\"))', " + - " s2:'min($custom_req_param)', " + - " s3:'min(field($custom_req_param))', " + - " s4:{ func:'min($custom_req_param)' }, " + - " s5:{ type:func, func:'min($custom_req_param)' }, " + - " s6:{ type:func, func:'min($custom_local_param)', custom_local_param:foo_i }, " + - " s7:{ type:func, func:'min($overridden_param)', overridden_param:foo_i }, " + - // test the test... - " diff:'min(field(\"bar_i\"))'," + - "}")); + // with valuesource + " f1:'min(field(\"foo_i\"))', " + + " f2:'min(field($custom_req_param))', " + + // with fieldName and query de-reference + " s1:'min(foo_i)', " + + " s2:'min($custom_req_param)', " + + " s3:{ func:'min($custom_req_param)' }, " + + " s4:{ type:func, func:'min($custom_req_param)' }, " + + " s5:{ type:func, func:'min($custom_local_param)', custom_local_param:foo_i }, " + + " s6:{ type:func, func:'min($overridden_param)', overridden_param:foo_i }, " + + // test the test... + " diff:'min(field(\"bar_i\"))'," + + "}")); final Map<String, AggValueSource> stats = fr.getFacetStats(); - assertEquals(8, stats.size()); + assertEquals(9, stats.size()); for (Map.Entry<String,AggValueSource> entry : stats.entrySet()) { final String key = entry.getKey(); final AggValueSource agg = entry.getValue(); assertEquals("name of " + key, "min", agg.name()); - assertThat("type of " + key, agg, instanceOf(SimpleAggValueSource.class)); + MatcherAssert.assertThat("type of " + key, agg, instanceOf(SimpleAggValueSource.class)); SimpleAggValueSource sagg = (SimpleAggValueSource) agg; - assertThat("vs of " + key, sagg.getArg(), instanceOf(IntFieldSource.class)); - - if ("diff".equals(key)) { - assertEquals("field of " + key, "bar_i", ((IntFieldSource)sagg.getArg()).getField()); - assertFalse("diff.equals(s1) ?!?!", agg.equals(stats.get("s1"))); - - } else { + + if (key.startsWith("f")) { // value source as arg to min + MatcherAssert.assertThat("vs of " + key, sagg.getArg(), instanceOf(IntFieldSource.class)); assertEquals("field of " + key, "foo_i", ((IntFieldSource)sagg.getArg()).getField()); - + assertEquals(key + ".equals(f1)", agg, stats.get("f1")); + } else if (key.startsWith("s")) { // field as arg to min + MatcherAssert.assertThat("vs of " + key, sagg.getArg(), instanceOf(FieldNameValueSource.class)); + assertEquals("field of " + key, "foo_i", ((FieldNameValueSource)sagg.getArg()).getFieldName()); assertEquals(key + ".equals(s1)", agg, stats.get("s1")); assertEquals("s1.equals("+key+")", stats.get("s1"), agg); + } else if ("diff".equals(key)) { + assertEquals("field of " + key, "bar_i", ((IntFieldSource)sagg.getArg()).getField()); + assertNotEquals("diff.equals(s1) ?!?!", agg, stats.get("f1")); } } } @@ -113,7 +117,7 @@ public class TestJsonFacetsStatsParsing extends SolrTestCaseJ4 { assertEquals(1, stats.size()); AggValueSource agg = stats.get("x"); assertNotNull(agg); - assertThat(agg, instanceOf(DebugAgg.class)); + MatcherAssert.assertThat(agg, instanceOf(DebugAgg.class)); DebugAgg x = (DebugAgg)agg; assertEquals(new String[] {"abc", "xyz"}, x.localParams.getParams("foo"));
