Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SOLR-17022: Support for glob patterns for fields in Export handler, Stream handler and with SelectStream streaming expression #1996

Merged
merged 14 commits into from
Dec 22, 2023
Merged

SOLR-17022: Support for glob patterns for fields in Export handler, Stream handler and with SelectStream streaming expression #1996

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
b210b1e
Allowing for glob patterns for fields in ExportWriter
Oct 10, 2023
8a04a11
Tidying code
Oct 10, 2023
fe66c1f
Adding support for glob expression for fields in SelectStream
Oct 10, 2023
23626fb
Breaking after a field matches any glob pattern to avoid continuing t…
Oct 10, 2023
ac25ebb
Adding GlobPatternUtil class to provide a common place for glob patte…
Oct 11, 2023
bfe31fa
Tidying code
Oct 11, 2023
fad2f18
Adding missing headers
Oct 11, 2023
0d862cf
Updating ref docs
Oct 11, 2023
f1c62f9
Fixing refdoc changes
Oct 11, 2023
0382445
Fixing arg order for SolrReturnFields glob pattern matching
Oct 11, 2023
19e97ae
Fixing up comments
Oct 13, 2023
7f7b49d
Modifying ExportWriter to have same behavior for glob pattern fields …
Oct 13, 2023
91133ae
Updating ExportWriter to use SolrReturnFields
Dec 19, 2023
c1ebac1
Tidying code
Dec 19, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
77 changes: 67 additions & 10 deletions solr/core/src/java/org/apache/solr/handler/export/ExportWriter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,9 +27,14 @@
import java.io.PrintWriter;
import java.lang.invoke.MethodHandles;
import java.nio.charset.StandardCharsets;
import java.util.ArrayList;
import java.util.HashSet;
import java.util.List;
import java.util.Map;
import java.util.Set;
import java.util.TreeSet;
import org.apache.commons.io.FilenameUtils;
risdenk marked this conversation as resolved.
Show resolved Hide resolved
import org.apache.lucene.index.FieldInfo;
import org.apache.lucene.index.LeafReader;
import org.apache.lucene.index.LeafReaderContext;
import org.apache.lucene.index.SortedDocValues;
Expand Down Expand Up @@ -487,19 +492,14 @@ void writeDoc(

public FieldWriter[] getFieldWriters(String[] fields, SolrIndexSearcher searcher)
throws IOException {
IndexSchema schema = searcher.getSchema();
FieldWriter[] writers = new FieldWriter[fields.length];
DocValuesIteratorCache dvIterCache = new DocValuesIteratorCache(searcher, false);
for (int i = 0; i < fields.length; i++) {
String field = fields[i];
SchemaField schemaField = null;

try {
schemaField = schema.getField(field);
} catch (Exception e) {
throw new IOException(e);
}
List<SchemaField> expandedFields = expandFieldList(fields, searcher);

FieldWriter[] writers = new FieldWriter[expandedFields.size()];
for (int i = 0; i < expandedFields.size(); i++) {
SchemaField schemaField = expandedFields.get(i);
String field = schemaField.getName();
if (!schemaField.hasDocValues()) {
throw new IOException(schemaField + " must have DocValues to use this feature.");
}
Expand Down Expand Up @@ -844,4 +844,61 @@ public String getMessage() {
return "Early Client Disconnect";
}
}

/**
* Creates a complete field list using the provided field list by expanding any glob patterns into
* field names
*
* @param fields the original set of fields provided
* @param searcher an index searcher to access schema info
* @return a complete list of fields included any fields matching glob patterns
* @throws IOException if a provided field does not exist or cannot be retrieved from the schema
* info
*/
private List<SchemaField> expandFieldList(String[] fields, SolrIndexSearcher searcher)
throws IOException {
List<SchemaField> expandedFields = new ArrayList<>(fields.length);
Set<String> fieldsProcessed = new HashSet<>();
for (String field : fields) {
try {
if (field.contains("*")) {
getGlobFields(field, searcher, fieldsProcessed, expandedFields);
} else {
if (fieldsProcessed.add(field)) {
expandedFields.add(searcher.getSchema().getField(field));
}
}
} catch (Exception e) {
throw new IOException(e);
}
}

return expandedFields;
}

/**
* Create a list of schema fields that match a given glob pattern
*
* @param fieldPattern the glob pattern to match
* @param searcher an index search to access schema info
* @param fieldsProcessed the set of field names already processed to avoid duplicating
* @param expandedFields the list of fields to add expanded field names into
*/
private void getGlobFields(
String fieldPattern,
SolrIndexSearcher searcher,
Set<String> fieldsProcessed,
List<SchemaField> expandedFields) {
for (FieldInfo fi : searcher.getFieldInfos()) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There can be many fieldInfo, and you're looping over a "matches" call that is going to internally build a regex each time. Maybe you should first do the hasDocValues etc. checks so we do this matches check last?

if (FilenameUtils.wildcardMatch(fi.getName(), fieldPattern)) {
SchemaField schemaField = searcher.getSchema().getField(fi.getName());
if (fieldsProcessed.add(fi.getName())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line might add to fieldsProcessed, yet exclude the field because doesn't "hasDocValues". This looks suspicious to me.

&& schemaField.hasDocValues()
&& (!(schemaField.getType() instanceof SortableTextField)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wanted to highlight this because @magibney and I were discussing this previously. The way this is implemented for the Export handler here actually deviates from how glob patterns work for the Select handler.

The Select handler will not match any fields where useDocValuesAsStored=false for glob patterns, those fields must be explicitly provided in the field list.

For the purposes of Export I did not follow that pattern and instead will match any fields that have docvalues enabled with the exception of SortableTextField which require useDocValuesAsStored=true to be used in Export in other places in the code.

My reasoning here is that there is a performance hit for getting non-stored, docvalues enabled fields in the Select handler that doesn't seem to be to be as impactful in the Export handler.

Open to other opinions on this topic, should we:

  1. Have glob patterns for fields in Export handler return any fields that are able to be exported
  2. Match the behavior in the Select handler and only return fields that match the pattern, have docvalues enabled AND have useDocValuesAsStored=true

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I vote for consistency if possible, since I think of both /select and /export as the same, just one is faster than the other ;-). If that isn't possible, then maybe if the ref guide makes it really clear "This is why you use /select and ramifications" and "THis is why you use /export and it's ramifications", then that might cover these differences. I can see the bug reports if we aren't clear ;-).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As @justinrsweeney foreshadowed, I am in favor of parity in how this is handled between select and export.

The purpose of udvas as I understand it is not simply to mitigate performance issues in select (and in fact, performance should be comparable between select and export as of #1938), but also as a config option to allow the configuration of docValues for a field without requiring that those values be returned when a field matches a glob pattern.

There are plenty of use cases where, e.g., a derivative string field is configured solely for the purpose of sort, faceting, [or other purpose that would make even less sense to return on export]. It should be possible to configure a field that's intended for one of the many other purposes docValues serve, without forcing it to be returned when the field name matches a glob pattern. Note also, if both stored=true and useDocValuesAsStored=true, stored fields will be used for select and docValues will be used for export.

I think what I'm missing is: how does this help usability? Are there cases where one would want docValues to be used for field value retrieval, but could not simply set that field to useDocValuesAsStored=true? And udvas defaults to true anyway (for all fields except SortableTextField) iirc.

I'm curious to hear others' thoughts on this as well!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I updated this PR to mimic the same behavior as the select handler, where useDocValuesAsStored=false fields will not be returned unless specifically requested. I'm in agreement that consistency here is better.

|| schemaField.useDocValuesAsStored())) {
expandedFields.add(schemaField);
}
}
}
}
}
37 changes: 37 additions & 0 deletions solr/core/src/test/org/apache/solr/handler/export/TestExportWriter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1298,6 +1298,43 @@ public void testExpr() throws Exception {
.contains("Must have useDocValuesAsStored='true'"));
}

