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

justinrsweeney · 2023-10-10T18:03:12Z

https://issues.apache.org/jira/browse/SOLR-17022

Description

This PR provides support for using glob patterns for fields in Export handler, Stream handler and with the SelectStream streaming expression. This creates similar ease of use for these handlers as we already have for the Select handler. Given how Solr export handler works, the glob pattern will be applied to only match fields that support stream, which are fields with docvalues enabled.

Solution

This solution uses similar methods to the Select handler to part glob patterns into matching fields. We then create a FieldWriter for each matching field to return the necessary data.

Tests

Implemented tests to ensure that we create the correct set of fields as a results of glob pattern matching.

Checklist

Please review the following and check all that apply:

I have reviewed the guidelines for How to Contribute and my code conforms to the standards described there to the best of my ability.
I have created a Jira issue and added the issue ID to my pull request title.
I have given Solr maintainers access to contribute to my PR branch. (optional but recommended)
I have developed this patch against the main branch.
I have run ./gradlew check.
I have added tests for my changes.
I have added documentation for the Reference Guide

…o iterate patterns for the same field

solr/core/src/java/org/apache/solr/handler/export/ExportWriter.java

epugh · 2023-10-10T19:30:33Z

This seems like a very useful feature! I also like the establishing parity in how things work. Will you make sure to update ref guide?

…rn matching, current implementation uses Java NIO path matching. Replaced uses of FilenameUtils.wildcardMatches to reduce commons-io usage.

justinrsweeney · 2023-10-11T14:55:33Z

solr/core/src/java/org/apache/solr/handler/export/ExportWriter.java

