Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make ParseContext's IndexQueryParserService accessible #8248

Closed
uschindler opened this issue Oct 28, 2014 · 7 comments · Fixed by #8252

Comments

@uschindler
Copy link
Contributor

commented Oct 28, 2014

I have the following problem writing an own query parser from my own package:

My query parser has to internally create another Elasticsaearch query and parse it (which is actually a workaround: I need to create a MoreLikeThis query, but I dont want to replicate all code to generate the native Lucene Query). So I use MoreLikeThisQueryBuilder to create a BytesReference of the newly constructed query, so I can parse it to a Lucene Query.

Of course I cannot do the parseContext.parseInnerQuery, because the newly constructed query is in a separate BytesReference. Instead I do the same trick like WrappedQuery:

    final BytesReference querySource = QueryBuilders.moreLikeThisQuery(FIELDNAME_MLT)
      .ids(Integer.toString(id))
      .include(true)
      .percentTermsToMatch(.5f)
      .minTermFreq(0)
      .minDocFreq(5)
      .maxQueryTerms(25)
      .boostTerms(1.0f)
      .buildAsBytes();
    // hack for parsing this as inner query
    try (XContentParser qSourceParser = XContentFactory.xContent(querySource).createParser(querySource)) {
      final QueryParseContext context = cloneParseContext();
      context.reset(qSourceParser);
      return context.parseInnerQuery();
    } catch (IOException ioe) {
      throw new AssertionError("IOException cannot happen here.", ioe);
    }

The problem is now: I need to clone the QueryParseContext. WrappedQueryParser just creates a new instance, but it needs the index name (easy to get) and IndexQueryParserService. Unfortunately, the latter is package private.

I would be happy if this could be solved. As this above stuff is used quite often, I think the best solution would be to allow to clone ParseContext or add a method to create a new one from an existion one (copy constructor). The other way would be to make the field accessible.

The current workaround is to use reflection to get the field...

@uschindler

This comment has been minimized.

Copy link
Contributor Author

commented Oct 28, 2014

The other "possible" workaround to inject IndexQueryParserService by @Inject into my own query parser does not work (circular dependency).

@kimchy

This comment has been minimized.

Copy link
Member

commented Oct 28, 2014

why not just expose a getter for indexQueryParser from the parse context? The fact that its package private is not really needed I think.

@uschindler

This comment has been minimized.

Copy link
Contributor Author

commented Oct 28, 2014

@kimchy thats of course the easy thing, as I said :-)

My idea here was to make the common case "parse some other query in context of current ParseContext" easier: The same code is duplicated over and over. Maybe add something like ParseContext#parseQueryInCurrentContext(BytesReference), a setup like parseInnerQuery() - just cloning itsself. By that it could reused in WrapperQueryParser, WrapperFilterParser, and TemplateQueryParser - and for my case.

@kimchy

This comment has been minimized.

Copy link
Member

commented Oct 28, 2014

The repetition is not terrible if you create a new instance of the QueryParseContext, but we could have a nice helper method, agreed.

Regardless, I don't see a reason why we can't expose the indexQueryParser in the context, which is a very simple change that would help you now (and might get to 1.4), and then look into simplifying the creation of a new instance of QueryParserContext.

@uschindler

This comment has been minimized.

Copy link
Contributor Author

commented Oct 28, 2014

I am fine with exposing that! Add a getter and change the field to private final.

@kimchy

This comment has been minimized.

Copy link
Member

commented Oct 28, 2014

@uschindler want to send a pull request?

@uschindler

This comment has been minimized.

Copy link
Contributor Author

commented Oct 28, 2014

Can do!

uschindler added a commit to uschindler/elasticsearch that referenced this issue Oct 28, 2014

@s1monw s1monw closed this in #8252 Oct 28, 2014

s1monw added a commit that referenced this issue Oct 28, 2014

@s1monw s1monw changed the title Make ParseContext's IndexQueryParserService accessible or allow to clone ParseContext Make ParseContext's IndexQueryParserService accessible Oct 28, 2014

@clintongormley clintongormley removed the discuss label Nov 3, 2014

mute pushed a commit to mute/elasticsearch that referenced this issue Jul 29, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.