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

Separate parsing impl from setter in SearchParseElement #3602

Closed
synhershko opened this Issue Sep 2, 2013 · 4 comments

Comments

Projects
None yet
2 participants
@synhershko
Copy link
Contributor

synhershko commented Sep 2, 2013

Currently all SearchParseElement classes are implemented like this:

public class HighlighterParseElement implements SearchParseElement {
@override
public void parse(XContentParser parser, SearchContext context) throws Exception {
// actual parsing logic
context.highlight(new SearchContextHighlight(fields));
}
}

For some (advanced) uses I want to be able to parse individual elements myself by relying on the actual core implementation, and to do that I need to use a hack like this:

                        HighlighterParseElement tmp = new HighlighterParseElement();
                        SearchContext ctx = new SearchContext(0, null, null, null, null, null, null, null);
                        tmp.parse(parser, ctx);
                        searchContextHighlight = ctx.highlight();

Just for the sake of parsing. It would be nice if the actual parsing logic could be separated from the call to the setter, something like this:

@Override
public void parse(XContentParser parser, SearchContext context) throws Exception {
    try {
        context.highlight(parseImpl(parser));
    }
    catch (IllegalArgumentException ex)
    {
        throw new SearchParseException(context, "Highlighter global preTags are set, but global postTags are not set");
    }
}

public static SearchContextHighlight parseImpl(XContentParser parser) throws Exception {
  // actual impl

}

@synhershko

This comment has been minimized.

Copy link
Contributor Author

synhershko commented Jun 24, 2014

Hey ES people is it something you'd consider or should I close this one?

@s1monw

This comment has been minimized.

Copy link
Contributor

s1monw commented Jul 1, 2014

@synhershko I think we should do this - do you wanna come up with a PR?

@synhershko

This comment has been minimized.

Copy link
Contributor Author

synhershko commented Jul 1, 2014

@s1monw will do

@synhershko

This comment has been minimized.

Copy link
Contributor Author

synhershko commented Jul 6, 2014

Hey @s1monw please see #6758

I added one commit changing only HighlighterParseElement for now, please review and if my approach makes sense (the re-throwing logic etc) I'll go ahead and apply this all over the place

@s1monw s1monw added the v1.4.0 label Jul 28, 2014

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

[QUERY] Separate parsing impl from setter in SearchParseElement
This commit makes it easier to reuse the inner highlighting, fetch
and rescore parsing logic by plugins or other internal parts.

Closes #3602

@s1monw s1monw closed this in dd0b428 Jul 28, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.