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.

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

Add feature processor to convert geojson feature to geoshape field #15

Merged
merged 2 commits into from
Jan 18, 2022

Conversation

VijayanB
Copy link
Member

@VijayanB VijayanB commented Jan 7, 2022

Description

Added new Processor of type geojson-feature to intercept GeoJSON Feature and convert into typical OpenSearch document.
Example: A GeoJSON object of type “Feature”, contains geometry object of type “LineString”.

{
    "type": "Feature",
    "geometry": {
        "type": "LineString",
        "coordinates": [
            [102.0, 0.0],
            [103.0, 1.0],
            [104.0, 0.0],
            [105.0, 1.0]
        ]
    },
    "properties": {
        "key0": "value0",
        "key1": "value1",
        "key2": "value2"
    }
}

The above feature can be converted to typical document for an index which has "location" as type geo_shape, by FeatureProcessor.

{
    "location": {
        "type": "LineString",
        "coordinates": [
            [102.0, 0.0],
            [103.0, 1.0],
            [104.0, 0.0],
            [105.0, 1.0]
        ]
    },
     "key0": "value0",
     "key1": "value1",
     "key2": "value2"
}

This processor converts Geometry Object into geo_shape field value, removes GeoJSON metadata like type, and convert properties into direct fields.

Check List

  • Commits are signed per the DCO using --signoff

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.

@VijayanB VijayanB requested a review from a team January 7, 2022 02:43
@VijayanB VijayanB changed the title Add processor Add GeoJSONFeature Processor Jan 7, 2022
@VijayanB VijayanB force-pushed the add-processor branch 2 times, most recently from f89d8e7 to 415380e Compare January 7, 2022 02:44
@VijayanB VijayanB self-assigned this Jan 7, 2022
@VijayanB VijayanB force-pushed the add-processor branch 14 times, most recently from 796a1a1 to 5de6691 Compare January 15, 2022 06:55
Copy link
Member

@jmazanec15 jmazanec15 left a comment

Choose a reason for hiding this comment

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

Can you add more detail to the description?

public class Feature {
public static final String TYPE = "Feature";
private final Map<String, Object> geometry;
private Map<String, Object> properties = new HashMap<>();
Copy link
Member

Choose a reason for hiding this comment

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

Why create this object here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Moved it to constructor


import java.util.Map;

public class GeoJSONFactory {
Copy link
Member

Choose a reason for hiding this comment

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

Should this be FeatureFactory if it produces a Feature?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ack

}

private static Feature readFeature(Map<String, Object> input) {
Map<String, Object> geometryMap = (Map<String, Object>) input.get(GEOJSON_GEOMETRY_KEY);
Copy link
Member

Choose a reason for hiding this comment

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

Does this need to be validated?

Copy link
Member Author

Choose a reason for hiding this comment

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

updated


@Override
public IngestDocument execute(IngestDocument ingestDocument) {
Feature feature = GeoJSONFactory.create(ingestDocument.getSourceAndMetadata());
Copy link
Member

Choose a reason for hiding this comment

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

Can you add comments here explaining what is being done?

Copy link
Member Author

Choose a reason for hiding this comment

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

updated.

@VijayanB VijayanB force-pushed the add-processor branch 5 times, most recently from 032d441 to a0727b7 Compare January 18, 2022 19:45
*/
static class FeatureBuilder {

private Feature feature;
Copy link
Member

Choose a reason for hiding this comment

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

can we make it final?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ack

* @return Feature Instance from input
* @throws IllegalArgumentException if invalid input is provided
*/
public static Feature create(Map<String, Object> input) throws IllegalArgumentException {
Copy link
Member

Choose a reason for hiding this comment

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

Why IllegalArgumentException is defined in the method signature, it's unchecked exception?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ack

if (!(geometry instanceof Map)) {
throw new IllegalArgumentException("key: " + Feature.GEOMETRY_KEY + "is not an instance of type Map");
}
Map<String, Object> geometryMap = (Map<String, Object>) geometry;
Copy link
Member

Choose a reason for hiding this comment

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

nit: are we sure this is map of <String, Object>? Although I'm sure how to check that properly, effectively you're getting Map<Object,Object>

Copy link
Member Author

Choose a reason for hiding this comment

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

created new method to convert

@VijayanB VijayanB force-pushed the add-processor branch 3 times, most recently from 0ee11a5 to 273a13d Compare January 18, 2022 21:26
@VijayanB VijayanB force-pushed the add-processor branch 2 times, most recently from 70b9b6a to c2b99b8 Compare January 18, 2022 21:39
@VijayanB VijayanB changed the title Add GeoJSONFeature Processor Add Feature Processor to convert geojson feature to geoshape field Jan 18, 2022
@VijayanB VijayanB changed the title Add Feature Processor to convert geojson feature to geoshape field Add feature processor to convert geojson feature to geoshape field Jan 18, 2022
@VijayanB VijayanB force-pushed the add-processor branch 2 times, most recently from 5c808f8 to 54c7553 Compare January 18, 2022 21:53
Copy link
Member

@martin-gaievski martin-gaievski left a comment

Choose a reason for hiding this comment

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

Lgtm, thank you

*
* @return Feature's type value
*/
public String getType() {
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need a getter on a public static type?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch. I forgot to remove this method after i made it as public static constant.

Map<String, Object> geometryMap = toStringObjectMap(geometry);
FeatureBuilder featureBuilder = new FeatureBuilder(geometryMap);
Object properties = input.get(Feature.PROPERTIES_KEY);
if (properties == null)
Copy link
Member

Choose a reason for hiding this comment

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

nit: Why not wrap this in curly braces?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ack

if (properties == null)
return featureBuilder;

if (!(geometry instanceof Map)) {
Copy link
Member

Choose a reason for hiding this comment

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

I think this is supposed to be properties

Copy link
Member Author

Choose a reason for hiding this comment

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

copy paste from previous check :( I corrected it now.

Added new Processor of type geojson-feature.

Signed-off-by: Vijayan Balasubramanian <balasvij@amazon.com>
Signed-off-by: Vijayan Balasubramanian <balasvij@amazon.com>
@VijayanB VijayanB merged commit c9efedb into opensearch-project:main Jan 18, 2022
@VijayanB VijayanB deleted the add-processor branch January 18, 2022 23:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants