dsmiley commented on code in PR #4748:
URL: https://github.com/apache/solr/pull/4748#discussion_r3801090014
##########
solr/core/src/java/org/apache/solr/update/processor/NestedUpdateProcessorFactory.java:
##########
@@ -78,87 +79,128 @@ private static class NestedUpdateProcessor extends
UpdateRequestProcessor {
this.storePath = storePath;
this.uniqueKeyFieldName = req.getSchema().getUniqueKeyField().getName();
this.schema = req.getSchema();
+ this.hasMultiValuedVectorField = hasMultiValuedVectorField(schema);
+ }
+
+ /** Whether any field, explicit or dynamic, could yield vectors to split
into nested docs. */
+ private static boolean hasMultiValuedVectorField(IndexSchema schema) {
+ for (SchemaField field : schema.getFields().values()) {
+ if (isMultiValuedVectorField(field)) {
+ return true;
+ }
+ }
+ for (IndexSchema.DynamicField dynamicField : schema.getDynamicFields()) {
+ if (isMultiValuedVectorField(dynamicField.getPrototype())) {
+ return true;
+ }
+ }
+ return false;
+ }
+
+ private static boolean isMultiValuedVectorField(SchemaField sfield) {
+ return sfield.getType() instanceof DenseVectorField &&
sfield.multiValued();
}
@Override
public void processAdd(AddUpdateCommand cmd) throws IOException {
SolrInputDocument doc = cmd.getSolrInputDocument();
- processDocChildren(doc, null);
+ final String rootPath = rootPathPrefix(doc);
+ processDocChildren(doc, rootPath);
+ if (hasMultiValuedVectorField) {
+ // after the children; the docs it generates must not be walked as
children themselves
+ processMultiValuedVectorFields(doc, rootPath);
+ }
Review Comment:
@alessandrobenedetti , I see you added multi-valued vector field detection
to this class. You added logic to processDocChildren so that only at the root
level, it did it's work. I found this made processDocChildren much longer and
somewhat more complex than if it was a separate pass scoped to this. So I did
that here and I'd like your opinion. My only concern is that we lookup the
SchemaField twice per root doc field instead of once. Probably a minor
concern but still. I added a boolean hasMultiValuedVectorField to this URP to
pre-compute wether it's impossible or not.
I suppose it'd be handy if SolrInputField was expanded to also include the
SchemaField so that _any_ processing relative to the schema by any URP + final
Document building does a lookup just once.
--
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]
