-
Notifications
You must be signed in to change notification settings - Fork 25.6k
Disk usage don't include synthetic _id postings #138745
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
base: main
Are you sure you want to change the base?
Disk usage don't include synthetic _id postings #138745
Conversation
|
Pinging @elastic/es-distributed-indexing (Team:Distributed Indexing) |
|
Hi @burqen, I've created a changelog YAML for you. |
|
I didn't add any tests for it but it will be tested through |
fcofdez
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
| continue; | ||
| } | ||
| if (SyntheticIdField.hasSyntheticIdAttributes(field.attributes())) { | ||
| // Synthetic _id field doesn't have an inverted index stored on disk, |
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.
maybe we can assert that the terms class is not any of the ones expected in getBlockTermState?
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.
Yes, some assertion here is a good idea.
I would rather assert that the Terms / TermsEnum / TermState is of the class that we expect but I cannot do that on this branch, since that will change in later PRs and we are not even reaching this if-branch on this git-branch #overloaded . Perhaps we can add that assertion in later PRs?
I'll at least assert that the field is _id as expected.
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.
Sounds good to me 👍
tlrx
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
…ndex-disk-size-analysis-for-synthetic-id
Synthetic
_idfields doesn't have an inverted index, but will pretend to have it on the read path by injectingIndexOptions.DOCSon the_idfield. We short circuitIndexDiskUsageAnalyzerif we see a synthetic_idfield.