@Test
public void testGlobFields() throws Exception {
assertU(delQ("*:*"));
assertU(commit());
createLargeIndex();
SolrQueryRequest req =
req("q", "*:*", "qt", "/export", "fl", "id,*_udvas,*_i_p", "sort", "id asc");
assertJQ(
req,
"response/numFound==100000",
"response/docs/[0]/id=='0'",
"response/docs/[1]/id=='1'",
"response/docs/[0]/sortabledv_udvas=='0'",
"response/docs/[1]/sortabledv_udvas=='1'",
"response/docs/[0]/small_i_p==0",
"response/docs/[1]/small_i_p==1");

assertU(delQ("*:*"));
assertU(commit());
createLargeIndex();
req = req("q", "*:*", "qt", "/export", "fl", "*", "sort", "id asc");
assertJQ(
req,
"response/numFound==100000",
"response/docs/[0]/id=='0'",
"response/docs/[1]/id=='1'",
"response/docs/[0]/sortabledv_udvas=='0'",
"response/docs/[1]/sortabledv_udvas=='1'",
"response/docs/[0]/small_i_p==0",
"response/docs/[1]/small_i_p==1");

String jq = JQ(req);
assertFalse(
"Fields without docvalues and useDocValuesAsStored should not be returned",
jq.contains("\"sortabledv\""));
}

