Support other geopoint formats #144
Conversation
| * SPDX-License-Identifier: Apache-2.0 | ||
| */ | ||
|
|
||
| const geoJSONTypes = [ |
There was a problem hiding this comment.
| const geoJSONTypes = [ | |
| const geoJSONTypes: string[] = [ |
| 'geometrycollection', | ||
| ]; | ||
|
|
||
| export function isGeoJSON(value: any) { |
There was a problem hiding this comment.
How about change the value type to value: unknown or to a more specific type, like value: object?
220e615 to
0a34e04
Compare
| coordinates: location.coordinates, | ||
| }; | ||
| // Geopoint as an array | ||
| if (Array.isArray(location) && location.length === 2) { |
There was a problem hiding this comment.
Couldn't the length be 3?
There was a problem hiding this comment.
Good point. Let me check
There was a problem hiding this comment.
updated to include 2/3, but, we will skip 3rd value
| // Geopoint as a string | ||
| if (typeof location === 'string') { | ||
| const values = location.trim().split(','); | ||
| if (values && values.length === 2) { |
There was a problem hiding this comment.
Same here. The length could be 3?
| coordinates: location.coordinates, | ||
| }; | ||
| } | ||
| if (fieldType !== 'geo_point') { |
There was a problem hiding this comment.
Maybe, better to clear out the invalid case on top?
const buildGeometry = (fieldType: string, location: any) => {
if (fileType !== 'geo_point' && !isGeoJSON(location)) {
return undefined;
}
// Rest of the logics
There was a problem hiding this comment.
refactored in such a way, we can extend the logic to geo_shape later.
| if (geometry) { | ||
| const feature = { | ||
| geometry, | ||
| properties: buildProperties(item, layerConfig.source.tooltipFields), |
There was a problem hiding this comment.
comma is not necessary here I guess?
There was a problem hiding this comment.
Might be mandatory from ES-lint. I will check again.
There was a problem hiding this comment.
It is required to have comma at end by ES-lint
| } | ||
|
|
||
| export function isLonLatObject(value: any) { | ||
| if (value?.lat && value?.lon) { |
There was a problem hiding this comment.
Can be simplified like
| if (value?.lat && value?.lon) { | |
| return value?.lat && value?.lon; |
| } | ||
|
|
||
| export function wktPointToGeoJSON(value: any) { | ||
| if (typeof value === 'string') { |
There was a problem hiding this comment.
Better to reduce the level of nested blocks.
if (typeof value !== 'string') {
return undefined;
}
...
| ]; | ||
|
|
||
| export function isGeoJSON(value: { type: any; coordinates: any }) { | ||
| if (!value) return false; |
There was a problem hiding this comment.
can be simplified like
return value &&
value.type &&
value.coordinates &&
geoJSONTypes.includes(value.type.toLowerCase());
| return false; | ||
| } | ||
|
|
||
| export function isLonLatObject(value: any) { |
There was a problem hiding this comment.
| export function isLonLatObject(value: any) { | |
| export function isLonLatObject(value: any) { | |
| return value?.lat && value?.lon; | |
| } |
| } | ||
|
|
||
| export function wktPointToGeoJSON(value: any) { | ||
| if (typeof value === 'string') { |
There was a problem hiding this comment.
Can reduce nested block like
if (typeof value !== 'string') {
return undefined;
}
... more code ...
| export function wktPointToGeoJSON(value: any) { | ||
| if (typeof value === 'string') { | ||
| const removedType = value.trim().replace('POINT', ''); | ||
| const coordinatesExpression = RegExp('[(](?<longitude>.+) (?<latitude>.+)[)]'); |
There was a problem hiding this comment.
I don't think this will cover WKT point format comprehensively. It will even parse POINT (1a 3b).
There was a problem hiding this comment.
Good point. I was thinking about adding lat/lon check after extraction. However, this will be called after geo_point data is retrieved from OpenSearch. I believe users won't be able to save POINT( 1a 3b) to OpenSearch.
There was a problem hiding this comment.
Will not be supporting WKT for now.
WKT and Geohash is not supported yet. Signed-off-by: Vijayan Balasubramanian <balasvij@amazon.com>
0a34e04 to
ccd2e76
Compare
| return buildGeoJSONOfTypePoint(location[0], location[1]); | ||
| } | ||
|
|
||
| if (typeof location !== 'string') { |
There was a problem hiding this comment.
I think this code is highly related with the next few lines. By combining them, there will be no ordering among three condition checks which will enhance readability imo.
if (type of location === 'string') {
const values = location.trim().split(',');
if (values && (values.length === 2 || values.length === 3)) {
return buildGeoJSONOfTypePoint(parseFloat(values[1].trim()), parseFloat(values[0].trim()));
}
}
Description
Support Geopoint as
WKT and Geohash will be supported in later release.
Signed-off-by: Vijayan Balasubramanian balasvij@amazon.com
Issues Resolved
List any issues this PR will resolve, e.g. Closes [...].
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.