Aklakan commented on code in PR #3480:
URL: https://github.com/apache/jena/pull/3480#discussion_r2396283778
##########
jena-geosparql/src/main/java/org/apache/jena/geosparql/geo/topological/GenericPropertyFunction.java:
##########
@@ -189,18 +183,7 @@ private QueryIterator findAll(Graph graph, Node boundNode,
Node unboundNode, Bin
}
private static ExtendedIterator<Triple> findSpatialTriples(Graph graph) {
- ExtendedIterator<Triple> spatialTriples;
- if (graph.contains(null, RDF.type.asNode(), Geo.SPATIAL_OBJECT_NODE)) {
- spatialTriples = graph.find(null, RDF.type.asNode(),
Geo.SPATIAL_OBJECT_NODE);
- } else if (graph.contains(null, RDF.type.asNode(), Geo.FEATURE_NODE)
|| graph.contains(null, RDF.type.asNode(), Geo.GEOMETRY_NODE)) {
- ExtendedIterator<Triple> featureTriples = graph.find(null,
RDF.type.asNode(), Geo.FEATURE_NODE);
- ExtendedIterator<Triple> geometryTriples = graph.find(null,
RDF.type.asNode(), Geo.GEOMETRY_NODE);
- spatialTriples = featureTriples.andThen(geometryTriples);
- } else {
- //Check for Geo Predicate Features in the Graph if no
GeometryLiterals found.
- spatialTriples = graph.find(null, SpatialExtension.GEO_LAT_NODE,
null);
- }
- return spatialTriples;
+ return SpatialObjectAccess.findSpatialTriplesByProperties(graph, null);
}
Review Comment:
TLDR: Probably the lookup strategy should become a config option for the
time being.
The code above is quite some opinionated/reasoning-centric logic - e.g. the
presence of a single `foo a SpatialObject` triple would mean that all
non-SpatialObject-resources of e.g. type `Geometry` would be ignored. Also, as
reported in the issue, geometry resources with e.g. a WKT serialization but
without the `Geometry` type are also ignored here.
My alternative implementation concats several iterators and adds filters
that drop duplicates, but for fully materialized datasets this is
performance-wise also far from ideal. Merge-scans of sorted iterators would
help greatly here to efficiently iterate large amounts of resources that have
co-occurrent properties, such as `geo:hasDefaultGeometry` and `geo:hasGeometry`
- but we don't have them. AFAIK the TDB2 internals would be capable of this but
not the general Graph API.
The `DatasetGraphRDFS` wrapper implementation doesn't solve this. In an
ideal world, data access could go uniformly through an (virtual) RDFS reasoning
layer - i.e. in the code above `?s a geo:SpatialObject` would be sufficient,
and the details would be handled by other layers.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
