-
Notifications
You must be signed in to change notification settings - Fork 25.6k
[DRAFT] Dont track source for keyworld fields that are multi-field, unless specifically designated so #132231
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?
Conversation
…ecifically designated so
| * Returns whether the given builder is synthetic source compatible. To be compatible, the builder must | ||
| * track source in some way, whether that be via the "store" param or using doc values. | ||
| */ | ||
| private boolean isSyntheticSourceCompatible(KeywordFieldMapper.Builder keywordBuilder) { |
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.
Do you think we can make MultiFields.Builder an interface and do this stuff in a decorator or something inside TextFieldMapper? I know we have hasSyntheticSourceCompatibleKeywordField already but this is growing more and more.
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 agree that its growing too much. I was also thinking about moving this logic somewhere closer to where it belongs. I'll see what I can do.
| /** | ||
| * Returns whether the given builder should be used to track source for synthetic source purposes. | ||
| */ | ||
| private boolean shouldTrackSource(KeywordFieldMapper.Builder keywordBuilder) { |
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 am struggling with this name. Isn't this something like isUsedForSyntheticSourceByParent?
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.
yeah... my thinking was to not expose this edge case as much through naming, but I can also see someone misunderstanding what this is used for. I'll change it
| ); | ||
| } | ||
|
|
||
| public Builder shouldTrackSourceForSyntheticSource(boolean shouldTrackSourceForSyntheticSource) { |
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.
Same - usedByParentFieldForSyntheticSource?
[DRAFT] (no tests yet)
Consider the following edge case:
If synthetic source is enabled, we'll have
name.footrack source (current behavior). And ifname.footripsignore_above, we'll create an extra StoredField to continue tracking source within thename.foo.This however becomes a problem if there are multiple
keywordmulti fields, all of which tripignore_above. In such a case, they'll all create an additionalStoredFieldand we'll be storing the source multiple times, which is not space efficient.This change designates one of the
keywordmulti fields as the one responsible for tracking source. Upon trippingignore_above, this field will create aStoredField(same behavior as now), while the remainingkeywordmulti fields will not (new behavior).