[GitHub] [lucene-solr] thomaswoeckinger commented on a change in pull request #665: Fixes SOLR-13539

[GitBox <gi...@apache.org>]

thomaswoeckinger commented on a change in pull request #665: Fixes SOLR-13539
URL: https://github.com/apache/lucene-solr/pull/665#discussion_r324729013
 
 

 ##########
 File path: solr/core/src/java/org/apache/solr/handler/loader/XMLLoader.java
 ##########
 @@ -429,7 +434,18 @@ public SolrInputDocument readDoc(XMLStreamReader parser) throws XMLStreamExcepti
             break;
           } else if ("field".equals(parser.getLocalName())) {
             // should I warn in some text has been found too
-            Object v = isNull ? null : text.toString();
+            Object v;
 
 Review comment:
   > Ah, I think I see. Without this piece of the change, the new tests would fail because particular field types have binary data, and as-is we don't handle that correctly in SolrJ/EmbeddedSolrServer for wt=xml.
   > 
   Yes, of course!
   
   > But figuring out how Solr should handle binary field data when using `wt=xml` is probably worth its own jira, for a few reasons: (1) Other people in the community are likely to have opinions and want to chime in. (2) It also, to be honest about my limitations, strays into areas of the code I don't know as well. (3) It's independent conceptually from the problem we started out trying to solve here (adding some atomic update tests to cover what we currently support).
   > 
   Added new issue SOLR-13762 and PR #883 
   
   > I'd like to see it get fixed, and I can work with you if you open a JIRA specifically for the binary-xml stuff. I just don't think that fixing it here is the right approach. Can you remove the binary-xml related changes, and either comment out the added tests that will fail, or add a TODO to add them once the underlying XML issue is fixed. (If you file a separate JIRA and reference that in your TODO comment, others will have the context if they want it).
   
   I was thinking about the same, so it was not much work anyway, and it is better separated now
   
   

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org