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

Override doc_value parameter in Spatial XPack module #53286

Closed
wants to merge 4 commits into from
Closed

Override doc_value parameter in Spatial XPack module #53286

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
db144f4
Override doc_value parameter in Spatial XPack module
nknize Mar 8, 2020
37d27c3
add `doc_values` mapping option to geo_shape/shape field mappers
talevy Feb 24, 2020
63cdbd6
wire up dataHandler to doc value parsing
nknize Mar 11, 2020
93c3720
remove accidental typo
nknize Mar 11, 2020
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
38 changes: 38 additions & 0 deletions server/src/main/java/org/elasticsearch/index/mapper/AbstractGeometryFieldMapper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,7 @@ public static class Defaults {
public static final Explicit<Boolean> IGNORE_Z_VALUE = new Explicit<>(true, false);
}

protected static List<ParserHandler> PARSER_EXTENSIONS = new ArrayList<>();
Copy link
Contributor

Choose a reason for hiding this comment

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

I do not think the parser-extension part of this PR is needed. I think that the lack of doc_values parsing support in the field mapper is a bug in our field-mapper infrastructure, not a feature that needs to be extended by a plugin.

I would want to see the equivalent to this PR: #47519 getting merged into master, so that this PR can focus on the Indexer extension points. what do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That would work except the builder is not overridden in xpack. It's the responsibility of the parser-extension (parser.config) to throw "doc values not supported" exceptions at mapping time as opposed to throwing them at index time. So when geo_shape doc values are moved to xpack we will want OSS behavior to remain the same (throwing doc values not supported exceptions) and xpack to implement them.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see, so these handlers must be passed in, but can we re-use the data-handler for this instead of creating a generic parsing abstraction? Would it be better to limit scope to just changing how doc-values are parsed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I originally had it that way but changed it because the data handler logic is specific to GeoShapeFieldMapper only (since index logic for both ShapeFieldMapper and LegacyGeoShapeFieldMapper is untouched). But parsing logic needs to be handled by both GeoShapeFieldMapper and ShapeFieldMapper so it was elevated to the AbstractGeometryFieldMapper in order to share the implementation (and not duplicate in ShapeFieldMapper).

This is just the start to the "tangled mess" I was referring to in today's meeting. It gets more hairy when projections (Geo3DPoint XYPoint LatLonPoint) are brought in the fray.

Copy link
Contributor

Choose a reason for hiding this comment

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

Would it work if there were dummy data-handlers/indexers unique for ShapeFieldMapper and LegacyGeoShapeFieldMapper?

regardless of how the extension point is implemented, I feel like it is important that all these field mappers understand how to parse and proclaim their lack of support for doc-values.

Do you see any reason why I should not add support for parsing doc_values to server's implementation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the implementation is mostly based on preference? I don't see a reason to not add support for parsing doc_values in server's implementation (and throwing a MapperParsingException in OSS deployments). I ultimately didn't go this route because I liked the existing behavior where unimplemented parameters are handled further upstream.

Copy link
Contributor

Choose a reason for hiding this comment

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

Like @talevy I have a preference for not making parsing pluggable. We might need to reconsider in the future with projections, but I'd like to explore the path that requires the least amount of new abstractions for now?

Copy link
Contributor

Choose a reason for hiding this comment

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

I opened #53351 to support parsing doc_values. There are two places I left TODO comments where I'd like to be sure this data-handler branch can properly hook into things.

let me know what you think!

Copy link
Contributor Author

@nknize nknize Mar 11, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

👍 I cherry-picked #53351 and wired up the data handler. Next will make the change to not register the default data handler unless no plugins are installed.


