add doc_values mapping option to geo_shape/shape field mappers
#53351
Conversation
This PR adds support for the `doc_values` field mapping parameter. `false` is currently the only supported value to explicitly set, and is also the default.
talevy
added
>non-issue
:Analytics/Geo
|
Pinging @elastic/es-analytics-geo (:Analytics/Geo) |
|
Pinging @elastic/es-search (:Search/Mapping) |
| @@ -352,15 +373,17 @@ public QueryProcessor geometryQueryBuilder() { | |||
| protected Explicit<Boolean> coerce; | |||
| protected Explicit<Boolean> ignoreMalformed; | |||
| protected Explicit<Boolean> ignoreZValue; | |||
| protected Explicit<Boolean> docValues; | |||
There was a problem hiding this comment.
This one should be on the fieldType since it also affects queries and aggs. Only parameters that only affect indexing may be stored on the mapper.
There was a problem hiding this comment.
hmmm. I suppose that was not intuitive to me. I found the relationship between the field-mapper's Explicit values, the docValuesSet, and the FieldType's hasDocValues all to compete in responsibility in some ways.
I think I see what you mean now, I will try and update to reflect this. thanks!
| protected Explicit<Boolean> docValues() { | ||
| // TODO(talevy): see how to best make this pluggable with the DataHandler work | ||
| // although these values can be true, an ElasticsearchParseException | ||
| // prevents this Explicit(true,true) path to ever occur in practice |
There was a problem hiding this comment.
One possibility would be to only enable doc values by default if there is a registered DataHandler.
| @@ -399,6 +425,9 @@ public void doXContentBody(XContentBuilder builder, boolean includeDefaults, Par | |||
| if (includeDefaults || ignoreZValue.explicit()) { | |||
| builder.field(GeoPointFieldMapper.Names.IGNORE_Z_VALUE.getPreferredName(), ignoreZValue.value()); | |||
| } | |||
| if (includeDefaults || docValues.explicit()) { | |||
There was a problem hiding this comment.
Hi @jpountz, in a follow-up commit I've removed the Explicit docValues field from the Mapper, but without it, this conditional is difficult to achieve (unless I'm missing something). It is difficult to check whether the value was set explicitly without access to docValuesSet which is in the field mapper's builder.
not sure this is a big deal, but it does feel like the doc values field should only be serialized in this scenaior
|
After speaking with Ryan in a video-conference, I now understand the intension of defaultDocValues and the For that reason, and the reason that this setting is likely only ever allowed to be |
This PR adds support for the
doc_valuesfield mapping parameter.falseis currently the only supported value to explicitlyset, and is also the default.