LUCENE-9999: CombinedFieldQuery can fail with an exception when document is missing fields #185

jimczi · 2021-06-15T10:27:18Z

This change fixes a bug in MultiNormsLeafSimScorer that assumes that each
field should have a norm for every term/document.

As part of the fix, it adds validation that the fields have consistent norms
settings.

…ent is missing some fields.

jpountz

LGTM besides a small comment I left.

jpountz · 2021-06-15T12:52:34Z

lucene/sandbox/src/java/org/apache/lucene/sandbox/search/MultiNormsLeafSimScorer.java

-    if (norms != null) {
-      boolean found = norms.advanceExact(doc);
-      assert found;
+    if (norms != null && norms.advanceExact(doc)) {


I believe that the old logic was correct?

The only case I can think of when it might fail is when some fields are indexed with norms and other fields without norms, but I'm not sure we should even try to support this case with CombinedFieldQuery.

We allow a StringField and a TextField to be used at the moment. We could disallow that upfront (all fields or none have norms enabled) because the old logic would fail with weird exceptions without this change.

+1 from me to disallow it upfront, the combined score won't be meaningful if there is a mix of norms being disabled/ enabled.

jtibshirani

Thank you @jimczi for fixing this!

jtibshirani · 2021-06-18T02:52:31Z

lucene/sandbox/src/java/org/apache/lucene/sandbox/search/MultiNormsLeafSimScorer.java

-    if (norms != null) {
-      boolean found = norms.advanceExact(doc);
-      assert found;
+    if (norms != null && norms.advanceExact(doc)) {


+1 from me to disallow it upfront, the combined score won't be meaningful if there is a mix of norms being disabled/ enabled.

jtibshirani · 2021-06-18T03:03:47Z

lucene/sandbox/src/test/org/apache/lucene/sandbox/search/TestCombinedFieldQuery.java

@@ -85,7 +86,7 @@ public void testToString() {
    assertEquals("CombinedFieldQuery((foo title^3.0)(bar baz))", builder.build().toString());
  }

-  public void testSameScore() throws IOException {
+  public void testSparseFieldSameScore() throws IOException {


It's a little unclear to me what we aim to test with the 'same score' checks. The testCopyField test seems to best capture the core behavior -- we could have a version of testCopyField where one field is sparse?

…_query_norm

jtibshirani · 2021-06-23T21:52:04Z

@jimczi asked if I could finish the PR -- I pushed some changes:

Add a check that either all fields or no fields have norms enabled
Rework the testing strategy

Let me know if it still looks okay or if you have other comments.

jimczi

Thanks @jtibshirani!

jimczi · 2021-06-24T08:59:17Z

lucene/sandbox/src/java/org/apache/lucene/sandbox/search/MultiNormsLeafSimScorer.java

+        throw new IllegalArgumentException(
+            getClass().getSimpleName()
+                + " requires norms to be consistent across fields: some fields cannot"
+                + " have norms enabled, while others have norms disabled");


It's kind of an edge case but should we also throw the exception if scoring is not needed ?

Properly throwing an error would require a more invasive change, since we rewrite the query very early if scores aren't needed:

lucene/lucene/sandbox/src/java/org/apache/lucene/sandbox/search/CombinedFieldQuery.java

Lines 262 to 265 in 9942d59

} else {

// rewrite to a simple disjunction if the score is not needed.

Query bq = rewriteToBoolean();

return searcher.rewrite(bq).createWeight(searcher, ScoreMode.COMPLETE_NO_SCORES, boost);

However I could refactor this part in MultiNormsLeafSimScorer just to be clearer/ more solid?

This also made me notice -- I'm using the fact that LeafReader#getNormsValues is null to detect that norms were disabled. This seems accurate but I couldn't confirm it's really part of the method contract. I wonder if checking field infos is better.

Doing more research, I don't think it's possible to detect if norms are disabled upfront: LeafReader#getNormsValues could be null if a field uses norms but happens to be missing from a segment. Using field infos would have the same problem.

I opted to just document this requirement (as we do for the fact norms must be additive and encoded in a certain way). If the calling application has its own notion of schema, it can enforce the norms requirements upfront.

This commit moves a fix for a bug in MultiNormsLeafSimScorer in Lucene that fixes an error in the new CombinedFieldQuery for missing values. Its based on a PR for LUCENE-9999 (apache/lucene#185). This is a temporary copy of the affected query and its updated dependencies that should be removed again once we are able to use the original fix from Lucene. Relastes to apache/lucene#185

This commit moves a fix for a bug in MultiNormsLeafSimScorer in Lucene that fixes an error in the new CombinedFieldQuery for missing values. Its based on a PR for LUCENE-9999 (apache/lucene#185). This is a temporary copy of the affected query and its updated dependencies that should be removed again once we are able to use the original fix from Lucene. Relates to apache/lucene#185

This commit moves a fix for a bug in MultiNormsLeafSimScorer in Lucene that fixes an error in the new CombinedFieldQuery for missing values. Its based on a PR for LUCENE-9999 (apache/lucene#185). This is a temporary copy of the affected query and its updated dependencies that should be removed again once we are able to use the original fix from Lucene. Relastes to apache/lucene#185 Co-authored-by: Christoph Büscher <cbuescher@posteo.de>

…_query_norm

jtibshirani · 2021-07-14T18:07:47Z

@jpountz would you be able to take another look?

jpountz

Maybe something we could check could be verifying that all fields that have a non-null FieldInfo have the same value for hasNorms on a per-segment basis. It wouldn't cover 100% of misuse, but probably most of it.

jtibshirani · 2021-07-17T01:39:06Z

I tried out your idea. I opted to do it on a global basis in createWeight to ensure we always validate this, even when the query is rewritten.

I'm going to merge, but feel free to give post merge feedback if anything seems off.

…ent is missing fields (apache#185) This change fixes a bug in `MultiNormsLeafSimScorer` that assumes that each field should have a norm for every term/document. As part of the fix, it adds validation that the fields have consistent norms settings.

This commit moves a fix for a bug in MultiNormsLeafSimScorer in Lucene that fixes an error in the new CombinedFieldQuery for missing values. Its based on a PR for LUCENE-9999 (apache/lucene#185). This is a temporary copy of the affected query and its updated dependencies that should be removed again once we are able to use the original fix from Lucene. Relastes to apache/lucene#185 Co-authored-by: Christoph Büscher <cbuescher@posteo.de>

LUCENE-9999: CombinedFieldQuery can fail with an exception when docum…

618cb9f

…ent is missing some fields.

jimczi added the bug label Jun 15, 2021

jimczi changed the title ~~CombinedFieldQuery can fail with an exception when document is missing fields~~ LUCENE-9999: CombinedFieldQuery can fail with an exception when document is missing fields Jun 15, 2021

apply splotless

8a3be15

jpountz approved these changes Jun 15, 2021

View reviewed changes

jtibshirani reviewed Jun 18, 2021

View reviewed changes

jtibshirani added 5 commits June 22, 2021 17:50

Merge remote-tracking branch 'upstream/main' into bugs/combined_field…

9fa8071

…_query_norm

Update testing strategy

1ab9788

Validate that either all or no fields have norms enabled

9dc319a

Update changelog

c2b4320

Correct test case

441e28d

jimczi commented Jun 24, 2021

View reviewed changes

cbuescher mentioned this pull request Jun 29, 2021

Fix CombinedFieldQuery (Lucene 9999) elastic/elasticsearch#74678

Merged

cbuescher mentioned this pull request Jun 29, 2021

Fix CombinedFieldQuery (Lucene 9999) (#74678) elastic/elasticsearch#74703

Merged

jimczi mentioned this pull request Jun 30, 2021

Simple combined_fields query fails with java.lang.IndexOutOfBoundsException elastic/elasticsearch#74037

Closed

jtibshirani added 2 commits July 13, 2021 19:58

Merge remote-tracking branch 'upstream/main' into bugs/combined_field…

af2c818

…_query_norm

Don't try to enforce that fields have consistent norms

e933959

Remove extra spaces

9def30e

jpountz approved these changes Jul 16, 2021

View reviewed changes

jtibshirani added 2 commits July 16, 2021 17:59

Validate that either all or no fields have norms enabled, take 2

891c737

Fix test

ba12998

jtibshirani merged commit f333b70 into apache:main Jul 17, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

LUCENE-9999: CombinedFieldQuery can fail with an exception when document is missing fields #185

LUCENE-9999: CombinedFieldQuery can fail with an exception when document is missing fields #185

jimczi commented Jun 15, 2021 •

edited by jtibshirani

Loading

jpountz left a comment

jpountz Jun 15, 2021

jimczi Jun 15, 2021

jtibshirani Jun 18, 2021

jtibshirani left a comment

jtibshirani Jun 18, 2021

jtibshirani Jun 18, 2021

jtibshirani commented Jun 23, 2021

jimczi left a comment

jimczi Jun 24, 2021

jtibshirani Jun 24, 2021 •

edited

Loading

jtibshirani Jul 14, 2021 •

edited

Loading

jtibshirani commented Jul 14, 2021

jpountz left a comment

jtibshirani commented Jul 17, 2021

	} else {
	// rewrite to a simple disjunction if the score is not needed.
	Query bq = rewriteToBoolean();
	return searcher.rewrite(bq).createWeight(searcher, ScoreMode.COMPLETE_NO_SCORES, boost);

LUCENE-9999: CombinedFieldQuery can fail with an exception when document is missing fields #185

LUCENE-9999: CombinedFieldQuery can fail with an exception when document is missing fields #185

Conversation

jimczi commented Jun 15, 2021 • edited by jtibshirani Loading

jpountz left a comment

Choose a reason for hiding this comment

jpountz Jun 15, 2021

Choose a reason for hiding this comment

jimczi Jun 15, 2021

Choose a reason for hiding this comment

jtibshirani Jun 18, 2021

Choose a reason for hiding this comment

jtibshirani left a comment

Choose a reason for hiding this comment

jtibshirani Jun 18, 2021

Choose a reason for hiding this comment

jtibshirani Jun 18, 2021

Choose a reason for hiding this comment

jtibshirani commented Jun 23, 2021

jimczi left a comment

Choose a reason for hiding this comment

jimczi Jun 24, 2021

Choose a reason for hiding this comment

jtibshirani Jun 24, 2021 • edited Loading

Choose a reason for hiding this comment

jtibshirani Jul 14, 2021 • edited Loading

Choose a reason for hiding this comment

jtibshirani commented Jul 14, 2021

jpountz left a comment

Choose a reason for hiding this comment

jtibshirani commented Jul 17, 2021

jimczi commented Jun 15, 2021 •

edited by jtibshirani

Loading

jtibshirani Jun 24, 2021 •

edited

Loading

jtibshirani Jul 14, 2021 •

edited

Loading