SOLR-16183: XML Loader: support indexing single nested child document #1448

vinayakphegde · 2023-03-10T10:33:42Z

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

Description

The XML Loader always treats labelled nested children as a "multi-valued pseudo-field", whereas the JSON Loader can also index a single child document as well as an array of 1..n child documents.

The XML Loader should have a corresponding syntax for a single labelled nested child.

Solution

support a name attribute on a not-in-a- nested to achieve single labelled nested child.

Tests

created a test called testXMLSingleLabeledNestedChild() in AddBlockUpdateTest to test the single labelled nested child.

Checklist

Please review the following and check all that apply:

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

mkhludnev

Overall, I really like it!

mkhludnev · 2023-03-13T08:37:48Z

solr/core/src/java/org/apache/solr/handler/loader/XMLLoader.java

@@ -332,6 +333,11 @@ public SolrInputDocument readDoc(XMLStreamReader parser) throws XMLStreamExcepti
        } else {
          log.debug(message);
        }
+      } else if (NAME.equals(attrName)) {


It also allows to specify this attribute for <doc it other context, but ignore it then. I can't say it's a blocker; it slightly changes behavior, and may create wrong expectation for users.. In doubts.. .

Yeah, you are right
we can pass an extra argument called isSingleLabeledChildDoc to the readDoc function
if isSingleLabeledChildDoc is false and still we have the name attribute, then we can log the warning like we are doing now.
what do you think?

mkhludnev · 2023-03-13T08:41:06Z

solr/core/src/java/org/apache/solr/handler/loader/XMLLoader.java

+              if (NAME.equals(attrName)) {
+                isSingleLabeledChildDoc = true;
+                doc.addField(attrVal, readDoc(parser));
+                break;


another unexpected attribute may hide after the valid name, or perhaps (??) it allows to repeat name

but we are not concerned about other attributes, right? before this, we were skipping all the attributes
does it make sense to repeat the name attribute? is there any use case for that?

good point. You are right.

mkhludnev · 2023-03-13T08:50:30Z

solr/core/src/java/org/apache/solr/handler/loader/XMLLoader.java

+              }
+            }
+            if (isSingleLabeledChildDoc) {
+              isSingleLabeledChildDoc = false;


can't we reduce the scope for this var declaring it at line 426 ?

true will do that.

noblepaul · 2023-03-13T10:57:31Z

just out of curiosity, do you really use XML for indexing docs?

vinayakphegde · 2023-03-13T11:12:30Z

just out of curiosity, do you really use XML for indexing docs?

I never used it. not sure about other

vinayakphegde · 2023-03-13T14:17:10Z

can I make it a normal PR now?

mkhludnev · 2023-03-13T14:37:12Z

can I make it a normal PR now?

Why not? Looks good to me

vinayakphegde · 2023-03-13T15:24:48Z

Hey, @mkhludnev Solr Tests check is failing here. How do I re-trigger that? because all the tests were passed locally.
and do I have to add something to CHANGES.txt?

mkhludnev · 2023-03-14T09:28:51Z

solr/core/src/java/org/apache/solr/handler/loader/XMLLoader.java

              // flag to prevent spaces after doc from being added
              isLabeledChildDoc = true;
-              if (!doc.containsKey(name)) {
-                doc.setField(name, Lists.newArrayList());


this is the slight loose in logic, but I think it's ok.

mkhludnev · 2023-03-14T09:39:59Z

solr/core/src/java/org/apache/solr/handler/loader/XMLLoader.java

      }
    }

    StringBuilder text = new StringBuilder();
-    String name = null;
+    String currentFieldName = null;


Couldn't resist from make it more descriptive.

mkhludnev · 2023-03-14T09:51:47Z

solr/CHANGES.txt

@@ -34,6 +34,20 @@ Other Changes
  Previously, the modules would come transitively.
  (David Smiley)

+==================  9.3.0 ==================


Ain't sure. Why so?

Hey @mkhludnev I don't understand the question here
can you elaborate a bit?

I expected === 9.3.0 === header occurrence in main. @vinayakphegde it should be fine.

epugh · 2023-03-14T09:52:23Z

I love that we're getting these small differences sorted out!

mkhludnev · 2023-03-14T10:16:18Z

solr/core/src/java/org/apache/solr/handler/loader/XMLLoader.java

              // flag to prevent spaces after doc from being added
              isLabeledChildDoc = true;
