Skip to content
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.

Sign up for GitHub

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

Allow plugins outside core package to access and override some function of FieldMapper #17575

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Allow plugins outside core package to access and override some function of FieldMapper #17575

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion ...nalysis-icu/src/main/java/org/opensearch/index/mapper/ICUCollationKeywordFieldMapper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,7 @@ public IndexFieldData.Builder fielddataBuilder(String fullyQualifiedIndexName, S
}

@Override
protected BytesRef indexedValueForSearch(Object value) {
public BytesRef indexedValueForSearch(Object value) {
if (value == null) {
return null;
}
Expand Down
2 changes: 1 addition & 1 deletion server/src/main/java/org/opensearch/index/mapper/FlatObjectFieldMapper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -294,7 +294,7 @@ public Object valueForDisplay(Object value) {
}

@Override
protected BytesRef indexedValueForSearch(Object value) {
public BytesRef indexedValueForSearch(Object value) {
if (getTextSearchInfo().getSearchAnalyzer() == Lucene.KEYWORD_ANALYZER) {
// flat_object analyzer with the default attribute source which encodes terms using UTF8
// in that case we skip normalization, which may be slow if there many terms need to
Expand Down
2 changes: 1 addition & 1 deletion server/src/main/java/org/opensearch/index/mapper/KeywordFieldMapper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -378,7 +378,7 @@ public Object valueForDisplay(Object value) {
}

@Override
protected BytesRef indexedValueForSearch(Object value) {
public BytesRef indexedValueForSearch(Object value) {
if (getTextSearchInfo().getSearchAnalyzer() == Lucene.KEYWORD_ANALYZER) {
// keyword analyzer with the default attribute source which encodes terms using UTF8
// in that case we skip normalization, which may be slow if there many terms need to
Expand Down
8 changes: 8 additions & 0 deletions server/src/main/java/org/opensearch/index/mapper/NumberFieldMapper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1557,6 +1557,14 @@
return type;
}

public Number nullValue() {
return nullValue;

Check warning on line 1561 in server/src/main/java/org/opensearch/index/mapper/NumberFieldMapper.java

View check run for this annotation

Codecov / codecov/patch

server/src/main/java/org/opensearch/index/mapper/NumberFieldMapper.java#L1561 

Added line #L1561 was not covered by tests
}

public boolean coerce() {
return coerce;

Check warning on line 1565 in server/src/main/java/org/opensearch/index/mapper/NumberFieldMapper.java

View check run for this annotation

Codecov / codecov/patch

server/src/main/java/org/opensearch/index/mapper/NumberFieldMapper.java#L1565 

Added line #L1565 was not covered by tests
}

public NumericType numericType() {
return type.numericType();
}
Expand Down
6 changes: 3 additions & 3 deletions server/src/main/java/org/opensearch/index/mapper/ParametrizedFieldMapper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ protected ParametrizedFieldMapper(String simpleName, MappedFieldType mappedField
public abstract ParametrizedFieldMapper.Builder getMergeBuilder();

@Override
public final ParametrizedFieldMapper merge(Mapper mergeWith) {
public ParametrizedFieldMapper merge(Mapper mergeWith) {

if (mergeWith instanceof FieldMapper == false) {
throw new IllegalArgumentException(
Expand Down Expand Up @@ -348,7 +348,7 @@ private void merge(FieldMapper toMerge, Conflicts conflicts) {
}
}

protected void toXContent(XContentBuilder builder, boolean includeDefaults) throws IOException {
public void toXContent(XContentBuilder builder, boolean includeDefaults) throws IOException {
if (serializerCheck.check(includeDefaults, isConfigured(), get())) {
serializer.serialize(builder, name, getValue());
}
Expand Down Expand Up @@ -649,7 +649,7 @@ protected String buildFullName(BuilderContext context) {
/**
* Writes the current builder parameter values as XContent
*/
protected final void toXContent(XContentBuilder builder, boolean includeDefaults) throws IOException {
public final void toXContent(XContentBuilder builder, boolean includeDefaults) throws IOException {
for (Parameter<?> parameter : getParameters()) {
parameter.toXContent(builder, includeDefaults);
}
Expand Down
2 changes: 1 addition & 1 deletion server/src/main/java/org/opensearch/index/mapper/TermBasedFieldType.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You mean TermBasedFieldType? We are extending the StringFieldType which extends the TermBasedFieldType. It can access the protected method in core package. But in neural plugin we cannot access it we only can override it. But we want to delegate the query work to the wrapped field type where we don't want to override the function but invoke the function of the wrapped field type directly. To do that we need to make it publish like other functions.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is mainly needed by KeywordFieldMapper since it overrides the default behavior of the indexedValueForSearch function. Otherwise we don't need to override it since when we are extending the StringFieldType which has a default implementation for this function.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right -- but indexedValueForSearch only needs to be implemented so that it can be called from the various *Query methods, which are declared in MappedFieldType.

If SemanticStringFieldType extends MappedFieldType, it can delegate all of the *Query methods to delegate without caring about the internal implementation details (indexedValueForSearch). Would that work?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we don't care about the internal implementation details we just want to delegate the work. To delegate the work we need to do something like:

    @Override
    public BytesRef indexedValueForSearch(Object value) {
        return delegateFieldType.indexedValueForSearch(value);
    }

We cannot simply rely on our semantic field type even it's already extending the string field type because it's possible the delegate field type can have new parameters. e.g. code. And it can make the semantic field type behave different from the delegate field type.

The way we can avoid this I think is to extend the delegate field type directly. e.g. If we have a semanticKeywordFieldType extending the KeywordFieldType and use the delegate keyword field type to set the same parameters for it then we don't need to override the function to delegate the query work. We can directly use it since it should behave the same as the delegate keyword field type.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, but if SemanticStringFieldType extends MappedFieldType, then it doesn't need to delegate indexedValueForSearch, because it doesn't care that it exists (it's not part of the signature of MappedFieldType).

The fact that the delegate field type internally has an indexedValueForSearch is hidden by the delegated calls to termQuery, etc.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Synced offline with Michael. We will create a FilterFieldType in core to handle the delegate work and in plugin we simply need to extend it. This PR will not be needed.

return BytesRefs.toBytesRef(value);
}

Expand Down