Move all skip processing to StageRunner.

Remove bad m_whereMerge from Filter.

pdal/Filter.cpp

@@ -73,7 +73,7 @@ void Filter::l_addArgs(ProgramArgs& args)
         "filter", m_args->m_where);
     m_args->m_whereMergeArg = &args.add("where_merge", "If 'where' option is set, describes "
         "how skipped points should be merged with kept points in standard mode.",
-        m_whereMerge, WhereMergeMode::Auto);
+        m_args->m_whereMerge, WhereMergeMode::Auto);
 }
 
 void Filter::l_prepared(PointTableRef table)
@@ -86,28 +86,20 @@ void Filter::l_prepared(PointTableRef table)
         throwError("Can't set 'where_merge' options without also setting 'where' option.");
 }
 
-void Filter::l_prerun(const PointViewSet& views, PointViewSet& keeps,
-    PointViewSet& skips)
+void Filter::splitView(const PointViewPtr& view, PointViewPtr& keep, PointViewPtr& skip)
 {
     if (m_args->m_whereArg->set())
     {
-        for (PointViewPtr v : views)
+        PointView *k = keep.get();
+        PointView *s = skip.get();
+        for (PointRef p : *view)
         {
-            PointViewPtr keep = v->makeNew();
-            keeps.insert(keep);
-            PointViewPtr skip = v->makeNew();
-            skips.insert(skip);
-            PointView *k = keep.get();
-            PointView *s = skip.get();
-            for (PointRef p : *v)
-            {
-                PointView *active = m_args->m_where.eval(p) ? k : s;
-                active->appendPoint(*v, p.pointId());
-            }
+            PointView *active = m_args->m_where.eval(p) ? k : s;
+            active->appendPoint(*view, p.pointId());
         }
     }
     else
-        keeps = std::move(views);
+        keep = view;
 }
 
 Stage::WhereMergeMode Filter::mergeMode() const

pdal/Filter.hpp

@@ -57,11 +57,10 @@ class PDAL_DLL Filter : public virtual Stage
     Filter& operator=(const Filter&) = delete;
     Filter(const Filter&) = delete;
 
+    void splitView(const PointViewPtr& views, PointViewPtr& keeps, PointViewPtr& skips);
     bool eval(PointRef& p) const;
 
 private:
-    virtual void l_prerun(const PointViewSet& views,
-        PointViewSet& keeps, PointViewSet& skips) final;
     virtual void l_initialize(PointTableRef table) final;
     virtual void l_addArgs(ProgramArgs& args) final;
     virtual void l_prepared(PointTableRef table) final;
@@ -71,7 +70,6 @@ class PDAL_DLL Filter : public virtual Stage
     {}
 
     std::unique_ptr<Args> m_args;
-    Stage::WhereMergeMode m_whereMerge;
 };
 
 }  // namespace pdal

pdal/Stage.cpp

@@ -253,33 +253,29 @@ PointViewSet Stage::execute(PointTableRef table, PointViewSet& views)
     // through the stage.
     ready(table);
 
-    // Filter out points based on "where" clause.
-    PointViewSet keeps, skips;
-    l_prerun(views, keeps, skips);
-    prerun(keeps);
-
-    // Note that keeps and skips should be paired in order because of the
-    // way the key is generated.
-    auto ki = keeps.begin();
-    auto si = skips.begin();
-    for (; ki != keeps.end(); ++ki)
+    // Create a runner for each view.
+    for (PointViewPtr v : views)
     {
-        PointViewPtr k = *ki;
-        PointViewPtr s = ((si != skips.end()) ? *si : PointViewPtr());
-        StageRunnerPtr runner(new StageRunner(this, k, s));
+        StageRunnerPtr runner(new StageRunner(this, v));
         runners.push_back(runner);
-        runner->run();
-        if (si != skips.end())
-            si++;
     }
 
+    // The stage runner separates the point view into keeps and skips. We put all the
+    // kept points together to pass to prerun().
+    PointViewSet keeps;
+    for (StageRunnerPtr r : runners)
+        keeps.insert(r->keeps());
+    prerun(keeps);
+
+    for (StageRunnerPtr r : runners)
+        r->run();
+
     // As the stages complete (synchronously at this time), propagate the
     // spatial reference and merge the output views.
     srs = getSpatialReference();