/**
* Interface representing an preprocessor in geo-shape indexing pipeline
Expand All @@ -80,6 +81,8 @@ public interface Indexer<Parsed, Processed> {
Class<Processed> processedClass();

List<IndexableField> indexShape(ParseContext context, Processed shape);

void indexDocValues(ParseContext context, Processed shape);
}

/**
Expand Down Expand Up @@ -222,6 +225,7 @@ protected Builder newBuilder(String name, Map<String, Object> params) {
public Mapper.Builder parse(String name, Map<String, Object> node, ParserContext parserContext) throws MapperParsingException {
Map<String, Object> params = new HashMap<>();
boolean parsedDeprecatedParameters = false;
boolean parsedExtension = false;
params.put(DEPRECATED_PARAMETERS_KEY, new DeprecatedParameters());
for (Iterator<Map.Entry<String, Object>> iterator = node.entrySet().iterator(); iterator.hasNext();) {
Map.Entry<String, Object> entry = iterator.next();
Expand All @@ -245,6 +249,13 @@ public Mapper.Builder parse(String name, Map<String, Object> node, ParserContext
XContentMapValues.nodeBooleanValue(fieldNode,
name + "." + GeoPointFieldMapper.Names.IGNORE_Z_VALUE.getPreferredName()));
iterator.remove();
} else if (PARSER_EXTENSIONS.isEmpty() == false) {
for (ParserHandler parser : PARSER_EXTENSIONS) {
if (parser.parse(iterator, fieldName, fieldNode, params, parserContext.indexVersionCreated())) {
parsedExtension = true;
break;
}
}
}
}
if (parsedDeprecatedParameters == false) {
Expand All @@ -268,6 +279,12 @@ public Mapper.Builder parse(String name, Map<String, Object> node, ParserContext
builder.orientation((Orientation)params.get(Names.ORIENTATION.getPreferredName()));
}

if (parsedExtension) {
for (ParserHandler parser : PARSER_EXTENSIONS) {
parser.config(params, builder);
}
}

return builder;
}
}
Expand Down Expand Up @@ -399,6 +416,9 @@ public void doXContentBody(XContentBuilder builder, boolean includeDefaults, Par
if (includeDefaults || ignoreZValue.explicit()) {
builder.field(GeoPointFieldMapper.Names.IGNORE_Z_VALUE.getPreferredName(), ignoreZValue.value());
}
if (indexCreatedVersion.onOrAfter(Version.V_8_0_0)) {
doXContentDocValues(builder, includeDefaults);
}
}

public Explicit<Boolean> coerce() {
Expand Down Expand Up @@ -440,6 +460,9 @@ public void parse(ParseContext context) throws IOException {
for (IndexableField field : fields) {
context.doc().add(field);
}
if (fieldType.hasDocValues()) {
geometryIndexer.indexDocValues(context, shape);
}
} catch (Exception e) {
if (ignoreMalformed.value() == false) {
throw new MapperParsingException("failed to parse field [{}] of type [{}]", e, fieldType().name(),
Expand All @@ -449,4 +472,19 @@ public void parse(ParseContext context) throws IOException {
}
}

public static void registerParserExtensions(List<ParserHandler> parserExtensions) {
PARSER_EXTENSIONS.addAll(parserExtensions);
}

public interface ParserHandler {
boolean parse(Iterator<Map.Entry<String, Object>> iterator, String name, Object node, Map<String, Object> params,
Version indexCreatedVersion);
@SuppressWarnings("rawtypes")
void config(Map<String, Object> params, Builder builder);
}

public interface ParserExtension {
List<ParserHandler> getParserExtensions();
}

}
70 changes: 67 additions & 3 deletions server/src/main/java/org/elasticsearch/index/mapper/GeoShapeFieldMapper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,13 +19,17 @@
package org.elasticsearch.index.mapper;

import org.apache.lucene.document.LatLonShape;
import org.elasticsearch.Version;
import org.elasticsearch.common.Explicit;
import org.elasticsearch.common.geo.GeometryParser;
import org.elasticsearch.common.geo.builders.ShapeBuilder;
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.geometry.Geometry;
import org.elasticsearch.index.query.VectorGeoShapeQueryProcessor;

import java.util.HashMap;
import java.util.Map;

/**
* FieldMapper for indexing {@link LatLonShape}s.
* <p>
Expand All @@ -49,9 +53,18 @@
public class GeoShapeFieldMapper extends AbstractGeometryFieldMapper<Geometry, Geometry> {
public static final String CONTENT_TYPE = "geo_shape";

public static Map<String, DataHandlerFactory> DATA_HANDLER_FACTORIES = new HashMap<>();

public static class Defaults extends AbstractGeometryFieldMapper.Defaults {
public static final Explicit<String> DATA_HANDLER = new Explicit<>("default", false);
}

public static class Builder extends AbstractGeometryFieldMapper.Builder<AbstractGeometryFieldMapper.Builder, GeoShapeFieldMapper> {
DataHandler dataHandler;

public Builder(String name) {
super (name, new GeoShapeFieldType(), new GeoShapeFieldType());
this.dataHandler = resolveDataHandler(Defaults.DATA_HANDLER.value());
}

@Override
Expand All @@ -61,18 +74,24 @@ public GeoShapeFieldMapper build(BuilderContext context) {
ignoreZValue(), context.indexSettings(), multiFieldsBuilder.build(this, context), copyTo);
}

@Override
protected boolean defaultDocValues(Version indexCreated) {
return dataHandler.defaultDocValues(indexCreated);
}

@Override
protected void setupFieldType(BuilderContext context) {
super.setupFieldType(context);

GeoShapeFieldType fieldType = (GeoShapeFieldType)fieldType();
boolean orientation = fieldType.orientation == ShapeBuilder.Orientation.RIGHT;

// @todo the GeometryParser can be static since it doesn't hold state?
GeometryParser geometryParser = new GeometryParser(orientation, coerce(context).value(), ignoreZValue().value());

fieldType.setGeometryIndexer(new GeoShapeIndexer(orientation, fieldType.name()));
fieldType.setGeometryParser( (parser, mapper) -> geometryParser.parse(parser));
fieldType.setGeometryQueryBuilder(new VectorGeoShapeQueryProcessor());

fieldType.setGeometryIndexer(dataHandler.newIndexer(orientation, fieldType));
fieldType.setGeometryQueryBuilder(dataHandler.newQueryProcessor());
}
}

Expand Down Expand Up @@ -124,4 +143,49 @@ public GeoShapeFieldType fieldType() {
protected String contentType() {
return CONTENT_TYPE;
}

public static void registerDataHandlers(Map<String, DataHandlerFactory> dataHandlerFactories) {
DATA_HANDLER_FACTORIES.putAll(dataHandlerFactories);
}

public static DataHandler resolveDataHandler(String handlerKey) {
if (DATA_HANDLER_FACTORIES.containsKey(handlerKey)) {
return DATA_HANDLER_FACTORIES.get(handlerKey).newDataHandler();
}
throw new IllegalArgumentException("dataHandler [" + handlerKey + "] not supported");
}

public interface DataHandlerFactory {
DataHandler newDataHandler();
}

public abstract static class DataHandler {
public abstract Indexer newIndexer(boolean orientation, MappedFieldType fieldType);
public abstract QueryProcessor newQueryProcessor();
public abstract boolean defaultDocValues(Version indexCreatedVersion);
}

static {
DATA_HANDLER_FACTORIES.put(Defaults.DATA_HANDLER.value(), () ->
new DataHandler() {
@Override
public boolean defaultDocValues(Version indexCreatedVersion) {
return false;
}

@Override
public Indexer newIndexer(boolean orientation, MappedFieldType fieldType) {
return new GeoShapeIndexer(orientation, fieldType.name());
}

@Override
public QueryProcessor newQueryProcessor() {
return new VectorGeoShapeQueryProcessor();
}
});
}

public interface Extension {
Map<String, DataHandlerFactory> getDataHandlerFactories();
}
}
7 changes: 6 additions & 1 deletion server/src/main/java/org/elasticsearch/index/mapper/GeoShapeIndexer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@
/**
* Utility class that converts geometries into Lucene-compatible form for indexing in a geo_shape field.
*/
public final class GeoShapeIndexer implements AbstractGeometryFieldMapper.Indexer<Geometry, Geometry> {
public class GeoShapeIndexer implements AbstractGeometryFieldMapper.Indexer<Geometry, Geometry> {

private final boolean orientation;
private final String name;
Expand Down Expand Up @@ -185,6 +185,11 @@ public List<IndexableField> indexShape(ParseContext context, Geometry shape) {
return visitor.fields();
}

@Override
public void indexDocValues(ParseContext context, Geometry shape) {
throw new UnsupportedOperationException("DocValues are not supported");
}

private static class LuceneGeometryIndexer implements GeometryVisitor<Void, RuntimeException> {
private List<IndexableField> fields = new ArrayList<>();
private String name;
Expand Down
5 changes: 5 additions & 0 deletions server/src/main/java/org/elasticsearch/index/mapper/LegacyGeoShapeFieldMapper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -264,6 +264,11 @@ private void setupPrefixTrees() {
protected void setupFieldType(BuilderContext context) {
super.setupFieldType(context);

if (context.indexCreatedVersion().onOrAfter(Version.V_8_0_0) && fieldType().hasDocValues()) {
throw new ElasticsearchParseException("Field parameter [{}] is not supported for [{}] field type while using prefix trees",
TypeParsers.DOC_VALUES, CONTENT_TYPE);
}

fieldType().setGeometryIndexer(new LegacyGeoShapeIndexer(fieldType()));
fieldType().setGeometryParser(ShapeParser::parse);
fieldType().setGeometryQueryBuilder(new LegacyGeoShapeQueryProcessor(fieldType()));
Expand Down
5 changes: 5 additions & 0 deletions server/src/main/java/org/elasticsearch/index/mapper/LegacyGeoShapeIndexer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,4 +69,9 @@ public List<IndexableField> indexShape(ParseContext context, Shape shape) {
}
return Arrays.asList(fieldType.defaultPrefixTreeStrategy().createIndexableFields(shape));
}

@Override
public void indexDocValues(ParseContext context, Shape shape) {
throw new UnsupportedOperationException("[" + fieldType.name() + "] does not support doc_values");
}
}
54 changes: 35 additions & 19 deletions server/src/main/java/org/elasticsearch/indices/IndicesModule.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@
import org.elasticsearch.indices.mapper.MapperRegistry;
import org.elasticsearch.indices.store.IndicesStore;
import org.elasticsearch.plugins.MapperPlugin;
import org.elasticsearch.plugins.Plugin;

import java.util.Arrays;
import java.util.Collection;
Expand All @@ -73,6 +74,7 @@
import java.util.List;
import java.util.Map;
import java.util.Optional;
import java.util.ServiceLoader;
import java.util.Set;
import java.util.function.Function;
import java.util.function.Predicate;
Expand All @@ -83,9 +85,9 @@
public class IndicesModule extends AbstractModule {
private final MapperRegistry mapperRegistry;

public IndicesModule(List<MapperPlugin> mapperPlugins) {
this.mapperRegistry = new MapperRegistry(getMappers(mapperPlugins), getMetadataMappers(mapperPlugins),
getFieldFilter(mapperPlugins));
public IndicesModule(List<Plugin> plugins) {
this.mapperRegistry = new MapperRegistry(getMappers(plugins), getMetadataMappers(plugins),
getFieldFilter(plugins));
}

public static List<NamedWriteableRegistry.Entry> getNamedWriteables() {
Expand All @@ -106,7 +108,7 @@ public static List<NamedXContentRegistry.Entry> getNamedXContents() {
);
}

public static Map<String, Mapper.TypeParser> getMappers(List<MapperPlugin> mapperPlugins) {
public static Map<String, Mapper.TypeParser> getMappers(List<Plugin> plugins) {
Map<String, Mapper.TypeParser> mappers = new LinkedHashMap<>();

// builtin mappers
Expand All @@ -132,10 +134,20 @@ public static Map<String, Mapper.TypeParser> getMappers(List<MapperPlugin> mappe
mappers.put(GeoPointFieldMapper.CONTENT_TYPE, new GeoPointFieldMapper.TypeParser());
mappers.put(GeoShapeFieldMapper.CONTENT_TYPE, new AbstractGeometryFieldMapper.TypeParser());

for (MapperPlugin mapperPlugin : mapperPlugins) {
for (Map.Entry<String, Mapper.TypeParser> entry : mapperPlugin.getMappers().entrySet()) {
if (mappers.put(entry.getKey(), entry.getValue()) != null) {
throw new IllegalArgumentException("Mapper [" + entry.getKey() + "] is already registered");
for (Plugin plugin : plugins) {
for (GeoShapeFieldMapper.Extension geoExtension : ServiceLoader.load(GeoShapeFieldMapper.Extension.class,
plugin.getClass().getClassLoader())) {
GeoShapeFieldMapper.registerDataHandlers(geoExtension.getDataHandlerFactories());
}
for (AbstractGeometryFieldMapper.ParserExtension geoParserExtension :
ServiceLoader.load(AbstractGeometryFieldMapper.ParserExtension.class, plugin.getClass().getClassLoader())) {
GeoShapeFieldMapper.registerParserExtensions(geoParserExtension.getParserExtensions());
}
if (plugin instanceof MapperPlugin) {
for (Map.Entry<String, Mapper.TypeParser> entry : ((MapperPlugin)plugin).getMappers().entrySet()) {
if (mappers.put(entry.getKey(), entry.getValue()) != null) {
throw new IllegalArgumentException("Mapper [" + entry.getKey() + "] is already registered");
}
}
}
}
Expand Down Expand Up @@ -165,7 +177,7 @@ private static Map<String, MetadataFieldMapper.TypeParser> initBuiltInMetadataMa
return Collections.unmodifiableMap(builtInMetadataMappers);
}

public static Map<String, MetadataFieldMapper.TypeParser> getMetadataMappers(List<MapperPlugin> mapperPlugins) {
public static Map<String, MetadataFieldMapper.TypeParser> getMetadataMappers(List<Plugin> plugins) {
Map<String, MetadataFieldMapper.TypeParser> metadataMappers = new LinkedHashMap<>();

int i = 0;
Expand All @@ -181,13 +193,15 @@ public static Map<String, MetadataFieldMapper.TypeParser> getMetadataMappers(Lis
}
assert fieldNamesEntry != null;

for (MapperPlugin mapperPlugin : mapperPlugins) {
for (Map.Entry<String, MetadataFieldMapper.TypeParser> entry : mapperPlugin.getMetadataMappers().entrySet()) {
if (entry.getKey().equals(FieldNamesFieldMapper.NAME)) {
throw new IllegalArgumentException("Plugin cannot contain metadata mapper [" + FieldNamesFieldMapper.NAME + "]");
}
if (metadataMappers.put(entry.getKey(), entry.getValue()) != null) {
throw new IllegalArgumentException("MetadataFieldMapper [" + entry.getKey() + "] is already registered");
for (Plugin plugin : plugins) {
if (plugin instanceof MapperPlugin) {
for (Map.Entry<String, MetadataFieldMapper.TypeParser> entry : ((MapperPlugin) plugin).getMetadataMappers().entrySet()) {
if (entry.getKey().equals(FieldNamesFieldMapper.NAME)) {
throw new IllegalArgumentException("Plugin cannot contain metadata mapper [" + FieldNamesFieldMapper.NAME + "]");
}
if (metadataMappers.put(entry.getKey(), entry.getValue()) != null) {
throw new IllegalArgumentException("MetadataFieldMapper [" + entry.getKey() + "] is already registered");
}
}
}
}
Expand All @@ -204,10 +218,12 @@ public static Set<String> getBuiltInMetaDataFields() {
return builtInMetadataMappers.keySet();
}

private static Function<String, Predicate<String>> getFieldFilter(List<MapperPlugin> mapperPlugins) {
private static Function<String, Predicate<String>> getFieldFilter(List<Plugin> plugins) {
Function<String, Predicate<String>> fieldFilter = MapperPlugin.NOOP_FIELD_FILTER;
for (MapperPlugin mapperPlugin : mapperPlugins) {
fieldFilter = and(fieldFilter, mapperPlugin.getFieldFilter());
for (Plugin plugin : plugins) {
if (plugin instanceof MapperPlugin) {
fieldFilter = and(fieldFilter, ((MapperPlugin)plugin).getFieldFilter());
}
}
return fieldFilter;
}
Expand Down
3 changes: 1 addition & 2 deletions server/src/main/java/org/elasticsearch/node/Node.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,7 +128,6 @@
import org.elasticsearch.plugins.EnginePlugin;
import org.elasticsearch.plugins.IndexStorePlugin;
import org.elasticsearch.plugins.IngestPlugin;
import org.elasticsearch.plugins.MapperPlugin;
import org.elasticsearch.plugins.MetaDataUpgrader;
import org.elasticsearch.plugins.NetworkPlugin;
import org.elasticsearch.plugins.PersistentTaskPlugin;
Expand Down Expand Up @@ -384,7 +383,7 @@ protected Node(final Environment initialEnvironment,
final MonitorService monitorService = new MonitorService(settings, nodeEnvironment, threadPool, clusterInfoService);
ClusterModule clusterModule = new ClusterModule(settings, clusterService, clusterPlugins, clusterInfoService);
modules.add(clusterModule);
IndicesModule indicesModule = new IndicesModule(pluginsService.filterPlugins(MapperPlugin.class));
IndicesModule indicesModule = new IndicesModule(pluginsService.filterPlugins(Plugin.class));
modules.add(indicesModule);

SearchModule searchModule = new SearchModule(settings, pluginsService.filterPlugins(SearchPlugin.class));
Expand Down
Loading