sis-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From ama...@apache.org
Subject [sis] 13/22: feat(SQLStore): improve query feature set to delegate count and subqueries to target database.
Date Thu, 14 Nov 2019 11:46:47 GMT
This is an automated email from the ASF dual-hosted git repository.

amanin pushed a commit to branch refactor/sql-store
in repository https://gitbox.apache.org/repos/asf/sis.git

commit 7acadb85587d32cb8ad725e3f66fc5a7bc0170ba
Author: Alexis Manin <amanin@apache.org>
AuthorDate: Wed Oct 16 18:14:29 2019 +0200

    feat(SQLStore): improve query feature set to delegate count and subqueries to target database.
---
 .../java/org/apache/sis/filter/DefaultBBOX.java    |  2 +-
 .../java/org/apache/sis/internal/feature/ESRI.java |  8 ++-
 .../java/org/apache/sis/internal/feature/JTS.java  | 33 ++++++++++-
 .../org/apache/sis/internal/feature/Java2D.java    | 12 +++-
 .../org/apache/sis/internal/feature/jts/JTS.java   | 38 ++++++++++--
 .../sis/internal/sql/feature/ANSIInterpreter.java  | 32 ++++-------
 .../apache/sis/internal/sql/feature/Analyzer.java  |  3 +-
 .../apache/sis/internal/sql/feature/Features.java  | 28 ++++-----
 .../sis/internal/sql/feature/QueryFeatureSet.java  | 67 +++++++++++++++++-----
 .../sis/internal/sql/feature/SQLQueryAdapter.java  | 31 ++++++----
 .../apache/sis/internal/sql/feature/StreamSQL.java |  6 +-
 .../org/apache/sis/internal/sql/feature/Table.java |  6 +-
 .../sis/internal/sql/feature/TableSubset.java      |  5 ++
 .../sis/internal/sql/feature/package-info.java     | 10 ++++
 .../sql/feature/FilterInterpreterTest.java         |  9 +--
 .../org/apache/sis/storage/sql/SQLStoreTest.java   | 27 ++++++++-
 16 files changed, 232 insertions(+), 85 deletions(-)

