You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@lucene.apache.org by mu...@apache.org on 2021/01/07 16:47:47 UTC
[lucene-solr] branch branch_8x updated (e1e5509 -> 0b95557)
This is an automated email from the ASF dual-hosted git repository.
munendrasn pushed a change to branch branch_8x
in repository https://gitbox.apache.org/repos/asf/lucene-solr.git.
from e1e5509 Revert "SOLR-15052: Per-replica states for reducing overseer bottlenecks (branch_8x) (#2148)"
new 0d66c55 SOLR-10860: Return proper error code for bad input incase of inplace updates (#2121)
new 35ef6d2 SOLR-14950: fix regenerating of copyfield with explicit src/dest matching dyn rule
new 6e55dae SOLR-12539: handle extra spaces in JSON facet shorthand syntax
new 0b95557 SOLR-14514: add extra checks for picking 'stream' method in JSON facet
The 4 revisions listed above as "new" are entirely new to this
repository and will be described in separate emails. The revisions
listed as "add" were already present in the repository and have only
been added to this reference.
Summary of changes:
solr/CHANGES.txt | 12 +++
.../org/apache/solr/schema/ManagedIndexSchema.java | 5 +-
.../org/apache/solr/search/facet/FacetField.java | 5 +-
.../org/apache/solr/search/facet/FacetParser.java | 7 +-
.../processor/AtomicUpdateDocumentMerger.java | 52 +++++++----
.../apache/solr/rest/schema/TestBulkSchemaAPI.java | 102 +++++++++++++++++++++
.../solr/search/facet/RangeFacetCloudTest.java | 6 --
.../solr/search/facet/TestCloudJSONFacetSKG.java | 29 +++---
.../search/facet/TestCloudJSONFacetSKGEquiv.java | 23 ++---
.../apache/solr/search/facet/TestJsonFacets.java | 30 +++---
.../solr/update/TestInPlaceUpdatesDistrib.java | 18 +++-
.../solr/update/TestInPlaceUpdatesStandalone.java | 32 +++++++
.../solr/update/processor/AtomicUpdatesTest.java | 13 +++
solr/solr-ref-guide/src/json-facet-api.adoc | 2 +-
14 files changed, 260 insertions(+), 76 deletions(-)
[lucene-solr] 01/04: SOLR-10860: Return proper error code for bad
input incase of inplace updates (#2121)
Posted by mu...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
munendrasn pushed a commit to branch branch_8x
in repository https://gitbox.apache.org/repos/asf/lucene-solr.git
commit 0d66c55749aec24b1cb2bed438b3410d22122c48
Author: S N Munendra <mu...@apache.org>
AuthorDate: Thu Jan 7 20:44:48 2021 +0530
SOLR-10860: Return proper error code for bad input incase of inplace updates (#2121)
Return proper error code on invalid value with in-place update.
Handle invalid value for inc op with the in-place update, uses toNativeType to convert increment value instead of direct parsing. Also, return an error when inc operation is specified for the non-numeric field
---
solr/CHANGES.txt | 2 +
.../processor/AtomicUpdateDocumentMerger.java | 52 +++++++++++++++-------
.../solr/update/TestInPlaceUpdatesDistrib.java | 18 +++++++-
.../solr/update/TestInPlaceUpdatesStandalone.java | 32 +++++++++++++
.../solr/update/processor/AtomicUpdatesTest.java | 13 ++++++
5 files changed, 100 insertions(+), 17 deletions(-)
diff --git a/solr/CHANGES.txt b/solr/CHANGES.txt
index a581443..1f151dd 100644
--- a/solr/CHANGES.txt
+++ b/solr/CHANGES.txt
@@ -105,6 +105,8 @@ Bug Fixes
* SOLR-15047: Fix collapse parser behavior when collapsing on numeric fields to differentiate '0' group from null group (hossman)
+ * SOLR-10860: Return proper error code for bad input in case of inplace updates (Tomas Eduardo Fernandez Lobbe, Munendra S N)
+
Other Changes
---------------------
diff --git a/solr/core/src/java/org/apache/solr/update/processor/AtomicUpdateDocumentMerger.java b/solr/core/src/java/org/apache/solr/update/processor/AtomicUpdateDocumentMerger.java
index ffc52a7..89cf341 100644
--- a/solr/core/src/java/org/apache/solr/update/processor/AtomicUpdateDocumentMerger.java
+++ b/solr/core/src/java/org/apache/solr/update/processor/AtomicUpdateDocumentMerger.java
@@ -129,13 +129,8 @@ public class AtomicUpdateDocumentMerger {
doAddDistinct(toDoc, sif, fieldVal);
break;
default:
- Object id = toDoc.containsKey(idField.getName())? toDoc.getFieldValue(idField.getName()):
- fromDoc.getFieldValue(idField.getName());
- String err = "Unknown operation for the an atomic update, operation ignored: " + key;
- if (id != null) {
- err = err + " for id:" + id;
- }
- throw new SolrException(ErrorCode.BAD_REQUEST, err);
+ throw new SolrException(ErrorCode.BAD_REQUEST,
+ "Error:" + getID(toDoc, schema) + " Unknown operation for the an atomic update: " + key);
}
// validate that the field being modified is not the id field.
if (idField.getName().equals(sif.getName())) {
@@ -152,6 +147,15 @@ public class AtomicUpdateDocumentMerger {
return toDoc;
}
+ private static String getID(SolrInputDocument doc, IndexSchema schema) {
+ String id = "";
+ SchemaField sf = schema.getUniqueKeyField();
+ if (sf != null) {
+ id = "[doc="+doc.getFieldValue( sf.getName() )+"] ";
+ }
+ return id;
+ }
+
/**
* Given a schema field, return whether or not such a field is supported for an in-place update.
* Note: If an update command has updates to only supported fields (and _version_ is also supported),
@@ -479,6 +483,11 @@ public class AtomicUpdateDocumentMerger {
protected void doInc(SolrInputDocument toDoc, SolrInputField sif, Object fieldVal) {
SolrInputField numericField = toDoc.get(sif.getName());
SchemaField sf = schema.getField(sif.getName());
+
+ if (sf.getType().getNumberType() == null) {
+ throw new SolrException(ErrorCode.BAD_REQUEST, "'inc' is not supported on non-numeric field " + sf.getName());
+ }
+
if (numericField != null || sf.getDefaultValue() != null) {
// TODO: fieldtype needs externalToObject?
String oldValS = (numericField != null) ?
@@ -487,20 +496,23 @@ public class AtomicUpdateDocumentMerger {
sf.getType().readableToIndexed(oldValS, term);
Object oldVal = sf.getType().toObject(sf, term.get());
- String fieldValS = fieldVal.toString();
- Number result;
+ // behavior similar to doAdd/doSet
+ Object resObj = getNativeFieldValue(sf.getName(), fieldVal);
+ if (!(resObj instanceof Number)) {
+ throw new SolrException(ErrorCode.BAD_REQUEST, "Invalid input '" + resObj + "' for field " + sf.getName());
+ }
+ Number result = (Number)resObj;
if (oldVal instanceof Long) {
- result = ((Long) oldVal).longValue() + Long.parseLong(fieldValS);
+ result = ((Long) oldVal).longValue() + result.longValue();
} else if (oldVal instanceof Float) {
- result = ((Float) oldVal).floatValue() + Float.parseFloat(fieldValS);
+ result = ((Float) oldVal).floatValue() + result.floatValue();
} else if (oldVal instanceof Double) {
- result = ((Double) oldVal).doubleValue() + Double.parseDouble(fieldValS);
+ result = ((Double) oldVal).doubleValue() + result.doubleValue();
} else {
// int, short, byte
- result = ((Integer) oldVal).intValue() + Integer.parseInt(fieldValS);
+ result = ((Integer) oldVal).intValue() + result.intValue();
}
-
- toDoc.setField(sif.getName(), result);
+ toDoc.setField(sif.getName(), result);
} else {
toDoc.setField(sif.getName(), fieldVal);
}
@@ -562,7 +574,15 @@ public class AtomicUpdateDocumentMerger {
return val;
}
SchemaField sf = schema.getField(fieldName);
- return sf.getType().toNativeType(val);
+ try {
+ return sf.getType().toNativeType(val);
+ } catch (SolrException ex) {
+ throw new SolrException(SolrException.ErrorCode.getErrorCode(ex.code()),
+ "Error converting field '" + sf.getName() + "'='" +val+"' to native type, msg=" + ex.getMessage(), ex);
+ } catch (Exception ex) {
+ throw new SolrException( SolrException.ErrorCode.BAD_REQUEST,
+ "Error converting field '" + sf.getName() + "'='" +val+"' to native type, msg=" + ex.getMessage(), ex);
+ }
}
private static boolean isChildDoc(Object obj) {
diff --git a/solr/core/src/test/org/apache/solr/update/TestInPlaceUpdatesDistrib.java b/solr/core/src/test/org/apache/solr/update/TestInPlaceUpdatesDistrib.java
index 3f13afa..7954633 100644
--- a/solr/core/src/test/org/apache/solr/update/TestInPlaceUpdatesDistrib.java
+++ b/solr/core/src/test/org/apache/solr/update/TestInPlaceUpdatesDistrib.java
@@ -47,6 +47,7 @@ import org.apache.solr.cloud.AbstractFullDistribZkTestBase;
import org.apache.solr.cloud.ZkShardTerms;
import org.apache.solr.common.SolrDocument;
import org.apache.solr.common.SolrDocumentList;
+import org.apache.solr.common.SolrException;
import org.apache.solr.common.SolrInputDocument;
import org.apache.solr.common.cloud.ClusterState;
import org.apache.solr.common.cloud.DocCollection;
@@ -56,18 +57,21 @@ import org.apache.solr.common.cloud.ZkStateReader;
import org.apache.solr.common.params.SolrParams;
import org.apache.solr.common.util.ExecutorUtil;
import org.apache.solr.common.util.NamedList;
+import org.apache.solr.common.util.SolrNamedThreadFactory;
import org.apache.solr.common.util.TimeSource;
import org.apache.solr.index.NoMergePolicyFactory;
import org.apache.solr.update.processor.DistributedUpdateProcessor;
-import org.apache.solr.common.util.SolrNamedThreadFactory;
import org.apache.solr.util.RefCounted;
import org.apache.solr.util.TimeOut;
import org.apache.zookeeper.KeeperException;
+import org.hamcrest.MatcherAssert;
import org.junit.BeforeClass;
import org.junit.Test;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
+import static org.hamcrest.core.StringContains.containsString;
+
/**
* Tests the in-place updates (docValues updates) for a one shard, three replica cluster.
*/
@@ -684,6 +688,18 @@ public class TestInPlaceUpdatesDistrib extends AbstractFullDistribZkTestBase {
assertTrue(currentVersion > version);
version = currentVersion;
+ // set operation with invalid value for field
+ SolrException e = expectThrows(SolrException.class,
+ () -> addDocAndGetVersion( "id", 100, "inplace_updatable_float", map("set", "NOT_NUMBER")));
+ assertEquals(SolrException.ErrorCode.BAD_REQUEST.code, e.code());
+ MatcherAssert.assertThat(e.getMessage(), containsString("For input string: \"NOT_NUMBER\""));
+
+ // inc operation with invalid inc value
+ e = expectThrows(SolrException.class,
+ () -> addDocAndGetVersion( "id", 100, "inplace_updatable_int", map("inc", "NOT_NUMBER")));
+ assertEquals(SolrException.ErrorCode.BAD_REQUEST.code, e.code());
+ MatcherAssert.assertThat(e.getMessage(), containsString("For input string: \"NOT_NUMBER\""));
+
// RTG from tlog(s)
for (SolrClient client : clients) {
final String clientDebug = client.toString() + (LEADER.equals(client) ? " (leader)" : " (not leader)");
diff --git a/solr/core/src/test/org/apache/solr/update/TestInPlaceUpdatesStandalone.java b/solr/core/src/test/org/apache/solr/update/TestInPlaceUpdatesStandalone.java
index 2867935..91586ca 100644
--- a/solr/core/src/test/org/apache/solr/update/TestInPlaceUpdatesStandalone.java
+++ b/solr/core/src/test/org/apache/solr/update/TestInPlaceUpdatesStandalone.java
@@ -18,6 +18,7 @@
package org.apache.solr.update;
+import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collections;
import java.util.HashMap;
@@ -48,6 +49,7 @@ import org.apache.solr.schema.IndexSchema;
import org.apache.solr.schema.SchemaField;
import org.apache.solr.update.processor.AtomicUpdateDocumentMerger;
import org.apache.solr.util.RefCounted;
+import org.hamcrest.MatcherAssert;
import org.junit.AfterClass;
import org.junit.Before;
import org.junit.BeforeClass;
@@ -122,6 +124,36 @@ public class TestInPlaceUpdatesStandalone extends SolrTestCaseJ4 {
}
@Test
+ public void testUpdateBadRequest() throws Exception {
+ final long version1 = addAndGetVersion(sdoc("id", "1", "title_s", "first", "inplace_updatable_float", 41), null);
+ assertU(commit());
+
+ // invalid value with set operation
+ SolrException e = expectThrows(SolrException.class,
+ () -> addAndAssertVersion(version1, "id", "1", "inplace_updatable_float", map("set", "NOT_NUMBER")));
+ assertEquals(SolrException.ErrorCode.BAD_REQUEST.code, e.code());
+ MatcherAssert.assertThat(e.getMessage(), containsString("For input string: \"NOT_NUMBER\""));
+
+ // invalid value with inc operation
+ e = expectThrows(SolrException.class,
+ () -> addAndAssertVersion(version1, "id", "1", "inplace_updatable_float", map("inc", "NOT_NUMBER")));
+ assertEquals(SolrException.ErrorCode.BAD_REQUEST.code, e.code());
+ MatcherAssert.assertThat(e.getMessage(), containsString("For input string: \"NOT_NUMBER\""));
+
+ // inc op with null value
+ e = expectThrows(SolrException.class,
+ () -> addAndAssertVersion(version1, "id", "1", "inplace_updatable_float", map("inc", null)));
+ assertEquals(SolrException.ErrorCode.BAD_REQUEST.code, e.code());
+ MatcherAssert.assertThat(e.getMessage(), containsString("Invalid input 'null' for field inplace_updatable_float"));
+
+ e = expectThrows(SolrException.class,
+ () -> addAndAssertVersion(version1, "id", "1", "inplace_updatable_float",
+ map("inc", new ArrayList<>(Collections.singletonList(123)))));
+ assertEquals(SolrException.ErrorCode.BAD_REQUEST.code, e.code());
+ MatcherAssert.assertThat(e.getMessage(), containsString("Invalid input '[123]' for field inplace_updatable_float"));
+ }
+
+ @Test
public void testUpdatingDocValues() throws Exception {
long version1 = addAndGetVersion(sdoc("id", "1", "title_s", "first", "inplace_updatable_float", 41), null);
long version2 = addAndGetVersion(sdoc("id", "2", "title_s", "second", "inplace_updatable_float", 42), null);
diff --git a/solr/core/src/test/org/apache/solr/update/processor/AtomicUpdatesTest.java b/solr/core/src/test/org/apache/solr/update/processor/AtomicUpdatesTest.java
index 54e9f9d..a0c1402 100644
--- a/solr/core/src/test/org/apache/solr/update/processor/AtomicUpdatesTest.java
+++ b/solr/core/src/test/org/apache/solr/update/processor/AtomicUpdatesTest.java
@@ -24,13 +24,17 @@ import java.util.List;
import com.google.common.collect.ImmutableMap;
import org.apache.solr.SolrTestCaseJ4;
+import org.apache.solr.common.SolrException;
import org.apache.solr.common.SolrInputDocument;
import org.apache.solr.util.DateMathParser;
+import org.hamcrest.MatcherAssert;
import org.junit.Before;
import org.junit.BeforeClass;
import org.junit.Ignore;
import org.junit.Test;
+import static org.hamcrest.core.StringContains.containsString;
+
public class AtomicUpdatesTest extends SolrTestCaseJ4 {
@BeforeClass
@@ -1420,6 +1424,15 @@ public class AtomicUpdatesTest extends SolrTestCaseJ4 {
assertQ(req("q", "id:123", "indent", "true"), "//result[@numFound = '1']");
assertQ(req("q", "id:12311", "indent", "true"), "//result[@numFound = '1']");
assertQ(req("q", "cat:ccc", "indent", "true"), "//result[@numFound = '1']");
+
+ // inc op on non-numeric field
+ SolrInputDocument invalidDoc = new SolrInputDocument();
+ invalidDoc.setField("id", "7");
+ invalidDoc.setField("cat", ImmutableMap.of("inc", "bbb"));
+
+ SolrException e = expectThrows(SolrException.class, () -> assertU(adoc(invalidDoc)));
+ assertEquals(SolrException.ErrorCode.BAD_REQUEST.code, e.code());
+ MatcherAssert.assertThat(e.getMessage(), containsString("'inc' is not supported on non-numeric field cat"));
}
public void testFieldsWithDefaultValuesWhenAtomicUpdatesAgainstTlog() {
[lucene-solr] 02/04: SOLR-14950: fix regenerating of copyfield with
explicit src/dest matching dyn rule
Posted by mu...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
munendrasn pushed a commit to branch branch_8x
in repository https://gitbox.apache.org/repos/asf/lucene-solr.git
commit 35ef6d2a41bc4ebf54a473998fcffeaf03293aca
Author: Munendra S N <mu...@apache.org>
AuthorDate: Wed Nov 25 08:52:38 2020 +0530
SOLR-14950: fix regenerating of copyfield with explicit src/dest matching dyn rule
CopyFields are regenerated in case of replace-field or replace-field-type.
While regenerating, source and destionation are checked against fields but source/dest
could match dynamic rule too.
For example,
<copyField source="something_s" dest="spellcheck"/>
<dynamicField name="*_s" type="string"/>
here, something_s is not present in schema but matches the dynamic rule.
To handle the above case, need to check dynamicFieldCache too while regenerating the
copyFields
---
solr/CHANGES.txt | 6 +-
.../org/apache/solr/schema/ManagedIndexSchema.java | 5 +-
.../apache/solr/rest/schema/TestBulkSchemaAPI.java | 102 +++++++++++++++++++++
3 files changed, 110 insertions(+), 3 deletions(-)
diff --git a/solr/CHANGES.txt b/solr/CHANGES.txt
index 1f151dd..881e573 100644
--- a/solr/CHANGES.txt
+++ b/solr/CHANGES.txt
@@ -105,7 +105,11 @@ Bug Fixes
* SOLR-15047: Fix collapse parser behavior when collapsing on numeric fields to differentiate '0' group from null group (hossman)
- * SOLR-10860: Return proper error code for bad input in case of inplace updates (Tomas Eduardo Fernandez Lobbe, Munendra S N)
+* SOLR-10860: Return proper error code for bad input in case of inplace updates (Tomas Eduardo Fernandez Lobbe, Munendra S N)
+
+* SOLR-14950: Fix error in copyField regeneration when explicit source/destination is not present in schema but
+ matches the dynamic rule. The copyFields are rebuilt with replace-field and replace-field-type, if required
+ (Andrew Shumway, Munendra S N)
Other Changes
---------------------
diff --git a/solr/core/src/java/org/apache/solr/schema/ManagedIndexSchema.java b/solr/core/src/java/org/apache/solr/schema/ManagedIndexSchema.java
index 3a06534..92e7016 100644
--- a/solr/core/src/java/org/apache/solr/schema/ManagedIndexSchema.java
+++ b/solr/core/src/java/org/apache/solr/schema/ManagedIndexSchema.java
@@ -930,8 +930,9 @@ public final class ManagedIndexSchema extends IndexSchema {
private void rebuildCopyFields(List<CopyField> oldCopyFields) {
if (oldCopyFields.size() > 0) {
for (CopyField copyField : oldCopyFields) {
- SchemaField source = fields.get(copyField.getSource().getName());
- SchemaField destination = fields.get(copyField.getDestination().getName());
+ // source or destination either could be explicit field which matches dynamic rule
+ SchemaField source = getFieldOrNull(copyField.getSource().getName());
+ SchemaField destination = getFieldOrNull(copyField.getDestination().getName());
registerExplicitSrcAndDestFields
(copyField.getSource().getName(), copyField.getMaxChars(), destination, source);
}
diff --git a/solr/core/src/test/org/apache/solr/rest/schema/TestBulkSchemaAPI.java b/solr/core/src/test/org/apache/solr/rest/schema/TestBulkSchemaAPI.java
index ed7bd3e..b6afd25 100644
--- a/solr/core/src/test/org/apache/solr/rest/schema/TestBulkSchemaAPI.java
+++ b/solr/core/src/test/org/apache/solr/rest/schema/TestBulkSchemaAPI.java
@@ -708,6 +708,108 @@ public class TestBulkSchemaAPI extends RestTestBase {
assertTrue("'bleh_s' copyField rule exists in the schema", l.isEmpty());
}
+ @SuppressWarnings({"rawtypes"})
+ public void testCopyFieldWithReplace() throws Exception {
+ RestTestHarness harness = restTestHarness;
+ String newFieldName = "test_solr_14950";
+
+ // add-field-type
+ String addFieldTypeAnalyzer = "{\n" +
+ "'add-field-type' : {" +
+ " 'name' : 'myNewTextField',\n" +
+ " 'class':'solr.TextField',\n" +
+ " 'analyzer' : {\n" +
+ " 'charFilters' : [{\n" +
+ " 'name':'patternReplace',\n" +
+ " 'replacement':'$1$1',\n" +
+ " 'pattern':'([a-zA-Z])\\\\\\\\1+'\n" +
+ " }],\n" +
+ " 'tokenizer' : { 'name':'whitespace' },\n" +
+ " 'filters' : [{ 'name':'asciiFolding' }]\n" +
+ " }\n"+
+ "}}";
+
+ String response = restTestHarness.post("/schema", json(addFieldTypeAnalyzer));
+ Map map = (Map) fromJSONString(response);
+ assertNull(response, map.get("error"));
+ map = getObj(harness, "myNewTextField", "fieldTypes");
+ assertNotNull("'myNewTextField' field type does not exist in the schema", map);
+
+ // add-field
+ String payload = "{\n" +
+ " 'add-field' : {\n" +
+ " 'name':'" + newFieldName + "',\n" +
+ " 'type':'myNewTextField',\n" +
+ " 'stored':true,\n" +
+ " 'indexed':true\n" +
+ " }\n" +
+ " }";
+
+ response = harness.post("/schema", json(payload));
+
+ map = (Map) fromJSONString(response);
+ assertNull(response, map.get("error"));
+
+ Map m = getObj(harness, newFieldName, "fields");
+ assertNotNull("'"+ newFieldName + "' field does not exist in the schema", m);
+
+ // add copy-field with explicit source and destination
+ List l = getSourceCopyFields(harness, "bleh_s");
+ assertTrue("'bleh_s' copyField rule exists in the schema", l.isEmpty());
+
+ payload = "{\n" +
+ " 'add-copy-field' : {\n" +
+ " 'source' :'bleh_s',\n" +
+ " 'dest':'"+ newFieldName + "'\n" +
+ " }\n" +
+ " }\n";
+ response = harness.post("/schema", json(payload));
+
+ map = (Map) fromJSONString(response);
+ assertNull(response, map.get("error"));
+
+ l = getSourceCopyFields(harness, "bleh_s");
+ assertFalse("'bleh_s' copyField rule doesn't exist", l.isEmpty());
+ assertEquals("bleh_s", ((Map)l.get(0)).get("source"));
+ assertEquals(newFieldName, ((Map)l.get(0)).get("dest"));
+
+ // replace-field-type
+ String replaceFieldTypeAnalyzer = "{\n" +
+ "'replace-field-type' : {" +
+ " 'name' : 'myNewTextField',\n" +
+ " 'class':'solr.TextField',\n" +
+ " 'analyzer' : {\n" +
+ " 'tokenizer' : { 'name':'whitespace' },\n" +
+ " 'filters' : [{ 'name':'asciiFolding' }]\n" +
+ " }\n"+
+ "}}";
+
+ response = restTestHarness.post("/schema", json(replaceFieldTypeAnalyzer));
+ map = (Map) fromJSONString(response);
+ assertNull(response, map.get("error"));
+
+ map = getObj(restTestHarness, "myNewTextField", "fieldTypes");
+ assertNotNull(map);
+ Map analyzer = (Map)map.get("analyzer");
+ assertNull("'myNewTextField' shouldn't contain charFilters", analyzer.get("charFilters"));
+
+ l = getSourceCopyFields(harness, "bleh_s");
+ assertFalse("'bleh_s' copyField rule doesn't exist", l.isEmpty());
+ assertEquals("bleh_s", ((Map)l.get(0)).get("source"));
+ assertEquals(newFieldName, ((Map)l.get(0)).get("dest"));
+
+ // with replace-field
+ String replaceField = "{'replace-field' : {'name':'" + newFieldName + "', 'type':'string'}}";
+ response = harness.post("/schema", json(replaceField));
+ map = (Map) fromJSONString(response);
+ assertNull(map.get("error"));
+
+ l = getSourceCopyFields(harness, "bleh_s");
+ assertFalse("'bleh_s' copyField rule doesn't exist", l.isEmpty());
+ assertEquals("bleh_s", ((Map)l.get(0)).get("source"));
+ assertEquals(newFieldName, ((Map)l.get(0)).get("dest"));
+ }
+
@SuppressWarnings({"unchecked", "rawtypes"})
public void testDeleteAndReplace() throws Exception {
RestTestHarness harness = restTestHarness;
[lucene-solr] 03/04: SOLR-12539: handle extra spaces in JSON facet
shorthand syntax
Posted by mu...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
munendrasn pushed a commit to branch branch_8x
in repository https://gitbox.apache.org/repos/asf/lucene-solr.git
commit 6e55dae815bed0ecd120819e599a64cbc810b047
Author: Munendra S N <mu...@apache.org>
AuthorDate: Wed Nov 25 12:21:19 2020 +0530
SOLR-12539: handle extra spaces in JSON facet shorthand syntax
---
solr/CHANGES.txt | 3 +++
solr/core/src/java/org/apache/solr/search/facet/FacetParser.java | 7 +++++--
.../src/test/org/apache/solr/search/facet/RangeFacetCloudTest.java | 6 ------
.../core/src/test/org/apache/solr/search/facet/TestJsonFacets.java | 2 +-
4 files changed, 9 insertions(+), 9 deletions(-)
diff --git a/solr/CHANGES.txt b/solr/CHANGES.txt
index 881e573..3debe13 100644
--- a/solr/CHANGES.txt
+++ b/solr/CHANGES.txt
@@ -111,6 +111,9 @@ Bug Fixes
matches the dynamic rule. The copyFields are rebuilt with replace-field and replace-field-type, if required
(Andrew Shumway, Munendra S N)
+* SOLR-12539: Handle parsing of values for 'other', 'include', and 'excludeTags' in JSON facets when specified as
+ comma-separated values with extra spaces (hossman, Munendra S N)
+
Other Changes
---------------------
diff --git a/solr/core/src/java/org/apache/solr/search/facet/FacetParser.java b/solr/core/src/java/org/apache/solr/search/facet/FacetParser.java
index d8bb697..393dc59 100644
--- a/solr/core/src/java/org/apache/solr/search/facet/FacetParser.java
+++ b/solr/core/src/java/org/apache/solr/search/facet/FacetParser.java
@@ -20,6 +20,7 @@ import java.util.List;
import java.util.ArrayList;
import java.util.Map;
import java.util.Optional;
+import java.util.stream.Collectors;
import org.apache.solr.common.SolrException;
import org.apache.solr.common.params.SolrParams;
@@ -388,8 +389,10 @@ abstract class FacetParser<FacetRequestT extends FacetRequest> {
return (List<String>)o;
}
if (o instanceof String) {
- // TODO: SOLR-12539 handle spaces in b/w comma & value ie, should the values be trimmed before returning??
- return StrUtils.splitSmart((String)o, ",", decode);
+ return StrUtils.splitSmart((String)o, ",", decode).stream()
+ .map(String::trim)
+ .filter(s -> !s.isEmpty())
+ .collect(Collectors.toList());
}
throw err("Expected list of string or comma separated string values for '" + paramName +
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 8e4cd11..0530bdd 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
@@ -938,12 +938,6 @@ public class RangeFacetCloudTest extends SolrCloudTestCase {
// - 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 separated list of values, JSON Facets 'other'
- // parsing can't handle any leading (or trailing?) whitespace
- val = val.replaceAll("\\s","");
}
return ", other:" + val;
}
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 acce852..c116d29 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
@@ -2193,7 +2193,7 @@ public class TestJsonFacets extends SolrTestCaseHS {
, "json.facet", "{ processEmpty:true," +
" f1:{query:{q:'${cat_s}:B', facet:{nj:{query:'${where_s}:NJ'}, ny:{query:'${where_s}:NY'}} , excludeTags:[xyz,qaz]}}" +
",f2:{query:{q:'${cat_s}:B', facet:{nj:{query:'${where_s}:NJ'}, ny:{query:'${where_s}:NY'}} , excludeTags:abc }}" +
- ",f3:{query:{q:'${cat_s}:B', facet:{nj:{query:'${where_s}:NJ'}, ny:{query:'${where_s}:NY'}} , excludeTags:'xyz,abc,qaz' }}" +
+ ",f3:{query:{q:'${cat_s}:B', facet:{nj:{query:'${where_s}:NJ'}, ny:{query:'${where_s}:NY'}} , excludeTags:'xyz ,abc ,qaz' }}" +
",f4:{query:{q:'${cat_s}:B', facet:{nj:{query:'${where_s}:NJ'}, ny:{query:'${where_s}:NY'}} , excludeTags:[xyz , abc , qaz] }}" +
",f5:{query:{q:'${cat_s}:B', facet:{nj:{query:'${where_s}:NJ'}, ny:{query:'${where_s}:NY'}} , excludeTags:[xyz,qaz]}}" + // this is repeated, but it did fail when a single context was shared among sub-facets
",f6:{query:{q:'${cat_s}:B', facet:{processEmpty:true, nj:{query:'${where_s}:NJ'}, ny:{ type:query, q:'${where_s}:NY', excludeTags:abc}} }}" + // exclude in a sub-facet
[lucene-solr] 04/04: SOLR-14514: add extra checks for picking
'stream' method in JSON facet
Posted by mu...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
munendrasn pushed a commit to branch branch_8x
in repository https://gitbox.apache.org/repos/asf/lucene-solr.git
commit 0b95557f1aaf6029f4c0bda885cea4ffbeea71c3
Author: Munendra S N <mu...@apache.org>
AuthorDate: Thu Nov 26 12:01:51 2020 +0530
SOLR-14514: add extra checks for picking 'stream' method in JSON facet
missing, allBuckets, and numBuckets is not supported with stream method.
So, avoiding picking stream method when any one of them is enabled even if
facet sort is 'index asc'
---
solr/CHANGES.txt | 3 +++
.../org/apache/solr/search/facet/FacetField.java | 5 +++-
.../solr/search/facet/TestCloudJSONFacetSKG.java | 29 ++++++++--------------
.../search/facet/TestCloudJSONFacetSKGEquiv.java | 23 ++++++-----------
.../apache/solr/search/facet/TestJsonFacets.java | 28 +++++++++++----------
solr/solr-ref-guide/src/json-facet-api.adoc | 2 +-
6 files changed, 42 insertions(+), 48 deletions(-)
diff --git a/solr/CHANGES.txt b/solr/CHANGES.txt
index 3debe13..eff85b5 100644
--- a/solr/CHANGES.txt
+++ b/solr/CHANGES.txt
@@ -114,6 +114,9 @@ Bug Fixes
* SOLR-12539: Handle parsing of values for 'other', 'include', and 'excludeTags' in JSON facets when specified as
comma-separated values with extra spaces (hossman, Munendra S N)
+* SOLR-14514: Avoid picking 'stream' method in JSON facet when any of 'allBuckets', 'numBuckets', and 'missing' parameters are enabled
+ (hossman, Munendra S N)
+
Other Changes
---------------------
diff --git a/solr/core/src/java/org/apache/solr/search/facet/FacetField.java b/solr/core/src/java/org/apache/solr/search/facet/FacetField.java
index 728cd6e..fa1c085 100644
--- a/solr/core/src/java/org/apache/solr/search/facet/FacetField.java
+++ b/solr/core/src/java/org/apache/solr/search/facet/FacetField.java
@@ -105,7 +105,10 @@ public class FacetField extends FacetRequestSorted {
method = FacetMethod.STREAM;
}
if (method == FacetMethod.STREAM && sf.indexed() && !ft.isPointField() &&
- // wether we can use stream processing depends on wether this is a shard request, wether
+ // streaming doesn't support allBuckets, numBuckets or missing
+ // so, don't use stream processor if anyone of them is enabled
+ !(allBuckets || numBuckets || missing) &&
+ // whether we can use stream processing depends on whether this is a shard request, whether
// re-sorting has been requested, and if the effective sort during collection is "index asc"
( fcontext.isShard()
// for a shard request, the effective per-shard sort must be index asc
diff --git a/solr/core/src/test/org/apache/solr/search/facet/TestCloudJSONFacetSKG.java b/solr/core/src/test/org/apache/solr/search/facet/TestCloudJSONFacetSKG.java
index 0992af8..f2b67d5 100644
--- a/solr/core/src/test/org/apache/solr/search/facet/TestCloudJSONFacetSKG.java
+++ b/solr/core/src/test/org/apache/solr/search/facet/TestCloudJSONFacetSKG.java
@@ -45,18 +45,17 @@ import org.apache.solr.cloud.SolrCloudTestCase;
import org.apache.solr.common.SolrInputDocument;
import org.apache.solr.common.params.SolrParams;
import org.apache.solr.common.util.NamedList;
-import static org.apache.solr.search.facet.RelatednessAgg.computeRelatedness;
-import static org.apache.solr.search.facet.RelatednessAgg.roundTo5Digits;
-
+import org.junit.AfterClass;
+import org.junit.BeforeClass;
import org.noggit.JSONUtil;
import org.noggit.JSONWriter;
import org.noggit.JSONWriter.Writable;
-
-import org.junit.AfterClass;
-import org.junit.BeforeClass;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
+import static org.apache.solr.search.facet.RelatednessAgg.computeRelatedness;
+import static org.apache.solr.search.facet.RelatednessAgg.roundTo5Digits;
+
/**
* <p>
* A randomized test of nested facets using the <code>relatedness()</code> function, that asserts the
@@ -65,13 +64,13 @@ import org.slf4j.LoggerFactory;
* <p>
* Note that unlike normal facet "count" verification, using a high limit + overrequest isn't a substitute
* for refinement in order to ensure accurate "skg" computation across shards. For that reason, this
- * tests forces <code>refine: true</code> (unlike {@link TestCloudJSONFacetJoinDomain}) and specifices a
- * <code>domain: { 'query':'*:*' }</code> for every facet, in order to garuntee that all shards
+ * tests forces <code>refine: true</code> (unlike {@link TestCloudJSONFacetJoinDomain}) and specifies a
+ * <code>domain: { 'query':'*:*' }</code> for every facet, in order to guarantee that all shards
* participate in all facets, so that the popularity & relatedness values returned can be proven
* with validation requests.
* </p>
* <p>
- * (Refinement alone is not enough. Using the '*:*' query as the facet domain is neccessary to
+ * (Refinement alone is not enough. Using the '*:*' query as the facet domain is necessary to
* prevent situations where a single shardX may return candidate bucket with no child-buckets due to
* the normal facet intersections, but when refined on other shardY(s), can produce "high scoring"
* SKG child-buckets, which would then be missing the foreground/background "size" contributions from
@@ -156,7 +155,7 @@ public class TestCloudJSONFacetSKG extends SolrCloudTestCase {
SolrInputDocument doc = sdoc("id", ""+id);
for (int fieldNum = 0; fieldNum < MAX_FIELD_NUM; fieldNum++) {
// NOTE: we ensure every doc has at least one value in each field
- // that way, if a term is returned for a parent there there is garunteed to be at least one
+ // that way, if a term is returned for a parent there there is guaranteed to be at least one
// one term in the child facet as well.
//
// otherwise, we'd face the risk of a single shardX returning parentTermX as a top term for
@@ -226,7 +225,7 @@ public class TestCloudJSONFacetSKG extends SolrCloudTestCase {
/**
* Given a (random) field number, returns a random (integer based) value for that field.
- * NOTE: The number of unique values in each field is constant acording to {@link #UNIQUE_FIELD_VALS}
+ * NOTE: The number of unique values in each field is constant according to {@link #UNIQUE_FIELD_VALS}
* but the precise <em>range</em> of values will vary for each unique field number, such that cross field joins
* will match fewer documents based on how far apart the field numbers are.
*
@@ -279,7 +278,7 @@ public class TestCloudJSONFacetSKG extends SolrCloudTestCase {
// NOTE that these two queries & facets *should* effectively identical given that the
// very large limit value is big enough no shard will ever return that may terms,
- // but the "limit=-1" case it actaully triggers slightly different code paths
+ // but the "limit=-1" case it actually triggers slightly different code paths
// because it causes FacetField.returnsPartial() to be "true"
for (int limit : new int[] { 999999999, -1 }) {
Map<String,TermFacet> facets = new LinkedHashMap<>();
@@ -769,14 +768,8 @@ public class TestCloudJSONFacetSKG extends SolrCloudTestCase {
*
*
* @return a Boolean, may be null
- * @see <a href="https://issues.apache.org/jira/browse/SOLR-14514">SOLR-14514: allBuckets ignored by method:stream</a>
*/
public static Boolean randomAllBucketsParam(final Random r, final String sort) {
-
- if ("index asc".equals(sort)) {
- return null;
- }
-
switch(r.nextInt(4)) {
case 0: return true;
case 1: return false;
diff --git a/solr/core/src/test/org/apache/solr/search/facet/TestCloudJSONFacetSKGEquiv.java b/solr/core/src/test/org/apache/solr/search/facet/TestCloudJSONFacetSKGEquiv.java
index eb68662..8e09593 100644
--- a/solr/core/src/test/org/apache/solr/search/facet/TestCloudJSONFacetSKGEquiv.java
+++ b/solr/core/src/test/org/apache/solr/search/facet/TestCloudJSONFacetSKGEquiv.java
@@ -20,8 +20,8 @@ import java.io.IOException;
import java.lang.invoke.MethodHandles;
import java.nio.file.Path;
import java.nio.file.Paths;
-import java.util.Arrays;
import java.util.ArrayList;
+import java.util.Arrays;
import java.util.Collections;
import java.util.EnumSet;
import java.util.LinkedHashMap;
@@ -47,22 +47,21 @@ import org.apache.solr.common.SolrInputDocument;
import org.apache.solr.common.params.ModifiableSolrParams;
import org.apache.solr.common.params.SolrParams;
import org.apache.solr.common.util.NamedList;
-import static org.apache.solr.search.facet.FacetField.FacetMethod;
-import static org.apache.solr.search.facet.SlotAcc.SweepingCountSlotAcc.SWEEP_COLLECTION_DEBUG_KEY;
-
+import org.junit.AfterClass;
+import org.junit.BeforeClass;
import org.noggit.JSONUtil;
import org.noggit.JSONWriter;
import org.noggit.JSONWriter.Writable;
-
-import org.junit.AfterClass;
-import org.junit.BeforeClass;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
+import static org.apache.solr.search.facet.FacetField.FacetMethod;
+import static org.apache.solr.search.facet.SlotAcc.SweepingCountSlotAcc.SWEEP_COLLECTION_DEBUG_KEY;
+
/**
* <p>
* A randomized test of nested facets using the <code>relatedness()</code> function, that asserts the
- * results are consistent and equivilent regardless of what <code>method</code> (ie: FacetFieldProcessor)
+ * results are consistent and equivalent regardless of what <code>method</code> (ie: FacetFieldProcessor)
* and/or <code>{@value RelatednessAgg#SWEEP_COLLECTION}</code> option is requested.
* </p>
* <p>
@@ -71,7 +70,7 @@ import org.slf4j.LoggerFactory;
* because this test does not attempt to prove the results with validation requests.
* </p>
* <p>
- * This test only concerns itself with the equivilency of results
+ * This test only concerns itself with the equivalency of results
* </p>
*
* @see TestCloudJSONFacetSKG
@@ -986,14 +985,8 @@ public class TestCloudJSONFacetSKGEquiv extends SolrCloudTestCase {
* </p>
*
* @return a Boolean, may be null
- * @see <a href="https://issues.apache.org/jira/browse/SOLR-14514">SOLR-14514: allBuckets ignored by method:stream</a>
*/
public static Boolean randomAllBucketsParam(final Random r, final String sort) {
-
- if ("index asc".equals(sort)) {
- return null;
- }
-
switch(r.nextInt(4)) {
case 0: return true;
case 1: return false;
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 c116d29..be88cd6 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
@@ -488,21 +488,17 @@ public class TestJsonFacets extends SolrTestCaseHS {
}
// relatedness shouldn't be computed for allBuckets, but it also shouldn't cause any problems
- //
- // NOTE: we can't test this with 'index asc' because STREAM processor
- // (which test may randomize as default) doesn't support allBuckets
- // see: https://issues.apache.org/jira/browse/SOLR-14514
- //
for (String sort : Arrays.asList("sort:'y desc'",
"sort:'z desc'",
"sort:'skg desc'",
+ "sort:'index asc'",
"prelim_sort:'count desc', sort:'skg desc'")) {
- // the relatedness score of each of our cat_s values is (conviniently) also alphabetical order,
+ // the relatedness score of each of our cat_s values is (conveniently) also alphabetical order,
// (and the same order as 'sum(num_i) desc' & 'min(num_i) desc')
//
// So all of these re/sort options should produce identical output (since the num buckets is < limit)
// - Testing "index" sort allows the randomized use of "stream" processor as default to be tested.
- // - Testing (re)sorts on other stats sanity checks code paths where relatedness() is a "defered" Agg
+ // - Testing (re)sorts on other stats sanity checks code paths where relatedness() is a "deferred" Agg
for (String limit : Arrays.asList(", ", ", limit:5, ", ", limit:-1, ")) {
// results shouldn't change regardless of our limit param"
assertJQ(req("q", "cat_s:[* TO *]", "rows", "0",
@@ -524,7 +520,7 @@ public class TestJsonFacets extends SolrTestCaseHS {
+ " background_popularity: 0.33333, },"
+ " }, "
+ " { val:'B', count:3, y:-3.0, z:-5, "
- + " skg : { relatedness: 0.0, " // perfectly average and uncorrolated
+ + " skg : { relatedness: 0.0, " // perfectly average and uncorrelated
//+ " foreground_count: 1, "
//+ " foreground_size: 2, "
//+ " background_count: 3, "
@@ -1003,11 +999,14 @@ public class TestJsonFacets extends SolrTestCaseHS {
// test streaming
assertJQ(req("q", "*:*", "rows", "0"
, "json.facet", "{ cat:{terms:{field:'cat_s', method:stream }}" + // won't stream; need sort:index asc
- ", cat2:{terms:{field:'cat_s', method:stream, sort:'index asc' }}" +
- ", cat3:{terms:{field:'cat_s', method:stream, sort:'index asc', mincount:3 }}" + // mincount
- ", cat4:{terms:{field:'cat_s', method:stream, sort:'index asc', prefix:B }}" + // prefix
- ", cat5:{terms:{field:'cat_s', method:stream, sort:'index asc', offset:1 }}" + // offset
- " }"
+ ", cat2:{terms:{field:'cat_s', method:stream, sort:'index asc' }}" +
+ ", cat3:{terms:{field:'cat_s', method:stream, sort:'index asc', mincount:3 }}" + // mincount
+ ", cat4:{terms:{field:'cat_s', method:stream, sort:'index asc', prefix:B }}" + // prefix
+ ", cat5:{terms:{field:'cat_s', method:stream, sort:'index asc', offset:1 }}" + // offset
+ ", cat6:{terms:{field:'cat_s', method:stream, sort:'index asc', missing:true }}" + // missing
+ ", cat7:{terms:{field:'cat_s', method:stream, sort:'index asc', numBuckets:true }}" + // numBuckets
+ ", cat8:{terms:{field:'cat_s', method:stream, sort:'index asc', allBuckets:true }}" + // allBuckets
+ " }"
)
, "facets=={count:6 " +
", cat :{buckets:[{val:B, count:3},{val:A, count:2}]}" +
@@ -1015,6 +1014,9 @@ public class TestJsonFacets extends SolrTestCaseHS {
", cat3:{buckets:[{val:B, count:3}]}" +
", cat4:{buckets:[{val:B, count:3}]}" +
", cat5:{buckets:[{val:B, count:3}]}" +
+ ", cat6:{missing:{count:1}, buckets:[{val:A, count:2},{val:B, count:3}]}" +
+ ", cat7:{numBuckets:2, buckets:[{val:A, count:2},{val:B, count:3}]}" +
+ ", cat8:{allBuckets:{count:5}, buckets:[{val:A, count:2},{val:B, count:3}]}" +
" }"
);
diff --git a/solr/solr-ref-guide/src/json-facet-api.adoc b/solr/solr-ref-guide/src/json-facet-api.adoc
index bc336c3..5e83807 100644
--- a/solr/solr-ref-guide/src/json-facet-api.adoc
+++ b/solr/solr-ref-guide/src/json-facet-api.adoc
@@ -220,7 +220,7 @@ This parameter indicates the facet algorithm to use:
* "uif" UnInvertedField, collect into ordinal array
* "dvhash" DocValues, collect into hash - improves efficiency over high cardinality fields
* "enum" TermsEnum then intersect DocSet (stream-able)
-* "stream" Presently equivalent to "enum"
+* "stream" Presently equivalent to "enum" - used for indexed, non-point fields with sort 'index asc' and allBuckets, numBuckets, missing disabled.
* "smart" Pick the best method for the field type (this is the default)
|prelim_sort |An optional parameter for specifying an approximation of the final `sort` to use during initial collection of top buckets when the <<json-facet-api.adoc#sorting-facets-by-nested-functions,`sort` parameter is very costly>>.