Support filtering EPT by OGR datasources #2771

hobu · 2019-10-01T16:45:53Z

Add an ogr option to readers.ept:

        "ogr": {
            "datasource":"/Users/hobu/dev/git/pdal/iowa/interstate-80.gpkg",
            "layer":"interstate-80-buffer",
            "sql":"select fid,geometry from \"freeway-exit\" ",
            "drivers":["GPKG"],
            "openoptions":["ADJUST_GEOM_TYPE=FIRST_SHAPE"],
            "options":{
                "geometry":"POLYGON ((-10147169.072127 5108605.30020218,-10147555.2667239 5107917.68543207,-10146792.2969105 5107267.74818361,-10146095.2627599 5107748.13658465,-10146462.618596 5108529.94515888,-10147169.072127 5108605.30020218))",
                "dialect":"OGRSQL"
            }
        },

If both layer and sql are provided, `sql is used.
If both bounds and sql are provided, they are intersected
drivers is a list of OGR driver shortnames to open the data source with (optional)
openoptions is a list of driver open options (optional)
options.geometry is a filter geometry (wkt or geojson) that is passed to the ExecuteSQL layer creation
dialect tells OGR which SQL dialect to use.

…readers.ept

…des.

…ilter-options

…ounds overlap a query but no contained points overlap.

…ilter-options

hobu · 2019-10-02T02:17:24Z

io/EptReader.cpp

+            Polygon p = pdal::Polygon(g, srs);
+            p.transform(getSpatialReference());
+            m_args->m_polys.emplace_back(p);
+        }


Need to delete the geometries in ogr_geoms.

abellgithub

I've tried to ignore all of Connor's EPT stuff.

abellgithub · 2019-10-02T12:06:59Z

io/EptReader.cpp

+        // convert whatever we were given for an OGR source to
+        // polygons and override m_args->m_polys
+        m_args->m_polys.clear();
+        std::vector<OGRGeometry*> ogr_geoms = pdal::gdal::fetchOGRGeometries(m_args->m_ogr);


I don't see any reason for us to pollute EPT with GDAL awfulness. Seems like this should be hidden in GDALUtils. I think all you do is end up converting to pdal::Polygon anyway.

ok. Then we must pollute pdal::Polygon with GDAL datasource reading.

abellgithub · 2019-10-02T12:09:17Z

io/EptReader.cpp

+        // Apply our overrides from the fetched layer by taking
+        // the intersection of the layer's bounds with whatever the
+        // user provided before
+        BOX3D filtered = m_args->m_bounds.to3d();


I think this is excessively complicated. One use of cropping seems sufficient, no? I think I would error if you specify both bounds and the OGR filtering. I'm not sure what extra you get out of using both at the same time.

removed. See latest

abellgithub · 2019-10-02T12:26:09Z

pdal/GDALUtils.cpp

+        for(const auto& s: driverOptions)
+            papszDriverOptions = CSLAddString(papszDriverOptions, s.c_str());
+    }
+    std::vector<const char*> openoptions{};


I'll suggest:

std::vector<std::string> openOpts; size_t optCount = ogr.count("openoptions"); std::vector<const char *> opts(optCount + 1); if (optCount) openOpts = ogr["openoptions"]; for (size_t i = 0; i < openOpts.size() - 1; ++i) opts[i] = openOpts[i].data();

You just have to make sure that openOpts stays in scope until you're done. The names I chose could be better.

The dataset code would look about the same. Guess you could write a lambda.

I did it the GDAL way so it was clear it was a GDAL thing, and if there were any weird DLL boundary issues with the array it was GDAL's fault and not ours.

There shouldn't be any DLL boundary issues because the memory is allocated/destroyed inside PDAL.

abellgithub · 2019-10-02T12:45:41Z

pdal/GDALUtils.cpp

+
+std::vector<OGRGeometry*> fetchOGRGeometries(const NL::json ogr)
+{
+    const NL::json& layer = ogr.at("layer");


This is why I hate JSON. It gives the appearance of being simple, but it's just a mess because there's no schema and there's no checking here for anything going wrong. We're pretty good about providing feedback with pipelines and this does none of that checking. As awful the JSON error checking is from a code standpoint, I think that it should be added. Users should know when something is mis-specified and letting it fall down into "bad query" or something similar is really difficult, especially when you have a one character spelling error or something for which we could provide much more specific feedback.

there's no schema and there's no checking here for anything going wrong

This code throws if the expected keys are not found. Keys that are optional are handled. It happens at runtime. There's a bunch of stuff we don't know about the given datasource until we try to open it. Opening OGR datasources can be expensive. We don't want to do it multiple times if we can avoid it.

We have no way in ProgramArgs to handle stuff like this ogr option. It doesn't make sense to flatten it out into 20 different options on a stage, and we also want to add this capability to other stages like filters.crop.

I'm not suggesting that we do something other than JSON. I'm suggesting that the JSON input be checked for errors.

Check for errors at what time? initialize() is when it is happening right now.

That's a fine time to check the pipeline input.

I think it would be better to do something like build the query in initialize() and then use it in ready(). But I'd have to look at the whole reader a bit to be sure what's feasible and reasonable.

My concern is just that the input validation occur at some point. Right now it's very loose, which is what tends to happen with this JSON stuff. I blame at least a couple of days' time with 3D tiles on this.

abellgithub · 2019-10-02T12:52:27Z

pdal/GDALUtils.cpp

+    while( (poFeature = poLayer->GetNextFeature()) != NULL )
+    {
+        OGRGeometry* poGeometry = poFeature->GetGeometryRef();
+        OGRGeometry* clone = poGeometry->clone();


I think it's better just to create pdal::Polygon here. It already handles all the stuff with OGRGeometry internally.

pdal::Polygon includes GDALUtils for its implementation. We would have to refactor things more heavily to do that.

I can take that on once this is committed.

abellgithub · 2019-10-02T13:05:27Z

pdal/Geometry.cpp

@@ -74,6 +74,19 @@ Geometry::Geometry(OGRGeometryH g, const SpatialReference& srs) :
 }


+Geometry::Geometry(OGRGeometryH gh, OGRSpatialReferenceH srs) :


The way to do this, IMO, is to add a ctor to GDALUtils::SpatialRef that takes an OGRSpatialReferenceH and clones it. Then everything else stays the same. No changes to Geometry or Polygon or SpatialReference and it keeps the GDAL code isolated.

abellgithub · 2019-10-02T13:15:11Z

pdal/GDALUtils.cpp

+            std::string srs;
+            if (isJson)
+            {
+                filterGeometry = createFromGeoJson(wkt_or_json, srs);


Seems leaky for filterGeometry. Maybe we should change things so that createFrom... creates pdal::Geometry().

abellgithub · 2019-10-03T19:34:49Z

If you merge master into this it should be close.

hobu and others added 16 commits September 30, 2019 16:58

initial implementation of OGR filtering with an 'ogr' json block for …

3dada3b

…readers.ept

remove lint

445d01a

remove more lint

1e40b50

Make EPT streaming occur asynchronously with a lookahead buffer of no…

2640bef

…des.

Merge remote-tracking branch 'origin/ept-reader-streaming' into ogr-f…

7a0a000

…ilter-options

Handle zero-point overlaps in streaming EPT reader, for when octree b…

fd9909b

…ounds overlap a query but no contained points overlap.

Respect the threads option for EPT reader streaming.

74e0f71

hammer CFLAGS and CXXFLAGS due to conda injecting them

7967379

Merge remote-tracking branch 'origin/ept-reader-streaming' into ogr-f…

08d07e5

…ilter-options

support driver specification and openoptions

6446370

Update EPT streaming test, correct usage of the output point layout.

8560925

Remove constness changes unrelated to EPT streaming for PR.

5efa01a

simplify filtering with OGR

5a8d4a5

Merge remote-tracking branch 'origin/ept-reader-streaming' into ogr-f…

27a0dd6

…ilter-options

initialize m_queryGrids in initialize()

aa00752

add test of OGR filtering

6d14921

hobu commented Oct 2, 2019

View reviewed changes

clean up ctors that are not needed

1f9f2eb

abellgithub reviewed Oct 2, 2019

View reviewed changes

hobu added this to the 2.1 milestone Oct 2, 2019

Merge remote-tracking branch 'origin/master' into ogr-filter-options

bfd479c

hobu merged commit 531b21c into master Oct 7, 2019

hobu modified the milestones: 2.1, 2.0.2 Oct 15, 2019

hobu modified the milestones: 2.0.2, 2.1 Dec 20, 2019

hobu added the feature label Jan 31, 2020

hobu deleted the ogr-filter-options branch February 3, 2020 19:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support filtering EPT by OGR datasources #2771

Support filtering EPT by OGR datasources #2771

hobu commented Oct 1, 2019

hobu Oct 2, 2019

abellgithub left a comment

abellgithub Oct 2, 2019

hobu Oct 2, 2019

abellgithub Oct 2, 2019

hobu Oct 2, 2019

abellgithub Oct 2, 2019

hobu Oct 2, 2019

abellgithub Oct 2, 2019

abellgithub Oct 2, 2019

hobu Oct 2, 2019

abellgithub Oct 2, 2019

hobu Oct 2, 2019

abellgithub Oct 2, 2019

abellgithub Oct 2, 2019

hobu Oct 2, 2019

abellgithub Oct 2, 2019

abellgithub Oct 2, 2019

abellgithub Oct 2, 2019

abellgithub commented Oct 3, 2019

		@@ -74,6 +74,19 @@ Geometry::Geometry(OGRGeometryH g, const SpatialReference& srs) :
		}


		Geometry::Geometry(OGRGeometryH gh, OGRSpatialReferenceH srs) :

Support filtering EPT by OGR datasources #2771

Support filtering EPT by OGR datasources #2771

Conversation

hobu commented Oct 1, 2019

Choose a reason for hiding this comment

abellgithub left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abellgithub commented Oct 3, 2019