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

Add support for combining fields to the FVH #3860

Closed
wants to merge 1 commit into from

Conversation

nik9000
Copy link
Member

@nik9000 nik9000 commented Oct 9, 2013

Closes #3750

@nik9000
Copy link
Member Author

nik9000 commented Oct 9, 2013

This still has a bunch of work to do around error handling, documentation, pushing the change to Lucene, and whatever structure Elasticsearch has around its "Lucene Monitor" classes but it seems promising to me.

@jpountz
Copy link
Contributor

jpountz commented Oct 10, 2013

@nik9000 +1 for trying to incorporate this feature into Lucene. Would you like to open an issue in Lucene's JIRA and attach a patch? Then once it's in Lucene we can have it pushed to Elasticsearch as well until the next Lucene release makes it unnecessary.

@nik9000
Copy link
Member Author

nik9000 commented Oct 10, 2013

Would you like to open an issue in Lucene's JIRA and attach a patch?

https://issues.apache.org/jira/browse/LUCENE-5274

I'll rework my Elasticsearch stuff into a Lucene patch.

@ghost ghost assigned jpountz Oct 10, 2013
@jpountz
Copy link
Contributor

jpountz commented Oct 10, 2013

Thanks @nik9000 I just assigned the issue to myself.

@nik9000
Copy link
Member Author

nik9000 commented Oct 22, 2013

Thanks so much for helping me get this into Lucene. Now that it is merged, what is the procedure like to get Elasticsearch using it?

@nik9000
Copy link
Member Author

nik9000 commented Oct 30, 2013

Just had a look and this feature is not in 4.5.1 that Elasticsearch is using. It'll be in 4.6. Anyway, pulling the code into Elasticsearch would require 4 or 5 of those XLuceneClassName classes which seems like a lot for this feature. I suppose it'd be best just to wait until Lucene 4.6 is released.

@kimchy
Copy link
Member

kimchy commented Oct 30, 2013

++ on waiting for Lucene 4.6 if its possible timing wise for you.

@nik9000
Copy link
Member Author

nik9000 commented Oct 30, 2013

I've just got a few bugs staring at me every time I open up my backlog that need this. I imagine I can comfortably wait another month or so. As time goes on more and more people are getting exposed to the subpar hack that I've got in place to emulate this which makes me uneasy.

@kimchy
Copy link
Member

kimchy commented Oct 30, 2013

@nik9000 understood, tell us if things become pressing, no problem with backporting to X classes for 0.90.7 if Lucene 4.6 doesn't happen by then...

@nik9000
Copy link
Member Author

nik9000 commented Nov 25, 2013

Now that 4.6 is merged I can finish this! Yay! I'm double checking it and running the whole test suite again. Also, another plug for #3757. That one has a patch waiting on the Lucene side and would let me get full use out of this feature.

@nik9000
Copy link
Member Author

nik9000 commented Nov 25, 2013

New update pushed. Rebased and working without X classes against Lucene 4.6.

@jpountz
Copy link
Contributor

jpountz commented Nov 27, 2013

Hey Nik, this patch looks good to me. I think it just misses some documentation (as mentionned in the commit message) before we can push it.

@nik9000
Copy link
Member Author

nik9000 commented Nov 27, 2013

Docs! I've forgotten docs! Will do it soon.

The Fast Vector Highlighter can combine matches on multiple fields to
highlight a single field using `matched_fields`.  This is most
intuitive for multifields that analyze the same string in different
ways.  Example:
{
    "query": {
        "query_string": {
            "query": "content.plain:running scissors",
            "fields": ["content"]
        }
    },
    "highlight": {
        "order": "score",
        "fields": {
            "content": {
                "matched_fields": ["content", "content.plain"],
                "type" : "fvh"
            }
        }
    }
}

Closes elastic#3750
@nik9000
Copy link
Member Author

nik9000 commented Nov 30, 2013

Added docs.

@jpountz
Copy link
Contributor

jpountz commented Dec 3, 2013

Pushed. Thanks, Nik!

@jpountz jpountz closed this Dec 3, 2013
@nik9000
Copy link
Member Author

nik9000 commented Dec 3, 2013

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

It'd be cool if the highlighter could combine fields
3 participants