+        SchemaField schemaField = searcher.getSchema().getField(fi.getName());
+        if (fieldsProcessed.add(fi.getName())
+            && schemaField.hasDocValues()
+            && (!(schemaField.getType() instanceof SortableTextField)


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:

Have glob patterns for fields in Export handler return any fields that are able to be exported

Match the behavior in the Select handler and only return fields that match the pattern, have docvalues enabled AND have useDocValuesAsStored=true

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

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!

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.

risdenk

Thanks @justinrsweeney looks much cleaner

solr/solrj/src/java/org/apache/solr/common/util/GlobPatternUtil.java

solr/solrj/src/test/org/apache/solr/common/util/TestGlobPatternUtil.java

justinrsweeney · 2023-10-11T15:01:35Z

This seems like a very useful feature! I also like the establishing parity in how things work. Will you make sure to update ref guide?

Yup, I can update the ref guide before merging.

dsmiley

I'm surprised to see that this isn't using SolrReturnFields, which is already doing field glob expansion. I know it's in solr-core and you seem to need something in solrj-streaming but we could move it.

dsmiley · 2023-10-13T15:15:35Z

solr/solrj/src/java/org/apache/solr/common/util/GlobPatternUtil.java

+public class GlobPatternUtil {
+
+  public static boolean matches(String pattern, String input) {
+    return FileSystems.getDefault().getPathMatcher("glob:" + pattern).matches(Paths.get(input));


This is bizarre just for some field wildcard support. If there is a reason we use FileSystems then a comment is necessary.

I took a look at how it's implemented. If only we could call ZipUtils.toRegexPattern but the class is package protected. It's a shame to recompile the glob on each call to matches!

solr/solrj/src/java/org/apache/solr/common/util/GlobPatternUtil.java

justinrsweeney · 2023-10-13T15:46:54Z

I'm surprised to see that this isn't using SolrReturnFields, which is already doing field glob expansion. I know it's in solr-core and you seem to need something in solrj-streaming but we could move it.

SolrReturnFields was using FilenameUtils.wildcardMatch() from commons-io and this just uses a very similar method but not requiring the third party library and allows solrj-streaming, SolrReturnFields, and ExportWriter to all use the same mechanism to match glob patterns.

…as select

dsmiley

My concern is duplication of logic between SolrReturnFields. If you think it's not an issue or is impossible to reconcile, then I trust you.

dsmiley · 2023-10-13T22:31:06Z

solr/solrj/src/java/org/apache/solr/common/util/GlobPatternUtil.java

+public class GlobPatternUtil {
+
+  public static boolean matches(String pattern, String input) {
+    return FileSystems.getDefault().getPathMatcher("glob:" + pattern).matches(Paths.get(input));


I took a look at how it's implemented. If only we could call ZipUtils.toRegexPattern but the class is package protected. It's a shame to recompile the glob on each call to matches!

dsmiley · 2023-10-13T22:35:33Z

solr/core/src/java/org/apache/solr/handler/export/ExportWriter.java

+      SolrIndexSearcher searcher,
+      Set<String> fieldsProcessed,
+      List<SchemaField> expandedFields) {
+    for (FieldInfo fi : searcher.getFieldInfos()) {


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?

dsmiley · 2023-10-13T22:36:36Z

solr/core/src/java/org/apache/solr/handler/export/ExportWriter.java

+    for (FieldInfo fi : searcher.getFieldInfos()) {
+      if (GlobPatternUtil.matches(fieldPattern, fi.getName())) {
+        SchemaField schemaField = searcher.getSchema().getField(fi.getName());
+        if (fieldsProcessed.add(fi.getName())


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

justinrsweeney · 2023-12-19T17:16:04Z

My concern is duplication of logic between SolrReturnFields. If you think it's not an issue or is impossible to reconcile, then I trust you.

I finally got back around to this. With the latest change, ExportWriter is using SolrReturnFields which in turn is using GlobPatternUtil so there is a consistent approach there.

justinrsweeney · 2023-12-19T20:29:58Z

@risdenk Pinging for re-review when you have a chance

dsmiley · 2023-12-20T05:24:50Z

solr/solrj-streaming/src/java/org/apache/solr/client/solrj/io/stream/SelectStream.java

@@ -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 (GlobPatternUtil.matches(globPattern, fieldName)) {


Shouldn't SelectStream also use SolrReturnFields and not use lower level GlobPattern stuff (it's something SRF can handle)?

Disclaimer: I haven't looked at this PR in a long time.

solrj-streaming does not currently have a dependency on core, in fact currently I think core depends on solrj-streaming. I didn't want to refactor SolrReturnFields to live elsewhere given the scope of this PR so not using that.

Oh right; of course.

dsmiley · 2023-12-20T23:33:57Z

solr/solrj-streaming/src/java/org/apache/solr/client/solrj/io/stream/SelectStream.java

@@ -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 (GlobPatternUtil.matches(globPattern, fieldName)) {


Oh right; of course.

…tream handler and with SelectStream streaming expression (#1996) * Adding support for Glob patterns in Export handler and Select stream handler using the same logic to match glob patterns to fields as is used in select requests

LuckyFrost · 2024-02-23T12:41:25Z

@justinrsweeney, hello, I'm not sure, but it seems your change causes a significant performance hit with prefix wildcard fl

https://lists.apache.org/thread/vbwnjxprl6s1qy0t1jzfcw8hprg1gvzh

magibney · 2024-02-23T16:27:21Z

It's also possible this could be related to SOLR-16989 -- sorry I missed the mailing list thread, I'll follow up there.

EDIT: on closer inspection it does look more likely related to the relative inefficiency of the new pattern matching approach (wrt previous commons-io FilenameUtils ...)

justinrsweeney added 4 commits October 10, 2023 13:48

Allowing for glob patterns for fields in ExportWriter

b210b1e

Tidying code

8a04a11

Adding support for glob expression for fields in SelectStream

fe66c1f

Breaking after a field matches any glob pattern to avoid continuing t…

23626fb

…o iterate patterns for the same field

risdenk requested changes Oct 10, 2023

View reviewed changes

solr/core/src/java/org/apache/solr/handler/export/ExportWriter.java Outdated Show resolved Hide resolved

justinrsweeney added 2 commits October 11, 2023 10:47

Adding GlobPatternUtil class to provide a common place for glob patte…

ac25ebb

…rn matching, current implementation uses Java NIO path matching. Replaced uses of FilenameUtils.wildcardMatches to reduce commons-io usage.

Tidying code

bfe31fa

justinrsweeney commented Oct 11, 2023

View reviewed changes

risdenk requested changes Oct 11, 2023

View reviewed changes

solr/solrj/src/java/org/apache/solr/common/util/GlobPatternUtil.java Show resolved Hide resolved

solr/solrj/src/test/org/apache/solr/common/util/TestGlobPatternUtil.java Show resolved Hide resolved

Adding missing headers

fad2f18

justinrsweeney added 3 commits October 11, 2023 11:18

Updating ref docs

0d862cf

Fixing refdoc changes

f1c62f9

Fixing arg order for SolrReturnFields glob pattern matching

0382445

dsmiley reviewed Oct 13, 2023

View reviewed changes

justinrsweeney added 2 commits October 13, 2023 11:53

Fixing up comments

19e97ae

Modifying ExportWriter to have same behavior for glob pattern fields …

7f7b49d

…as select

dsmiley reviewed Oct 13, 2023

View reviewed changes

Updating ExportWriter to use SolrReturnFields

91133ae

Tidying code

c1ebac1

dsmiley reviewed Dec 20, 2023

View reviewed changes

dsmiley approved these changes Dec 20, 2023

View reviewed changes

justinrsweeney merged commit 17f8ee4 into main Dec 22, 2023
3 checks passed

justinrsweeney deleted the SOLR-17022-glob-patterns-export-stream-handlers branch December 22, 2023 12:29

justinrsweeney mentioned this pull request Feb 26, 2024

SOLR-17181: Using apache commons implementation for wildcard matching for glob patterns #2301

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

justinrsweeney commented Oct 10, 2023 •

edited

epugh commented Oct 10, 2023

justinrsweeney Oct 11, 2023

epugh Oct 11, 2023

magibney Oct 11, 2023

justinrsweeney Oct 13, 2023

risdenk left a comment

justinrsweeney commented Oct 11, 2023

dsmiley left a comment

dsmiley Oct 13, 2023 •

edited

dsmiley Oct 13, 2023

justinrsweeney commented Oct 13, 2023

dsmiley left a comment

dsmiley Oct 13, 2023

dsmiley Oct 13, 2023

dsmiley Oct 13, 2023

justinrsweeney commented Dec 19, 2023

justinrsweeney commented Dec 19, 2023

dsmiley Dec 20, 2023

justinrsweeney Dec 20, 2023

dsmiley Dec 20, 2023

dsmiley Dec 20, 2023

LuckyFrost commented Feb 23, 2024 •

edited

magibney commented Feb 23, 2024 •

edited

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

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

Conversation

justinrsweeney commented Oct 10, 2023 • edited

Description

Solution

Tests

Checklist

epugh commented Oct 10, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

risdenk left a comment

Choose a reason for hiding this comment

justinrsweeney commented Oct 11, 2023

dsmiley left a comment

Choose a reason for hiding this comment

dsmiley Oct 13, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justinrsweeney commented Oct 13, 2023

dsmiley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justinrsweeney commented Dec 19, 2023

justinrsweeney commented Dec 19, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LuckyFrost commented Feb 23, 2024 • edited

magibney commented Feb 23, 2024 • edited

justinrsweeney commented Oct 10, 2023 •

edited

dsmiley Oct 13, 2023 •

edited

LuckyFrost commented Feb 23, 2024 •

edited

magibney commented Feb 23, 2024 •

edited