Allow plugins outside core package to access and override some function of FieldMapper #17575
Conversation
…ons of the FieldMapper. Signed-off-by: Bo Zhang <[email protected]>
bzhangam
requested review from
anasalkouz,
andrross,
ashking94,
Bukhtawar,
CEHENKLE,
cwperks,
dblock,
dbwiddis,
gbbafna,
jed326,
kotwanikunal,
mch2,
msfroh,
nknize,
owaiskazi19,
reta,
Rishikesh1159,
sachinpkale,
saratvemulapalli,
shwetathareja,
sohami,
VachaShah,
bugmakerrrrrr,
jainankitk and
linuxpi
as code owners
| @@ -69,7 +69,7 @@ public TermBasedFieldType( | |||
| /** Returns the indexed value used to construct search "values". | |||
| * This method is used for the default implementations of most | |||
| * query factory methods such as {@link #termQuery}. */ | |||
| protected BytesRef indexedValueForSearch(Object value) { | |||
| public BytesRef indexedValueForSearch(Object value) { | |||
There was a problem hiding this comment.
This change relax the access to this function but all its subclasses override it with protected access modifier and we also need to change them as public. I have updated all subclasses in core. But if there is any plugin extending it we also need to update the plugin accordingly. Not sure how should we communicate change like this.
There was a problem hiding this comment.
It's not clear to me why these need to be changed from protected to public.
Subclasses can still override protected methods, even it they're defined in a plugin.
There was a problem hiding this comment.
We want to make it public to allow plugin to invoke it through a field type not override it. Since we want to delegate the query work to the delegate field type and in plugin we cannot invoke the protected function even we are in the subclass. Check this code change.
There was a problem hiding this comment.
Got it -- I think you would need SemanticFieldMapper to extend TermBasedFieldMapper in order to have access to the protected method. I wonder if that would make sense? It does look like you want your SemanticFieldMapper to have term-based behavior, but be decoupled from the specifics of which term-based field type is being wrapped.
|
❌ Gradle check result for 1cc9d6b: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
Description
Allow plugin out side core package to access some functions and fields of the FieldMapper.
Allow plugin to override the merge function of the FieldMapper.
Related Issues
This is needed to support this PR for this feature.
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.