Adding a null pointer check to fix index_prefix query #2879

VachaShah · 2022-04-12T21:01:06Z

Signed-off-by: Vacha Shah vachshah@amazon.com

Description

When the length of the query prefix is = minChars-1, the code follows the path in TextFieldMapper and throws an NPE here. Adding a null check for the method, the query returns a result without an NPE now:

curl -XPUT localhost:9200/test --data '{                                                                             
"mappings": {
    "properties": {
      "t": {
        "type": "text",
        "index_prefixes": { "min_chars": "3" }
      }
    }
  }
}' -H "Content-Type:Application/json"
{{"acknowledged":true,"shards_acknowledged":true,"index":"test"}

curl 'localhost:9200/test/_search?pretty' --data '{"query":{"prefix":{"t": "st"}}}' -H "Content-Type:Application/json"
{
  "took" : 9,
  "timed_out" : false,
  "_shards" : {
    "total" : 1,
    "successful" : 1,
    "skipped" : 0,
    "failed" : 0
  },
  "hits" : {
    "total" : {
      "value" : 0,
      "relation" : "eq"
    },
    "max_score" : null,
    "hits" : [ ]
  }
}

Tested with various scenarios mentioned in the issue. Adding the details in the comments of this PR.

Issues Resolved

#2826

Check List

New functionality includes testing.
- All tests pass
New functionality has been documented.
- New functionality has javadoc added
Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

VachaShah · 2022-04-12T21:03:25Z

For index test with minChars=3 for index_prefixes:

curl -XPUT localhost:9200/test --data '{                                                                             
"mappings": {
    "properties": {
      "t": {
        "type": "text",
        "index_prefixes": { "min_chars": "3" }
      }
    }
  }
}' -H "Content-Type:Application/json"
{{"acknowledged":true,"shards_acknowledged":true,"index":"test"}
ubuntu@ip-172-31-51-78:~$ curl 'localhost:9200/test/_search?pretty' --data '{"query":{"prefix":{"t": ""}}}' -H "Content-Type:Application/json"                                                             
{
  "took" : 38,
  "timed_out" : false,
  "_shards" : {
    "total" : 1,
    "successful" : 1,
    "skipped" : 0,
    "failed" : 0
  },
  "hits" : {
    "total" : {
      "value" : 0,
      "relation" : "eq"
    },
    "max_score" : null,
    "hits" : [ ]
  }
}
ubuntu@ip-172-31-51-78:~$ curl 'localhost:9200/test/_search?pretty' --data '{"query":{"prefix":{"t": "s"}}}' -H "Content-Type:Application/json"
{
  "took" : 5,
  "timed_out" : false,
  "_shards" : {
    "total" : 1,
    "successful" : 1,
    "skipped" : 0,
    "failed" : 0
  },
  "hits" : {
    "total" : {
      "value" : 0,
      "relation" : "eq"
    },
    "max_score" : null,
    "hits" : [ ]
  }
}
ubuntu@ip-172-31-51-78:~$ curl 'localhost:9200/test/_search?pretty' --data '{"query":{"prefix":{"t": "st"}}}' -H "Content-Type:Application/json"
{
  "took" : 9,
  "timed_out" : false,
  "_shards" : {
    "total" : 1,
    "successful" : 1,
    "skipped" : 0,
    "failed" : 0
  },
  "hits" : {
    "total" : {
      "value" : 0,
      "relation" : "eq"
    },
    "max_score" : null,
    "hits" : [ ]
  }
}
ubuntu@ip-172-31-51-78:~$ curl 'localhost:9200/test/_search?pretty' --data '{"query":{"prefix":{"t": "str"}}}' -H "Content-Type:Application/json"
{
  "took" : 4,
  "timed_out" : false,
  "_shards" : {
    "total" : 1,
    "successful" : 1,
    "skipped" : 0,
    "failed" : 0
  },
  "hits" : {
    "total" : {
      "value" : 0,
      "relation" : "eq"
    },
    "max_score" : null,
    "hits" : [ ]
  }
}

For index test-2 with minChars=2 for index_prefixes:

curl -XPUT localhost:9200/test-2 --data '{                                                                             
"mappings": {
    "properties": {
      "t": {
        "type": "text",
        "index_prefixes": { "min_chars": "2" }
      }
    }
  }
}' -H "Content-Type:Application/json"
{"acknowledged":true,"shards_acknowledged":true,"index":"test-2"}ubuntu@ip-172-31-51-78:~$ 
ubuntu@ip-172-31-51-78:~$ curl 'localhost:9200/test-2/_search?pretty' --data '{"query":{"prefix":{"t": ""}}}' -H "Content-Type:Application/json"
{
  "took" : 36,
  "timed_out" : false,
  "_shards" : {
    "total" : 1,
    "successful" : 1,
    "skipped" : 0,
    "failed" : 0
  },
  "hits" : {
    "total" : {
      "value" : 0,
      "relation" : "eq"
    },
    "max_score" : null,
    "hits" : [ ]
  }
}
ubuntu@ip-172-31-51-78:~$ curl 'localhost:9200/test-2/_search?pretty' --data '{"query":{"prefix":{"t": "s"}}}' -H "Content-Type:Application/json"
{
  "took" : 10,
  "timed_out" : false,
  "_shards" : {
    "total" : 1,
    "successful" : 1,
    "skipped" : 0,
    "failed" : 0
  },
  "hits" : {
    "total" : {
      "value" : 0,
      "relation" : "eq"
    },
    "max_score" : null,
    "hits" : [ ]
  }
}
ubuntu@ip-172-31-51-78:~$ curl 'localhost:9200/test-2/_search?pretty' --data '{"query":{"prefix":{"t": "st"}}}' -H "Content-Type:Application/json"
{
  "took" : 4,
  "timed_out" : false,
  "_shards" : {
    "total" : 1,
    "successful" : 1,
    "skipped" : 0,
    "failed" : 0
  },
  "hits" : {
    "total" : {
      "value" : 0,
      "relation" : "eq"
    },
    "max_score" : null,
    "hits" : [ ]
  }
}
ubuntu@ip-172-31-51-78:~$ curl 'localhost:9200/test-2/_search?pretty' --data '{"query":{"prefix":{"t": "str"}}}' -H "Content-Type:Application/json"
{
  "took" : 3,
  "timed_out" : false,
  "_shards" : {
    "total" : 1,
    "successful" : 1,
    "skipped" : 0,
    "failed" : 0
  },
  "hits" : {
    "total" : {
      "value" : 0,
      "relation" : "eq"
    },
    "max_score" : null,
    "hits" : [ ]
  }
}

For index test with minChars=5 for index_prefixes:

curl -XPUT localhost:9200/test-3 --data '{                                                                             
"mappings": {
    "properties": {
      "t": {
        "type": "text",
        "index_prefixes": { "min_chars": "5" }
      }
    }
  }
}' -H "Content-Type:Application/json"
{"acknowledged":true,"shards_acknowledged":true,"index":"test-3"}ubuntu@ip-172-31-51-78:~$ 
ubuntu@ip-172-31-51-78:~$ curl 'localhost:9200/test-3/_search?pretty' --data '{"query":{"prefix":{"t": ""}}}' -H "Content-Type:Application/json"
{
  "took" : 3,
  "timed_out" : false,
  "_shards" : {
    "total" : 1,
    "successful" : 1,
    "skipped" : 0,
    "failed" : 0
  },
  "hits" : {
    "total" : {
      "value" : 0,
      "relation" : "eq"
    },
    "max_score" : null,
    "hits" : [ ]
  }
}
ubuntu@ip-172-31-51-78:~$ curl 'localhost:9200/test-3/_search?pretty' --data '{"query":{"prefix":{"t": "s"}}}' -H "Content-Type:Application/json"
{
  "took" : 3,
  "timed_out" : false,
  "_shards" : {
    "total" : 1,
    "successful" : 1,
    "skipped" : 0,
    "failed" : 0
  },
  "hits" : {
    "total" : {
      "value" : 0,
      "relation" : "eq"
    },
    "max_score" : null,
    "hits" : [ ]
  }
}
ubuntu@ip-172-31-51-78:~$ curl 'localhost:9200/test-3/_search?pretty' --data '{"query":{"prefix":{"t": "st"}}}' -H "Content-Type:Application/json"
{
  "took" : 4,
  "timed_out" : false,
  "_shards" : {
    "total" : 1,
    "successful" : 1,
    "skipped" : 0,
    "failed" : 0
  },
  "hits" : {
    "total" : {
      "value" : 0,
      "relation" : "eq"
    },
    "max_score" : null,
    "hits" : [ ]
  }
}
ubuntu@ip-172-31-51-78:~$ curl 'localhost:9200/test-3/_search?pretty' --data '{"query":{"prefix":{"t": "str"}}}' -H "Content-Type:Application/json"
{
  "took" : 3,
  "timed_out" : false,
  "_shards" : {
    "total" : 1,
    "successful" : 1,
    "skipped" : 0,
    "failed" : 0
  },
  "hits" : {
    "total" : {
      "value" : 0,
      "relation" : "eq"
    },
    "max_score" : null,
    "hits" : [ ]
  }
}
ubuntu@ip-172-31-51-78:~$ curl 'localhost:9200/test-3/_search?pretty' --data '{"query":{"prefix":{"t": "strt"}}}' -H "Content-Type:Application/json"
{
  "took" : 4,
  "timed_out" : false,
  "_shards" : {
    "total" : 1,
    "successful" : 1,
    "skipped" : 0,
    "failed" : 0
  },
  "hits" : {
    "total" : {
      "value" : 0,
      "relation" : "eq"
    },
    "max_score" : null,
    "hits" : [ ]
  }
}
ubuntu@ip-172-31-51-78:~$ curl 'localhost:9200/test-3/_search?pretty' --data '{"query":{"prefix":{"t": "strtt"}}}' -H "Content-Type:Application/json"
{
  "took" : 3,
  "timed_out" : false,
  "_shards" : {
    "total" : 1,
    "successful" : 1,
    "skipped" : 0,
    "failed" : 0
  },
  "hits" : {
    "total" : {
      "value" : 0,
      "relation" : "eq"
    },
    "max_score" : null,
    "hits" : [ ]
  }
}

