Skip to content

Add null_value support to geo_point type#29451

Merged
imotov merged 2 commits intoelastic:masterfrom
imotov:issue-12998-support-null-value-for-geopoints-reuse-parser
Apr 17, 2018
Merged

Add null_value support to geo_point type#29451
imotov merged 2 commits intoelastic:masterfrom
imotov:issue-12998-support-null-value-for-geopoints-reuse-parser

Conversation

@imotov
Copy link
Copy Markdown
Contributor

@imotov imotov commented Apr 10, 2018

Adds support for null_value attribute to the geo_point types.

Closes #12998

Adds support for null_value attribute to the geo_point types.

Closes elastic#12998
@imotov imotov added >enhancement :Analytics/Geo Indexing, search aggregations of geo points and shapes v7.0.0 v6.3.0 labels Apr 10, 2018
@imotov imotov requested review from jpountz and nknize April 10, 2018 15:04
@elasticmachine
Copy link
Copy Markdown
Collaborator

Pinging @elastic/es-search-aggs

@jpountz
Copy link
Copy Markdown
Contributor

jpountz commented Apr 10, 2018

I'd like to understand what the use-case is for such a feature. I can understand how a default value equal to "n/a" can help on keyword fields, but I can't think of a use-case for a default location.

@imotov
Copy link
Copy Markdown
Contributor Author

imotov commented Apr 10, 2018

@kurtado, since you opened the original issue, could you comment on it?

@kurtado
Copy link
Copy Markdown
Contributor

kurtado commented Apr 10, 2018

Sure, from what I recall, we had a customer in training who wished to set a default, exactly like this. I can't remember the exact use case, but a null_value works fine.

@imotov
Copy link
Copy Markdown
Contributor Author

imotov commented Apr 10, 2018

While reading the original discussion I was imagining a scenario of some large central location and satellite offices where most of the documents correspond to the main location but occasionally may contain alternative locations. So, I think in this scenario assigning them to the central location by default would make sense.

Copy link
Copy Markdown
Contributor

@jpountz jpountz left a comment

Choose a reason for hiding this comment

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

Fair enough. I doubt this will be used much, but at least this is consistent with other fields.

public static GeoPoint parseGeoPoint(Object value, final boolean ignoreZValue) throws ElasticsearchParseException {
try {
XContentBuilder content = JsonXContent.contentBuilder();
content.value(value);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should we put the value under a field in a top-level object? My reasoning is that I wouldn't be surprised that we start failing producing malformed json documents at some point, which would be the case here if you give a top-level string which is not enclosed in an object?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes, I was thinking about it as well. Why the current solution is a bit faster it indeed feels somewhat hacky. I will add a top level object.

@imotov imotov merged commit 983d6c1 into elastic:master Apr 17, 2018
imotov added a commit that referenced this pull request Apr 17, 2018
Adds support for null_value attribute to the geo_point types.

Closes #12998
dnhatn added a commit that referenced this pull request Apr 18, 2018
* master:
  Remove the index thread pool (#29556)
  Remove extra copy in ScriptDocValues.Strings
  Fix full cluster restart test recovery (#29545)
  Fix binary doc values fetching in _search (#29567)
  Mutes failing MovAvgIT tests
  Fix the assertion message for an incorrect current version. (#29572)
  Fix the version ID for v5.6.10. (#29570)
  Painless Spec Documentation Clean Up (#29441)
  Add versions 5.6.10 and 6.2.5
  [TEST] test against scaled value instead of fixed epsilon in MovAvgIT
  Remove `flatSettings` support from request classes (#29560)
  MapperService to wrap a single DocumentMapper. (#29511)
  Fix dependency checks on libs when generating Eclipse configuration. (#29550)
  Add null_value support to geo_point type (#29451)
  Add documentation about the include_type_name option. (#29555)
  Enforce translog access via engine (#29542)
@imotov imotov deleted the issue-12998-support-null-value-for-geopoints-reuse-parser branch May 1, 2020 22:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

:Analytics/Geo Indexing, search aggregations of geo points and shapes >enhancement v6.3.0 v7.0.0-beta1

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants