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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