saratvemulapalli

You are faster than me :)
Thanks for taking care of this.

I think the right fix is to take care of the acceptance check.
Also could you add tests to make sure we dont break it in the future?

saratvemulapalli · 2022-04-12T21:17:44Z

server/src/main/java/org/opensearch/index/mapper/TextFieldMapper.java

@@ -583,7 +583,9 @@ public Query prefixQuery(String value, MultiTermQuery.RewriteMethod method, bool
            }
            Automaton automaton = Operations.concatenate(automata);
            AutomatonQuery query = new AutomatonQuery(new Term(name(), value + "*"), automaton);
-            query.setRewriteMethod(method);
+            if (method != null) {


This approach solves the problem but the real root cause is the check for min, max chars.

The right fix here is to not accept() the query if the len is less than min_chars.
This makes sure the accept fails at: Line-762 and doesnt really need to call: Line-765

Suggested change

if (method != null) {

boolean accept(int length) {

return length >= minChars && length <= maxChars;

}

Oh I didn't know we should not be accepting the value less than minChars. I will make that change and add tests as well.

Also take a look at: dd540ef.
There might be other cases which might fail.

Looks like the support for minChars - 1 was specifically added. Would the fix in this PR then be the solution since it changes the behavior if we don't accept the minChars - 1 length?

I think the fix the original commit put in lead to this problem. My hunch says this should be fixed.
Could you read through the commit and understand what other problems we will see?

From the commit, the operation for the index_prefix query for length = minChars-1 is remapped to a? from a* to make it less expensive. Also, looking at the current code base, I see that the setRewriteMethod is wrapped by a null check at other places: for example https://github.com/opensearch-project/OpenSearch/blame/3c5d997a765e24ffa32d35219fd5026cfb143a9d/server/src/main/java/org/opensearch/index/mapper/StringFieldType.java#L116.

Thanks Vacha for this.
I take back my suggestion, as its probably not supporting the optimization in performance.
I agree with you that, verifying the method being null solves the problem while being efficient.

Could you add some tests and we should be good to go.

Also, is query.setRewriteMethod(null) a valid thing by itself? Can the rewrite method be null? If not throw in an assert or something like that to prevent hiding other errors.

null looks to be valid, also @nknize opened an issue #2896 to remove these usages next since the method is deprecated.

opensearch-ci-bot · 2022-04-12T21:32:00Z

✅ Gradle Check success b6b9d3561c9869ff26326526b217d1fdab4e3a00
Log 4418

Reports 4418

Signed-off-by: Vacha Shah <vachshah@amazon.com>

opensearch-ci-bot · 2022-04-13T22:52:04Z

✅ Gradle Check success 1c5494b
Log 4452

Reports 4452

saratvemulapalli · 2022-04-14T00:18:14Z

Looks good to me. Thanks @VachaShah !

nknize

I'm fine approving and merging this PR to fix the immediate bug but we should open a follow-up PR for #2896. Let us know if you'd like to take care of that as well @VachaShah

nknize · 2022-04-14T04:19:10Z

server/src/main/java/org/opensearch/index/mapper/TextFieldMapper.java

@@ -583,7 +583,9 @@ public Query prefixQuery(String value, MultiTermQuery.RewriteMethod method, bool
            }
            Automaton automaton = Operations.concatenate(automata);
            AutomatonQuery query = new AutomatonQuery(new Term(name(), value + "*"), automaton);
-            query.setRewriteMethod(method);
+            if (method != null) {
+                query.setRewriteMethod(method);


query.setRewriteMethod is removed in Lucene 9.2. I've opened an issue to remove all references to the method and use CONSTANT_SCORE_REWRITE in places where null is passed in as the method parameter.

VachaShah · 2022-04-14T18:05:45Z

I'm fine approving and merging this PR to fix the immediate bug but we should open a follow-up PR for #2896. Let us know if you'd like to take care of that as well @VachaShah

Thanks @nknize! Sure I can take care of that.

* Adding a null pointer check to fix index_prefix query Signed-off-by: Vacha Shah <vachshah@amazon.com> * Adding test Signed-off-by: Vacha Shah <vachshah@amazon.com> (cherry picked from commit 452e368)

* Adding a null pointer check to fix index_prefix query Signed-off-by: Vacha Shah <vachshah@amazon.com> * Adding test Signed-off-by: Vacha Shah <vachshah@amazon.com> (cherry picked from commit 452e368) Co-authored-by: Vacha Shah <vachshah@amazon.com>

VachaShah requested a review from a team as a code owner April 12, 2022 21:01

VachaShah requested a review from saratvemulapalli April 12, 2022 21:03

saratvemulapalli reviewed Apr 12, 2022

View reviewed changes

VachaShah added 2 commits April 13, 2022 15:12

Adding a null pointer check to fix index_prefix query

a6340ea

Signed-off-by: Vacha Shah <vachshah@amazon.com>

Adding test

1c5494b

Signed-off-by: Vacha Shah <vachshah@amazon.com>

VachaShah force-pushed the issue-2826 branch from b6b9d35 to 1c5494b Compare April 13, 2022 22:14

VachaShah requested a review from saratvemulapalli April 13, 2022 22:40

saratvemulapalli approved these changes Apr 14, 2022

View reviewed changes

saratvemulapalli added backport 2.x Backport to 2.x branch v3.0.0 Issues and PRs related to version 3.0.0 backport 2.0 Backport to 2.0 branch bug Something isn't working Indexing & Search labels Apr 14, 2022

nknize mentioned this pull request Apr 14, 2022

Remove usages of MultiTermQuery.setRewriteMethod #2896

Closed

nknize approved these changes Apr 14, 2022

View reviewed changes

VachaShah merged commit 452e368 into opensearch-project:main Apr 14, 2022

opensearch-trigger-bot bot mentioned this pull request Apr 14, 2022

[Backport 2.x] Adding a null pointer check to fix index_prefix query #2902

Merged

opensearch-trigger-bot bot mentioned this pull request Apr 14, 2022

[Backport 2.0] Adding a null pointer check to fix index_prefix query #2903

Merged

mattjarnevic mentioned this pull request Jul 13, 2022

NPE happens on short prefix query on a field with index_prefixes elastic/elasticsearch-rails#1038

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding a null pointer check to fix index_prefix query #2879

Adding a null pointer check to fix index_prefix query #2879

VachaShah commented Apr 12, 2022

VachaShah commented Apr 12, 2022

saratvemulapalli left a comment •

edited

Loading

saratvemulapalli Apr 12, 2022 •

edited

Loading

VachaShah Apr 12, 2022

saratvemulapalli Apr 12, 2022

VachaShah Apr 13, 2022

saratvemulapalli Apr 13, 2022

VachaShah Apr 13, 2022

saratvemulapalli Apr 13, 2022

dblock Apr 13, 2022

VachaShah Apr 14, 2022

opensearch-ci-bot commented Apr 12, 2022

opensearch-ci-bot commented Apr 13, 2022

saratvemulapalli commented Apr 14, 2022

nknize left a comment

nknize Apr 14, 2022

VachaShah commented Apr 14, 2022

-            if (method != null) {
+        boolean accept(int length) {
+            return length >= minChars && length <= maxChars;
+        }

Adding a null pointer check to fix index_prefix query #2879

Adding a null pointer check to fix index_prefix query #2879

Conversation

VachaShah commented Apr 12, 2022

Description

Issues Resolved

Check List

VachaShah commented Apr 12, 2022

saratvemulapalli left a comment • edited Loading

Choose a reason for hiding this comment

saratvemulapalli Apr 12, 2022 • edited Loading

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

Choose a reason for hiding this comment

opensearch-ci-bot commented Apr 12, 2022

opensearch-ci-bot commented Apr 13, 2022

saratvemulapalli commented Apr 14, 2022

nknize left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VachaShah commented Apr 14, 2022

saratvemulapalli left a comment •

edited

Loading

saratvemulapalli Apr 12, 2022 •

edited

Loading