-
Notifications
You must be signed in to change notification settings - Fork 25.7k
Fix bug in tsdb doc value codec #134979
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
Fix bug in tsdb doc value codec #134979
Conversation
The ordinal range encoding doesn't support multi-valued fields, but the logic that prohibits its usage for multi-valued field was missing, which resulted in multi-valued fields being incorrectly encoded. The ordinal range encoding was introduced for primary sort fields only to allow a more efficient look-ahead / skip logic. This is to efficiently determine whether a constant block could be used for a range of matching doc ids in compute engine. Closes elastic#134950
| final RandomAccessInput addressesInput = data.randomAccessSlice(entry.addressesOffset, entry.addressesLength); | ||
| final LongValues addresses = DirectMonotonicReader.getInstance(entry.addressesMeta, addressesInput, merging); | ||
|
|
||
| assert entry.sortedOrdinals == null : "encoded ordinal range supports only one value per document"; |
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.
Invoking getValues(...) will result into a NPE if entry.sortedOrdinals != null.
Maybe we can return a singleton SortedNumericDocValues instance here, so that at least the the first value of every document is available?
|
Pinging @elastic/es-storage-engine (Team:StorageEngine) |
|
@martijnvg Thanks for fixing this - this is the correct solution. However, indexes with multi-valued fields that have already been encoded with ordinal range will remain broken. Should we also support reading them? |
|
Thanks Nhat for taking a look.
Yes, I was thinking about this too: #134979 (comment) |
dnhatn
left a comment
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, thanks Martijn!
The ordinal range encoding doesn't support multi-valued fields, but the logic that prohibits its usage for multi-valued field was missing, which resulted in multi-valued fields being incorrectly encoded.
The ordinal range encoding was introduced for primary sort fields only to allow a more efficient look-ahead / skip logic (#133018). This is to more efficiently determine whether a constant block could be used for a range of matching doc ids in compute engine.
Closes #134950
(marking as non-issue as this is a bug doesn't occur in a stateful release)