-
Notifications
You must be signed in to change notification settings - Fork 24.9k
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
Support synthetic _source for _doc_count field #91465
Conversation
Pinging @elastic/es-analytics-geo (Team:Analytics) |
Hi @nik9000, I've created a changelog YAML for you. |
@@ -173,4 +173,6 @@ public void postParse(DocumentParserContext context) throws IOException { | |||
// do nothing | |||
} | |||
|
|||
@Override | |||
public abstract SourceLoader.SyntheticFieldLoader syntheticFieldLoader(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would like to make this abstract across the board just so everyone who makes a field has to think about it. If they don't want to support it, fine, but I'd like folks to think about it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But I didn't want to make it abstract in Mapper
yet. It's just twice as many changes....
This add synthetic `_source` support for the `_doc_count` field so downsampling should play nicely with sythetic `_source`.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One question, LGTM otherwise!
public void testSyntheticSourceMany() throws IOException { | ||
DocumentMapper mapper = createDocumentMapper(syntheticSourceMapping(b -> {})); | ||
List<Integer> counts = randomList(2, 10000, () -> between(1, Integer.MAX_VALUE)); | ||
try (Directory directory = newDirectory()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does withLuceneIndex
not work here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me have another look. I think there was a reason, but it might not apply right here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
|
||
@Override | ||
public DocValuesLoader docValuesLoader(LeafReader leafReader, int[] docIdsInLeaf) throws IOException { | ||
postings = leafReader.postings(new Term(DocCountFieldMapper.NAME, DocCountFieldMapper.NAME)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Does it make sense to make new Term(DocCountFieldMapper.NAME, DocCountFieldMapper.NAME)
a static final
constant?
if (hasValue == false) { | ||
return; | ||
} | ||
b.field("_doc_count", postings.freq()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
another nit: Maybe change "_doc_count"
to DocCountFieldMapper.NAME
?
This add synthetic
_source
support for the_doc_count
field so downsampling should play nicely with sythetic_source
.