TSDB: Change _tsid field to SortedDocValuesField
#83045
Conversation
csoulios
added
:Search Foundations/Mapping
|
|
||
| @Override | ||
| public SortField sortField(@Nullable Object missingValue, MultiValueMode sortMode, Nested nested, boolean reverse) { | ||
| SortField sortField = new SortField(getFieldName(), SortField.Type.STRING, reverse); |
There was a problem hiding this comment.
I wonder if setting this to SortField.Type.STRING by default is ok.
If I leave it to the default SortField.Type.CUSTOM method IndexSortConfig#validateIndexSortField fails
There was a problem hiding this comment.
Type.STRING is correct here.
There was a problem hiding this comment.
Type.CUSTOM means that it's expecting you to give it a custom FieldComparator, which we don't need. It is a horrible API from about 2004 and I keep meaning to hack it out and replace it with something less awful...
elasticmachine
added
Team:Search
|
Pinging @elastic/es-search (Team:Search) |
|
Pinging @elastic/es-analytics-geo (Team:Analytics) |
romseygeek
left a comment
romseygeek
left a comment
There was a problem hiding this comment.
Looks good, I left a couple of suggestions.
|
|
||
| @Override | ||
| public SortField sortField(@Nullable Object missingValue, MultiValueMode sortMode, Nested nested, boolean reverse) { | ||
| SortField sortField = new SortField(getFieldName(), SortField.Type.STRING, reverse); |
There was a problem hiding this comment.
Type.STRING is correct here.
| SortField sortField = new SortField(getFieldName(), SortField.Type.STRING, reverse); | ||
| sortField.setMissingValue( | ||
| sortMissingLast(missingValue) ^ reverse ? SortedSetSortField.STRING_LAST : SortedSetSortField.STRING_FIRST | ||
| ); |
There was a problem hiding this comment.
Do we expect anything to be missing a _tsid field? If not we can just ignore setMissingValue.
There was a problem hiding this comment.
In time series indices _tsid field must always be present. I only left it because SortedOrdinalsIndexFieldData may possibly be used for other fields too.
| iterator = scorer.iterator(); | ||
| docBase = context.docBase; | ||
| tsids = context.reader().getSortedSetDocValues(TimeSeriesIdFieldMapper.NAME); | ||
| tsids = context.reader().getSortedDocValues(TimeSeriesIdFieldMapper.NAME); |
There was a problem hiding this comment.
I'd use DocValues.getSorted(context.reader(), TimeSeriesIdFieldMapper.NAME) here as it will protect you from NPEs and also automatically convert between SortedSet and Sorted if we need it.
| if (docId != DocIdSetIterator.NO_MORE_DOCS && (liveDocs == null || liveDocs.get(docId))) { | ||
| if (tsids.advanceExact(docId)) { | ||
| BytesRef tsid = tsids.lookupOrd(tsids.nextOrd()); | ||
| BytesRef tsid = tsids.lookupOrd(tsids.ordValue()); |
There was a problem hiding this comment.
We can replace this with an ord comparison now that we know there's a single value, but let's do that in a follow-up.
|
|
||
| @Override | ||
| public LeafOrdinalsFieldData load(LeafReaderContext context) { | ||
| return new SortedSetBytesLeafFieldData(context.reader(), getFieldName(), toScriptField); |
There was a problem hiding this comment.
Might be worth a comment here that SortedSetBytesLeafFieldData will happily load SortedDocValues as well, so there's no need for a single-valued implementation.
|
|
||
| @Override | ||
| public SortField sortField(@Nullable Object missingValue, MultiValueMode sortMode, Nested nested, boolean reverse) { | ||
| SortField sortField = new SortField(getFieldName(), SortField.Type.STRING, reverse); |
There was a problem hiding this comment.
Type.CUSTOM means that it's expecting you to give it a custom FieldComparator, which we don't need. It is a horrible API from about 2004 and I keep meaning to hack it out and replace it with something less awful...
4 participants
Since
_tsidcannot be a multi-value field, this PR modifies theTimeSeriesIdFieldMapperso that_tsidis added as aSortedDocValuesField(instead of aSortedSetDocValuesField)Relates to #80276