MoreLikeThis incorrectly does toString on Field object [LUCENE-7354]

[asfimport]

In MoreLikeThis.java, circa line 763, when calling addTermFrequencies on a Field object, we are incorrectly calling toString on the Field object, which puts the Field attributes (indexed, stored, et. al) into the String that is returned.

I'll put up a patch/fix shortly.

---

Migrated from LUCENE-7354 by Grant Ingersoll (@gsingers), updated Jul 01 2016
Attachments: LUCENE-7354-mlt-fix

[asfimport]

Grant Ingersoll (@gsingers) (migrated from JIRA)

In looking at this code, it seems a bit more broken than I first thought. AFAICT, in retrieveTerms (circa line 760) we loop over the MLT configured fields, then we loop over the filteredDocument entries, but we don't actually check that the values in the filtered document are the ones configured on the MLT object.

[asfimport]

Grant Ingersoll (@gsingers) (migrated from JIRA)

This is also a Solr issue. I'm not a big fan of the API for the like call in this particular case:
codepublic Query like(Map<String, Collection<Object>> filteredDocument) throws IOException code is just asking for trouble, even if that saves a few operations in Solr.

My proposal:

1. Change that signature, which is only used by Solr in CloudMLTQParser, to take Map<String, Collection<String>>, thus forcing some type safety
2. Fix the retrieveTerms to check the fields line up.
3. Write some tests for the code path in question.

[asfimport]

Grant Ingersoll (@gsingers) (migrated from JIRA)

Mostly complete, but sharing early, as it's been a while since I've last patched Lucene/Solr. Not thrilled about the instanceof in CloudMLTQParser, but not much choice.

Still need to wait on the full test suite to pass and to remove no commits and TODOs.

[asfimport]

Steven Rowe (@sarowe) (migrated from JIRA)

In MoreLikeThis.java, circa line 763, when calling addTermFrequencies on a Field object, we are incorrectly calling toString on the Field object, which puts the Field attributes (indexed, stored, et. al) into the String that is returned.

I don't see this - when I run CloudMLTQParserTest without your patch, and I look at MoreLikeThis.retrieveTerms() where String.valueOf(fieldValue) is called (by pulling the value of that expression out into a variable and breaking there in the debugger), I only see the actual field values - no indexed stored et al.

Indexed, stored, et al. are Field*Type* attributes, not Field attributes, right?

In CloudMLTQParser.parse() where the filtered doc is composed, in your patch you have a nocommit (the only one I see in your patch) - Field.stringValue() returns value.toString(), but only if it's a String or a Number, and otherwise null, so it's definitely possible to not have a string value for binary fields or geo fields - I guess the question is whether people want to use non-text/non-scalar fields for MLT?:

    for (String field : fieldNames) {
      Collection<Object> fieldValues = doc.getFieldValues(field);
      if (fieldValues != null) {
        Collection<String> strings = new ArrayList<>(fieldValues.size());
        for (Object value : fieldValues) {
          if (value instanceof Field){
            String sv = ((Field) value).stringValue();
            if (sv != null) {
              strings.add(sv);
            }//TODO: nocommit: what to do when we don't have StringValue? I don't think it is possible in this case, but need to check on this
          } else {
            strings.add(value.toString());
          }
        }
        filteredDocument.put(field, strings);
      }
    }

[asfimport]

Grant Ingersoll (@gsingers) (migrated from JIRA)

I don't see this - when I run CloudMLTQParserTest without your patch, and I look at MoreLikeThis.retrieveTerms() where String.valueOf(fieldValue) is called (by pulling the value of that expression out into a variable and breaking there in the debugger), I only see the actual field values - no indexed stored et al.

I'll check again, @sarowe. I was doing the same thing you describe. Could be a version issue. I am on Solr 5.5.1.

[asfimport]

Grant Ingersoll (@gsingers) (migrated from JIRA)

I'm certain this is occurring on 5.5.1 at a minimum.

[asfimport]

Grant Ingersoll (@gsingers) (migrated from JIRA)

And also still seeing it on master, but the plot thickens:

1. When you debug via the test, it is as @sarowe says above
2. When you stand up solr (bin/solr) and issue an MLT query from curl (or the like), I see the Field objects

[asfimport]

Grant Ingersoll (@gsingers) (migrated from JIRA)

I think the culprit is in RealTimeGetComponent.java, circa lines 278:

if (ids ==  null && allIds.length <= 1) {
     // if the doc was not found, then use a value of null.
     rsp.add("doc", docList.size() > 0 ? docList.get(0) : null);
   } else {
     docList.setNumFound(docList.size());
     rsp.addResponse(docList);
   }

When debugging the test class (CloudMLTQParserTest), we end up in the else clause. When connecting to standalone via curl (e.g. http://localhost:8983/solr/mlt/select?indent=on&q={!mlt%20qf=resourcename}ID&wt=json)), we end up in the if clause.

Still debugging what is causing ids and allIds to be set in the former case and not in the latter, as the query parameter looks almost identical.

[asfimport]

Grant Ingersoll (@gsingers) (migrated from JIRA)

OK, more detail:

In my standalone server, I have a collection that has a single shard (I created it using bin/solr create_collection). The test has 2 shards, which then invokes RealTimeGetComponent.createSubRequests() which then adds an "ids" params, which is what then causes the the else clause cited above to get invoked.

@yonik can you provide some insight? (Most of the code was written by you)