-
Notifications
You must be signed in to change notification settings - Fork 25.6k
Enable a sparse doc values index for @timestamp in time-series indices
#123191
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
Changes from all commits
e251d86
d3ad4e6
9e51083
d1ca991
7cd7974
bd40507
ce9f792
b325699
da28201
0b3d514
645949a
677f2f7
26cb18c
c8607b2
bc89576
e7cee43
675e175
3bee698
752881a
6ffac04
e9c619a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -10,6 +10,9 @@ | |
| package org.elasticsearch.common.lucene.uid; | ||
|
|
||
| import org.apache.lucene.document.LongPoint; | ||
| import org.apache.lucene.index.DocValuesSkipIndexType; | ||
| import org.apache.lucene.index.DocValuesSkipper; | ||
| import org.apache.lucene.index.FieldInfo; | ||
| import org.apache.lucene.index.LeafReader; | ||
| import org.apache.lucene.index.LeafReaderContext; | ||
| import org.apache.lucene.index.NumericDocValues; | ||
|
|
@@ -94,15 +97,27 @@ final class PerThreadIDVersionAndSeqNoLookup { | |
| this.loadedTimestampRange = loadTimestampRange; | ||
| // Also check for the existence of the timestamp field, because sometimes a segment can only contain tombstone documents, | ||
| // which don't have any mapped fields (also not the timestamp field) and just some meta fields like _id, _seq_no etc. | ||
| if (loadTimestampRange && reader.getFieldInfos().fieldInfo(DataStream.TIMESTAMP_FIELD_NAME) != null) { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe leave this if statement in tact and add the following check below here: The reason here is that when There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. From what I can tell, there isn't an easy way to get the current IndexSettings instance into this method. I could update the constructor signature to take some extra info (either the whole IndexSettings instance or just the boolean indicating if doc values skipper is enabled), but it seems easier to just check the FieldInfo. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No need to get an There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh I see, you're saying I should check the feature flag, not the index setting. But what if the feature flag is enabled but the skipper is disabled by the index setting? Then we still want to check the point values There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good point. Let's keep this as is then. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I actually still had to update this check. I'm keeping the same logic, still checking the FieldInfo to see if the doc values skipper is enabled, but it seems retrieving the FieldInfo even if There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think there are cases where we fake the reader. I think when we read from translog, for realtime get. I suspect those IndexReaders sometimes don't work well with field infos. |
||
| PointValues tsPointValues = reader.getPointValues(DataStream.TIMESTAMP_FIELD_NAME); | ||
| assert tsPointValues != null : "no timestamp field for reader:" + reader + " and parent:" + reader.getContext().parent.reader(); | ||
| minTimestamp = LongPoint.decodeDimension(tsPointValues.getMinPackedValue(), 0); | ||
| maxTimestamp = LongPoint.decodeDimension(tsPointValues.getMaxPackedValue(), 0); | ||
| } else { | ||
| minTimestamp = 0; | ||
| maxTimestamp = Long.MAX_VALUE; | ||
| long minTimestamp = 0; | ||
| long maxTimestamp = Long.MAX_VALUE; | ||
| if (loadTimestampRange) { | ||
| FieldInfo info = reader.getFieldInfos().fieldInfo(DataStream.TIMESTAMP_FIELD_NAME); | ||
| if (info != null) { | ||
| if (info.docValuesSkipIndexType() == DocValuesSkipIndexType.RANGE) { | ||
| DocValuesSkipper skipper = reader.getDocValuesSkipper(DataStream.TIMESTAMP_FIELD_NAME); | ||
| assert skipper != null : "no skipper for reader:" + reader + " and parent:" + reader.getContext().parent.reader(); | ||
| minTimestamp = skipper.minValue(); | ||
| maxTimestamp = skipper.maxValue(); | ||
| } else { | ||
| PointValues tsPointValues = reader.getPointValues(DataStream.TIMESTAMP_FIELD_NAME); | ||
| assert tsPointValues != null | ||
| : "no timestamp field for reader:" + reader + " and parent:" + reader.getContext().parent.reader(); | ||
| minTimestamp = LongPoint.decodeDimension(tsPointValues.getMinPackedValue(), 0); | ||
| maxTimestamp = LongPoint.decodeDimension(tsPointValues.getMaxPackedValue(), 0); | ||
| } | ||
| } | ||
| } | ||
| this.minTimestamp = minTimestamp; | ||
| this.maxTimestamp = maxTimestamp; | ||
| } | ||
|
|
||
| PerThreadIDVersionAndSeqNoLookup(LeafReader reader, boolean loadTimestampRange) throws IOException { | ||
|
|
||
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.
👍