-              if (!doc.containsKey(name)) {
-                doc.setField(name, Lists.newArrayList());


It's caused too much hard, turn back to wrapping.

Do you want me to revert the changes that you added now?

Hold on. I'm on it.

mkhludnev · 2023-03-14T11:01:20Z

Merged. Thanks @vinayakphegde .
Would you mind to create cherry-pick PR into branch_9x?

vinayakphegde · 2023-03-14T11:05:10Z

Merged. Thanks @vinayakphegde . Would you mind to create cherry-pick PR into branch_9x?

sure

…apache#1448) * support indexing single nested child document --------- Co-authored-by: Mikhail Khludnev <mkhl@apache.org>

…#1448) (#1462) * support indexing single nested child document --------- Co-authored-by: vinayak hegde <vinayakph123@gmail.com>

mkhludnev reviewed Mar 13, 2023

View reviewed changes

vinayakphegde marked this pull request as ready for review March 13, 2023 14:46

mkhludnev reviewed Mar 14, 2023

View reviewed changes

vinayakphegde and others added 7 commits March 14, 2023 12:47

support indexing single nested child document

0a0ba82

add testcase of indexing single labelled nested child

7498914

added isSingleLabeledChildDocProcessing parameter to readDoc function

c686876

SOLR-16183: reduce change, simplify the code

f57fd0d

SOLR-16183: revert tiny change

6829fbc

SOLR-16183: rebase changes

f57f138

SOLR-16183: cut unintended usage.

a167a9f

mkhludnev force-pushed the SOLR_16183 branch from 99a4c89 to a167a9f Compare March 14, 2023 09:50

mkhludnev reviewed Mar 14, 2023

View reviewed changes

mkhludnev added 2 commits March 14, 2023 13:35

SOLR-16183: rollback slight logic change

bc43fde

SOLR-16183: remove reverted change

aec81f5

mkhludnev merged commit 6bd9e2a into apache:main Mar 14, 2023
1 of 2 checks passed

mkhludnev added a commit that referenced this pull request Mar 15, 2023

SOLR-16183: XML Loader: support indexing single nested child document (…

f23e784

…#1448) (#1462) * support indexing single nested child document --------- Co-authored-by: vinayak hegde <vinayakph123@gmail.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SOLR-16183: XML Loader: support indexing single nested child document #1448

SOLR-16183: XML Loader: support indexing single nested child document #1448

vinayakphegde commented Mar 10, 2023

mkhludnev left a comment

mkhludnev Mar 13, 2023 •

edited

vinayakphegde Mar 13, 2023

mkhludnev Mar 13, 2023

mkhludnev Mar 13, 2023

vinayakphegde Mar 13, 2023

mkhludnev Mar 13, 2023

mkhludnev Mar 13, 2023

vinayakphegde Mar 13, 2023

noblepaul commented Mar 13, 2023

vinayakphegde commented Mar 13, 2023

vinayakphegde commented Mar 13, 2023

mkhludnev commented Mar 13, 2023

vinayakphegde commented Mar 13, 2023

mkhludnev Mar 14, 2023

mkhludnev Mar 14, 2023

vinayakphegde Mar 14, 2023

mkhludnev Mar 14, 2023

vinayakphegde Mar 14, 2023

mkhludnev Mar 14, 2023

epugh commented Mar 14, 2023

mkhludnev Mar 14, 2023

vinayakphegde Mar 14, 2023

mkhludnev Mar 14, 2023

mkhludnev commented Mar 14, 2023

vinayakphegde commented Mar 14, 2023

SOLR-16183: XML Loader: support indexing single nested child document #1448

SOLR-16183: XML Loader: support indexing single nested child document #1448

Conversation

vinayakphegde commented Mar 10, 2023

Description

Solution

Tests

Checklist

mkhludnev left a comment

Choose a reason for hiding this comment

mkhludnev Mar 13, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

noblepaul commented Mar 13, 2023

vinayakphegde commented Mar 13, 2023

vinayakphegde commented Mar 13, 2023

mkhludnev commented Mar 13, 2023

vinayakphegde commented Mar 13, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

epugh commented Mar 14, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkhludnev commented Mar 14, 2023

vinayakphegde commented Mar 14, 2023

mkhludnev Mar 13, 2023 •

edited