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 1c8df66ca3a0b0045b73bc5c0095cd795f8bc64e Author: Alexis Manin <[email protected]> AuthorDate: Wed Oct 16 18:14:29 2019 +0200 feat(SQLStore): improve query feature set to delegate count and subqueries to target database. --- .../apache/sis/internal/sql/feature/Analyzer.java | 3 +- .../apache/sis/internal/sql/feature/Features.java | 16 ++++-- .../sis/internal/sql/feature/QueryFeatureSet.java | 65 ++++++++++++++++++---- .../sis/internal/sql/feature/SQLQueryAdapter.java | 31 +++++++---- .../org/apache/sis/internal/sql/feature/Table.java | 4 +- .../sis/internal/sql/feature/TableSubset.java | 5 ++ .../sis/internal/sql/feature/package-info.java | 10 ++++ .../org/apache/sis/storage/sql/SQLStoreTest.java | 27 ++++++++- 8 files changed, 126 insertions(+), 35 deletions(-) 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..42522b1 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 @@ -667,12 +667,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 +686,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..706130f 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 { @@ -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/Table.java b/storage/sis-sqlstore/src/main/java/org/apache/sis/internal/sql/feature/Table.java index b60ec46..bd35fbb 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 @@ -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/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()); } }
