feature: support simple filtering operation#209
feature: support simple filtering operation#209Abolfazlrezaei93 wants to merge 7 commits intodevelopfrom
Conversation
7e20fab to
df579db
Compare
|
Great description and contribution, thanks @Abolfazlrezaei93 ! The problem with large files and the |
|
I like how you validate the parameters first with PS. I would only work for data on Earth ;-) |
There was a problem hiding this comment.
Here you provided a Unit Test. Unless you don't want any user to run it through an HTTP request, it's ok. But since you'll finally merge it into the project, I suggest you update the test into an integration test.
The issue: you instantiate and call the operation directly final GeoJsonFilterOperation operation = new GeoJsonFilterOperation(); final ArrayList<String> output = operation.apply(input);
insteal of wiring the producer's (CKANProducer here, according to your operation and queries) output through the SequentialRunner into the operation input as it is done in all the integration tests. This will provide a real query payload with CKANProducer as the data source and GeoJsonOperation as the operation to apply, and this is consistent with how the other operation is tested.
You can get inspired by MergeOperationTest.java
There was a problem hiding this comment.
Thanks for the comment and your review. I have changed this to an integration test with AI's help
There was a problem hiding this comment.
Also, I disabled testFilterDhnBuildingsByPointRadius, the buildings CKAN dataset is too large for the CI heap.
I switched testMissingRadiusParameterReturnsError to use the roads payload instead of buildings: the roads dataset fetches successfully within the heap limits, then the operation fails fast with IllegalArgumentException when radiusMeters is missing.
MinetteMeyo
left a comment
There was a problem hiding this comment.
Great job, please consider Yann's comment to your code first, and update the test.
Globally, the code is fine, nothing wrong. You followed the same logic we are using in the codebase, which is great, and your PR could be merge as it is. However, I suggest you make the change your unit test to make it an integration test before I approve the PR.
df579db to
1f6e1b3
Compare
|
Thank you @yann-gael for reviewing the PR. Regarding the deepCopy, Yes, it was unnecessary to deep copy the entire inputObject because the features array inside it gets replaced immediately after. Changed to a selective copy: iterate over all entries of inputObject, skip "features", then add the filteredFeatures. This avoids allocating and then discarding a potentially large copy of the original features array. Regarding null, I used a NullObject sentinel on Coordinate instead. And indeed it only works on earth :) |
d22e10f to
73e5aa9
Compare
73e5aa9 to
4be83d2
Compare
🚀 feature: support simple filtering operation
Add
GeoJsonFilterOperationfor point-radius filtering of GeoJSON features + tests + DHN examples📌 Summary
This PR adds a new CITYData operation to filter GeoJSON features by centroid distance from a target point (
longitude,latitude) within a radius (meters). It also adds tests and runnable example queries for DHN roads/buildings.🎯 Why
Current filtering is mostly string/JSON-key based.
For spatial workflows (roads/buildings), we needed a geospatial filter over GeoJSON
FeatureCollectiondata.✅ What Was Added
src/main/java/ca/concordia/encs/citydata/operations/GeoJsonFilterOperation.javasrc/test/java/ca/concordia/encs/citydata/operations/GeoJsonFilterOperationTest.java/operations/listassertions):src/test/java/ca/concordia/encs/citydata/core/DiscoveryRoutesTest.javadocs/examples/queries/dhnRoadsGeoJsonFilterByPointRadius.jsondocs/examples/queries/dhnBuildingsGeoJsonFilterByPointRadius.json⚙️ How
GeoJsonFilterOperationWorksArrayList<String>(compatible withCKANProduceroutput)FeatureCollection, iterates features<= radiusMetersGeometryCollection🧩 New Parameters
centerLongitude(Double, range[-180, 180])centerLatitude(Double, range[-90, 90])radiusMeters(Double, must be> 0)🔄 Intended Workflow
CKANMetadataProducerto findresourceIdCKANProducerto fetch resource contentGeoJsonFilterOperationwith point + radius📝 Example Query Shape
{ "use": "ca.concordia.encs.citydata.producers.CKANProducer", "withParams": [ {"name":"url","value":"https://ngci.encs.concordia.ca/ckan/api/3"}, {"name":"resourceId","value":"<resource-id>"} ], "apply": [ { "name":"ca.concordia.encs.citydata.operations.GeoJsonFilterOperation", "withParams": [ {"name":"centerLongitude","value":-73.74085}, {"name":"centerLatitude","value":45.51895}, {"name":"radiusMeters","value":200.0} ] } ] }🧪 Tests Added (Coverage)
testFilterByRadiusUsingFeatureCentroidstestMissingParametersThrowsErrorIllegalArgumentExceptionDiscoveryRoutesTest/operations/listincludes:GeoJsonFilterOperationcenterLongitudecenterLatituderadiusMetersCKANProducerflow due to memory-heavy fetching.🧱 Commit Breakdown
f1432a8—feat(operations): add GeoJsonFilterOperation for centroid radius filtering9599bf1—test(operations): add GeoJsonFilterOperation tests and discovery assertions6a1e9d1—docs(queries): add DHN point-radius GeoJSON filter examples