Make Document a top-level class #74472
Conversation
There is no reason for Document to be an inner class of ParseContext, especially as it is public and accessed directly from many different places. This commit takes it out to its own top-level class file, which has the advantage o simplifying ParseContext that can use some love too.
javanna
added
:Search Foundations/Mapping
|
Pinging @elastic/es-search (Team:Search) |
|
|
||
| private Engine.Index getIndex(final String id) { | ||
| final ParseContext.Document document = new ParseContext.Document(); | ||
| final org.elasticsearch.index.mapper.Document document = new org.elasticsearch.index.mapper.Document(); |
There was a problem hiding this comment.
This is odd, in fact we have now a name clash between the lucene document class and our own. Was it better when it was part of ParseContext? Is it ok now or shall we rename the top-level class to something else that does not clash with the lucene one?
There was a problem hiding this comment.
Our version is essentially a fork, I think, and can be used as a drop-in replacement for lucene's version everywhere. So we could keep it as Document and just use it in the tests where there's a name clash? Or do what we do with other forks and call it XDocument, although I think the X-prefix implies that it's a temporary fork and we plan on pushing changes upstream which I don't think is the case here.
There was a problem hiding this comment.
What would you prefer? Do you find it an improvement to have it as a top-level class with the downside of the name clash? We could also call it ESDocument if we really are concerned about the name clash
There was a problem hiding this comment.
I definitely like it as a top-level class. ESDocument works for me.
There was a problem hiding this comment.
Cool I will wait for Julie to chime in before merging ;)
There was a problem hiding this comment.
Moving it to the top-level makes sense to me, since it's relevant and accessed even when a ParseContext isn't around. I'm not as sure about the name ESDocument, it feels very general when the class is really always meant to be used in a parsing situation, representing a parsed document. The name ParsedDocument is already taken though unfortunately.
Just sharing some thoughts, I don't have a strong opinion and okay with whatever you decide for a name.
There was a problem hiding this comment.
I also don't love ESDocument, maybe LuceneDocument would make more sense actually. Or we could just accept the name clash where it happens, which should not be many places.
There was a problem hiding this comment.
Thanks for bringing up ParsedDocument, I was not super familiar with this area. As far as I understand it now, ParsedDocument is more like the ES document, that can hold one or more lucene docs. So I am going with LuceneDocument if there are no objections.
romseygeek
left a comment
romseygeek
left a comment
There was a problem hiding this comment.
LGTM, thanks @javanna
There is no reason for Document to be an inner class of ParseContext, especially as it is public and accessed directly from many different places. This commit takes it out to its own top-level class file, which has the advantage of simplifying ParseContext which could use some love too.
There is no reason for Document to be an inner class of ParseContext, especially as it is public and accessed directly from many different places. This commit takes it out to its own top-level class file, which has the advantage of simplifying ParseContext which could use some love too.
5 participants
There is no reason for Document to be an inner class of ParseContext, especially as it is public and accessed directly from many different places.
This commit takes it out to its own top-level class file, which has the advantage o simplifying ParseContext that can use some love too.