Updated patch with more tests.

Anshum Gupta can you please review this.

Thanks for the patch Varun. What makes you change the following code to be executed for every term, accidental?

String termStatsString = StatsUtil.termStatsMapToString(statsMap);
rb.rsp.add(TERM_STATS_KEY, termStatsString);

Updated patch:

What makes you change the following code to be executed for every term, accidental?

Reverted this change.

In ExactStatsCache#mergeToGlobalStats we insert the terms n times ( n number of shards ). Now when a shard doesn't have a term List<Object> terms = nl.getAll(TERMS_KEY) will return empty and not null since NamedList#getAll works like that. Thus depending on the order in which the shards get hit, if the last shard had no terms the terms key was overwritten with blank causing distributed idf to not get calculated correctly. Fixed this in the patch by changing the condition appropriately.

Minor refactoring

All tests pass now.

New patch where the test picks either LRUStatsCache or ExactStatsCache randomly.

The patch also fixes the NPE found in LRUStatsCache

Thanks Varun. Looks good. Here are a few suggestions:

• You should reset solr.test.sys.* system properties during teardown.
• It'd be good to refactor the test a little bit (nothing pressing)
• Do we really need 3 shards in the test? I think we can do with just 2 and save time for the test run.

There's also a bunch of formatting changes that's a part of the patch. I just glanced through it, but in case it's something that's not required, it'd be good to not refactor those.
P.S: If the current formatting is screwed up, by all means clean it up.

You should reset solr.test.sys.* system properties during teardown.

Fixed

Do we really need 3 shards in the test? I think we can do with just 2 and save time for the test run.

Only testSimpleQuery uses 3 shards in which one is empty. The idea of having an extra shard is that shard will not contain terms and expose bugs like SOLR-7818. We could do with 2 shards with if we indexed something like "football world" instead of "football" in one of the documents. But then we'll have no way to compare scores of the two returned docuemnts and we can't do an equality check on the score. Maybe we could once SOLR-7759 is fixed thereby checking debug output and not comparing scores.

There's also a bunch of formatting changes that's a part of the patch. I just glanced through it, but in case it's something that's not required, it'd be good to not refactor those.

Some of this got formatted with the code style that was added for Intellij which got fixed in LUCENE-6514.

In ExactStatsCache#mergeToGlobalStats we insert the terms n times ( n number of shards ). Now when a shard doesn't have a term List<Object> terms = nl.getAll(TERMS_KEY) will return empty and not null since NamedList#getAll works like that. Thus depending on the order in which the shards get hit, if the last shard had no terms the terms key was overwritten with blank causing distributed idf to not get calculated correctly.

Let's fix this as part of SOLR-7818.

Thanks for addressing my comments. LGTM now.

One change in this patch. TestDistribIDF#testMultiCollectionQuery is not added here. I'll add that as part of SOLR-7818 since we're tacking them separately.

I'm running the tests and will commit it after that.

Commit 1694182 from Varun Thacker in branch 'dev/trunk'
[ https://svn.apache.org/r1694182 ]

SOLR-7756: ExactStatsCache and LRUStatsCache will throw an NPE when a term is not present on a shard

Commit 1694193 from Varun Thacker in branch 'dev/branches/branch_5x'
[ https://svn.apache.org/r1694193 ]

SOLR-7756: ExactStatsCache and LRUStatsCache will throw an NPE when a term is not present on a shard (merged trunk r1694182)

Thanks Anshum for the review.

Commit 1694213 from Varun Thacker in branch 'dev/trunk'
[ https://svn.apache.org/r1694213 ]

SOLR-7818 SOLR-7756 Added better descriptions in the CHANGES entry for these two issues

Commit 1694214 from Varun Thacker in branch 'dev/branches/branch_5x'
[ https://svn.apache.org/r1694214 ]

SOLR-7818 SOLR-7756 Added better descriptions in the CHANGES entry for these two issues (merged from trunk r1694213)

Bulk close for 5.3.0 release

SOLR-9328 has a reproducible test failure for TestDistribIDF.testMultiCollectionQuery(), which fails on the 5.3.0 release's source, which is when the test was introduced by this issue.