You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@solr.apache.org by "kotman12 (via GitHub)" <gi...@apache.org> on 2026/01/15 22:57:25 UTC
[PR] SOLR-18071 Support Stored Fields in Export Writer [solr]
kotman12 opened a new pull request, #4053:
URL: https://github.com/apache/solr/pull/4053
https://issues.apache.org/jira/browse/SOLR-18071
# Description
Adds support for exporting stored-only fields (fields without docValues) in the `/export` request handler via a new `includeStoredFields` parameter. Previously, all fields in the field list (fl) were required to have docValues enabled. This change allows users to include stored fields that don't have docValues, which can be useful, i.e. when exporting fields which don't support docValues or trying to export data that has already been indexed without DVs.
# Solution
If fl explicitly names a stored-only field and includeStoredFields is not enabled, the request fails with a 400 and a hint to add includeStoredFields=true. For glob patterns (e.g., fl=\* or fl=intdv,\*), stored-only fields are skipped unless includeStoredFields=true, to preserve backward compatibility. The current implementation fetches from `StoredFields` DV-enabled fields when some stored fields have _already_ been requested. This avoids DV-lookup for a field, which makes sense since we have to parse the StoredFields anyway. My (somewhat limited) benchmarks appear to corroborate that this is the best choice for performance.
A quirky thing about this implementation is that the very much internal `FieldWriter` API was changed to support more than one field. This makes it more interchangeable with the existing `StoredFieldVisitor` interface, which assumes one visitor per many fields. I landed on this rather than creating an adapter to bridge the two as it appeared to be simpler. It's worth stressing again that the `FieldWriter` is very much internal to the export package and the boolean it returned was effectively discarded (the local `fieldIndex` it drives isn't even used anywhere). It could be argued that `FieldWriter::write` could be `void` and I'd also be open to such a change.
# Tests
- Explicit stored-only field export succeeds with includeStoredFields=true (single-valued and multi-valued).
- Explicit stored-only field export fails without the parameter and includes the includeStoredFields=true hint and field name.
- Glob fl skips stored-only fields without the parameter (request succeeds, stored-only fields not present).
- Glob fl includes stored-only fields with the parameter.
- Coverage for stored field types: string, int, long, float, double, boolean, date.
Also have some performance comparisons of exporting vs not exporting stored fields:
[stored-fields-export-writer-1k-doc-benchmark.txt](https://github.com/user-attachments/files/24655605/stored-fields-export-writer-1k-doc-benchmark.txt)
[stored-fields-export-writer-140k-doc-benchmark.txt](https://github.com/user-attachments/files/24655606/stored-fields-export-writer-140k-doc-benchmark.txt)
# Checklist
Please review the following and check all that apply:
- [x] I have reviewed the guidelines for [How to Contribute](https://github.com/apache/solr/blob/main/CONTRIBUTING.md) and my code conforms to the standards described there to the best of my ability.
- [x] I have created a Jira issue and added the issue ID to my pull request title.
- [x] I have given Solr maintainers [access](https://help.github.com/en/articles/allowing-changes-to-a-pull-request-branch-created-from-a-fork) to contribute to my PR branch. (optional but recommended, not available for branches on forks living under an organisation)
- [x] I have developed this patch against the `main` branch.
- [x] I have run `./gradlew check`.
- [x] I have added tests for my changes.
- [x] I have added documentation for the [Reference Guide](https://github.com/apache/solr/tree/main/solr/solr-ref-guide)
- [x] I have added a [changelog entry](https://github.com/apache/solr/blob/main/dev-docs/changelog.adoc) for my change
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-18071 Support Stored Fields in Export Writer [solr]
Posted by "kotman12 (via GitHub)" <gi...@apache.org>.
kotman12 commented on PR #4053:
URL: https://github.com/apache/solr/pull/4053#issuecomment-3775198967
> I really like how overall simple the implementation turned out to be. This wasn't a heavy lift. Nice job!
>
> Curious... at this point, how do you think /export contrasts with /select + cursorMark? Pros/cons... (this could be documented in the ref guide as well to durably record these insights)
I've added a "Comparison with Cursors" section to the doc with my best effort comparing the two. Definitely needs a second pair of eyes if you don't mind.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-18071 Support Stored Fields in Export Writer [solr]
Posted by "kotman12 (via GitHub)" <gi...@apache.org>.
kotman12 commented on code in PR #4053:
URL: https://github.com/apache/solr/pull/4053#discussion_r2702692930
##########
solr/core/src/java/org/apache/solr/handler/export/StoredFieldsWriter.java:
##########
@@ -0,0 +1,148 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.solr.handler.export;
+
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.Date;
+import java.util.List;
+import java.util.Map;
+import java.util.WeakHashMap;
+import org.apache.lucene.index.FieldInfo;
+import org.apache.lucene.index.IndexReader;
+import org.apache.lucene.index.LeafReader;
+import org.apache.lucene.index.LeafReaderContext;
+import org.apache.lucene.index.StoredFieldVisitor;
+import org.apache.lucene.index.StoredFields;
+import org.apache.solr.common.MapWriter.EntryWriter;
+import org.apache.solr.schema.BoolField;
+import org.apache.solr.schema.DateValueFieldType;
+import org.apache.solr.schema.SchemaField;
+
+class StoredFieldsWriter extends FieldWriter {
+
+ private final Map<String, SchemaField> fields;
+ private final ThreadLocal<WeakHashMap<IndexReader.CacheKey, StoredFields>> storedFieldsMap =
+ new ThreadLocal<>();
+
+ public StoredFieldsWriter(Map<String, SchemaField> fieldsToRead) {
+ this.fields = fieldsToRead;
+ }
+
+ @Override
+ public int write(
+ SortDoc sortDoc, LeafReaderContext readerContext, EntryWriter out, int fieldIndex)
+ throws IOException {
+ WeakHashMap<IndexReader.CacheKey, StoredFields> map = storedFieldsMap.get();
+ if (map == null) {
+ map = new WeakHashMap<>();
+ storedFieldsMap.set(map);
+ }
+ LeafReader reader = readerContext.reader();
+ StoredFields storedFields = map.get(reader.getReaderCacheHelper().getKey());
+ if (storedFields == null) {
+ storedFields = reader.storedFields();
+ map.put(reader.getReaderCacheHelper().getKey(), storedFields);
+ }
+ ExportVisitor visitor = new ExportVisitor(out);
+ storedFields.document(sortDoc.docId, visitor);
+ return visitor.flush();
+ }
+
+ class ExportVisitor extends StoredFieldVisitor {
+
+ final EntryWriter out;
+ String lastFieldName;
+ List<Object> multiValue = null;
+ int fieldsVisited;
+
+ public ExportVisitor(EntryWriter out) {
+ this.out = out;
+ }
+
+ @Override
+ public void stringField(FieldInfo fieldInfo, String value) throws IOException {
+ var schemaField = fields.get(fieldInfo.name);
+ var fieldType = schemaField == null ? null : schemaField.getType();
+ if (fieldType instanceof BoolField) {
+ // Convert "T"/"F" stored value to boolean true/false
+ addField(fieldInfo.name, Boolean.valueOf(fieldType.indexedToReadable(value)));
+ } else {
+ addField(fieldInfo.name, value);
+ }
+ }
+
+ @Override
+ public void intField(FieldInfo fieldInfo, int value) throws IOException {
+ addField(fieldInfo.name, value);
+ }
+
+ @Override
+ public void longField(FieldInfo fieldInfo, long value) throws IOException {
+ var schemaField = fields.get(fieldInfo.name);
+ var fieldType = schemaField == null ? null : schemaField.getType();
+ if (fieldType instanceof DateValueFieldType) {
+ Date date = new Date(value);
+ addField(fieldInfo.name, date);
+ } else {
+ addField(fieldInfo.name, value);
+ }
+ }
+
+ @Override
+ public void floatField(FieldInfo fieldInfo, float value) throws IOException {
+ addField(fieldInfo.name, value);
+ }
+
+ @Override
+ public void doubleField(FieldInfo fieldInfo, double value) throws IOException {
+ addField(fieldInfo.name, value);
+ }
+
+ @Override
+ public Status needsField(FieldInfo fieldInfo) {
+ return fields.containsKey(fieldInfo.name) ? Status.YES : Status.NO;
+ }
+
+ private <T> void addField(String fieldName, T value) throws IOException {
Review Comment:
You've never seen this pattern before where you have to track state of multi-valued fields in a callback-style API where the caller sends one element at a time? It's not the greatest but it's what I'm dealing with here. The flush here can be thought of as being akin to, say, Jackson's `JsonGenerator::writeEndObject`. The look-behind is because I need to buffer and I don't get a signal on the last element that it is the last element. I don't really control that. Nota bene something similar happens in Solr [here](https://github.com/apache/solr/blob/6409f19f2a65c27471b2c04a36e37c4194713839/solr/core/src/java/org/apache/solr/response/DocsStreamer.java#L174-L193) although I suppose it is less-contortion inducing because it happens all in one function. But this is hard to do here given the API.
One thing I can do is buffer into a multi-map-like thing instead of rolling my own `StoredFieldVisitor`, i.e. I can buffer into a `Document` (basically a multi-map), i.e. maybe with `DocumentStoredFieldVisitor`. I'd need to convert the multi-map thing (i.e. `Document`) to a collection-property-aware thing like `SolrDocument`, perhaps using the same `convertLuceneDocToSolrDoc` I just linked. But that is a lot of transformation so I am pretty neutral about it.
Maybe you can elaborate on what is confusing you? Also, any thoughts on the alternatives I just suggested would be welcome.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-18071 Support Stored Fields in Export Writer [solr]
Posted by "kotman12 (via GitHub)" <gi...@apache.org>.
kotman12 commented on code in PR #4053:
URL: https://github.com/apache/solr/pull/4053#discussion_r2710008273
##########
solr/core/src/java/org/apache/solr/handler/export/ExportWriter.java:
##########
@@ -484,43 +488,75 @@ void writeDoc(
LeafReaderContext context = leaves.get(ord);
int fieldIndex = 0;
for (FieldWriter fieldWriter : writers) {
- if (fieldWriter.write(sortDoc, context, ew, fieldIndex)) {
- ++fieldIndex;
- }
+ fieldIndex += fieldWriter.write(sortDoc, context, ew, fieldIndex);
}
}
public List<FieldWriter> getFieldWriters(String[] fields, SolrQueryRequest req)
throws IOException {
DocValuesIteratorCache dvIterCache = new DocValuesIteratorCache(req.getSearcher(), false);
-
SolrReturnFields solrReturnFields = new SolrReturnFields(fields, req);
+ boolean includeStoredFields = req.getParams().getBool(INCLUDE_STORED_FIELDS_PARAM, false);
List<FieldWriter> writers = new ArrayList<>();
+ Set<SchemaField> docValueFields = new LinkedHashSet<>();
+ Map<String, SchemaField> storedFields = new LinkedHashMap<>();
+
for (String field : req.getSearcher().getFieldNames()) {
if (!solrReturnFields.wantsField(field)) {
continue;
}
SchemaField schemaField = req.getSchema().getField(field);
- if (!schemaField.hasDocValues()) {
- throw new IOException(schemaField + " must have DocValues to use this feature.");
- }
- boolean multiValued = schemaField.multiValued();
FieldType fieldType = schemaField.getType();
- FieldWriter writer;
- if (fieldType instanceof SortableTextField && !schemaField.useDocValuesAsStored()) {
- if (solrReturnFields.getRequestedFieldNames() != null
- && solrReturnFields.getRequestedFieldNames().contains(field)) {
- // Explicitly requested field cannot be used due to not having useDocValuesAsStored=true,
- // throw exception
+ // Check if field can use DocValues
+ boolean canUseDocValues =
+ schemaField.hasDocValues()
+ && (!(fieldType instanceof SortableTextField) || schemaField.useDocValuesAsStored());
Review Comment:
The reason is for `/export` it was decided that you don't need an explicit `udvas` for "cheap" fields I guess (so anything that is _not_ a text field)? Idk, I am trying to follow this [thread](https://github.com/apache/solr/pull/1996#discussion_r1355153153).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-18071 Support Stored Fields in Export Writer [solr]
Posted by "kotman12 (via GitHub)" <gi...@apache.org>.
kotman12 commented on code in PR #4053:
URL: https://github.com/apache/solr/pull/4053#discussion_r2710137055
##########
solr/core/src/java/org/apache/solr/handler/export/StoredFieldsWriter.java:
##########
@@ -0,0 +1,148 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.solr.handler.export;
+
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.Date;
+import java.util.List;
+import java.util.Map;
+import java.util.WeakHashMap;
+import org.apache.lucene.index.FieldInfo;
+import org.apache.lucene.index.IndexReader;
+import org.apache.lucene.index.LeafReader;
+import org.apache.lucene.index.LeafReaderContext;
+import org.apache.lucene.index.StoredFieldVisitor;
+import org.apache.lucene.index.StoredFields;
+import org.apache.solr.common.MapWriter.EntryWriter;
+import org.apache.solr.schema.BoolField;
+import org.apache.solr.schema.DateValueFieldType;
+import org.apache.solr.schema.SchemaField;
+
+class StoredFieldsWriter extends FieldWriter {
+
+ private final Map<String, SchemaField> fields;
+ private final ThreadLocal<WeakHashMap<IndexReader.CacheKey, StoredFields>> storedFieldsMap =
Review Comment:
Yikes, it almost defeats the point. For some reason I had the rule disabled. I have enabled all threading-related rules in Intellij's inspections.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-18071 Support Stored Fields in Export Writer [solr]
Posted by "kotman12 (via GitHub)" <gi...@apache.org>.
kotman12 commented on code in PR #4053:
URL: https://github.com/apache/solr/pull/4053#discussion_r2699834975
##########
solr/solr-ref-guide/modules/query-guide/pages/exporting-result-sets.adoc:
##########
@@ -44,6 +46,11 @@ Filter queries are also supported.
An optional parameter `batchSize` determines the size of the internal buffers for partial results.
The default value is `30000` but users may want to specify smaller values to limit the memory use (at the cost of degraded performance) or higher values to improve export performance (the relationship is not linear and larger values don't bring proportionally larger performance increases).
+An optional parameter `includeStoredFields` (default `false`) enables exporting fields that only have stored values (no docValues).
Review Comment:
Actually, can I put the explanation in the CHANGELOG? The docs already warn:
```
Note that retrieving stored fields may significantly impact export performance compared to docValues fields, as stored fields require additional I/O operations.
```
##########
solr/solr-ref-guide/modules/query-guide/pages/exporting-result-sets.adoc:
##########
@@ -44,6 +46,11 @@ Filter queries are also supported.
An optional parameter `batchSize` determines the size of the internal buffers for partial results.
The default value is `30000` but users may want to specify smaller values to limit the memory use (at the cost of degraded performance) or higher values to improve export performance (the relationship is not linear and larger values don't bring proportionally larger performance increases).
+An optional parameter `includeStoredFields` (default `false`) enables exporting fields that only have stored values (no docValues).
Review Comment:
Actually, can I put the explanation in the CHANGELOG? The docs already warn:
> Note that retrieving stored fields may significantly impact export performance compared to docValues fields, as stored fields require additional I/O operations.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-18071 Support Stored Fields in Export Writer [solr]
Posted by "kotman12 (via GitHub)" <gi...@apache.org>.
kotman12 commented on code in PR #4053:
URL: https://github.com/apache/solr/pull/4053#discussion_r2699818403
##########
solr/core/src/test/org/apache/solr/handler/export/TestExportWriter.java:
##########
@@ -1476,4 +1474,261 @@ private void addField(SolrInputDocument doc, String type, String value, boolean
doc.addField("number_" + type + (mv ? "s" : "") + "_ni_t", value);
doc.addField("number_" + type + (mv ? "s" : "") + "_ni_p", value);
}
+
+ @Test
+ public void testIncludeStoredFieldsExplicitRequest() throws Exception {
+ // Test that stored-only fields are returned when includeStoredFields=true
+ clearIndex();
Review Comment:
I leave it to your judgement, should this be in this PR or a follow-up? Honestly, I just copied the pattern of the existing tests.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-18071 Support Stored Fields in Export Writer [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on code in PR #4053:
URL: https://github.com/apache/solr/pull/4053#discussion_r2701769601
##########
solr/core/src/java/org/apache/solr/handler/export/StoredFieldsWriter.java:
##########
@@ -0,0 +1,148 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.solr.handler.export;
+
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.Date;
+import java.util.List;
+import java.util.Map;
+import java.util.WeakHashMap;
+import org.apache.lucene.index.FieldInfo;
+import org.apache.lucene.index.IndexReader;
+import org.apache.lucene.index.LeafReader;
+import org.apache.lucene.index.LeafReaderContext;
+import org.apache.lucene.index.StoredFieldVisitor;
+import org.apache.lucene.index.StoredFields;
+import org.apache.solr.common.MapWriter.EntryWriter;
+import org.apache.solr.schema.BoolField;
+import org.apache.solr.schema.DateValueFieldType;
+import org.apache.solr.schema.SchemaField;
+
+class StoredFieldsWriter extends FieldWriter {
+
+ private final Map<String, SchemaField> fields;
+ private final ThreadLocal<WeakHashMap<IndexReader.CacheKey, StoredFields>> storedFieldsMap =
+ new ThreadLocal<>();
+
+ public StoredFieldsWriter(Map<String, SchemaField> fieldsToRead) {
+ this.fields = fieldsToRead;
+ }
+
+ @Override
+ public int write(
+ SortDoc sortDoc, LeafReaderContext readerContext, EntryWriter out, int fieldIndex)
+ throws IOException {
+ WeakHashMap<IndexReader.CacheKey, StoredFields> map = storedFieldsMap.get();
+ if (map == null) {
+ map = new WeakHashMap<>();
+ storedFieldsMap.set(map);
+ }
Review Comment:
BTW I'm fine with your use of it here but was speaking out-loud, maybe a little too out-loud :-)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-18071 Support Stored Fields in Export Writer [solr]
Posted by "kotman12 (via GitHub)" <gi...@apache.org>.
kotman12 commented on code in PR #4053:
URL: https://github.com/apache/solr/pull/4053#discussion_r2743245156
##########
solr/solr-ref-guide/modules/query-guide/pages/exporting-result-sets.adoc:
##########
@@ -23,6 +23,24 @@ This feature uses a stream sorting technique that begins to send records within
The cases where this functionality may be useful include: session analysis, distributed merge joins, time series roll-ups, aggregations on high cardinality fields, fully distributed field collapsing, and sort-based stats.
+== Comparison with Cursors
+
+The `/export` handler offers several advantages over xref:pagination-of-results.adoc#fetching-a-large-number-of-sorted-results-cursors[cursor-based pagination] for streaming large result sets.
+
+With cursors, the query is re-executed for each page of results.
+In contrast, `/export` runs the filter query once and the resulting segment-level bitmasks are applied once per segment, after which the documents are simply iterated over.
+Additionally, the segments that existed when the stream was opened are held open for the duration of the export, eliminating the disappearing or duplicate document issues that can occur with cursors.
+The trade-off is that IndexReaders are kept around for longer periods of time.
Review Comment:
@gus-asf I added the 2x2 summary table if you wanted to take another look
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-18071 Support Stored Fields in Export Writer [solr]
Posted by "kotman12 (via GitHub)" <gi...@apache.org>.
kotman12 commented on code in PR #4053:
URL: https://github.com/apache/solr/pull/4053#discussion_r2710008273
##########
solr/core/src/java/org/apache/solr/handler/export/ExportWriter.java:
##########
@@ -484,43 +488,75 @@ void writeDoc(
LeafReaderContext context = leaves.get(ord);
int fieldIndex = 0;
for (FieldWriter fieldWriter : writers) {
- if (fieldWriter.write(sortDoc, context, ew, fieldIndex)) {
- ++fieldIndex;
- }
+ fieldIndex += fieldWriter.write(sortDoc, context, ew, fieldIndex);
}
}
public List<FieldWriter> getFieldWriters(String[] fields, SolrQueryRequest req)
throws IOException {
DocValuesIteratorCache dvIterCache = new DocValuesIteratorCache(req.getSearcher(), false);
-
SolrReturnFields solrReturnFields = new SolrReturnFields(fields, req);
+ boolean includeStoredFields = req.getParams().getBool(INCLUDE_STORED_FIELDS_PARAM, false);
List<FieldWriter> writers = new ArrayList<>();
+ Set<SchemaField> docValueFields = new LinkedHashSet<>();
+ Map<String, SchemaField> storedFields = new LinkedHashMap<>();
+
for (String field : req.getSearcher().getFieldNames()) {
if (!solrReturnFields.wantsField(field)) {
continue;
}
SchemaField schemaField = req.getSchema().getField(field);
- if (!schemaField.hasDocValues()) {
- throw new IOException(schemaField + " must have DocValues to use this feature.");
- }
- boolean multiValued = schemaField.multiValued();
FieldType fieldType = schemaField.getType();
- FieldWriter writer;
- if (fieldType instanceof SortableTextField && !schemaField.useDocValuesAsStored()) {
- if (solrReturnFields.getRequestedFieldNames() != null
- && solrReturnFields.getRequestedFieldNames().contains(field)) {
- // Explicitly requested field cannot be used due to not having useDocValuesAsStored=true,
- // throw exception
+ // Check if field can use DocValues
+ boolean canUseDocValues =
+ schemaField.hasDocValues()
+ && (!(fieldType instanceof SortableTextField) || schemaField.useDocValuesAsStored());
Review Comment:
The reason is for `/export` it was decided that you don't need an explicit `udvas` for "cheap" fields I guess? Idk, I am trying to follow this [thread](https://github.com/apache/solr/pull/1996#discussion_r1355153153).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-18071 Support Stored Fields in Export Writer [solr]
Posted by "kotman12 (via GitHub)" <gi...@apache.org>.
kotman12 commented on code in PR #4053:
URL: https://github.com/apache/solr/pull/4053#discussion_r2710011478
##########
solr/core/src/java/org/apache/solr/handler/export/ExportWriter.java:
##########
@@ -484,43 +488,75 @@ void writeDoc(
LeafReaderContext context = leaves.get(ord);
int fieldIndex = 0;
for (FieldWriter fieldWriter : writers) {
- if (fieldWriter.write(sortDoc, context, ew, fieldIndex)) {
- ++fieldIndex;
- }
+ fieldIndex += fieldWriter.write(sortDoc, context, ew, fieldIndex);
}
}
public List<FieldWriter> getFieldWriters(String[] fields, SolrQueryRequest req)
throws IOException {
DocValuesIteratorCache dvIterCache = new DocValuesIteratorCache(req.getSearcher(), false);
-
SolrReturnFields solrReturnFields = new SolrReturnFields(fields, req);
+ boolean includeStoredFields = req.getParams().getBool(INCLUDE_STORED_FIELDS_PARAM, false);
List<FieldWriter> writers = new ArrayList<>();
+ Set<SchemaField> docValueFields = new LinkedHashSet<>();
+ Map<String, SchemaField> storedFields = new LinkedHashMap<>();
+
for (String field : req.getSearcher().getFieldNames()) {
if (!solrReturnFields.wantsField(field)) {
continue;
}
SchemaField schemaField = req.getSchema().getField(field);
- if (!schemaField.hasDocValues()) {
- throw new IOException(schemaField + " must have DocValues to use this feature.");
- }
- boolean multiValued = schemaField.multiValued();
FieldType fieldType = schemaField.getType();
- FieldWriter writer;
- if (fieldType instanceof SortableTextField && !schemaField.useDocValuesAsStored()) {
- if (solrReturnFields.getRequestedFieldNames() != null
- && solrReturnFields.getRequestedFieldNames().contains(field)) {
- // Explicitly requested field cannot be used due to not having useDocValuesAsStored=true,
- // throw exception
+ // Check if field can use DocValues
+ boolean canUseDocValues =
+ schemaField.hasDocValues()
+ && (!(fieldType instanceof SortableTextField) || schemaField.useDocValuesAsStored());
Review Comment:
do you want me to add a comment here? It's unrelated to my change but I can do it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-18071 Support Stored Fields in Export Writer [solr]
Posted by "fsparv (via GitHub)" <gi...@apache.org>.
fsparv commented on code in PR #4053:
URL: https://github.com/apache/solr/pull/4053#discussion_r2723685924
##########
solr/solr-ref-guide/modules/query-guide/pages/exporting-result-sets.adoc:
##########
@@ -23,6 +23,24 @@ This feature uses a stream sorting technique that begins to send records within
The cases where this functionality may be useful include: session analysis, distributed merge joins, time series roll-ups, aggregations on high cardinality fields, fully distributed field collapsing, and sort-based stats.
+== Comparison with Cursors
+
+The `/export` handler offers several advantages over xref:pagination-of-results.adoc#fetching-a-large-number-of-sorted-results-cursors[cursor-based pagination] for streaming large result sets.
+
+With cursors, the query is re-executed for each page of results.
+In contrast, `/export` runs the filter query once and the resulting segment-level bitmasks are applied once per segment, after which the documents are simply iterated over.
+Additionally, the segments that existed when the stream was opened are held open for the duration of the export, eliminating the disappearing or duplicate document issues that can occur with cursors.
+The trade-off is that IndexReaders are kept around for longer periods of time.
Review Comment:
> I feel we're potentially suggesting the contributor here put more work into this than he bargained for. Any documentation he's comfortable writing is encouraged... and beyond that, well let's just get this merged and have real users kick the tires and we'll see.
I added comments since he asked for a second set of eyes on the docs, and didn't mark it changes requested. Feel free to take em or leave em. These are the things I suspect a reader might wonder. I have a notion of some of the answers, though it's quite likely @kotman12 has thought more carefully about it more recently than I.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-18071 Support Stored Fields in Export Writer [solr]
Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh commented on code in PR #4053:
URL: https://github.com/apache/solr/pull/4053#discussion_r2699831614
##########
solr/core/src/test/org/apache/solr/handler/export/TestExportWriter.java:
##########
@@ -1476,4 +1474,261 @@ private void addField(SolrInputDocument doc, String type, String value, boolean
doc.addField("number_" + type + (mv ? "s" : "") + "_ni_t", value);
doc.addField("number_" + type + (mv ? "s" : "") + "_ni_p", value);
}
+
+ @Test
+ public void testIncludeStoredFieldsExplicitRequest() throws Exception {
+ // Test that stored-only fields are returned when includeStoredFields=true
+ clearIndex();
Review Comment:
probably *should* be in a follow up, and I suspect that this pattern is in many tests, that all should be cleaned up to fix this bad pattern and establish a better new one. (off topic, but having a prompts.md that captures this community knowlege for AI is also good for humans).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-18071 Support Stored Fields in Export Writer [solr]
Posted by "kotman12 (via GitHub)" <gi...@apache.org>.
kotman12 commented on code in PR #4053:
URL: https://github.com/apache/solr/pull/4053#discussion_r2699814833
##########
solr/solr-ref-guide/modules/query-guide/pages/exporting-result-sets.adoc:
##########
@@ -44,6 +46,11 @@ Filter queries are also supported.
An optional parameter `batchSize` determines the size of the internal buffers for partial results.
The default value is `30000` but users may want to specify smaller values to limit the memory use (at the cost of degraded performance) or higher values to improve export performance (the relationship is not linear and larger values don't bring proportionally larger performance increases).
+An optional parameter `includeStoredFields` (default `false`) enables exporting fields that only have stored values (no docValues).
Review Comment:
Good point. I will add an explanation.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-18071 Support Stored Fields in Export Writer [solr]
Posted by "kotman12 (via GitHub)" <gi...@apache.org>.
kotman12 commented on code in PR #4053:
URL: https://github.com/apache/solr/pull/4053#discussion_r2723442052
##########
solr/solr-ref-guide/modules/query-guide/pages/exporting-result-sets.adoc:
##########
@@ -23,6 +23,26 @@ This feature uses a stream sorting technique that begins to send records within
The cases where this functionality may be useful include: session analysis, distributed merge joins, time series roll-ups, aggregations on high cardinality fields, fully distributed field collapsing, and sort-based stats.
+== Comparison with Cursors
+
+The `/export` handler offers several advantages over xref:pagination-of-results.adoc#fetching-a-large-number-of-sorted-results-cursors[cursor-based pagination] for streaming large result sets.
+
+With cursors, the query is re-executed for each page of results.
+In contrast, `/export` runs the filter query once and the resulting segment-level bitmasks are applied once per segment, after which the documents are simply iterated over.
+Additionally, the segments that existed when the stream was opened are held open for the duration of the export, eliminating the disappearing or duplicate document issues that can occur with cursors.
+The trade-off is that IndexReaders are kept around for longer periods of time.
+
+Another advantage of `/export` is significantly lower latency until the first document is returned, because the internal batch size is decoupled from the response message size.
+With cursors, you typically need to set the `rows` parameter to a high value (e.g., 100,000) to achieve decent throughput.
+However, this creates a "glugging" effect: when you request a large batch, Solr must build the entire payload and send it over the wire while your client waits.
+Only after receiving and decoding this large payload can the client request the next batch, but in the interim Solr sits idle on this request.
+With the `/export` handler, these steps are decoupled - Solr can continue sorting and decoding/encoding documents while waiting for more demand from the client.
+
+The advantage of cursors is flexibility.
+A cursor mark can be persisted and resumed later, even across restarts, whereas an `/export` stream is entirely in-memory and must be consumed in a single session.
Review Comment:
I do feel it is implied that an export stream doesn't need to be fully consumed. Like what if the client crashes? It would be unreasonable to implement export in a way that can't handle a crashing client. I suppose one could mention `close` but not sure if this is the right place.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-18071 Support Stored Fields in Export Writer [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on code in PR #4053:
URL: https://github.com/apache/solr/pull/4053#discussion_r2723464293
##########
solr/solr-ref-guide/modules/query-guide/pages/exporting-result-sets.adoc:
##########
@@ -23,6 +23,26 @@ This feature uses a stream sorting technique that begins to send records within
The cases where this functionality may be useful include: session analysis, distributed merge joins, time series roll-ups, aggregations on high cardinality fields, fully distributed field collapsing, and sort-based stats.
+== Comparison with Cursors
+
+The `/export` handler offers several advantages over xref:pagination-of-results.adoc#fetching-a-large-number-of-sorted-results-cursors[cursor-based pagination] for streaming large result sets.
+
+With cursors, the query is re-executed for each page of results.
+In contrast, `/export` runs the filter query once and the resulting segment-level bitmasks are applied once per segment, after which the documents are simply iterated over.
+Additionally, the segments that existed when the stream was opened are held open for the duration of the export, eliminating the disappearing or duplicate document issues that can occur with cursors.
+The trade-off is that IndexReaders are kept around for longer periods of time.
+
+Another advantage of `/export` is significantly lower latency until the first document is returned, because the internal batch size is decoupled from the response message size.
+With cursors, you typically need to set the `rows` parameter to a high value (e.g., 100,000) to achieve decent throughput.
+However, this creates a "glugging" effect: when you request a large batch, Solr must build the entire payload and send it over the wire while your client waits.
Review Comment:
bq. My recollection is that the full responses of each shard are merged together at once (not streamed) but I could be misunderstanding.
Aaaah, ok right, you have a good point here! I wasn't considering distributed-search, only a single shard's contribution. Albeit comparing /export with cursor under apple's to apple's, it'd be single-shard to single-shard. But the big picture changes things, so you're right about that in practice. I suppose there's some useful need for a streaming expression aggregator that can cursor-mark over the shards individually to avoid that cost. I looked for cursorMark usage in streaming expressions and I'm surprised to see none. There will be even less need for such a thing after this PR!
Any way, I don't think the pros/cons in this document needs to go into technical/internal depth; it's so rare that the ref guide speaks of such geeky internal things (e.g. searchers).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-18071 Support Stored Fields in Export Writer [solr]
Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh commented on code in PR #4053:
URL: https://github.com/apache/solr/pull/4053#discussion_r2699220549
##########
solr/core/src/java/org/apache/solr/handler/export/ExportWriter.java:
##########
@@ -484,43 +494,75 @@ void writeDoc(
LeafReaderContext context = leaves.get(ord);
int fieldIndex = 0;
for (FieldWriter fieldWriter : writers) {
- if (fieldWriter.write(sortDoc, context, ew, fieldIndex)) {
- ++fieldIndex;
- }
+ fieldIndex += fieldWriter.write(sortDoc, context, ew, fieldIndex);
}
}
public List<FieldWriter> getFieldWriters(String[] fields, SolrQueryRequest req)
throws IOException {
DocValuesIteratorCache dvIterCache = new DocValuesIteratorCache(req.getSearcher(), false);
-
SolrReturnFields solrReturnFields = new SolrReturnFields(fields, req);
+ boolean includeStoredFields = req.getParams().getBool(INCLUDE_STORED_FIELDS_PARAM, false);
List<FieldWriter> writers = new ArrayList<>();
+ Set<SchemaField> docValueFields = new LinkedHashSet<>();
+ Map<String, SchemaField> storedFields = new LinkedHashMap<>();
+
for (String field : req.getSearcher().getFieldNames()) {
if (!solrReturnFields.wantsField(field)) {
continue;
}
SchemaField schemaField = req.getSchema().getField(field);
- if (!schemaField.hasDocValues()) {
- throw new IOException(schemaField + " must have DocValues to use this feature.");
Review Comment:
Love this! I've been burned so many times on wanting to do this and discovering missing DocValues.
##########
solr/solr-ref-guide/modules/query-guide/pages/exporting-result-sets.adoc:
##########
@@ -44,6 +46,11 @@ Filter queries are also supported.
An optional parameter `batchSize` determines the size of the internal buffers for partial results.
The default value is `30000` but users may want to specify smaller values to limit the memory use (at the cost of degraded performance) or higher values to improve export performance (the relationship is not linear and larger values don't bring proportionally larger performance increases).
+An optional parameter `includeStoredFields` (default `false`) enables exporting fields that only have stored values (no docValues).
Review Comment:
some day we'll have a way of thinking about should this be "true" ;-). Challenge of open source is we struggle to know what hsould be a default yes even when it has knock on impacts until a lot of time passes. ;-)
##########
solr/core/src/test/org/apache/solr/handler/export/TestExportWriter.java:
##########
@@ -1476,4 +1474,261 @@ private void addField(SolrInputDocument doc, String type, String value, boolean
doc.addField("number_" + type + (mv ? "s" : "") + "_ni_t", value);
doc.addField("number_" + type + (mv ? "s" : "") + "_ni_p", value);
}
+
+ @Test
+ public void testIncludeStoredFieldsExplicitRequest() throws Exception {
+ // Test that stored-only fields are returned when includeStoredFields=true
+ clearIndex();
Review Comment:
maybe not speciifc to your goal, but should `clearIndex()` be in a before method/setup step? If it's on every unit test?
##########
solr/core/src/java/org/apache/solr/handler/export/DoubleFieldWriter.java:
##########
@@ -34,7 +34,7 @@ public DoubleFieldWriter(
}
@Override
- public boolean write(
+ public int write(
Review Comment:
I went down thorugh the code, why the boolean -> int? Looks like we only ever return 0 or 1?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-18071 Support Stored Fields in Export Writer [solr]
Posted by "kotman12 (via GitHub)" <gi...@apache.org>.
kotman12 commented on code in PR #4053:
URL: https://github.com/apache/solr/pull/4053#discussion_r2705560913
##########
solr/core/src/java/org/apache/solr/handler/export/StoredFieldsWriter.java:
##########
@@ -0,0 +1,148 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.solr.handler.export;
+
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.Date;
+import java.util.List;
+import java.util.Map;
+import java.util.WeakHashMap;
+import org.apache.lucene.index.FieldInfo;
+import org.apache.lucene.index.IndexReader;
+import org.apache.lucene.index.LeafReader;
+import org.apache.lucene.index.LeafReaderContext;
+import org.apache.lucene.index.StoredFieldVisitor;
+import org.apache.lucene.index.StoredFields;
+import org.apache.solr.common.MapWriter.EntryWriter;
+import org.apache.solr.schema.BoolField;
+import org.apache.solr.schema.DateValueFieldType;
+import org.apache.solr.schema.SchemaField;
+
+class StoredFieldsWriter extends FieldWriter {
+
+ private final Map<String, SchemaField> fields;
+ private final ThreadLocal<WeakHashMap<IndexReader.CacheKey, StoredFields>> storedFieldsMap =
+ new ThreadLocal<>();
+
+ public StoredFieldsWriter(Map<String, SchemaField> fieldsToRead) {
+ this.fields = fieldsToRead;
+ }
+
+ @Override
+ public int write(
+ SortDoc sortDoc, LeafReaderContext readerContext, EntryWriter out, int fieldIndex)
+ throws IOException {
+ WeakHashMap<IndexReader.CacheKey, StoredFields> map = storedFieldsMap.get();
+ if (map == null) {
+ map = new WeakHashMap<>();
+ storedFieldsMap.set(map);
+ }
+ LeafReader reader = readerContext.reader();
+ StoredFields storedFields = map.get(reader.getReaderCacheHelper().getKey());
+ if (storedFields == null) {
+ storedFields = reader.storedFields();
+ map.put(reader.getReaderCacheHelper().getKey(), storedFields);
+ }
+ ExportVisitor visitor = new ExportVisitor(out);
+ storedFields.document(sortDoc.docId, visitor);
+ return visitor.flush();
+ }
+
+ class ExportVisitor extends StoredFieldVisitor {
+
+ final EntryWriter out;
+ String lastFieldName;
+ List<Object> multiValue = null;
+ int fieldsVisited;
+
+ public ExportVisitor(EntryWriter out) {
+ this.out = out;
+ }
+
+ @Override
+ public void stringField(FieldInfo fieldInfo, String value) throws IOException {
+ var schemaField = fields.get(fieldInfo.name);
+ var fieldType = schemaField == null ? null : schemaField.getType();
+ if (fieldType instanceof BoolField) {
+ // Convert "T"/"F" stored value to boolean true/false
+ addField(fieldInfo.name, Boolean.valueOf(fieldType.indexedToReadable(value)));
+ } else {
+ addField(fieldInfo.name, value);
+ }
+ }
+
+ @Override
+ public void intField(FieldInfo fieldInfo, int value) throws IOException {
+ addField(fieldInfo.name, value);
+ }
+
+ @Override
+ public void longField(FieldInfo fieldInfo, long value) throws IOException {
+ var schemaField = fields.get(fieldInfo.name);
+ var fieldType = schemaField == null ? null : schemaField.getType();
+ if (fieldType instanceof DateValueFieldType) {
+ Date date = new Date(value);
+ addField(fieldInfo.name, date);
+ } else {
+ addField(fieldInfo.name, value);
+ }
+ }
+
+ @Override
+ public void floatField(FieldInfo fieldInfo, float value) throws IOException {
+ addField(fieldInfo.name, value);
+ }
+
+ @Override
+ public void doubleField(FieldInfo fieldInfo, double value) throws IOException {
+ addField(fieldInfo.name, value);
+ }
+
+ @Override
+ public Status needsField(FieldInfo fieldInfo) {
+ return fields.containsKey(fieldInfo.name) ? Status.YES : Status.NO;
+ }
+
+ private <T> void addField(String fieldName, T value) throws IOException {
Review Comment:
This looks good to me. Regarding the extra defensive check in `flush`; so I originally "hand-wrote" this code in a stand-alone patch that I had for experimentation and my check was simple like yours. I tasked Claude to merge that code into the Solr project as-is. As far as I could tell Claude faithfully copied everything over but I see now that it just couldn't help itself with some things 😬. This is more of a cautionary tale...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-18071 Support Stored Fields in Export Writer [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley merged PR #4053:
URL: https://github.com/apache/solr/pull/4053
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-18071 Support Stored Fields in Export Writer [solr]
Posted by "kotman12 (via GitHub)" <gi...@apache.org>.
kotman12 commented on code in PR #4053:
URL: https://github.com/apache/solr/pull/4053#discussion_r2710008273
##########
solr/core/src/java/org/apache/solr/handler/export/ExportWriter.java:
##########
@@ -484,43 +488,75 @@ void writeDoc(
LeafReaderContext context = leaves.get(ord);
int fieldIndex = 0;
for (FieldWriter fieldWriter : writers) {
- if (fieldWriter.write(sortDoc, context, ew, fieldIndex)) {
- ++fieldIndex;
- }
+ fieldIndex += fieldWriter.write(sortDoc, context, ew, fieldIndex);
}
}
public List<FieldWriter> getFieldWriters(String[] fields, SolrQueryRequest req)
throws IOException {
DocValuesIteratorCache dvIterCache = new DocValuesIteratorCache(req.getSearcher(), false);
-
SolrReturnFields solrReturnFields = new SolrReturnFields(fields, req);
+ boolean includeStoredFields = req.getParams().getBool(INCLUDE_STORED_FIELDS_PARAM, false);
List<FieldWriter> writers = new ArrayList<>();
+ Set<SchemaField> docValueFields = new LinkedHashSet<>();
+ Map<String, SchemaField> storedFields = new LinkedHashMap<>();
+
for (String field : req.getSearcher().getFieldNames()) {
if (!solrReturnFields.wantsField(field)) {
continue;
}
SchemaField schemaField = req.getSchema().getField(field);
- if (!schemaField.hasDocValues()) {
- throw new IOException(schemaField + " must have DocValues to use this feature.");
- }
- boolean multiValued = schemaField.multiValued();
FieldType fieldType = schemaField.getType();
- FieldWriter writer;
- if (fieldType instanceof SortableTextField && !schemaField.useDocValuesAsStored()) {
- if (solrReturnFields.getRequestedFieldNames() != null
- && solrReturnFields.getRequestedFieldNames().contains(field)) {
- // Explicitly requested field cannot be used due to not having useDocValuesAsStored=true,
- // throw exception
+ // Check if field can use DocValues
+ boolean canUseDocValues =
+ schemaField.hasDocValues()
+ && (!(fieldType instanceof SortableTextField) || schemaField.useDocValuesAsStored());
Review Comment:
The reason is for `/export` it was decided that you don't need an explicit `udvas` for "cheap" fields I guess (so anything that is _not_ a text field)? Idk, I am trying to follow this [thread](https://github.com/apache/solr/pull/1996#discussion_r1355153153).
Edit: could also be a backwards compat thing since this udvas requirement wasn't there for the other fields and then when `SortableTextField` was added it was decided to handle it a more correct way.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-18071 Support Stored Fields in Export Writer [solr]
Posted by "kotman12 (via GitHub)" <gi...@apache.org>.
kotman12 commented on code in PR #4053:
URL: https://github.com/apache/solr/pull/4053#discussion_r2710257429
##########
solr/core/src/java/org/apache/solr/handler/export/ExportWriter.java:
##########
@@ -484,43 +488,75 @@ void writeDoc(
LeafReaderContext context = leaves.get(ord);
int fieldIndex = 0;
for (FieldWriter fieldWriter : writers) {
- if (fieldWriter.write(sortDoc, context, ew, fieldIndex)) {
- ++fieldIndex;
- }
+ fieldIndex += fieldWriter.write(sortDoc, context, ew, fieldIndex);
}
}
public List<FieldWriter> getFieldWriters(String[] fields, SolrQueryRequest req)
throws IOException {
DocValuesIteratorCache dvIterCache = new DocValuesIteratorCache(req.getSearcher(), false);
-
SolrReturnFields solrReturnFields = new SolrReturnFields(fields, req);
+ boolean includeStoredFields = req.getParams().getBool(INCLUDE_STORED_FIELDS_PARAM, false);
List<FieldWriter> writers = new ArrayList<>();
+ Set<SchemaField> docValueFields = new LinkedHashSet<>();
+ Map<String, SchemaField> storedFields = new LinkedHashMap<>();
+
for (String field : req.getSearcher().getFieldNames()) {
if (!solrReturnFields.wantsField(field)) {
continue;
}
SchemaField schemaField = req.getSchema().getField(field);
- if (!schemaField.hasDocValues()) {
- throw new IOException(schemaField + " must have DocValues to use this feature.");
- }
- boolean multiValued = schemaField.multiValued();
FieldType fieldType = schemaField.getType();
- FieldWriter writer;
- if (fieldType instanceof SortableTextField && !schemaField.useDocValuesAsStored()) {
- if (solrReturnFields.getRequestedFieldNames() != null
- && solrReturnFields.getRequestedFieldNames().contains(field)) {
- // Explicitly requested field cannot be used due to not having useDocValuesAsStored=true,
- // throw exception
+ // Check if field can use DocValues
+ boolean canUseDocValues =
+ schemaField.hasDocValues()
+ && (!(fieldType instanceof SortableTextField) || schemaField.useDocValuesAsStored());
Review Comment:
I've added a comment for this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-18071 Support Stored Fields in Export Writer [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on PR #4053:
URL: https://github.com/apache/solr/pull/4053#issuecomment-3764173111
Just a quick observation -- I see a force push that came after Eric's review. I strongly recommend against force pushing because it resets the review state for anyone who has already reviewed -- Eric in this case.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-18071 Support Stored Fields in Export Writer [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on code in PR #4053:
URL: https://github.com/apache/solr/pull/4053#discussion_r2701331616
##########
solr/core/src/java/org/apache/solr/handler/export/FieldWriter.java:
##########
@@ -22,7 +22,7 @@
import org.apache.solr.common.MapWriter;
abstract class FieldWriter {
Review Comment:
deserves javadoc, at least for clarifying it's return semantics
##########
solr/core/src/java/org/apache/solr/handler/export/StoredFieldsWriter.java:
##########
@@ -0,0 +1,148 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.solr.handler.export;
+
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.Date;
+import java.util.List;
+import java.util.Map;
+import java.util.WeakHashMap;
+import org.apache.lucene.index.FieldInfo;
+import org.apache.lucene.index.IndexReader;
+import org.apache.lucene.index.LeafReader;
+import org.apache.lucene.index.LeafReaderContext;
+import org.apache.lucene.index.StoredFieldVisitor;
+import org.apache.lucene.index.StoredFields;
+import org.apache.solr.common.MapWriter.EntryWriter;
+import org.apache.solr.schema.BoolField;
+import org.apache.solr.schema.DateValueFieldType;
+import org.apache.solr.schema.SchemaField;
+
+class StoredFieldsWriter extends FieldWriter {
+
+ private final Map<String, SchemaField> fields;
+ private final ThreadLocal<WeakHashMap<IndexReader.CacheKey, StoredFields>> storedFieldsMap =
+ new ThreadLocal<>();
+
+ public StoredFieldsWriter(Map<String, SchemaField> fieldsToRead) {
+ this.fields = fieldsToRead;
+ }
+
+ @Override
+ public int write(
+ SortDoc sortDoc, LeafReaderContext readerContext, EntryWriter out, int fieldIndex)
+ throws IOException {
+ WeakHashMap<IndexReader.CacheKey, StoredFields> map = storedFieldsMap.get();
+ if (map == null) {
+ map = new WeakHashMap<>();
+ storedFieldsMap.set(map);
+ }
Review Comment:
this pattern can be improved to basically be handled at the ThreadLocal declaration to provide an initializer.
But moreover I'm concerned about the use of ThreadLocal in the first place -- it's typically a tool of last resort. And further true with use of weak references.
##########
solr/core/src/java/org/apache/solr/handler/export/StoredFieldsWriter.java:
##########
@@ -0,0 +1,148 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.solr.handler.export;
+
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.Date;
+import java.util.List;
+import java.util.Map;
+import java.util.WeakHashMap;
+import org.apache.lucene.index.FieldInfo;
+import org.apache.lucene.index.IndexReader;
+import org.apache.lucene.index.LeafReader;
+import org.apache.lucene.index.LeafReaderContext;
+import org.apache.lucene.index.StoredFieldVisitor;
+import org.apache.lucene.index.StoredFields;
+import org.apache.solr.common.MapWriter.EntryWriter;
+import org.apache.solr.schema.BoolField;
+import org.apache.solr.schema.DateValueFieldType;
+import org.apache.solr.schema.SchemaField;
+
+class StoredFieldsWriter extends FieldWriter {
+
+ private final Map<String, SchemaField> fields;
Review Comment:
nit: I'd prefer this be named `schema` or something including that word
##########
solr/core/src/java/org/apache/solr/handler/export/StoredFieldsWriter.java:
##########
@@ -0,0 +1,148 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.solr.handler.export;
+
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.Date;
+import java.util.List;
+import java.util.Map;
+import java.util.WeakHashMap;
+import org.apache.lucene.index.FieldInfo;
+import org.apache.lucene.index.IndexReader;
+import org.apache.lucene.index.LeafReader;
+import org.apache.lucene.index.LeafReaderContext;
+import org.apache.lucene.index.StoredFieldVisitor;
+import org.apache.lucene.index.StoredFields;
+import org.apache.solr.common.MapWriter.EntryWriter;
+import org.apache.solr.schema.BoolField;
+import org.apache.solr.schema.DateValueFieldType;
+import org.apache.solr.schema.SchemaField;
+
+class StoredFieldsWriter extends FieldWriter {
+
+ private final Map<String, SchemaField> fields;
+ private final ThreadLocal<WeakHashMap<IndexReader.CacheKey, StoredFields>> storedFieldsMap =
Review Comment:
ThreadLocals are typically static. In this case, I think it should be because the information in it isn't specific to this instance of StoredFieldsWriter in any way that I can see. Java static analyzers including built into IntelliJ will generally advise you on this point.
##########
solr/core/src/test/org/apache/solr/handler/export/TestExportWriter.java:
##########
@@ -1476,4 +1474,261 @@ private void addField(SolrInputDocument doc, String type, String value, boolean
doc.addField("number_" + type + (mv ? "s" : "") + "_ni_t", value);
doc.addField("number_" + type + (mv ? "s" : "") + "_ni_p", value);
}
+
+ @Test
+ public void testIncludeStoredFieldsExplicitRequest() throws Exception {
+ // Test that stored-only fields are returned when includeStoredFields=true
+ clearIndex();
+
+ assertU(
+ adoc(
+ "id", "1",
+ "intdv", "1",
+ "str_s_stored", "hello",
+ "num_i_stored", "42",
+ "num_l_stored", "1234567890123",
+ "num_f_stored", "3.14",
+ "num_d_stored", "2.71828",
+ "date_dt_stored", "2024-01-15T10:30:00Z",
+ "bool_b_stored", "true"));
+ assertU(commit());
+
+ String resp =
+ h.query(
+ req(
+ "q", "*:*",
+ "qt", "/export",
Review Comment:
nit: I prefer to see this first
##########
solr/core/src/test/org/apache/solr/handler/export/TestExportWriter.java:
##########
@@ -1476,4 +1474,261 @@ private void addField(SolrInputDocument doc, String type, String value, boolean
doc.addField("number_" + type + (mv ? "s" : "") + "_ni_t", value);
doc.addField("number_" + type + (mv ? "s" : "") + "_ni_p", value);
}
+
+ @Test
+ public void testIncludeStoredFieldsExplicitRequest() throws Exception {
+ // Test that stored-only fields are returned when includeStoredFields=true
+ clearIndex();
+
+ assertU(
+ adoc(
+ "id", "1",
+ "intdv", "1",
+ "str_s_stored", "hello",
+ "num_i_stored", "42",
+ "num_l_stored", "1234567890123",
+ "num_f_stored", "3.14",
+ "num_d_stored", "2.71828",
+ "date_dt_stored", "2024-01-15T10:30:00Z",
+ "bool_b_stored", "true"));
+ assertU(commit());
+
+ String resp =
+ h.query(
+ req(
+ "q", "*:*",
+ "qt", "/export",
+ "fl",
+ "id,str_s_stored,num_i_stored,num_l_stored,num_f_stored,num_d_stored,date_dt_stored,bool_b_stored",
+ "sort", "intdv asc",
+ "includeStoredFields", "true"));
+
+ assertJsonEquals(
+ resp,
+ "{\n"
+ + " \"responseHeader\":{\"status\":0},\n"
+ + " \"response\":{\n"
+ + " \"numFound\":1,\n"
+ + " \"docs\":[{\n"
+ + " \"id\":\"1\",\n"
+ + " \"str_s_stored\":\"hello\",\n"
+ + " \"num_i_stored\":42,\n"
+ + " \"num_l_stored\":1234567890123,\n"
+ + " \"num_f_stored\":3.14,\n"
+ + " \"num_d_stored\":2.71828,\n"
+ + " \"date_dt_stored\":\"2024-01-15T10:30:00Z\",\n"
+ + " \"bool_b_stored\":true}]}}");
+ }
+
+ @Test
+ public void testIncludeStoredFieldsErrorWithoutParam() throws Exception {
+ // Test that error with hint is thrown when requesting stored-only field without
+ // includeStoredFields
+ clearIndex();
+
+ assertU(adoc("id", "1", "intdv", "1", "str_s_stored", "hello"));
+ assertU(commit());
+
+ // Request stored-only field without includeStoredFields=true should error
+ String resp =
+ h.query(
+ req(
+ "q", "*:*",
+ "qt", "/export",
+ "fl", "id,str_s_stored",
+ "sort", "intdv asc"));
+
+ assertTrue(
+ "Expected error message to contain hint about includeStoredFields",
+ resp.contains("includeStoredFields=true"));
+ assertTrue("Expected error message to mention the field", resp.contains("str_s_stored"));
+ }
+
+ @Test
+ public void testIncludeStoredFieldsGlobSkipsWithoutParam() throws Exception {
+ // Test that glob pattern silently skips stored-only fields when includeStoredFields=false
+ clearIndex();
+
+ assertU(
+ adoc(
+ "id", "1",
+ "intdv", "1",
+ "stringdv", "docvalue_string",
+ "str_s_stored", "stored_string"));
+ assertU(commit());
+
+ // Explicit fl with stored-only field should error
+ String resp =
+ h.query(
+ req(
+ "q", "*:*",
+ "qt", "/export",
+ "fl", "id,intdv,stringdv,str_s_stored",
+ "sort", "intdv asc"));
+
+ // Should error because str_s_stored is explicitly requested
+ assertTrue(
+ "Expected error for explicitly requested stored-only field", resp.contains("str_s_stored"));
+ assertTrue(
+ "Expected hint about includeStoredFields", resp.contains("includeStoredFields=true"));
+
+ // Now test with glob - should silently skip stored-only fields and succeed
+ resp =
+ h.query(
+ req(
+ "q", "*:*",
+ "qt", "/export",
+ "fl", "intdv,*",
+ "sort", "intdv asc"));
+
+ assertJsonEquals(
+ resp,
+ "{\n"
+ + " \"responseHeader\":{\"status\":0},\n"
+ + " \"response\":{\n"
+ + " \"numFound\":1,\n"
+ + " \"docs\":[{\n"
+ + " \"id\":\"1\",\n"
+ + " \"intdv\":1,\n"
+ + " \"stringdv\":\"docvalue_string\"}]}}");
+ }
+
+ @Test
+ public void testIncludeStoredFieldsGlobIncludesWithParam() throws Exception {
+ // Test that glob pattern includes stored-only fields when includeStoredFields=true
+ clearIndex();
+
+ assertU(
+ adoc(
+ "id", "1",
+ "intdv", "1",
+ "stringdv", "docvalue_string",
+ "str_s_stored", "stored_string"));
+ assertU(commit());
+
+ // Glob fl=* with includeStoredFields=true should include stored-only fields
+ String resp =
+ h.query(
+ req(
+ "q", "*:*",
+ "qt", "/export",
+ "fl", "*",
+ "sort", "intdv asc",
+ "includeStoredFields", "true"));
+
+ assertJsonEquals(
+ resp,
+ "{\n"
+ + " \"responseHeader\":{\"status\":0},\n"
+ + " \"response\":{\n"
+ + " \"numFound\":1,\n"
+ + " \"docs\":[{\n"
+ + " \"intdv\":1,\n"
+ + " \"stringdv\":\"docvalue_string\",\n"
+ + " \"id\":\"1\",\n"
+ + " \"str_s_stored\":\"stored_string\"}]}}");
+ }
+
+ @Test
+ public void testIncludeStoredFieldsMultiValued() throws Exception {
+ // Test that multi-valued stored-only fields work correctly
+ clearIndex();
+
+ assertU(
+ adoc(
+ "id", "1",
+ "intdv", "1",
+ "strs_ss_stored", "value1",
+ "strs_ss_stored", "value2",
+ "strs_ss_stored", "value3",
+ "nums_is_stored", "10",
+ "nums_is_stored", "20",
+ "nums_is_stored", "30"));
+ assertU(commit());
+
+ String resp =
+ h.query(
+ req(
+ "q", "*:*",
+ "qt", "/export",
+ "fl", "id,strs_ss_stored,nums_is_stored",
+ "sort", "intdv asc",
+ "includeStoredFields", "true"));
+
+ assertJsonEquals(
+ resp,
+ "{\n"
Review Comment:
if you didn't get the memo -- we're on Java 21 now :-). multi-line string please
##########
solr/core/src/java/org/apache/solr/handler/export/StoredFieldsWriter.java:
##########
@@ -0,0 +1,148 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.solr.handler.export;
+
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.Date;
+import java.util.List;
+import java.util.Map;
+import java.util.WeakHashMap;
+import org.apache.lucene.index.FieldInfo;
+import org.apache.lucene.index.IndexReader;
+import org.apache.lucene.index.LeafReader;
+import org.apache.lucene.index.LeafReaderContext;
+import org.apache.lucene.index.StoredFieldVisitor;
+import org.apache.lucene.index.StoredFields;
+import org.apache.solr.common.MapWriter.EntryWriter;
+import org.apache.solr.schema.BoolField;
+import org.apache.solr.schema.DateValueFieldType;
+import org.apache.solr.schema.SchemaField;
+
+class StoredFieldsWriter extends FieldWriter {
+
+ private final Map<String, SchemaField> fields;
+ private final ThreadLocal<WeakHashMap<IndexReader.CacheKey, StoredFields>> storedFieldsMap =
+ new ThreadLocal<>();
+
+ public StoredFieldsWriter(Map<String, SchemaField> fieldsToRead) {
+ this.fields = fieldsToRead;
+ }
+
+ @Override
+ public int write(
+ SortDoc sortDoc, LeafReaderContext readerContext, EntryWriter out, int fieldIndex)
+ throws IOException {
+ WeakHashMap<IndexReader.CacheKey, StoredFields> map = storedFieldsMap.get();
+ if (map == null) {
+ map = new WeakHashMap<>();
+ storedFieldsMap.set(map);
+ }
+ LeafReader reader = readerContext.reader();
+ StoredFields storedFields = map.get(reader.getReaderCacheHelper().getKey());
+ if (storedFields == null) {
+ storedFields = reader.storedFields();
+ map.put(reader.getReaderCacheHelper().getKey(), storedFields);
+ }
+ ExportVisitor visitor = new ExportVisitor(out);
+ storedFields.document(sortDoc.docId, visitor);
+ return visitor.flush();
+ }
+
+ class ExportVisitor extends StoredFieldVisitor {
+
+ final EntryWriter out;
+ String lastFieldName;
+ List<Object> multiValue = null;
+ int fieldsVisited;
+
+ public ExportVisitor(EntryWriter out) {
+ this.out = out;
+ }
+
+ @Override
+ public void stringField(FieldInfo fieldInfo, String value) throws IOException {
+ var schemaField = fields.get(fieldInfo.name);
+ var fieldType = schemaField == null ? null : schemaField.getType();
+ if (fieldType instanceof BoolField) {
+ // Convert "T"/"F" stored value to boolean true/false
+ addField(fieldInfo.name, Boolean.valueOf(fieldType.indexedToReadable(value)));
+ } else {
+ addField(fieldInfo.name, value);
+ }
+ }
+
+ @Override
+ public void intField(FieldInfo fieldInfo, int value) throws IOException {
+ addField(fieldInfo.name, value);
+ }
+
+ @Override
+ public void longField(FieldInfo fieldInfo, long value) throws IOException {
+ var schemaField = fields.get(fieldInfo.name);
+ var fieldType = schemaField == null ? null : schemaField.getType();
+ if (fieldType instanceof DateValueFieldType) {
+ Date date = new Date(value);
+ addField(fieldInfo.name, date);
+ } else {
+ addField(fieldInfo.name, value);
+ }
+ }
+
+ @Override
+ public void floatField(FieldInfo fieldInfo, float value) throws IOException {
+ addField(fieldInfo.name, value);
+ }
+
+ @Override
+ public void doubleField(FieldInfo fieldInfo, double value) throws IOException {
+ addField(fieldInfo.name, value);
+ }
+
+ @Override
+ public Status needsField(FieldInfo fieldInfo) {
+ return fields.containsKey(fieldInfo.name) ? Status.YES : Status.NO;
+ }
+
+ private <T> void addField(String fieldName, T value) throws IOException {
Review Comment:
the logic flow is confusing... I started to write why it wouldn't work but end up finally contorting myself to see how it could work. Could you try to reflow it somehow? Or if you like it's flow then add comments? (or both)
##########
solr/core/src/java/org/apache/solr/handler/export/ExportWriter.java:
##########
@@ -484,43 +488,75 @@ void writeDoc(
LeafReaderContext context = leaves.get(ord);
int fieldIndex = 0;
for (FieldWriter fieldWriter : writers) {
- if (fieldWriter.write(sortDoc, context, ew, fieldIndex)) {
- ++fieldIndex;
- }
+ fieldIndex += fieldWriter.write(sortDoc, context, ew, fieldIndex);
}
}
public List<FieldWriter> getFieldWriters(String[] fields, SolrQueryRequest req)
throws IOException {
DocValuesIteratorCache dvIterCache = new DocValuesIteratorCache(req.getSearcher(), false);
-
SolrReturnFields solrReturnFields = new SolrReturnFields(fields, req);
+ boolean includeStoredFields = req.getParams().getBool(INCLUDE_STORED_FIELDS_PARAM, false);
List<FieldWriter> writers = new ArrayList<>();
+ Set<SchemaField> docValueFields = new LinkedHashSet<>();
+ Map<String, SchemaField> storedFields = new LinkedHashMap<>();
+
for (String field : req.getSearcher().getFieldNames()) {
if (!solrReturnFields.wantsField(field)) {
continue;
}
SchemaField schemaField = req.getSchema().getField(field);
- if (!schemaField.hasDocValues()) {
- throw new IOException(schemaField + " must have DocValues to use this feature.");
- }
- boolean multiValued = schemaField.multiValued();
FieldType fieldType = schemaField.getType();
- FieldWriter writer;
- if (fieldType instanceof SortableTextField && !schemaField.useDocValuesAsStored()) {
- if (solrReturnFields.getRequestedFieldNames() != null
- && solrReturnFields.getRequestedFieldNames().contains(field)) {
- // Explicitly requested field cannot be used due to not having useDocValuesAsStored=true,
- // throw exception
+ // Check if field can use DocValues
+ boolean canUseDocValues =
+ schemaField.hasDocValues()
+ && (!(fieldType instanceof SortableTextField) || schemaField.useDocValuesAsStored());
Review Comment:
is there precedent for this special case RE `instanceof SortableTextField`?
##########
solr/core/src/java/org/apache/solr/handler/export/StoredFieldsWriter.java:
##########
@@ -0,0 +1,148 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.solr.handler.export;
+
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.Date;
+import java.util.List;
+import java.util.Map;
+import java.util.WeakHashMap;
+import org.apache.lucene.index.FieldInfo;
+import org.apache.lucene.index.IndexReader;
+import org.apache.lucene.index.LeafReader;
+import org.apache.lucene.index.LeafReaderContext;
+import org.apache.lucene.index.StoredFieldVisitor;
+import org.apache.lucene.index.StoredFields;
+import org.apache.solr.common.MapWriter.EntryWriter;
+import org.apache.solr.schema.BoolField;
+import org.apache.solr.schema.DateValueFieldType;
+import org.apache.solr.schema.SchemaField;
+
+class StoredFieldsWriter extends FieldWriter {
+
+ private final Map<String, SchemaField> fields;
+ private final ThreadLocal<WeakHashMap<IndexReader.CacheKey, StoredFields>> storedFieldsMap =
+ new ThreadLocal<>();
+
+ public StoredFieldsWriter(Map<String, SchemaField> fieldsToRead) {
+ this.fields = fieldsToRead;
+ }
+
+ @Override
+ public int write(
+ SortDoc sortDoc, LeafReaderContext readerContext, EntryWriter out, int fieldIndex)
+ throws IOException {
+ WeakHashMap<IndexReader.CacheKey, StoredFields> map = storedFieldsMap.get();
+ if (map == null) {
+ map = new WeakHashMap<>();
+ storedFieldsMap.set(map);
+ }
+ LeafReader reader = readerContext.reader();
+ StoredFields storedFields = map.get(reader.getReaderCacheHelper().getKey());
+ if (storedFields == null) {
+ storedFields = reader.storedFields();
+ map.put(reader.getReaderCacheHelper().getKey(), storedFields);
+ }
+ ExportVisitor visitor = new ExportVisitor(out);
+ storedFields.document(sortDoc.docId, visitor);
+ return visitor.flush();
+ }
+
+ class ExportVisitor extends StoredFieldVisitor {
+
+ final EntryWriter out;
+ String lastFieldName;
+ List<Object> multiValue = null;
+ int fieldsVisited;
+
+ public ExportVisitor(EntryWriter out) {
+ this.out = out;
+ }
+
+ @Override
+ public void stringField(FieldInfo fieldInfo, String value) throws IOException {
+ var schemaField = fields.get(fieldInfo.name);
+ var fieldType = schemaField == null ? null : schemaField.getType();
+ if (fieldType instanceof BoolField) {
+ // Convert "T"/"F" stored value to boolean true/false
+ addField(fieldInfo.name, Boolean.valueOf(fieldType.indexedToReadable(value)));
+ } else {
+ addField(fieldInfo.name, value);
+ }
+ }
+
+ @Override
+ public void intField(FieldInfo fieldInfo, int value) throws IOException {
+ addField(fieldInfo.name, value);
+ }
+
+ @Override
+ public void longField(FieldInfo fieldInfo, long value) throws IOException {
+ var schemaField = fields.get(fieldInfo.name);
+ var fieldType = schemaField == null ? null : schemaField.getType();
+ if (fieldType instanceof DateValueFieldType) {
+ Date date = new Date(value);
+ addField(fieldInfo.name, date);
+ } else {
+ addField(fieldInfo.name, value);
+ }
+ }
+
+ @Override
+ public void floatField(FieldInfo fieldInfo, float value) throws IOException {
+ addField(fieldInfo.name, value);
+ }
+
+ @Override
+ public void doubleField(FieldInfo fieldInfo, double value) throws IOException {
+ addField(fieldInfo.name, value);
+ }
+
+ @Override
+ public Status needsField(FieldInfo fieldInfo) {
+ return fields.containsKey(fieldInfo.name) ? Status.YES : Status.NO;
+ }
+
+ private <T> void addField(String fieldName, T value) throws IOException {
+ if (fields.get(fieldName).multiValued()) {
+ if (fieldName.equals(lastFieldName)) {
+ multiValue.add(value);
+ } else {
+ if (multiValue != null) {
+ out.put(lastFieldName, multiValue);
+ }
+ multiValue = new ArrayList<>();
+ lastFieldName = fieldName;
+ multiValue.add(value);
+ fieldsVisited++;
+ }
+ } else {
+ out.put(fieldName, value);
+ fieldsVisited++;
+ }
+ }
+
+ private int flush() throws IOException {
+ if (lastFieldName != null && multiValue != null && !multiValue.isEmpty()) {
Review Comment:
I think the logic should be improved to read super simple here. Like, if multiValue is not null then put it -- that's it. No lastFieldName check, no isEmpty check.
##########
solr/core/src/test/org/apache/solr/handler/export/TestExportWriter.java:
##########
@@ -1476,4 +1474,261 @@ private void addField(SolrInputDocument doc, String type, String value, boolean
doc.addField("number_" + type + (mv ? "s" : "") + "_ni_t", value);
doc.addField("number_" + type + (mv ? "s" : "") + "_ni_p", value);
}
+
+ @Test
+ public void testIncludeStoredFieldsExplicitRequest() throws Exception {
+ // Test that stored-only fields are returned when includeStoredFields=true
+ clearIndex();
Review Comment:
being consistent with nearby tests in a single source file (aka suite) makes sense.
##########
solr/core/src/java/org/apache/solr/handler/export/StoredFieldsWriter.java:
##########
@@ -0,0 +1,148 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.solr.handler.export;
+
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.Date;
+import java.util.List;
+import java.util.Map;
+import java.util.WeakHashMap;
+import org.apache.lucene.index.FieldInfo;
+import org.apache.lucene.index.IndexReader;
+import org.apache.lucene.index.LeafReader;
+import org.apache.lucene.index.LeafReaderContext;
+import org.apache.lucene.index.StoredFieldVisitor;
+import org.apache.lucene.index.StoredFields;
+import org.apache.solr.common.MapWriter.EntryWriter;
+import org.apache.solr.schema.BoolField;
+import org.apache.solr.schema.DateValueFieldType;
+import org.apache.solr.schema.SchemaField;
+
+class StoredFieldsWriter extends FieldWriter {
+
+ private final Map<String, SchemaField> fields;
+ private final ThreadLocal<WeakHashMap<IndexReader.CacheKey, StoredFields>> storedFieldsMap =
+ new ThreadLocal<>();
+
+ public StoredFieldsWriter(Map<String, SchemaField> fieldsToRead) {
+ this.fields = fieldsToRead;
+ }
+
+ @Override
+ public int write(
+ SortDoc sortDoc, LeafReaderContext readerContext, EntryWriter out, int fieldIndex)
+ throws IOException {
+ WeakHashMap<IndexReader.CacheKey, StoredFields> map = storedFieldsMap.get();
+ if (map == null) {
+ map = new WeakHashMap<>();
+ storedFieldsMap.set(map);
+ }
+ LeafReader reader = readerContext.reader();
+ StoredFields storedFields = map.get(reader.getReaderCacheHelper().getKey());
+ if (storedFields == null) {
+ storedFields = reader.storedFields();
+ map.put(reader.getReaderCacheHelper().getKey(), storedFields);
+ }
+ ExportVisitor visitor = new ExportVisitor(out);
+ storedFields.document(sortDoc.docId, visitor);
+ return visitor.flush();
+ }
+
+ class ExportVisitor extends StoredFieldVisitor {
+
+ final EntryWriter out;
+ String lastFieldName;
+ List<Object> multiValue = null;
+ int fieldsVisited;
+
+ public ExportVisitor(EntryWriter out) {
+ this.out = out;
+ }
+
+ @Override
+ public void stringField(FieldInfo fieldInfo, String value) throws IOException {
+ var schemaField = fields.get(fieldInfo.name);
+ var fieldType = schemaField == null ? null : schemaField.getType();
+ if (fieldType instanceof BoolField) {
+ // Convert "T"/"F" stored value to boolean true/false
+ addField(fieldInfo.name, Boolean.valueOf(fieldType.indexedToReadable(value)));
+ } else {
+ addField(fieldInfo.name, value);
+ }
Review Comment:
when I see special cases like this, I ask myself... is there a fieldType method that should be handling this? If not do we need to add one?
CC @hossman if you are interested in review; you've looked at this topic in a nearby issue lately
##########
solr/core/src/java/org/apache/solr/handler/export/DoubleFieldWriter.java:
##########
@@ -34,7 +34,7 @@ public DoubleFieldWriter(
}
@Override
- public boolean write(
+ public int write(
Review Comment:
> I would be equally happy to just make FieldWriter::write be void instead of tracking this unused index.
+1
Then you could retract my comment on javadocs for `write`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-18071 Support Stored Fields in Export Writer [solr]
Posted by "kotman12 (via GitHub)" <gi...@apache.org>.
kotman12 commented on code in PR #4053:
URL: https://github.com/apache/solr/pull/4053#discussion_r2710102591
##########
solr/core/src/java/org/apache/solr/handler/export/StoredFieldsWriter.java:
##########
@@ -0,0 +1,148 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.solr.handler.export;
+
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.Date;
+import java.util.List;
+import java.util.Map;
+import java.util.WeakHashMap;
+import org.apache.lucene.index.FieldInfo;
+import org.apache.lucene.index.IndexReader;
+import org.apache.lucene.index.LeafReader;
+import org.apache.lucene.index.LeafReaderContext;
+import org.apache.lucene.index.StoredFieldVisitor;
+import org.apache.lucene.index.StoredFields;
+import org.apache.solr.common.MapWriter.EntryWriter;
+import org.apache.solr.schema.BoolField;
+import org.apache.solr.schema.DateValueFieldType;
+import org.apache.solr.schema.SchemaField;
+
+class StoredFieldsWriter extends FieldWriter {
+
+ private final Map<String, SchemaField> fields;
+ private final ThreadLocal<WeakHashMap<IndexReader.CacheKey, StoredFields>> storedFieldsMap =
+ new ThreadLocal<>();
+
+ public StoredFieldsWriter(Map<String, SchemaField> fieldsToRead) {
+ this.fields = fieldsToRead;
+ }
+
+ @Override
+ public int write(
+ SortDoc sortDoc, LeafReaderContext readerContext, EntryWriter out, int fieldIndex)
+ throws IOException {
+ WeakHashMap<IndexReader.CacheKey, StoredFields> map = storedFieldsMap.get();
+ if (map == null) {
+ map = new WeakHashMap<>();
+ storedFieldsMap.set(map);
+ }
Review Comment:
I also dislike `ThreadLocal`, personally coming from reactive programming experience it's especially painful. But I am not sure how else to share this non-thread-safe resource here without adding significant complexity.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-18071 Support Stored Fields in Export Writer [solr]
Posted by "gus-asf (via GitHub)" <gi...@apache.org>.
gus-asf commented on code in PR #4053:
URL: https://github.com/apache/solr/pull/4053#discussion_r2722465199
##########
solr/solr-ref-guide/modules/query-guide/pages/exporting-result-sets.adoc:
##########
@@ -23,6 +23,24 @@ This feature uses a stream sorting technique that begins to send records within
The cases where this functionality may be useful include: session analysis, distributed merge joins, time series roll-ups, aggregations on high cardinality fields, fully distributed field collapsing, and sort-based stats.
+== Comparison with Cursors
+
+The `/export` handler offers several advantages over xref:pagination-of-results.adoc#fetching-a-large-number-of-sorted-results-cursors[cursor-based pagination] for streaming large result sets.
+
+With cursors, the query is re-executed for each page of results.
+In contrast, `/export` runs the filter query once and the resulting segment-level bitmasks are applied once per segment, after which the documents are simply iterated over.
+Additionally, the segments that existed when the stream was opened are held open for the duration of the export, eliminating the disappearing or duplicate document issues that can occur with cursors.
+The trade-off is that IndexReaders are kept around for longer periods of time.
Review Comment:
Maybe a pro/con 2x2 table?
##########
solr/solr-ref-guide/modules/query-guide/pages/exporting-result-sets.adoc:
##########
@@ -23,6 +23,24 @@ This feature uses a stream sorting technique that begins to send records within
The cases where this functionality may be useful include: session analysis, distributed merge joins, time series roll-ups, aggregations on high cardinality fields, fully distributed field collapsing, and sort-based stats.
+== Comparison with Cursors
+
+The `/export` handler offers several advantages over xref:pagination-of-results.adoc#fetching-a-large-number-of-sorted-results-cursors[cursor-based pagination] for streaming large result sets.
+
+With cursors, the query is re-executed for each page of results.
+In contrast, `/export` runs the filter query once and the resulting segment-level bitmasks are applied once per segment, after which the documents are simply iterated over.
+Additionally, the segments that existed when the stream was opened are held open for the duration of the export, eliminating the disappearing or duplicate document issues that can occur with cursors.
+The trade-off is that IndexReaders are kept around for longer periods of time.
Review Comment:
One more sentence clarifying/quantifying the significance of readers being kept around would probably be good.
##########
solr/solr-ref-guide/modules/query-guide/pages/exporting-result-sets.adoc:
##########
@@ -23,6 +23,24 @@ This feature uses a stream sorting technique that begins to send records within
The cases where this functionality may be useful include: session analysis, distributed merge joins, time series roll-ups, aggregations on high cardinality fields, fully distributed field collapsing, and sort-based stats.
+== Comparison with Cursors
+
+The `/export` handler offers several advantages over xref:pagination-of-results.adoc#fetching-a-large-number-of-sorted-results-cursors[cursor-based pagination] for streaming large result sets.
+
+With cursors, the query is re-executed for each page of results.
+In contrast, `/export` runs the filter query once and the resulting segment-level bitmasks are applied once per segment, after which the documents are simply iterated over.
+Additionally, the segments that existed when the stream was opened are held open for the duration of the export, eliminating the disappearing or duplicate document issues that can occur with cursors.
+The trade-off is that IndexReaders are kept around for longer periods of time.
Review Comment:
Is it dangerous to export a very large stored field? What is the risk? How big is big?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-18071 Support Stored Fields in Export Writer [solr]
Posted by "kotman12 (via GitHub)" <gi...@apache.org>.
kotman12 commented on code in PR #4053:
URL: https://github.com/apache/solr/pull/4053#discussion_r2723454503
##########
solr/solr-ref-guide/modules/query-guide/pages/exporting-result-sets.adoc:
##########
@@ -23,6 +23,24 @@ This feature uses a stream sorting technique that begins to send records within
The cases where this functionality may be useful include: session analysis, distributed merge joins, time series roll-ups, aggregations on high cardinality fields, fully distributed field collapsing, and sort-based stats.
+== Comparison with Cursors
+
+The `/export` handler offers several advantages over xref:pagination-of-results.adoc#fetching-a-large-number-of-sorted-results-cursors[cursor-based pagination] for streaming large result sets.
+
+With cursors, the query is re-executed for each page of results.
+In contrast, `/export` runs the filter query once and the resulting segment-level bitmasks are applied once per segment, after which the documents are simply iterated over.
+Additionally, the segments that existed when the stream was opened are held open for the duration of the export, eliminating the disappearing or duplicate document issues that can occur with cursors.
+The trade-off is that IndexReaders are kept around for longer periods of time.
Review Comment:
> Maybe a pro/con 2x2 table?
If I have time I can give this a try. This is the first time I am updating the ref guide so I probably should figure out how to build it locally to see how something like this renders.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-18071 Support Stored Fields in Export Writer [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on code in PR #4053:
URL: https://github.com/apache/solr/pull/4053#discussion_r2723478805
##########
solr/solr-ref-guide/modules/query-guide/pages/exporting-result-sets.adoc:
##########
@@ -23,6 +23,26 @@ This feature uses a stream sorting technique that begins to send records within
The cases where this functionality may be useful include: session analysis, distributed merge joins, time series roll-ups, aggregations on high cardinality fields, fully distributed field collapsing, and sort-based stats.
+== Comparison with Cursors
+
+The `/export` handler offers several advantages over xref:pagination-of-results.adoc#fetching-a-large-number-of-sorted-results-cursors[cursor-based pagination] for streaming large result sets.
+
+With cursors, the query is re-executed for each page of results.
+In contrast, `/export` runs the filter query once and the resulting segment-level bitmasks are applied once per segment, after which the documents are simply iterated over.
+Additionally, the segments that existed when the stream was opened are held open for the duration of the export, eliminating the disappearing or duplicate document issues that can occur with cursors.
+The trade-off is that IndexReaders are kept around for longer periods of time.
+
+Another advantage of `/export` is significantly lower latency until the first document is returned, because the internal batch size is decoupled from the response message size.
+With cursors, you typically need to set the `rows` parameter to a high value (e.g., 100,000) to achieve decent throughput.
+However, this creates a "glugging" effect: when you request a large batch, Solr must build the entire payload and send it over the wire while your client waits.
+Only after receiving and decoding this large payload can the client request the next batch, but in the interim Solr sits idle on this request.
+With the `/export` handler, these steps are decoupled - Solr can continue sorting and decoding/encoding documents while waiting for more demand from the client.
+
+The advantage of cursors is flexibility.
+A cursor mark can be persisted and resumed later, even across restarts, whereas an `/export` stream is entirely in-memory and must be consumed in a single session.
Review Comment:
right, of course, I mean I wonder if there was testing/care to ensure Solr isn't unreasonably noisy in its logs if a client were to do this routinely.
For example, a year ago I was faced with a use-case that wanted to consume an unknown number of docs so it could post-filter those to ultimately come up with a target threshold of documents. I was thinking of cursorMark at the time; I wanted the score returned. If /export is a possibility, it could have a benefit of being able to stop exactly when I'm finished consuming docs. But if that would spew logs/errors then maybe that wouldn't be a great idea.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-18071 Support Stored Fields in Export Writer [solr]
Posted by "kotman12 (via GitHub)" <gi...@apache.org>.
kotman12 commented on code in PR #4053:
URL: https://github.com/apache/solr/pull/4053#discussion_r2723452801
##########
solr/solr-ref-guide/modules/query-guide/pages/exporting-result-sets.adoc:
##########
@@ -23,6 +23,24 @@ This feature uses a stream sorting technique that begins to send records within
The cases where this functionality may be useful include: session analysis, distributed merge joins, time series roll-ups, aggregations on high cardinality fields, fully distributed field collapsing, and sort-based stats.
+== Comparison with Cursors
+
+The `/export` handler offers several advantages over xref:pagination-of-results.adoc#fetching-a-large-number-of-sorted-results-cursors[cursor-based pagination] for streaming large result sets.
+
+With cursors, the query is re-executed for each page of results.
+In contrast, `/export` runs the filter query once and the resulting segment-level bitmasks are applied once per segment, after which the documents are simply iterated over.
+Additionally, the segments that existed when the stream was opened are held open for the duration of the export, eliminating the disappearing or duplicate document issues that can occur with cursors.
+The trade-off is that IndexReaders are kept around for longer periods of time.
Review Comment:
> One more sentence clarifying/quantifying the significance of readers being kept around would probably be good.
Would the drawback be keeping older segments around longer than you would otherwise as well as increased memory usage as the index drifts away from these "old" readers stuck on a view of the index from when the stream was started?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-18071 Support Stored Fields in Export Writer [solr]
Posted by "kotman12 (via GitHub)" <gi...@apache.org>.
kotman12 commented on code in PR #4053:
URL: https://github.com/apache/solr/pull/4053#discussion_r2723433426
##########
solr/solr-ref-guide/modules/query-guide/pages/exporting-result-sets.adoc:
##########
@@ -23,6 +23,26 @@ This feature uses a stream sorting technique that begins to send records within
The cases where this functionality may be useful include: session analysis, distributed merge joins, time series roll-ups, aggregations on high cardinality fields, fully distributed field collapsing, and sort-based stats.
+== Comparison with Cursors
+
+The `/export` handler offers several advantages over xref:pagination-of-results.adoc#fetching-a-large-number-of-sorted-results-cursors[cursor-based pagination] for streaming large result sets.
+
+With cursors, the query is re-executed for each page of results.
+In contrast, `/export` runs the filter query once and the resulting segment-level bitmasks are applied once per segment, after which the documents are simply iterated over.
+Additionally, the segments that existed when the stream was opened are held open for the duration of the export, eliminating the disappearing or duplicate document issues that can occur with cursors.
+The trade-off is that IndexReaders are kept around for longer periods of time.
+
+Another advantage of `/export` is significantly lower latency until the first document is returned, because the internal batch size is decoupled from the response message size.
+With cursors, you typically need to set the `rows` parameter to a high value (e.g., 100,000) to achieve decent throughput.
+However, this creates a "glugging" effect: when you request a large batch, Solr must build the entire payload and send it over the wire while your client waits.
Review Comment:
> For SearchHandler, The payload is streamed/produced on the fly as it iterates documents. The code isn't there; it's elsewhere in a ResponseWriter, if I recall. Solr does have to do some up-front work -- producing a list of document IDs that match the search, and sorted as desired.
If this is the case then one should be able to get a very large number of rows with one shot with the `SearchHandler`. I assume this is a Solr doc ID list? If so, it will be larger than a doc Id bitset but can still be pretty small for a lot of cases making the need for export handler more exotic. I do wonder how this SearchHandler response writing/streaming works in the multi-sharded case? My recollection is that the full responses of each shard are merged together at once (not streamed) but I could be misunderstanding. I do know that `/stream`ing over the `/export` handler is pretty efficient even in the multi-shard case.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-18071 Support Stored Fields in Export Writer [solr]
Posted by "fsparv (via GitHub)" <gi...@apache.org>.
fsparv commented on code in PR #4053:
URL: https://github.com/apache/solr/pull/4053#discussion_r2723702437
##########
solr/solr-ref-guide/modules/query-guide/pages/exporting-result-sets.adoc:
##########
@@ -23,6 +23,24 @@ This feature uses a stream sorting technique that begins to send records within
The cases where this functionality may be useful include: session analysis, distributed merge joins, time series roll-ups, aggregations on high cardinality fields, fully distributed field collapsing, and sort-based stats.
+== Comparison with Cursors
+
+The `/export` handler offers several advantages over xref:pagination-of-results.adoc#fetching-a-large-number-of-sorted-results-cursors[cursor-based pagination] for streaming large result sets.
+
+With cursors, the query is re-executed for each page of results.
+In contrast, `/export` runs the filter query once and the resulting segment-level bitmasks are applied once per segment, after which the documents are simply iterated over.
+Additionally, the segments that existed when the stream was opened are held open for the duration of the export, eliminating the disappearing or duplicate document issues that can occur with cursors.
+The trade-off is that IndexReaders are kept around for longer periods of time.
Review Comment:
> > Maybe a pro/con 2x2 table?
>
> If I have time I can give this a try. This is the first time I am updating the ref guide so I probably should figure out how to build it locally to see how something like this renders.
The table idea is just one way to present pro/con stuff. The prose I was reading didn't seem to clearly group all the pros for one thing where they could easily be compared to the pro's for the other etc. Bullet lists or careful use of paragraphs could also be effective.
As for building the guide I think that these days it's much easier than it used to be. Should be just a matter of running the right gradle task. (see https://github.com/apache/solr/blob/main/solr/solr-ref-guide/README.adoc) At one time there was a lot of fiddly stuff with getting the right NPM deps installed, but I ***think*** that has gone away.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-18071 Support Stored Fields in Export Writer [solr]
Posted by "fsparv (via GitHub)" <gi...@apache.org>.
fsparv commented on code in PR #4053:
URL: https://github.com/apache/solr/pull/4053#discussion_r2723703290
##########
solr/solr-ref-guide/modules/query-guide/pages/exporting-result-sets.adoc:
##########
@@ -23,6 +23,24 @@ This feature uses a stream sorting technique that begins to send records within
The cases where this functionality may be useful include: session analysis, distributed merge joins, time series roll-ups, aggregations on high cardinality fields, fully distributed field collapsing, and sort-based stats.
+== Comparison with Cursors
+
+The `/export` handler offers several advantages over xref:pagination-of-results.adoc#fetching-a-large-number-of-sorted-results-cursors[cursor-based pagination] for streaming large result sets.
+
+With cursors, the query is re-executed for each page of results.
+In contrast, `/export` runs the filter query once and the resulting segment-level bitmasks are applied once per segment, after which the documents are simply iterated over.
+Additionally, the segments that existed when the stream was opened are held open for the duration of the export, eliminating the disappearing or duplicate document issues that can occur with cursors.
+The trade-off is that IndexReaders are kept around for longer periods of time.
Review Comment:
Oh crud it's late and I've forgotten to switch browser profiles... so I've got the wrong user name (again). fsparv <==> gus-asf
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-18071 Support Stored Fields in Export Writer [solr]
Posted by "kotman12 (via GitHub)" <gi...@apache.org>.
kotman12 commented on code in PR #4053:
URL: https://github.com/apache/solr/pull/4053#discussion_r2710011478
##########
solr/core/src/java/org/apache/solr/handler/export/ExportWriter.java:
##########
@@ -484,43 +488,75 @@ void writeDoc(
LeafReaderContext context = leaves.get(ord);
int fieldIndex = 0;
for (FieldWriter fieldWriter : writers) {
- if (fieldWriter.write(sortDoc, context, ew, fieldIndex)) {
- ++fieldIndex;
- }
+ fieldIndex += fieldWriter.write(sortDoc, context, ew, fieldIndex);
}
}
public List<FieldWriter> getFieldWriters(String[] fields, SolrQueryRequest req)
throws IOException {
DocValuesIteratorCache dvIterCache = new DocValuesIteratorCache(req.getSearcher(), false);
-
SolrReturnFields solrReturnFields = new SolrReturnFields(fields, req);
+ boolean includeStoredFields = req.getParams().getBool(INCLUDE_STORED_FIELDS_PARAM, false);
List<FieldWriter> writers = new ArrayList<>();
+ Set<SchemaField> docValueFields = new LinkedHashSet<>();
+ Map<String, SchemaField> storedFields = new LinkedHashMap<>();
+
for (String field : req.getSearcher().getFieldNames()) {
if (!solrReturnFields.wantsField(field)) {
continue;
}
SchemaField schemaField = req.getSchema().getField(field);
- if (!schemaField.hasDocValues()) {
- throw new IOException(schemaField + " must have DocValues to use this feature.");
- }
- boolean multiValued = schemaField.multiValued();
FieldType fieldType = schemaField.getType();
- FieldWriter writer;
- if (fieldType instanceof SortableTextField && !schemaField.useDocValuesAsStored()) {
- if (solrReturnFields.getRequestedFieldNames() != null
- && solrReturnFields.getRequestedFieldNames().contains(field)) {
- // Explicitly requested field cannot be used due to not having useDocValuesAsStored=true,
- // throw exception
+ // Check if field can use DocValues
+ boolean canUseDocValues =
+ schemaField.hasDocValues()
+ && (!(fieldType instanceof SortableTextField) || schemaField.useDocValuesAsStored());
Review Comment:
do you want me to add a comment here? It's unrelated to my change but I can do it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-18071 Support Stored Fields in Export Writer [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on code in PR #4053:
URL: https://github.com/apache/solr/pull/4053#discussion_r2723043185
##########
solr/solr-ref-guide/modules/query-guide/pages/exporting-result-sets.adoc:
##########
@@ -23,6 +23,26 @@ This feature uses a stream sorting technique that begins to send records within
The cases where this functionality may be useful include: session analysis, distributed merge joins, time series roll-ups, aggregations on high cardinality fields, fully distributed field collapsing, and sort-based stats.
+== Comparison with Cursors
+
+The `/export` handler offers several advantages over xref:pagination-of-results.adoc#fetching-a-large-number-of-sorted-results-cursors[cursor-based pagination] for streaming large result sets.
+
+With cursors, the query is re-executed for each page of results.
+In contrast, `/export` runs the filter query once and the resulting segment-level bitmasks are applied once per segment, after which the documents are simply iterated over.
+Additionally, the segments that existed when the stream was opened are held open for the duration of the export, eliminating the disappearing or duplicate document issues that can occur with cursors.
+The trade-off is that IndexReaders are kept around for longer periods of time.
+
+Another advantage of `/export` is significantly lower latency until the first document is returned, because the internal batch size is decoupled from the response message size.
+With cursors, you typically need to set the `rows` parameter to a high value (e.g., 100,000) to achieve decent throughput.
+However, this creates a "glugging" effect: when you request a large batch, Solr must build the entire payload and send it over the wire while your client waits.
Review Comment:
I affirm the glugging but your rationale/guessing is certainly false. For SearchHandler, The payload is streamed/produced on the fly as it iterates documents. The code isn't there; it's elsewhere in a ResponseWriter, if I recall. Solr does have to do some up-front work -- producing a list of document IDs that match the search, and sorted as desired. This is the "QTime". Retrieving data to return it is after; it's not accumulated in memory; it's streamed, and lengthens the true elapsed time.
Wouldn't "export" have similar up-front costs to execute the query?
Any way, the broad strokes of your message look good.
##########
solr/solr-ref-guide/modules/query-guide/pages/exporting-result-sets.adoc:
##########
@@ -23,6 +23,26 @@ This feature uses a stream sorting technique that begins to send records within
The cases where this functionality may be useful include: session analysis, distributed merge joins, time series roll-ups, aggregations on high cardinality fields, fully distributed field collapsing, and sort-based stats.
+== Comparison with Cursors
+
+The `/export` handler offers several advantages over xref:pagination-of-results.adoc#fetching-a-large-number-of-sorted-results-cursors[cursor-based pagination] for streaming large result sets.
+
+With cursors, the query is re-executed for each page of results.
+In contrast, `/export` runs the filter query once and the resulting segment-level bitmasks are applied once per segment, after which the documents are simply iterated over.
+Additionally, the segments that existed when the stream was opened are held open for the duration of the export, eliminating the disappearing or duplicate document issues that can occur with cursors.
+The trade-off is that IndexReaders are kept around for longer periods of time.
+
+Another advantage of `/export` is significantly lower latency until the first document is returned, because the internal batch size is decoupled from the response message size.
+With cursors, you typically need to set the `rows` parameter to a high value (e.g., 100,000) to achieve decent throughput.
+However, this creates a "glugging" effect: when you request a large batch, Solr must build the entire payload and send it over the wire while your client waits.
+Only after receiving and decoding this large payload can the client request the next batch, but in the interim Solr sits idle on this request.
+With the `/export` handler, these steps are decoupled - Solr can continue sorting and decoding/encoding documents while waiting for more demand from the client.
+
+The advantage of cursors is flexibility.
+A cursor mark can be persisted and resumed later, even across restarts, whereas an `/export` stream is entirely in-memory and must be consumed in a single session.
Review Comment:
```suggestion
A `cursorMark` can be persisted and resumed later, even across restarts, or never continued if enough results were consumed to satisfy the use-case.
An `/export` stream must be consumed in a single session.
```
I'm tempted to say that a stream should be completely consumed but maybe /export can handle a client that doesn't want more data, gracefully? Do you know?
##########
solr/solr-ref-guide/modules/query-guide/pages/exporting-result-sets.adoc:
##########
@@ -23,6 +23,26 @@ This feature uses a stream sorting technique that begins to send records within
The cases where this functionality may be useful include: session analysis, distributed merge joins, time series roll-ups, aggregations on high cardinality fields, fully distributed field collapsing, and sort-based stats.
+== Comparison with Cursors
Review Comment:
BTW I very much appreciate the extra effort here!
Unless you are in the mood, don't go off an do performance experiments just because we're asking questions. Say what you're comfortable claiming and not more and that's fine :-)
##########
solr/solr-ref-guide/modules/query-guide/pages/exporting-result-sets.adoc:
##########
Review Comment:
I think we should at least cross-link between pagination-of-results.adoc with exporting-result-sets.adoc because they are obviously related. Their embeddings ought to be similar ;-)
##########
solr/solr-ref-guide/modules/query-guide/pages/exporting-result-sets.adoc:
##########
@@ -78,6 +98,10 @@ The `fl` property defines the fields that will be exported with the result set.
Any of the field types that can be sorted (i.e., int, long, float, double, string, date, boolean) can be used in the field list.
The fields can be single or multi-valued.
+By default, fields in the field list must have docValues enabled.
+However, when the `includeStoredFields` parameter is set to `true`, fields with only stored values (no docValues) can also be included.
+Note that sort fields still require docValues regardless of this setting.
Review Comment:
```suggestion
Note that sort fields still require docValues, regardless of this setting.
```
##########
solr/solr-ref-guide/modules/query-guide/pages/exporting-result-sets.adoc:
##########
@@ -49,7 +69,7 @@ The default value is `30000` but users may want to specify smaller values to lim
An optional parameter `includeStoredFields` (default `false`) enables exporting fields that only have stored values (no docValues).
When set to `true`, fields without docValues but with stored values can be included in the field list (`fl`).
Note that retrieving stored fields may significantly impact export performance compared to docValues fields, as stored fields require additional I/O operations.
-Fields that have both docValues and stored values will always use docValues for optimal performance, regardless of this parameter setting.
+If all requested fields are `docValues=true` then the data will be read only from docValues. This behavior applies to fields that are also `stored=true` and does not depend on the value of the `includeStoredFields` parameter.
Review Comment:
```suggestion
If all requested fields are `docValues=true` then the data will only be read from docValues.
This behavior applies to fields that are also `stored=true` and does not depend on the value of the `includeStoredFields` parameter.
```
##########
solr/solr-ref-guide/modules/query-guide/pages/exporting-result-sets.adoc:
##########
@@ -23,6 +23,26 @@ This feature uses a stream sorting technique that begins to send records within
The cases where this functionality may be useful include: session analysis, distributed merge joins, time series roll-ups, aggregations on high cardinality fields, fully distributed field collapsing, and sort-based stats.
+== Comparison with Cursors
+
+The `/export` handler offers several advantages over xref:pagination-of-results.adoc#fetching-a-large-number-of-sorted-results-cursors[cursor-based pagination] for streaming large result sets.
+
+With cursors, the query is re-executed for each page of results.
+In contrast, `/export` runs the filter query once and the resulting segment-level bitmasks are applied once per segment, after which the documents are simply iterated over.
+Additionally, the segments that existed when the stream was opened are held open for the duration of the export, eliminating the disappearing or duplicate document issues that can occur with cursors.
+The trade-off is that IndexReaders are kept around for longer periods of time.
+
+Another advantage of `/export` is significantly lower latency until the first document is returned, because the internal batch size is decoupled from the response message size.
+With cursors, you typically need to set the `rows` parameter to a high value (e.g., 100,000) to achieve decent throughput.
+However, this creates a "glugging" effect: when you request a large batch, Solr must build the entire payload and send it over the wire while your client waits.
+Only after receiving and decoding this large payload can the client request the next batch, but in the interim Solr sits idle on this request.
+With the `/export` handler, these steps are decoupled - Solr can continue sorting and decoding/encoding documents while waiting for more demand from the client.
+
+The advantage of cursors is flexibility.
Review Comment:
```suggestion
The advantage of cursors is _flexibility_.
Cursors impose no constraints on the sort criteria accept that you must include a unique key, which isn't a real constraint.
Cursors work as part of `SearchHandler` and thus can include most/all capabilities of it like highlighting.
```
##########
solr/solr-ref-guide/modules/query-guide/pages/exporting-result-sets.adoc:
##########
@@ -23,6 +23,24 @@ This feature uses a stream sorting technique that begins to send records within
The cases where this functionality may be useful include: session analysis, distributed merge joins, time series roll-ups, aggregations on high cardinality fields, fully distributed field collapsing, and sort-based stats.
+== Comparison with Cursors
+
+The `/export` handler offers several advantages over xref:pagination-of-results.adoc#fetching-a-large-number-of-sorted-results-cursors[cursor-based pagination] for streaming large result sets.
+
+With cursors, the query is re-executed for each page of results.
+In contrast, `/export` runs the filter query once and the resulting segment-level bitmasks are applied once per segment, after which the documents are simply iterated over.
+Additionally, the segments that existed when the stream was opened are held open for the duration of the export, eliminating the disappearing or duplicate document issues that can occur with cursors.
+The trade-off is that IndexReaders are kept around for longer periods of time.
Review Comment:
I feel we're potentially suggesting the contributor here put more work into this than he bargained for. Any documentation he's comfortable writing is encouraged... and beyond that, well let's just get this merged and have real users kick the tires and we'll see.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-18071 Support Stored Fields in Export Writer [solr]
Posted by "kotman12 (via GitHub)" <gi...@apache.org>.
kotman12 commented on code in PR #4053:
URL: https://github.com/apache/solr/pull/4053#discussion_r2723437715
##########
solr/solr-ref-guide/modules/query-guide/pages/exporting-result-sets.adoc:
##########
@@ -23,6 +23,26 @@ This feature uses a stream sorting technique that begins to send records within
The cases where this functionality may be useful include: session analysis, distributed merge joins, time series roll-ups, aggregations on high cardinality fields, fully distributed field collapsing, and sort-based stats.
+== Comparison with Cursors
+
+The `/export` handler offers several advantages over xref:pagination-of-results.adoc#fetching-a-large-number-of-sorted-results-cursors[cursor-based pagination] for streaming large result sets.
+
+With cursors, the query is re-executed for each page of results.
+In contrast, `/export` runs the filter query once and the resulting segment-level bitmasks are applied once per segment, after which the documents are simply iterated over.
+Additionally, the segments that existed when the stream was opened are held open for the duration of the export, eliminating the disappearing or duplicate document issues that can occur with cursors.
+The trade-off is that IndexReaders are kept around for longer periods of time.
+
+Another advantage of `/export` is significantly lower latency until the first document is returned, because the internal batch size is decoupled from the response message size.
+With cursors, you typically need to set the `rows` parameter to a high value (e.g., 100,000) to achieve decent throughput.
+However, this creates a "glugging" effect: when you request a large batch, Solr must build the entire payload and send it over the wire while your client waits.
+Only after receiving and decoding this large payload can the client request the next batch, but in the interim Solr sits idle on this request.
+With the `/export` handler, these steps are decoupled - Solr can continue sorting and decoding/encoding documents while waiting for more demand from the client.
+
+The advantage of cursors is flexibility.
+A cursor mark can be persisted and resumed later, even across restarts, whereas an `/export` stream is entirely in-memory and must be consumed in a single session.
Review Comment:
You can `close` a `TupleStream`. IIRC this results in an exception on the server side indicating the client closed the stream.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-18071 Support Stored Fields in Export Writer [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on code in PR #4053:
URL: https://github.com/apache/solr/pull/4053#discussion_r2722994417
##########
solr/core/src/java/org/apache/solr/handler/export/ExportWriter.java:
##########
@@ -484,43 +488,75 @@ void writeDoc(
LeafReaderContext context = leaves.get(ord);
int fieldIndex = 0;
for (FieldWriter fieldWriter : writers) {
- if (fieldWriter.write(sortDoc, context, ew, fieldIndex)) {
- ++fieldIndex;
- }
+ fieldIndex += fieldWriter.write(sortDoc, context, ew, fieldIndex);
}
}
public List<FieldWriter> getFieldWriters(String[] fields, SolrQueryRequest req)
throws IOException {
DocValuesIteratorCache dvIterCache = new DocValuesIteratorCache(req.getSearcher(), false);
-
SolrReturnFields solrReturnFields = new SolrReturnFields(fields, req);
+ boolean includeStoredFields = req.getParams().getBool(INCLUDE_STORED_FIELDS_PARAM, false);
List<FieldWriter> writers = new ArrayList<>();
+ Set<SchemaField> docValueFields = new LinkedHashSet<>();
+ Map<String, SchemaField> storedFields = new LinkedHashMap<>();
+
for (String field : req.getSearcher().getFieldNames()) {
if (!solrReturnFields.wantsField(field)) {
continue;
}
SchemaField schemaField = req.getSchema().getField(field);
- if (!schemaField.hasDocValues()) {
- throw new IOException(schemaField + " must have DocValues to use this feature.");
- }
- boolean multiValued = schemaField.multiValued();
FieldType fieldType = schemaField.getType();
- FieldWriter writer;
- if (fieldType instanceof SortableTextField && !schemaField.useDocValuesAsStored()) {
- if (solrReturnFields.getRequestedFieldNames() != null
- && solrReturnFields.getRequestedFieldNames().contains(field)) {
- // Explicitly requested field cannot be used due to not having useDocValuesAsStored=true,
- // throw exception
+ // Check if field can use DocValues
+ boolean canUseDocValues =
+ schemaField.hasDocValues()
+ && (!(fieldType instanceof SortableTextField) || schemaField.useDocValuesAsStored());
Review Comment:
Could the selection logic here, even if somewhat trivial, be extracted so it's also used in multiple places in ExportWriter? That would help keep them in sync and also show via find-usages that the logic is used from multiple use-cases.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-18071 Support Stored Fields in Export Writer [solr]
Posted by "kotman12 (via GitHub)" <gi...@apache.org>.
kotman12 commented on code in PR #4053:
URL: https://github.com/apache/solr/pull/4053#discussion_r2723454032
##########
solr/solr-ref-guide/modules/query-guide/pages/exporting-result-sets.adoc:
##########
@@ -23,6 +23,24 @@ This feature uses a stream sorting technique that begins to send records within
The cases where this functionality may be useful include: session analysis, distributed merge joins, time series roll-ups, aggregations on high cardinality fields, fully distributed field collapsing, and sort-based stats.
+== Comparison with Cursors
+
+The `/export` handler offers several advantages over xref:pagination-of-results.adoc#fetching-a-large-number-of-sorted-results-cursors[cursor-based pagination] for streaming large result sets.
+
+With cursors, the query is re-executed for each page of results.
+In contrast, `/export` runs the filter query once and the resulting segment-level bitmasks are applied once per segment, after which the documents are simply iterated over.
+Additionally, the segments that existed when the stream was opened are held open for the duration of the export, eliminating the disappearing or duplicate document issues that can occur with cursors.
+The trade-off is that IndexReaders are kept around for longer periods of time.
Review Comment:
> Is it dangerous to export a very large stored field? What is the risk? How big is big?
I didn't test the limits of this. I assume if your field could be ingested then it could also be exported although that may be a naive assumption. I imagine very large fields, i.e. >100MB would be problematic in a variety of circumstances not just for the ExportHandler.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-18071 Support Stored Fields in Export Writer [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on code in PR #4053:
URL: https://github.com/apache/solr/pull/4053#discussion_r2703013880
##########
solr/core/src/java/org/apache/solr/handler/export/StoredFieldsWriter.java:
##########
@@ -0,0 +1,148 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.solr.handler.export;
+
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.Date;
+import java.util.List;
+import java.util.Map;
+import java.util.WeakHashMap;
+import org.apache.lucene.index.FieldInfo;
+import org.apache.lucene.index.IndexReader;
+import org.apache.lucene.index.LeafReader;
+import org.apache.lucene.index.LeafReaderContext;
+import org.apache.lucene.index.StoredFieldVisitor;
+import org.apache.lucene.index.StoredFields;
+import org.apache.solr.common.MapWriter.EntryWriter;
+import org.apache.solr.schema.BoolField;
+import org.apache.solr.schema.DateValueFieldType;
+import org.apache.solr.schema.SchemaField;
+
+class StoredFieldsWriter extends FieldWriter {
+
+ private final Map<String, SchemaField> fields;
+ private final ThreadLocal<WeakHashMap<IndexReader.CacheKey, StoredFields>> storedFieldsMap =
+ new ThreadLocal<>();
+
+ public StoredFieldsWriter(Map<String, SchemaField> fieldsToRead) {
+ this.fields = fieldsToRead;
+ }
+
+ @Override
+ public int write(
+ SortDoc sortDoc, LeafReaderContext readerContext, EntryWriter out, int fieldIndex)
+ throws IOException {
+ WeakHashMap<IndexReader.CacheKey, StoredFields> map = storedFieldsMap.get();
+ if (map == null) {
+ map = new WeakHashMap<>();
+ storedFieldsMap.set(map);
+ }
+ LeafReader reader = readerContext.reader();
+ StoredFields storedFields = map.get(reader.getReaderCacheHelper().getKey());
+ if (storedFields == null) {
+ storedFields = reader.storedFields();
+ map.put(reader.getReaderCacheHelper().getKey(), storedFields);
+ }
+ ExportVisitor visitor = new ExportVisitor(out);
+ storedFields.document(sortDoc.docId, visitor);
+ return visitor.flush();
+ }
+
+ class ExportVisitor extends StoredFieldVisitor {
+
+ final EntryWriter out;
+ String lastFieldName;
+ List<Object> multiValue = null;
+ int fieldsVisited;
+
+ public ExportVisitor(EntryWriter out) {
+ this.out = out;
+ }
+
+ @Override
+ public void stringField(FieldInfo fieldInfo, String value) throws IOException {
+ var schemaField = fields.get(fieldInfo.name);
+ var fieldType = schemaField == null ? null : schemaField.getType();
+ if (fieldType instanceof BoolField) {
+ // Convert "T"/"F" stored value to boolean true/false
+ addField(fieldInfo.name, Boolean.valueOf(fieldType.indexedToReadable(value)));
+ } else {
+ addField(fieldInfo.name, value);
+ }
+ }
+
+ @Override
+ public void intField(FieldInfo fieldInfo, int value) throws IOException {
+ addField(fieldInfo.name, value);
+ }
+
+ @Override
+ public void longField(FieldInfo fieldInfo, long value) throws IOException {
+ var schemaField = fields.get(fieldInfo.name);
+ var fieldType = schemaField == null ? null : schemaField.getType();
+ if (fieldType instanceof DateValueFieldType) {
+ Date date = new Date(value);
+ addField(fieldInfo.name, date);
+ } else {
+ addField(fieldInfo.name, value);
+ }
+ }
+
+ @Override
+ public void floatField(FieldInfo fieldInfo, float value) throws IOException {
+ addField(fieldInfo.name, value);
+ }
+
+ @Override
+ public void doubleField(FieldInfo fieldInfo, double value) throws IOException {
+ addField(fieldInfo.name, value);
+ }
+
+ @Override
+ public Status needsField(FieldInfo fieldInfo) {
+ return fields.containsKey(fieldInfo.name) ? Status.YES : Status.NO;
+ }
+
+ private <T> void addField(String fieldName, T value) throws IOException {
Review Comment:
I have; I'm saying nonetheless I had some trouble reading it how you coded it. It's subjective; maybe "i'm the problem". In particular, I was surprised to see the very first bit of logic was to check if the current field is multi-valued... instead I expected it to check if we're adding to an existing field or starting a new field. There's no right answer here... it's subjective as to who finds what easier to read. So updated the logic and added a bit more comments. LMK what you think. If you don't like it than I'll refer it back and maybe you add more comments.
(I don't like the alternative suggestion of incorporating use of `Document`; seems quite unnecessary).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-18071 Support Stored Fields in Export Writer [solr]
Posted by "kotman12 (via GitHub)" <gi...@apache.org>.
kotman12 commented on code in PR #4053:
URL: https://github.com/apache/solr/pull/4053#discussion_r2699812724
##########
solr/core/src/java/org/apache/solr/handler/export/DoubleFieldWriter.java:
##########
@@ -34,7 +34,7 @@ public DoubleFieldWriter(
}
@Override
- public boolean write(
+ public int write(
Review Comment:
I cover this a little in the second paragraph of `#Solution`. To elaborate, I introduced `StoredFieldsWriter` which is a special type of `FieldWriter` that writes _many_ fields. I did this because it was the easiest way I saw to hook into the output of a `StoredFieldVisitor `, which writes multiple fields. Up until this point `FieldWriter` would only write one field at a time. It would return a boolean representing whether it successfully wrote the field. If the boolean was true an internal loop would increment the `fieldIndex` which then ... did absolutely nothing. So not entirely clear what the purpose is of keeping track of how many fields were written. But in order to maintain this potentially useful (although **not** currently useful) feature, I decided to expand the API to return an int which returns how many fields a particular `FieldWriter` wrote so we can increment by an arbitrary amount to support the new `StoredFieldsWriter `. I would be equally happy to just make `Fiel
dWriter::write` be void instead of tracking this unused index.
I also realize you may have missed `StoredFieldsWriter` because I put the class **inside** the `ExportWriter`. I think I am going to put it in a standalone file like the other `FieldWriter`s so it is more consistent and clear.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org