@SuppressWarnings("rawtypes")
private void validateSort(int numDocs) throws Exception {
// 10 fields
Expand Down
1 change: 1 addition & 0 deletions solr/solrj-streaming/build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ dependencies {
implementation 'org.apache.httpcomponents:httpclient'
implementation 'org.apache.httpcomponents:httpcore'
implementation 'org.apache.commons:commons-math3'
implementation 'commons-io:commons-io'

testImplementation project(':solr:test-framework')
testImplementation project(':solr:core')
Expand Down
30 changes: 28 additions & 2 deletions solr/solrj-streaming/src/java/org/apache/solr/client/solrj/io/stream/SelectStream.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
import java.util.Locale;
import java.util.Map;
import java.util.Set;
import org.apache.commons.io.FilenameUtils;
import org.apache.solr.client.solrj.io.Tuple;
import org.apache.solr.client.solrj.io.comp.StreamComparator;
import org.apache.solr.client.solrj.io.eval.EvaluatorException;
Expand Down Expand Up @@ -52,14 +53,21 @@ public class SelectStream extends TupleStream implements Expressible {
private TupleStream stream;
private StreamContext streamContext;
private Map<String, String> selectedFields;
private List<String> selectedFieldGlobPatterns;
private Map<StreamEvaluator, String> selectedEvaluators;
private List<StreamOperation> operations;

public SelectStream(TupleStream stream, List<String> selectedFields) throws IOException {
this.stream = stream;
this.selectedFields = new HashMap<>();
this.selectedFieldGlobPatterns = new ArrayList<>();
for (String selectedField : selectedFields) {
this.selectedFields.put(selectedField, selectedField);
if (selectedField.contains("*")) {
// selected field is a glob pattern
this.selectedFieldGlobPatterns.add(selectedField);
} else {
this.selectedFields.put(selectedField, selectedField);
}
}
operations = new ArrayList<>();
selectedEvaluators = new LinkedHashMap<>();
Expand All @@ -68,6 +76,7 @@ public SelectStream(TupleStream stream, List<String> selectedFields) throws IOEx
public SelectStream(TupleStream stream, Map<String, String> selectedFields) throws IOException {
this.stream = stream;
this.selectedFields = selectedFields;
selectedFieldGlobPatterns = new ArrayList<>();
operations = new ArrayList<>();
selectedEvaluators = new LinkedHashMap<>();
}
Expand Down Expand Up @@ -123,6 +132,7 @@ public SelectStream(StreamExpression expression, StreamFactory factory) throws I
stream = factory.constructStream(streamExpressions.get(0));

selectedFields = new HashMap<>();
selectedFieldGlobPatterns = new ArrayList<>();
selectedEvaluators = new LinkedHashMap<>();
for (StreamExpressionParameter parameter : selectAsFieldsExpressions) {
StreamExpressionValue selectField = (StreamExpressionValue) parameter;
Expand Down Expand Up @@ -175,7 +185,11 @@ public SelectStream(StreamExpression expression, StreamFactory factory) throws I
selectedFields.put(asValue, asName);
}
} else {
selectedFields.put(value, value);
if (value.contains("*")) {
selectedFieldGlobPatterns.add(value);
} else {
selectedFields.put(value, value);
}
}
}

Expand Down Expand Up @@ -217,6 +231,11 @@ private StreamExpression toExpression(StreamFactory factory, boolean includeStre
}
}

// selected glob patterns
for (String selectFieldGlobPattern : selectedFieldGlobPatterns) {
expression.addParameter(selectFieldGlobPattern);
}

// selected evaluators
for (Map.Entry<StreamEvaluator, String> selectedEvaluator : selectedEvaluators.entrySet()) {
expression.addParameter(
Expand Down Expand Up @@ -308,6 +327,13 @@ public Tuple read() throws IOException {
workingForEvaluators.put(fieldName, original.get(fieldName));
if (selectedFields.containsKey(fieldName)) {
workingToReturn.put(selectedFields.get(fieldName), original.get(fieldName));
} else {
for (String globPattern : selectedFieldGlobPatterns) {
if (FilenameUtils.wildcardMatch(fieldName, globPattern)) {
workingToReturn.put(fieldName, original.get(fieldName));
break;
}
}
}
}

Expand Down
3 changes: 2 additions & 1 deletion ...ming/src/test/org/apache/solr/client/solrj/io/stream/StreamExpressionToExpessionTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,14 +105,15 @@ public void testSelectStream() throws Exception {
try (SelectStream stream =
new SelectStream(
StreamExpressionParser.parse(
"select(\"a_s as fieldA\", search(collection1, q=*:*, fl=\"id,a_s,a_i,a_f\", sort=\"a_f asc, a_i asc\"))"),
"select(\"a_s as fieldA\", a_*, search(collection1, q=*:*, fl=\"id,a_s,a_i,a_f\", sort=\"a_f asc, a_i asc\"))"),
factory)) {
expressionString = stream.toExpression(factory).toString();
assertTrue(expressionString.contains("select(search(collection1,"));
assertTrue(expressionString.contains("q=\"*:*\""));
assertTrue(expressionString.contains("fl=\"id,a_s,a_i,a_f\""));
assertTrue(expressionString.contains("sort=\"a_f asc, a_i asc\""));
assertTrue(expressionString.contains("a_s as fieldA"));
assertTrue(expressionString.contains("a_*"));
}
}

Expand Down
Loading