-    for (auto const& it : runners)
+    for (StageRunnerPtr r : runners)
     {
-        StageRunnerPtr runner(it);
-        PointViewSet temp = runner->wait();
+        PointViewSet temp = r->wait();
 
         // If our stage has a spatial reference, the view takes it on once
         // the stage has been run.
@@ -438,6 +434,7 @@ std::istream& operator>>(std::istream& in, Stage::WhereMergeMode& mode)
     in >> s;
 
     s = Utils::tolower(s);
+    std::cerr << "Mode = " << s << "!\n";
     if (s == "auto")
         mode = Stage::WhereMergeMode::Auto;
     else if (s == "true")

pdal/Stage.hpp

@@ -464,13 +464,6 @@ class PDAL_DLL Stage
     virtual void ready(PointTableRef /*table*/)
         {}
 
-     /**
-      Prerun version that doesn't bleed down to the actual stages.
-     */
-     virtual void l_prerun(const PointViewSet& views,
-         PointViewSet& keeps, PointViewSet& skips)
-     { keeps = std::move(views); }
-
     /**
       Pass all the point views at once to the stage for cases where we need
       such information.  You normally shouldn't need to implement this call.

pdal/private/StageRunner.cpp

@@ -34,28 +34,43 @@
 
 #include "StageRunner.hpp"
 
-#include <pdal/Stage.hpp>
+#include <pdal/Filter.hpp>
 
 namespace pdal
 {
 
-StageRunner::StageRunner(Stage *s, PointViewPtr keeps, PointViewPtr skips) :
-    m_stage(s), m_keep(keeps), m_skip(skips)
-{}
+StageRunner::StageRunner(Stage *s, PointViewPtr view) : m_stage(s)
+{
+    m_keeps = view->makeNew();
+    Filter *f = dynamic_cast<Filter *>(m_stage);
+    if (f)
+    {
+        m_skips = view->makeNew();
+        f->splitView(view, m_keeps, m_skips);
+    }
+    else
+        m_keeps = view;
+}
+
+PointViewPtr StageRunner::keeps()
+{
+    return m_keeps;
+}
 
 // For now this is all synchronous
 void StageRunner::run()
 {
-    point_count_t keepSize = m_keep->size();
-    int keepId = m_keep->id();
-    m_viewSet = m_stage->run(m_keep);
-    if (m_skip && m_skip->size())
+    point_count_t keepSize = m_keeps->size();
+    int keepId = m_keeps->id();
+    m_viewSet = m_stage->run(m_keeps);
+
+    if (m_skips && m_skips->size())
     {
         if (m_stage->mergeMode() == Stage::WhereMergeMode::True)
         {
             if (m_viewSet.size())
             {
-                (*m_viewSet.begin())->append(*m_skip);
+                (*m_viewSet.begin())->append(*m_skips);
                 return;
             }
 
@@ -64,15 +79,15 @@ void StageRunner::run()
         {
             if (m_viewSet.size() == 1)
             {
-                PointViewPtr keep = *m_viewSet.begin();
-                if (keep->id() == keepId && keepSize == keep->size())
+                PointViewPtr keeps = *m_viewSet.begin();
+                if (keeps->id() == keepId && keepSize == keeps->size())
                 {
-                    keep->append(*m_skip);
+                    keeps->append(*m_skips);
                     return;
                 }
             }
         }
-        m_viewSet.insert(m_skip);
+        m_viewSet.insert(m_skips);
     }
 }
 

pdal/private/StageRunner.hpp

@@ -44,15 +44,16 @@ class Stage;
 class StageRunner
 {
 public:
-    StageRunner(Stage *s, PointViewPtr keeps, PointViewPtr skips);
+    StageRunner(Stage *s, PointViewPtr view);
 
     void run();
+    PointViewPtr keeps();
     PointViewSet wait();
 
 private:
     Stage *m_stage;
-    PointViewPtr m_keep;
-    PointViewPtr m_skip;
+    PointViewPtr m_keeps;
+    PointViewPtr m_skips;
     PointViewSet m_viewSet;
 };
 typedef std::shared_ptr<StageRunner> StageRunnerPtr;

test/unit/CMakeLists.txt

@@ -118,6 +118,7 @@ endif()
 if (PDAL_HAVE_ZSTD)
 PDAL_ADD_TEST(pdal_zstd_test FILES ZstdTest.cpp)
 endif()
+PDAL_ADD_TEST(pdal_where_test FILES WhereTest.cpp)
 
 #
 # sources for the native io

test/unit/WhereTest.cpp

@@ -0,0 +1,87 @@
+/******************************************************************************
+* Copyright (c) 2020, Hobu Inc. (info@hobu.co)
+*
+* All rights reserved.
+*
+* Redistribution and use in source and binary forms, with or without
+* modification, are permitted provided that the following
+* conditions are met:
+*
+*     * Redistributions of source code must retain the above copyright
+*       notice, this list of conditions and the following disclaimer.
+*     * Redistributions in binary form must reproduce the above copyright
+*       notice, this list of conditions and the following disclaimer in
+*       the documentation and/or other materials provided
+*       with the distribution.
+*     * Neither the name of Hobu, Inc. or Flaxen Geo Consulting nor the
+*       names of its contributors may be used to endorse or promote
+*       products derived from this software without specific prior
+*       written permission.
+*
+* THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+* "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+* LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS
+* FOR A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE
+* COPYRIGHT OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT,
+* INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING,
+* BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS
+* OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED
+* AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY,
+* OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT
+* OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY
+* OF SUCH DAMAGE.
+****************************************************************************/
+
+#include <pdal/pdal_test_main.hpp>
+
+#include <pdal/Filter.hpp>
+#include <pdal/StageFactory.hpp>
+#include <pdal/util/Bounds.hpp>
+
+namespace pdal
+{
+
+namespace
+{
+    size_t g_count;
+}
+
+TEST(WhereTest, t1)
+{
+    StageFactory factory;
+
+    Stage *r = factory.createStage("readers.faux");
+    Options ro;
+    ro.add("count", 100);
+    ro.add("bounds", BOX3D(0, 0, 100, 99, 9.9, 199));
+    ro.add("mode", "ramp");
+    r->setOptions(ro);
+
+
+    class TestFilter : public Filter
+    {
+        std::string getName() const
+        { return "filters.test"; }
+
+        void filter(PointView& v)
+        {
+            g_count = v.size();
+        }
+    };
+
+    TestFilter f;
+    Options fo;
+    fo.add("where", "X<50");
+    f.setOptions(fo);
+    f.setInput(*r);
+
+    PointTable t;
+    f.prepare(t);
+    PointViewSet s = f.execute(t);
+    for (auto vp : s)
+        std::cerr << "View size = " << vp->size() << "!\n";
+    PointViewPtr v = *s.begin();
+    std::cerr << "Filter/Post size = " << g_count << "/" << v->size() << "!\n";
+}
+
+} // namespace pdal