diff --git a/core/sis-feature/src/main/java/org/apache/sis/filter/DefaultBBOX.java b/core/sis-feature/src/main/java/org/apache/sis/filter/DefaultBBOX.java
index a597e45..6444070 100644
--- a/core/sis-feature/src/main/java/org/apache/sis/filter/DefaultBBOX.java
+++ b/core/sis-feature/src/main/java/org/apache/sis/filter/DefaultBBOX.java
@@ -79,7 +79,7 @@ final class DefaultBBOX implements BBOX {
 
     private boolean nonOptimizedIntersect(Object candidate) {
         Envelope leftEval = left == null ? null : asEnvelope(left, candidate);
-        Envelope rightEval = left == null ? null : asEnvelope(right, candidate);
+        Envelope rightEval = right == null ? null : asEnvelope(right, candidate);
         if (left == null) {
             return multiIntersect(candidate, rightEval);
         } else if (right == null) {
diff --git a/core/sis-feature/src/main/java/org/apache/sis/internal/feature/ESRI.java b/core/sis-feature/src/main/java/org/apache/sis/internal/feature/ESRI.java
index 80a71dab..9659549 100644
--- a/core/sis-feature/src/main/java/org/apache/sis/internal/feature/ESRI.java
+++ b/core/sis-feature/src/main/java/org/apache/sis/internal/feature/ESRI.java
@@ -146,6 +146,12 @@ final class ESRI extends Geometries<Geometry> {
         return path;
     }
 
+    @Override
+    public Geometry toPolygon(Geometry polyline) throws IllegalArgumentException {
+        if (polyline instanceof Polygon) return polyline;
+        return createMultiPolygonImpl(polyline);
+    }
+
     /**
      * Merges a sequence of points or paths if the first instance is an implementation of
this library.
      *
@@ -206,7 +212,7 @@ add:    for (;;) {
     }
 
     @Override
-    Object createMultiPolygonImpl(Object... polygonsOrLinearRings) {
+    Polygon createMultiPolygonImpl(Object... polygonsOrLinearRings) {
         final Polygon poly = new Polygon();
         for (final Object polr : polygonsOrLinearRings) {
             if (polr instanceof MultiPath) {
diff --git a/core/sis-feature/src/main/java/org/apache/sis/internal/feature/JTS.java b/core/sis-feature/src/main/java/org/apache/sis/internal/feature/JTS.java
index 2e9b184..34c5ba0 100644
--- a/core/sis-feature/src/main/java/org/apache/sis/internal/feature/JTS.java
+++ b/core/sis-feature/src/main/java/org/apache/sis/internal/feature/JTS.java
@@ -21,10 +21,14 @@ import java.util.Arrays;
 import java.util.Iterator;
 import java.util.List;
 
+import org.opengis.referencing.crs.CoordinateReferenceSystem;
+import org.opengis.util.FactoryException;
+
 import org.apache.sis.geometry.GeneralEnvelope;
 import org.apache.sis.math.Vector;
 import org.apache.sis.setup.GeometryLibrary;
 import org.apache.sis.util.Classes;
+import org.apache.sis.util.collection.BackingStoreException;
 
 import org.locationtech.jts.geom.Coordinate;
 import org.locationtech.jts.geom.Envelope;
@@ -33,6 +37,7 @@ import org.locationtech.jts.geom.GeometryFactory;
 import org.locationtech.jts.geom.LineString;
 import org.locationtech.jts.geom.LinearRing;
 import org.locationtech.jts.geom.MultiLineString;
+import org.locationtech.jts.geom.MultiPolygon;
 import org.locationtech.jts.geom.Point;
 import org.locationtech.jts.geom.Polygon;
 import org.locationtech.jts.io.ParseException;
@@ -191,6 +196,32 @@ final class JTS extends Geometries<Geometry> {
         return toGeometry(lines);
     }
 
+    @Override
+    public Geometry toPolygon(Geometry polyline) throws IllegalArgumentException {
+        if (polyline instanceof Polygon) return polyline;
+
+        Polygon result = null;
+        if (polyline instanceof LinearRing) {
+            result = factory.createPolygon((LinearRing) polyline);
+        } else if (polyline instanceof LineString) {
+            final LineString myLine = (LineString) polyline;
+            if (myLine.getEndPoint().equals(myLine.getStartPoint())) {
+                result = factory.createPolygon(myLine.getCoordinateSequence());
+            }
+        }
+
+        if (result == null) throw new IllegalArgumentException("Input is not a closed line.");
+
+        try {
+            final CoordinateReferenceSystem crs = org.apache.sis.internal.feature.jts.JTS.getCoordinateReferenceSystem(polyline);
+            org.apache.sis.internal.feature.jts.JTS.setCoordinateReferenceSystem(result,
crs);
+        } catch (FactoryException e) {
+            throw new BackingStoreException("Cannot extract CRS from geometry", e);
+        }
+
+        return result;
+    }
+
     /**
      * Makes a line string or linear ring from the given coordinates, and add the line string
to the given list.
      * If the given coordinates array is empty, then this method does nothing.
@@ -291,7 +322,7 @@ add:    for (;;) {
     }
 
     @Override
-    Object createMultiPolygonImpl(Object... polygonsOrLinearRings) {
+    MultiPolygon createMultiPolygonImpl(Object... polygonsOrLinearRings) {
         final Polygon[] polys = new Polygon[polygonsOrLinearRings.length];
         for (int i = 0 ; i < polys.length ; i++) {
             Object o = polygonsOrLinearRings[i];
diff --git a/core/sis-feature/src/main/java/org/apache/sis/internal/feature/Java2D.java b/core/sis-feature/src/main/java/org/apache/sis/internal/feature/Java2D.java
index 991a723..35f9a04 100644
--- a/core/sis-feature/src/main/java/org/apache/sis/internal/feature/Java2D.java
+++ b/core/sis-feature/src/main/java/org/apache/sis/internal/feature/Java2D.java
@@ -178,7 +178,6 @@ final class Java2D extends Geometries<Shape> {
                 }
                 if (Double.isNaN(x) || Double.isNaN(y)) {
                     if (lastX == startX && lastY == startY) path.closePath();
-                    startX = Double.NaN;
                     lineTo = false;
                     startX = startY = Double.NaN;
                 } else if (lineTo) {
@@ -258,13 +257,20 @@ add:    for (;;) {
     }
 
     @Override
-    Object createMultiPolygonImpl(Object... polygonsOrLinearRings) {
+    Shape createMultiPolygonImpl(Object... polygonsOrLinearRings) {
         ensureNonEmpty("Polygons or linear rings to merge", polygonsOrLinearRings);
-        if (polygonsOrLinearRings.length == 1) return polygonsOrLinearRings[0];
+        if (polygonsOrLinearRings.length == 1 && polygonsOrLinearRings[0] instanceof
Shape)
+            return (Shape) polygonsOrLinearRings[0];
         final Iterator<Object> it = Arrays.asList(polygonsOrLinearRings).iterator();
         return tryMergePolylines(it.next(), it);
     }
 
+    @Override
+    public Shape toPolygon(Shape polyline) throws IllegalArgumentException {
+        // TODO: check that path ends with close.
+        return polyline;
+    }
+
     /**
      * If the given object is a Java2D shape, builds its WKT representation.
      * Current implementation assumes that all closed shapes are polygons and that polygons
have no hole
diff --git a/core/sis-feature/src/main/java/org/apache/sis/internal/feature/jts/JTS.java b/core/sis-feature/src/main/java/org/apache/sis/internal/feature/jts/JTS.java
index af9d29a..d3e1174 100644
--- a/core/sis-feature/src/main/java/org/apache/sis/internal/feature/jts/JTS.java
+++ b/core/sis-feature/src/main/java/org/apache/sis/internal/feature/jts/JTS.java
@@ -17,23 +17,29 @@
 package org.apache.sis.internal.feature.jts;
 
 import java.util.Map;
-import org.opengis.util.FactoryException;
+import java.util.Optional;
+
 import org.opengis.referencing.crs.CoordinateReferenceSystem;
 import org.opengis.referencing.operation.CoordinateOperation;
 import org.opengis.referencing.operation.MathTransform;
 import org.opengis.referencing.operation.TransformException;
+import org.opengis.util.FactoryException;
+
+import org.apache.sis.geometry.Envelope2D;
+import org.apache.sis.internal.system.Loggers;
 import org.apache.sis.internal.util.Constants;
+import org.apache.sis.metadata.iso.extent.DefaultGeographicBoundingBox;
 import org.apache.sis.referencing.CRS;
 import org.apache.sis.util.Static;
 import org.apache.sis.util.Utilities;
 import org.apache.sis.util.logging.Logging;
 import org.apache.sis.util.resources.Errors;
-import org.apache.sis.metadata.iso.extent.DefaultGeographicBoundingBox;
-import org.apache.sis.geometry.Envelope2D;
-import org.apache.sis.internal.system.Loggers;
+
 import org.locationtech.jts.geom.Envelope;
 import org.locationtech.jts.geom.Geometry;
 
+import static org.apache.sis.util.ArgumentChecks.ensureNonNull;
+
 
 /**
  * Utilities for Java Topology Suite (JTS) objects.
@@ -101,6 +107,30 @@ public final class JTS extends Static {
         return null;
     }
 
+    public static Optional<CoordinateReferenceSystem> setCoordinateReferenceSystem(final
Geometry target, final CoordinateReferenceSystem toSet) {
+        ensureNonNull("Target geometry", target);
+        final Object ud = target.getUserData();
+        if (ud == null) {
+            // By security, we reset SRID in case old CRS was defined this way.
+            target.setSRID(0);
+            target.setUserData(toSet);
+            return Optional.empty();
+        } else if (ud instanceof CoordinateReferenceSystem) {
+            target.setUserData(toSet);
+            return Optional.of((CoordinateReferenceSystem)ud);
+        } else if (ud instanceof Map) {
+            final Map asMap = (Map) ud;
+            // In case user-data contains other useful data, we don't switch from map to
CRS. We also reset SRID.
+            final Object oldVal = asMap.put(CRS_KEY, toSet);
+            // By security, we reset SRID in case old CRS was defined this way.
+            if (oldVal == null) {
+                target.setSRID(0);
+            }
+        }
+
+        throw new IllegalArgumentException("Cannot modify input geometry, because user-data
does not comply with SIS convention (should be a map or null, but was "+ud.getClass().getCanonicalName()+").");
+    }
+
     /**
      * Transforms the given geometry to the specified Coordinate Reference System (CRS).
      * If the given CRS or the given geometry is null, the geometry is returned unchanged.
diff --git a/storage/sis-sqlstore/src/main/java/org/apache/sis/internal/sql/feature/ANSIInterpreter.java
b/storage/sis-sqlstore/src/main/java/org/apache/sis/internal/sql/feature/ANSIInterpreter.java
index 676fa1a..943df9e 100644
--- a/storage/sis-sqlstore/src/main/java/org/apache/sis/internal/sql/feature/ANSIInterpreter.java
+++ b/storage/sis-sqlstore/src/main/java/org/apache/sis/internal/sql/feature/ANSIInterpreter.java
@@ -182,26 +182,9 @@ public class ANSIInterpreter implements FilterVisitor, ExpressionVisitor
{
 
     @Override
     public Object visit(BBOX filter, Object extraData) {
-        final CharSequence left = evaluateMandatory(filter.getExpression1(), extraData);
-        final CharSequence right = evaluateMandatory(filter.getExpression2(), extraData);
-
-        // TODO: In case source expression is already an envelope, we do not need to force
envelope conversion. It would
-        // only be micro-optimisation however.
-        boolean leftToEnvelope = true;
-        boolean rightToEnvelope = true;
-
-        final StringBuilder sb = new StringBuilder("ST_Intersects(");
-        if (leftToEnvelope) {
-            sb.append("ST_Envelope(").append(left).append(')');
-        } else sb.append(left);
-
-        sb.append(", ");
-
-        if (rightToEnvelope) {
-            sb.append("ST_Envelope(").append(right).append(')');
-        } else sb.append(right);
-
-        return sb.append(')');
+        // TODO: This is a wrong interpretation, but sqlmm has no equivalent of filter encoding
bbox, so we'll
+        // fallback on a standard intersection. However, PostGIS, H2, etc. have their own
versions of such filters.
+        return function("ST_Intersects(", filter, extraData);
     }
 
     @Override
@@ -442,6 +425,11 @@ public class ANSIInterpreter implements FilterVisitor, ExpressionVisitor
{
         return join(candidate::getExpression1, candidate::getExpression2, operator, extraData);
     }
 
+
+    protected CharSequence join(BinarySpatialOperator candidate, String operator, Object
extraData) {
+        return join(candidate::getExpression1, candidate::getExpression2, operator, extraData);
+    }
+
     protected CharSequence join(
             Supplier<Expression> leftOperand,
             Supplier<Expression> rightOperand,
@@ -526,9 +514,9 @@ public class ANSIInterpreter implements FilterVisitor, ExpressionVisitor
{
                 .mapToDouble(env::getSpan)
                 .average()
                 .orElseThrow(() -> new IllegalArgumentException("Given geometry envelope
dimension is 0"));
-        return new StringBuilder("ST_GeomFromText(")
+        return new StringBuilder("ST_GeomFromText('")
                 .append(Geometries.formatWKT(source, flatness))
-                .append(')');
+                .append("')");
     }
 
     protected static double clampInfinity(final double candidate) {
diff --git a/storage/sis-sqlstore/src/main/java/org/apache/sis/internal/sql/feature/Analyzer.java
b/storage/sis-sqlstore/src/main/java/org/apache/sis/internal/sql/feature/Analyzer.java
index d947ad2..11c9e35 100644
--- a/storage/sis-sqlstore/src/main/java/org/apache/sis/internal/sql/feature/Analyzer.java
+++ b/storage/sis-sqlstore/src/main/java/org/apache/sis/internal/sql/feature/Analyzer.java
@@ -408,12 +408,11 @@ final class Analyzer {
 
     private void addImports(SQLTypeSpecification spec, FeatureTypeBuilder target) throws
SQLException {
         final List<Relation> imports = spec.getImports();
-        // TODO: add an abstraction here, so we can specify source table when origin is one.
         for (Relation r : imports) {
             final GenericName foreignTypeName = r.getName(Analyzer.this);
             final Table foreignTable;
             try {
-                foreignTable = table(r, foreignTypeName, null);
+                foreignTable = table(r, foreignTypeName, spec instanceof TableMetadata ?
((TableMetadata) spec).id : null);
             } catch (DataStoreException e) {
                 throw new BackingStoreException(e);
             }
diff --git a/storage/sis-sqlstore/src/main/java/org/apache/sis/internal/sql/feature/Features.java
b/storage/sis-sqlstore/src/main/java/org/apache/sis/internal/sql/feature/Features.java
index 5ce2195..dd97df9 100644
--- a/storage/sis-sqlstore/src/main/java/org/apache/sis/internal/sql/feature/Features.java
+++ b/storage/sis-sqlstore/src/main/java/org/apache/sis/internal/sql/feature/Features.java
@@ -151,6 +151,8 @@ final class Features implements Spliterator<Feature> {
      */
     private final long estimatedSize;
 
+    private final FeatureAdapter adapter;
+
     /**
      * Creates a new iterator over the feature instances.
      *
@@ -180,6 +182,7 @@ final class Features implements Spliterator<Feature> {
             attributeNames[i++] = column.getAttributeName();
         }
         this.featureType = table.featureType;
+        this.adapter = table.adapter;
         final DatabaseMetaData metadata = connection.getMetaData();
         estimatedSize = following.isEmpty() ? table.countRows(metadata, true) : 0;
         /*
@@ -414,13 +417,8 @@ final class Features implements Spliterator<Feature> {
      */
     private boolean fetch(final Consumer<? super Feature> action, final boolean all)
throws SQLException {
         while (result.next()) {
-            final Feature feature = featureType.newInstance();
-            for (int i=0; i < attributeNames.length; i++) {
-                final Object value = result.getObject(i+1);
-                if (!result.wasNull()) {
-                    feature.setPropertyValue(attributeNames[i], value);
-                }
-            }
+            // TODO: give connection to adapter.
+            final Feature feature = adapter.read(result, null);
             for (int i=0; i < dependencies.length; i++) {
                 final Features dependency = dependencies[i];
                 final int[] columnIndices = foreignerKeyIndices[i];
@@ -667,12 +665,7 @@ final class Features implements Spliterator<Feature> {
             sql.append(" FROM ").appendIdentifier(source.parent.name.catalog, source.parent.name.schema,
source.parent.name.table);
 
             appendWhere(sql, where);
-            if (!count && sort != null && sort.length > 0) {
-                sql.append(" ORDER BY ");
-                append(sql, sort[0]);
-                for (int i = 1 ; i < sort.length ; i++)
-                    append(sql.append(", "), sort[i]);
-            }
+            if (!count) appendOrderBy(sql, sort);
 
             addOffsetLimit(sql, source.offset, source.limit);
 
@@ -691,6 +684,15 @@ final class Features implements Spliterator<Feature> {
         }
     }
 
+    static void appendOrderBy(SQLBuilder sql, SortBy[] sort) {
+        if (sort != null && sort.length > 0) {
+            sql.append(" ORDER BY ");
+            append(sql, sort[0]);
+            for (int i = 1 ; i < sort.length ; i++)
+                append(sql.append(", "), sort[i]);
+        }
+    }
+
     private static void append(SQLBuilder target, SortBy toAppend) {
         target.appendIdentifier(toAppend.getPropertyName().getPropertyName()).append(" ");
         if (toAppend.getSortOrder() != null) target.append(toAppend.getSortOrder().toSQL());
diff --git a/storage/sis-sqlstore/src/main/java/org/apache/sis/internal/sql/feature/QueryFeatureSet.java
b/storage/sis-sqlstore/src/main/java/org/apache/sis/internal/sql/feature/QueryFeatureSet.java
index 6c16b6a..e5d991c 100644
--- a/storage/sis-sqlstore/src/main/java/org/apache/sis/internal/sql/feature/QueryFeatureSet.java
+++ b/storage/sis-sqlstore/src/main/java/org/apache/sis/internal/sql/feature/QueryFeatureSet.java
@@ -6,6 +6,7 @@ import java.sql.ResultSet;
 import java.sql.SQLException;
 import java.util.List;
 import java.util.Spliterator;
+import java.util.StringJoiner;
 import java.util.function.Consumer;
 import java.util.regex.Matcher;
 import java.util.regex.Pattern;
@@ -15,10 +16,15 @@ import javax.sql.DataSource;
 
 import org.opengis.feature.Feature;
 import org.opengis.feature.FeatureType;
+import org.opengis.filter.sort.SortBy;
 
 import org.apache.sis.internal.metadata.sql.SQLBuilder;
 import org.apache.sis.internal.storage.AbstractFeatureSet;
+import org.apache.sis.internal.storage.query.SimpleQuery;
 import org.apache.sis.storage.DataStoreException;
+import org.apache.sis.storage.FeatureSet;
+import org.apache.sis.storage.Query;
+import org.apache.sis.storage.UnsupportedQueryException;
 import org.apache.sis.util.collection.BackingStoreException;
 import org.apache.sis.util.logging.WarningListeners;
 
@@ -30,6 +36,8 @@ import org.apache.sis.util.logging.WarningListeners;
  *
  * Note that this component models query result as close as possible, so built data type
will be simple feature type (no
  * association).
+ *
+ * TODO: move query analysis in a dedicated class.
  */
 public class QueryFeatureSet extends AbstractFeatureSet {
 
@@ -127,10 +135,10 @@ public class QueryFeatureSet extends AbstractFeatureSet {
      *                 can use {@link #QueryFeatureSet(SQLBuilder, DataSource, Connection)
another constructor}.
      */
     QueryFeatureSet(SQLBuilder queryBuilder, Analyzer analyzer, DataSource source, Connection
conn) throws SQLException {
-        this(queryBuilder, createAdapter(queryBuilder, analyzer, conn), analyzer.listeners,
source, conn);
+        this(queryBuilder, createAdapter(queryBuilder, analyzer, conn), analyzer.listeners,
source);
     }
 
-    QueryFeatureSet(SQLBuilder queryBuilder, FeatureAdapter adapter, WarningListeners listeners,
DataSource source, Connection conn) throws SQLException {
+    QueryFeatureSet(SQLBuilder queryBuilder, FeatureAdapter adapter, WarningListeners listeners,
DataSource source) {
         super(listeners);
         this.source = source;
         this.adapter = adapter;
@@ -168,6 +176,26 @@ public class QueryFeatureSet extends AbstractFeatureSet {
         this.queryBuilder.append(sql);
     }
 
+    @Override
+    public FeatureType getType() {
+        return adapter.type;
+    }
+
+    @Override
+    public Stream<Feature> features(boolean parallel) {
+        return new StreamSQL(new QueryAdapter(queryBuilder, parallel), source, parallel);
+    }
+
+    @Override
+    public FeatureSet subset(Query query) throws UnsupportedQueryException, DataStoreException
{
+        if (query instanceof SimpleQuery) {
+            final org.apache.sis.internal.storage.SubsetAdapter subsetAdapter = new org.apache.sis.internal.storage.SubsetAdapter(fs
-> new SubsetAdapter());
+            return subsetAdapter.subset(this, (SimpleQuery) query);
+        }
+
+        return super.subset(query);
+    }
+
     /**
      * Acquire a connection over parent database, forcing a few parameters to ensure optimal
read performance and
      * limiting user rights :
@@ -196,14 +224,27 @@ public class QueryFeatureSet extends AbstractFeatureSet {
         return c;
     }
 
-    @Override
-    public FeatureType getType() {
-        return adapter.type;
+    class SubsetAdapter extends SQLQueryAdapter {
+
+        @Override
+        protected FeatureSet create(CharSequence where, SortBy[] sorting, ColumnRef[] columns)
{
+            // TODO: use columns.
+            final SQLBuilder newQuery = amendQuery(where, sorting);
+            return new QueryFeatureSet(newQuery, adapter, null, source);
+        }
     }
 
-    @Override
-    public Stream<Feature> features(boolean parallel) {
-        return new StreamSQL(new QueryAdapter(queryBuilder, parallel), source, parallel);
+    private SQLBuilder amendQuery(CharSequence where, SortBy[] sorting) {
+        // As we do not know user query complexity, what we'll do is make a query wrapper
to ensure we won't break the
+        // original query. Note that it will surely be less performant, though.
+        final SQLBuilder newBuilder = new SQLBuilder(queryBuilder);
+        newBuilder.append("SELECT * FROM (")
+                .append(queryBuilder.toString())
+                .append(')')
+                .append(" AS ORIGIN_QUERY");
+        if (where != null && where.length() > 0) newBuilder.append(" WHERE ").append(where.toString());
+        Features.appendOrderBy(newBuilder, sorting);
+        return newBuilder.append(";");
     }
 
     private final class QueryAdapter implements StreamSQL.QueryBuilder {
@@ -248,7 +289,7 @@ public class QueryFeatureSet extends AbstractFeatureSet {
                     nativeOffset = originOffset + additionalOffset;
                 }
 
-                if (originLimit < 0) {
+                if (originLimit <= 0) {
                     javaLimit = this.additionalLimit;
                 } else if (originLimit > 0 || additionalLimit > 0) {
                     nativeLimit = Math.min(originLimit, additionalLimit);
@@ -280,7 +321,7 @@ public class QueryFeatureSet extends AbstractFeatureSet {
         private final boolean parallel;
 
         private PreparedQueryConnector(String sql, long additionalOffset, long additionalLimit,
boolean parallel) {
-            this.sql = sql;
+            this.sql = sql.replaceFirst(";\\s*$", "");
             this.additionalOffset = additionalOffset;
             this.additionalLimit = additionalLimit;
             this.parallel = parallel;
@@ -314,8 +355,7 @@ public class QueryFeatureSet extends AbstractFeatureSet {
         @Override
         public String estimateStatement(boolean count) {
             if (count) {
-                // We should check if user query is already a count operation, in which case
we would return "select 1"
-                throw new UnsupportedOperationException("Not supported yet"); // "Alexis
Manin (Geomatys)" on 24/09/2019
+                return "SELECT COUNT(*) FROM ("+sql+") AS count_all";
             } else {
                 return sql;
             }
@@ -350,7 +390,7 @@ public class QueryFeatureSet extends AbstractFeatureSet {
 
         @Override
         public int characteristics() {
-            // TODO: determine if it's order by analysing user query. SIZED is not possible,
as limit is an upper threshold.
+            // TODO: determine if it's ordered by analysing user query. SIZED is not possible,
as limit is an upper threshold.
             return Spliterator.IMMUTABLE | Spliterator.NONNULL | (distinct ? Spliterator.DISTINCT
: 0);
         }
     }
@@ -400,6 +440,7 @@ public class QueryFeatureSet extends AbstractFeatureSet {
 
         int idx;
         List<Feature> chunk;
+
         /**
          * According to {@link Spliterator#trySplit()} documentation, the original size estimation
must be reduced after
          * split to remain consistent.
diff --git a/storage/sis-sqlstore/src/main/java/org/apache/sis/internal/sql/feature/SQLQueryAdapter.java
b/storage/sis-sqlstore/src/main/java/org/apache/sis/internal/sql/feature/SQLQueryAdapter.java
index e471567..94c2d11 100644
--- a/storage/sis-sqlstore/src/main/java/org/apache/sis/internal/sql/feature/SQLQueryAdapter.java
+++ b/storage/sis-sqlstore/src/main/java/org/apache/sis/internal/sql/feature/SQLQueryAdapter.java
@@ -11,19 +11,13 @@ import org.apache.sis.internal.storage.SubsetAdapter;
 import org.apache.sis.internal.storage.query.SimpleQuery;
 import org.apache.sis.storage.FeatureSet;
 
-public class SQLQueryAdapter implements SubsetAdapter.AdapterBuilder {
-
-    final Table parent;
+abstract class SQLQueryAdapter implements SubsetAdapter.AdapterBuilder {
 
     private ColumnRef[] columns;
     private SortBy[] sorting;
 
     private CharSequence where;
 
-    public SQLQueryAdapter(Table parent) {
-        this.parent = parent;
-    }
-
     /**
      * No-op implementation. SQL optimisation is dynamically applied through {@link StreamSQL}.
      * @param offset The offset to handle.
@@ -45,7 +39,7 @@ public class SQLQueryAdapter implements SubsetAdapter.AdapterBuilder {
     }
 
     @Override
-    public Filter filter(Filter filter) {
+    public final Filter filter(Filter filter) {
         try {
             final Object result = filter.accept(new ANSIInterpreter(), null);
             if (ANSIInterpreter.isNonEmptyText(result)) {
@@ -78,14 +72,31 @@ public class SQLQueryAdapter implements SubsetAdapter.AdapterBuilder {
     }
 
     @Override
-    public Optional<FeatureSet> build() {
+    public final Optional<FeatureSet> build() {
         if (isNoOp()) return Optional.empty();
-        return Optional.of(new TableSubset(parent, sorting, where));
+        return Optional.of(create(where, sorting, columns));
     }
 
+    protected abstract FeatureSet create(final CharSequence where, final SortBy[] sorting,
final ColumnRef[] columns);
+
     private boolean isNoOp() {
         return (sorting == null || sorting.length < 1)
                 && (columns == null || columns.length < 1)
                 && (where == null || where.length() < 1);
     }
+
+    static class Table extends SQLQueryAdapter {
+        final org.apache.sis.internal.sql.feature.Table parent;
+        public Table(org.apache.sis.internal.sql.feature.Table parent) {
+            this.parent = parent;
+        }
+
+        @Override
+        protected FeatureSet create(CharSequence where, SortBy[] sorting, ColumnRef[] columns)
{
+            // TODO: column information is lost for now. What should be done is factorize/sanitize
feature set
+            // implementations from this package to better handle SQL filtering.
+            return new TableSubset(parent, sorting, where);
+        }
+    }
+
 }
diff --git a/storage/sis-sqlstore/src/main/java/org/apache/sis/internal/sql/feature/StreamSQL.java
b/storage/sis-sqlstore/src/main/java/org/apache/sis/internal/sql/feature/StreamSQL.java
index 30820a9..7291a04 100644
--- a/storage/sis-sqlstore/src/main/java/org/apache/sis/internal/sql/feature/StreamSQL.java
+++ b/storage/sis-sqlstore/src/main/java/org/apache/sis/internal/sql/feature/StreamSQL.java
@@ -55,9 +55,9 @@ import static org.apache.sis.util.ArgumentChecks.ensurePositive;
  * Manages query lifecycle and optimizations. Operations like {@link #count()}, {@link #distinct()},
{@link #skip(long)}
  * and {@link #limit(long)} are delegated to underlying SQL database. This class consistently
propagate optimisation
  * strategies through streams obtained using {@link #map(Function)}, {@link #mapToDouble(ToDoubleFunction)}
and
- * {@link #peek(Consumer)} operations. However, for result consistency, no optimization is
stacked once either
- * {@link #filter(Predicate)} or {@link #flatMap(Function)} operations are called, as they
modify browing flow (the
- * count of stream elements is not bound 1 to 1 to query result rows).
+ * {@link #peek(Consumer)} operations. However, for result consistency, no optimization is
stacked anymore after either
+ * {@link #filter(Predicate)} or {@link #flatMap(Function)} operations are called, as they
modify volumetry (the count
+ * of stream elements is not bound 1 to 1 to query result rows).
  *
  * @since 1.0
  *
diff --git a/storage/sis-sqlstore/src/main/java/org/apache/sis/internal/sql/feature/Table.java
b/storage/sis-sqlstore/src/main/java/org/apache/sis/internal/sql/feature/Table.java
index b60ec46..7205f70 100644
--- a/storage/sis-sqlstore/src/main/java/org/apache/sis/internal/sql/feature/Table.java
+++ b/storage/sis-sqlstore/src/main/java/org/apache/sis/internal/sql/feature/Table.java
@@ -145,7 +145,7 @@ final class Table extends AbstractFeatureSet {
      */
     private final SQLBuilder sqlTemplate;
 
-    private final FeatureAdapter adapter;
+    protected final FeatureAdapter adapter;
 
     /**
      * Creates a description of the table of the given name.
@@ -208,7 +208,7 @@ final class Table extends AbstractFeatureSet {
     @Override
     public FeatureSet subset(Query query) throws UnsupportedQueryException, DataStoreException
{
         if (query instanceof SimpleQuery) {
-            final SubsetAdapter subsetAdapter = new SubsetAdapter(fs -> new SQLQueryAdapter(this));
+            final SubsetAdapter subsetAdapter = new SubsetAdapter(fs -> new SQLQueryAdapter.Table(this));
             return subsetAdapter.subset(this, (SimpleQuery) query);
         }
 
@@ -230,7 +230,7 @@ final class Table extends AbstractFeatureSet {
      *
      * @param  tables  all tables created.
      */
-    final void setDeferredSearchTables(final Analyzer analyzer, final Map<GenericName,Table>
tables) throws DataStoreException {
+    final void setDeferredSearchTables(final Analyzer analyzer, final Map<GenericName,
Table> tables) throws DataStoreException {
         for (final Relation.Direction direction : Relation.Direction.values()) {
             final Relation[] relations;
             switch (direction) {
diff --git a/storage/sis-sqlstore/src/main/java/org/apache/sis/internal/sql/feature/TableSubset.java
b/storage/sis-sqlstore/src/main/java/org/apache/sis/internal/sql/feature/TableSubset.java
index dc9e427..52da6a5 100644
--- a/storage/sis-sqlstore/src/main/java/org/apache/sis/internal/sql/feature/TableSubset.java
+++ b/storage/sis-sqlstore/src/main/java/org/apache/sis/internal/sql/feature/TableSubset.java
@@ -12,9 +12,14 @@ import org.opengis.util.GenericName;
 
 import org.apache.sis.storage.DataStoreException;
 import org.apache.sis.storage.FeatureSet;
+import org.apache.sis.storage.Query;
 import org.apache.sis.storage.event.ChangeEvent;
 import org.apache.sis.storage.event.ChangeListener;
 
+/**
+ * A {@link Table} feature set on which a query has been applied.
+ * TODO: Override {@link #subset(Query)} method to allow stacking of SQL filter and sorting.
+ */
 public class TableSubset implements FeatureSet {
 
     final Table parent;
diff --git a/storage/sis-sqlstore/src/main/java/org/apache/sis/internal/sql/feature/package-info.java
b/storage/sis-sqlstore/src/main/java/org/apache/sis/internal/sql/feature/package-info.java
index 19c3151..e531ce5 100644
--- a/storage/sis-sqlstore/src/main/java/org/apache/sis/internal/sql/feature/package-info.java
+++ b/storage/sis-sqlstore/src/main/java/org/apache/sis/internal/sql/feature/package-info.java
@@ -29,6 +29,16 @@
  * It relies on internal {@link org.apache.sis.internal.sql.feature.SQLTypeSpecification}
API to fetch SQL schema
  * information, and build {@link org.apache.sis.internal.sql.feature.FeatureAdapter an adapter
to feature model from it}.
  *
+ * This package provides two main {@link org.apache.sis.storage.FeatureSet feature set} implementations:
+ * <ul>
+ *     <li>{@link org.apache.sis.internal.sql.feature.QueryFeatureSet}: execute a prepared
SQL query, then interpret its result as Simple Feature collection.</li>
+ *     <li>{@link org.apache.sis.internal.sql.feature.Table}: Analysis of SQL Table
to provide a complex feature type modeling associations.</li>
+ * </ul>
+ *
+ * TODO: a lot of code could be factorized to reduce splitting of code base for both use
cases above. Notably, all
+ * association management is done specifically in table implementation, but should be deported
in {@link org.apache.sis.internal.sql.feature.FeatureAdapter}.
+ * With that, we could reduce feature set implementations to only QueryFeatureSet, and delegating
model analysis upstream.
+ *
  * @author  Johann Sorel (Geomatys)
  * @author  Martin Desruisseaux (Geomatys)
  * @author  Alexis Manin (Geomatys)
diff --git a/storage/sis-sqlstore/src/test/java/org/apache/sis/internal/sql/feature/FilterInterpreterTest.java
b/storage/sis-sqlstore/src/test/java/org/apache/sis/internal/sql/feature/FilterInterpreterTest.java
index 7c73016..b9f59d7 100644
--- a/storage/sis-sqlstore/src/test/java/org/apache/sis/internal/sql/feature/FilterInterpreterTest.java
+++ b/storage/sis-sqlstore/src/test/java/org/apache/sis/internal/sql/feature/FilterInterpreterTest.java
@@ -18,15 +18,12 @@ public class FilterInterpreterTest extends TestCase {
 
     @Test
     public void testGeometricFilter() {
-        final ANSIInterpreter interpreter = new ANSIInterpreter();
         final BBOX filter = FF.bbox(FF.property("Toto"), new GeneralEnvelope(new DefaultGeographicBoundingBox(-12.3,
2.1, 43.3, 51.7)));
         assertConversion(filter,
                 "ST_Intersects(" +
-                            "ST_Envelope(\"Toto\"), " +
-                            "ST_Envelope(" +
-                                "ST_GeomFromText(" +
-                                    "POLYGON ((-12.3 43.3, -12.3 51.7, 2.1 51.7, 2.1 43.3,
-12.3 43.3))" +
-                                ")" +
+                            "\"Toto\", " +
+                            "ST_GeomFromText(" +
+                                "'POLYGON ((-12.3 43.3, -12.3 51.7, 2.1 51.7, 2.1 43.3, -12.3
43.3))'" +
                             ")" +
                         ")"
         );
diff --git a/storage/sis-sqlstore/src/test/java/org/apache/sis/storage/sql/SQLStoreTest.java
b/storage/sis-sqlstore/src/test/java/org/apache/sis/storage/sql/SQLStoreTest.java
index 5142fa1..d1bcd97 100644
--- a/storage/sis-sqlstore/src/test/java/org/apache/sis/storage/sql/SQLStoreTest.java
+++ b/storage/sis-sqlstore/src/test/java/org/apache/sis/storage/sql/SQLStoreTest.java
@@ -20,6 +20,7 @@ import java.sql.Connection;
 import java.sql.SQLException;
 import java.util.Arrays;
 import java.util.Collection;
+import java.util.Collections;
 import java.util.HashMap;
 import java.util.HashSet;
 import java.util.Iterator;
@@ -157,8 +158,8 @@ public final strictfp class SQLStoreTest extends TestCase {
                         new Object[] {null,             String.class, String.class,  String.class,
  Integer.class, "Countries", "Parks"});
 
                 verifyFeatureType(((FeatureSet) store.findResource("Countries")).getType(),
-                        new String[] {"sis:identifier", "code",       "native_name",  "sis:Cities"},
-                        new Object[] {null,             String.class, String.class, "Cities"});
+                        new String[] {"sis:identifier", "code",       "native_name"},
+                        new Object[] {null,             String.class, String.class});
 
                 verifyFeatureType(((FeatureSet) store.findResource("Parks")).getType(),
                         new String[] {"sis:identifier", "country", "city",       "native_name",
"english_name", "sis:FK_City"},
@@ -234,6 +235,26 @@ public final strictfp class SQLStoreTest extends TestCase {
         verifyFetchCityTableAsQuery(source);
         verifyLimitOffsetAndColumnSelectionFromQuery(source);
         verifyDistinctQuery(source);
+        verifyNestedSQLQuery(source);
+    }
+
+    private void verifyNestedSQLQuery(DataSource source) throws Exception {
+        final QueryFeatureSet qfs;
+        try (Connection c = source.getConnection()) {
+            qfs = new QueryFeatureSet("SELECT * FROM features.\"Parks\"", source, c);
+        }
+
+        final SimpleQuery sq = new SimpleQuery();
+        sq.setSortBy(FF.sort("native_name", SortOrder.DESCENDING));
+        sq.setFilter(FF.equals(FF.property("country"), FF.literal("FRA")));
+        sq.setColumns(new SimpleQuery.Column(FF.property("native_name")));
+        final FeatureSet frenchParks = qfs.subset(sq);
+        checkQueryType(Collections.singletonMap("native_name", String.class), frenchParks.getType());
+        try (Stream<Feature> fs = frenchParks.features(false)) {
+            final Object[] queryResult = fs.map(f -> f.getPropertyValue("native_name"))
+                    .toArray(size -> new Object[size]);
+            assertArrayEquals(new String[]{"Jardin du Luxembourg", "Jardin des Tuileries"},
queryResult);
+        }
     }
 
     private void verifyFetchCityTableAsQuery(DataSource source) throws Exception {
@@ -304,7 +325,7 @@ public final strictfp class SQLStoreTest extends TestCase {
             final String pName = p.getName().toString();
             final Class expectedClass = expectedAttrs.get(pName);
             assertNotNull("Unexpected property: "+pName, expectedClass);
-            assertEquals("Unepected type for property: "+pName, expectedClass, ((AttributeType)p).getValueClass());
+            assertEquals("Unexpected type for property: "+pName, expectedClass, ((AttributeType)p).getValueClass());
         }
     }
 


Mime
View raw message