Percolator to support parsing script score query with params #101051
Conversation
While dot expansion is disabled when parsing percolator queries at index time, as that would interfere with query parsing, we still use a wrapper parser that is conservative about what methods it supports, assuming that document parsing needs nextToken and not much more. Turns out that when parsing queries instead, we need to support all the XContentParser methods including map, list etc. This commit adds a test for script score query parsing through document parsing via percolator field mapper, and removes the limitations in the wrapper parser.
javanna
added
>bug
:Search Foundations/Mapping
|
Pinging @elastic/es-search (Team:Search) |
|
Hi @javanna, I've created a changelog YAML for you. |
|
Hi @javanna, I've updated the changelog YAML for you. |
| @Override | ||
| public List<Object> listOrderedMap() throws IOException { | ||
| throw new UnsupportedOperationException(); | ||
| } |
There was a problem hiding this comment.
I wonder if this is safe. The intention where I added these was to make sure that we support the bare minimum we need, as I was not sure if these methods could be safely supported when expanding dots. Maybe we could throw uoe only if we are actually expanding dots, or not at all like I do here.
There was a problem hiding this comment.
The actual implementations here will be those in AbstractXContentParser which just delegates things to nextToken(), currentToken(), and the various value methods. So I think it should be safe. It would be good to add some tests that check it does actually do what we expect though?
There was a problem hiding this comment.
It would be good to add some tests that check it does actually do what we expect though?
Right, that's a good idea. I tried through the percolator tests but it became difficult, yet we can just expand the unit test for the parser itself. Will do that.
There was a problem hiding this comment.
i added tests, and figured that when expanding dots is enabled, the behaviour is unexpected in that we don't expand dots at all (we'd need to override an additional nextFieldName method). Given that we don't need the functionality when expanding dots, as we can safely assume that when parsing ordinary documents we never call map or list, I made all methods conditional of whether dots expansion is enabled or not.
javanna
added
the
auto-backport
server/src/main/java/org/elasticsearch/index/mapper/DotExpandingXContentParser.java
Outdated
Show resolved
Hide resolved
|
I took a bit more time to add extensive test coverage. I realized I could register a custom query plugin in |
💚 Backport successful
|
…#101051) While dot expansion is disabled when parsing percolator queries at index time, as that would interfere with query parsing, we still use a wrapper parser that is conservative about what methods it supports, assuming that document parsing needs nextToken and not much more. Turns out that when parsing queries instead, we need to support all the XContentParser methods including map, list etc. This commit adds a test for script score query parsing through document parsing via percolator field mapper, and removes the limitations in the wrapper parser when dots expansion is disabled.
While dot expansion is disabled when parsing percolator queries at index time, as that would interfere with query parsing, we still use a wrapper parser that is conservative about what methods it supports, assuming that document parsing needs nextToken and not much more. Turns out that when parsing queries instead, we need to support all the XContentParser methods including map, list etc. This commit adds a test for script score query parsing through document parsing via percolator field mapper, and removes the limitations in the wrapper parser when dots expansion is disabled. Closes #97377
While dot expansion is disabled when parsing percolator queries at index time, as that would interfere with query parsing, we still use a wrapper parser that is conservative about what methods it supports, assuming that document parsing needs nextToken and not much more. Turns out that when parsing queries instead, we need to support all the XContentParser methods including map, list etc.
This commit adds a test for script score query parsing through document parsing via percolator field mapper, and removes the limitations in the wrapper parser.
Closes #97377