sis-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From desruisse...@apache.org
Subject [sis] branch geoapi-4.0 updated: Cache the features fetched by identifier. This allow to preserve the dependency graph when the same row is referenced (by foreigner keys) many times.
Date Tue, 17 Jul 2018 16:36:43 GMT
This is an automated email from the ASF dual-hosted git repository.

desruisseaux pushed a commit to branch geoapi-4.0
in repository https://gitbox.apache.org/repos/asf/sis.git


The following commit(s) were added to refs/heads/geoapi-4.0 by this push:
     new 50fac4c  Cache the features fetched by identifier. This allow to preserve the dependency graph when the same row is referenced (by foreigner keys) many times.
50fac4c is described below

commit 50fac4cd9bcf1cd46a8a8a4d89e1b87487d03410
Author: Martin Desruisseaux <martin.desruisseaux@geomatys.com>
AuthorDate: Tue Jul 17 18:35:00 2018 +0200

    Cache the features fetched by identifier. This allow to preserve the dependency graph when the same row is referenced (by foreigner keys) many times.
---
 .../apache/sis/internal/sql/feature/Analyzer.java  |  40 ++++-
 .../apache/sis/internal/sql/feature/Database.java  |   8 +-
 .../apache/sis/internal/sql/feature/Features.java  | 145 +++++++++++++---
 .../apache/sis/internal/sql/feature/Relation.java  | 189 ++++++++++++++++-----
 .../apache/sis/internal/sql/feature/Resources.java |   9 +-
 .../sis/internal/sql/feature/Resources.properties  |   3 +-
 .../internal/sql/feature/Resources_fr.properties   |   3 +-
 .../org/apache/sis/internal/sql/feature/Table.java |  95 +++++++++--
 .../org/apache/sis/storage/sql/SQLStoreTest.java   |  55 +++++-
 9 files changed, 445 insertions(+), 102 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 ecce4bc..74c802c 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
@@ -30,6 +30,7 @@ import java.util.logging.LogRecord;
 import javax.sql.DataSource;
 import java.sql.SQLException;
 import java.sql.DatabaseMetaData;
+import java.sql.ResultSet;
 import org.opengis.util.NameSpace;
 import org.opengis.util.NameFactory;
 import org.opengis.util.GenericName;
@@ -78,6 +79,15 @@ final class Analyzer {
     final NameFactory nameFactory;
 
     /**
+     * A pool of strings read from database metadata. Those strings are mostly catalog, schema and column names.
+     * The same names are repeated often (in primary keys, foreigner keys, <i>etc.</i>), and using a pool allows
+     * us to replace equal character strings by the same {@link String} instances.
+     *
+     * @see #getUniqueString(ResultSet, String)
+     */
+    private final Map<String,String> strings;
+
+    /**
      * The string to insert before wildcard characters ({@code '_'} or {@code '%'}) to escape.
      * This is used by {@link #escape(String)} before to pass argument values (e.g. table name)
      * to {@link DatabaseMetaData} methods expecting a pattern.
@@ -137,6 +147,7 @@ final class Analyzer {
         this.metadata    = metadata;
         this.listeners   = listeners;
         this.locale      = locale;
+        this.strings     = new HashMap<>();
         this.escape      = metadata.getSearchStringEscape();
         this.functions   = new SpatialFunctions(metadata);
         this.nameFactory = DefaultFactories.forBuildin(NameFactory.class);
@@ -198,6 +209,25 @@ final class Analyzer {
     }
 
     /**
+     * Reads a string from the given result set and return a unique instance of that string.
+     * This method should be invoked only for {@code String} instances that are going to be
+     * stored in {@link Table} or {@link Relation} structures; there is no point to invoke
+     * this method for example before to parse the string as a boolean.
+     *
+     * @param  reflect  the result set from which to read a string.
+     * @param  column   the column to read.
+     * @return the value in the given column, returned as a unique string.
+     */
+    final String getUniqueString(final ResultSet reflect, final String column) throws SQLException {
+        String value = reflect.getString(column);
+        if (value != null) {
+            final String p = strings.putIfAbsent(value, value);
+            if (p != null) value = p;
+        }
+        return value;
+    }
+
+    /**
      * Returns whether a table is reserved for database internal working.
      * If this method returns {@code false}, then the given table is a candidate
      * for use as a {@code FeatureType}.
@@ -255,13 +285,21 @@ final class Analyzer {
             table = new Table(this, id, isDependency);
             if (tables.put(name, table) != null) {
                 // Should never happen. If thrown, we have a bug (e.g. synchronization) in this package.
-                throw new DataStoreException(Resources.format(Resources.Keys.InternalError));
+                throw new DataStoreException(internalError());
             }
         }
         return table;
     }
 
     /**
+     * Returns a message for unexpected errors. Those errors are caused by a bug in this
+     * {@code org.apache.sis.internal.sql.feature} package instead than a database issue.
+     */
+    final String internalError() {
+        return Resources.forLocale(locale).getString(Resources.Keys.InternalError);
+    }
+
+    /**
      * Reports a warning. Duplicated warnings will be ignored.
      *
      * @param  key       one of {@link Resources.Keys} values.
diff --git a/storage/sis-sqlstore/src/main/java/org/apache/sis/internal/sql/feature/Database.java b/storage/sis-sqlstore/src/main/java/org/apache/sis/internal/sql/feature/Database.java
index 026a5e2..62a3bd8 100644
--- a/storage/sis-sqlstore/src/main/java/org/apache/sis/internal/sql/feature/Database.java
+++ b/storage/sis-sqlstore/src/main/java/org/apache/sis/internal/sql/feature/Database.java
@@ -119,14 +119,14 @@ public final class Database {
             final String[] names = TableReference.splitName(tableName);
             try (ResultSet reflect = analyzer.metadata.getTables(names[2], names[1], names[0], tableTypes)) {
                 while (reflect.next()) {
-                    final String table = reflect.getString(Reflection.TABLE_NAME);
+                    final String table = analyzer.getUniqueString(reflect, Reflection.TABLE_NAME);
                     if (analyzer.isIgnoredTable(table)) {
                         continue;
                     }
                     declared.add(new TableReference(
-                            reflect.getString(Reflection.TABLE_CAT),
-                            reflect.getString(Reflection.TABLE_SCHEM),
-                            table, reflect.getString(Reflection.REMARKS)));
+                            analyzer.getUniqueString(reflect, Reflection.TABLE_CAT),
+                            analyzer.getUniqueString(reflect, Reflection.TABLE_SCHEM), table,
+                            analyzer.getUniqueString(reflect, Reflection.REMARKS)));
                 }
             }
         }
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 1358bc4..af6309e 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
@@ -26,7 +26,9 @@ import java.sql.Statement;
 import java.sql.PreparedStatement;
 import java.sql.ResultSet;
 import java.sql.SQLException;
+import java.lang.reflect.Array;
 import org.apache.sis.internal.metadata.sql.SQLBuilder;
+import org.apache.sis.util.collection.WeakValueHashMap;
 import org.apache.sis.util.collection.BackingStoreException;
 
 // Branch-dependent imports
@@ -87,6 +89,25 @@ final class Features implements Spliterator<Feature>, Runnable {
     private ResultSet result;
 
     /**
+     * Feature instances already created, or {@code null} if the features created by this iterator are not cached.
+     * This map is used when requesting a feature by identifier, not when iterating over all features (note: we
+     * could perform an opportunistic check in a future SIS version). The same map may be shared by all iterators
+     * on the same {@link Table}, but {@link WeakValueHashMap} already provides the required synchronizations.
+     *
+     * <p>This {@code Features} class does not require the identifiers to be built from primary key columns.
+     * However if this map has been provided by {@link Table#instanceForPrimaryKeys()}, then the identifiers
+     * need to be primary keys with columns in the exact same order for allowing the same map to be shared.</p>
+     */
+    private final WeakValueHashMap<?,Object> instances;
+
+    /**
+     * The component class of the keys in the {@link #instances} map, or {@code null} if the keys are not array.
+     * For example if a primary key is made of two columns of type {@code String}, then this field may be set to
+     * {@code String}.
+     */
+    private final Class<?> keyComponentClass;
+
+    /**
      * Estimated number of rows, or {@literal <= 0} if unknown.
      */
     private final long estimatedSize;
@@ -127,7 +148,7 @@ final class Features implements Spliterator<Feature>, Runnable {
             for (int i=0; i<n;) {
                 final Relation dependency = importedKeys[i];
                 associationNames[i] = dependency.getPropertyName();
-                importedFeatures[i] = dependency.getRelatedTable().features(connection, dependency);
+                importedFeatures[i] = dependency.getSearchTable().features(connection, dependency);
                 for (final String column : dependency.getForeignerKeys()) {
                     if (count != 0) sql.append(',');
                     sql.append(' ').append(column);
@@ -141,24 +162,47 @@ final class Features implements Spliterator<Feature>, Runnable {
             foreignerKeyIndices = null;
         }
         /*
-         * Create a Statement if we don't need any condition, or a PreparedStatement
-         * if we need to add a "WHERE" clause.
+         * Create a Statement if we don't need any condition, or a PreparedStatement if we need to add
+         * a "WHERE" clause. In the later case, we will cache the features already created if there is
+         * a possibility that many rows reference the same feature instance.
          */
         sql.append(" FROM ").appendIdentifier(table.schema, table.table);
         if (componentOf == null) {
             statement = null;
+            instances = null;       // A future SIS version could use the map opportunistically if it exists.
+            keyComponentClass = null;
             result = connection.createStatement().executeQuery(sql.toString());
         } else {
             String separator = " WHERE ";
-            for (String primaryKey : componentOf.getPrimaryKeys()) {
+            for (String primaryKey : componentOf.getSearchColumns()) {
                 sql.append(separator).append(primaryKey).append("=?");
                 separator = " AND ";
             }
             statement = connection.prepareStatement(sql.toString());
+            /*
+             * Following assumes that the foreigner key references the primary key of this table,
+             * in which case 'table.primaryKeyClass' should never be null. This assumption may not
+             * hold if the relation has been defined by DatabaseMetaData.getCrossReference(…) instead.
+             */
+            if (componentOf.isFullKey()) {
+                instances = table.instanceForPrimaryKeys();
+                keyComponentClass = table.primaryKeyClass.getComponentType();
+            } else {
+                instances = new WeakValueHashMap<>(Object.class);       // Can not share the table cache.
+                keyComponentClass = Object.class;
+            }
         }
     }
 
     /**
+     * Returns an array of the given length capable to hold the identifier,
+     * or {@code null} if there is no need for an array.
+     */
+    private Object identifierArray(final int columnCount) {
+        return (columnCount > 1) ? Array.newInstance(keyComponentClass, columnCount) : null;
+    }
+
+    /**
      * Declares that this iterator never returns {@code null} elements.
      */
     @Override
@@ -224,55 +268,100 @@ final class Features implements Spliterator<Feature>, Runnable {
             if (importedFeatures != null) {
                 for (int i=0; i < importedFeatures.length; i++) {
                     final Features dependency = importedFeatures[i];
-                    final int last = foreignerKeyIndices[i+1];
-                    for (int p=1, c = foreignerKeyIndices[i]; ++c <= last; p++) {
-                        dependency.statement.setObject(p, result.getObject(c));
+                    int columnIndice = foreignerKeyIndices[i];
+                    final int columnCount = foreignerKeyIndices[i+1] - columnIndice;
+                    /*
+                     * If the foreigner key uses only one column, we will store the foreigner key value
+                     * in the 'key' variable without creating array. But if the foreigner key uses more
+                     * than one column, then we need to create an array holding all values.
+                     */
+                    final Object keys = dependency.identifierArray(columnCount);
+                    Object key = null;
+                    for (int p=0; p < columnCount;) {
+                        key = result.getObject(++columnIndice);
+                        if (keys != null) Array.set(keys, p, key);
+                        dependency.statement.setObject(++p, key);
                     }
-                    final Object value = dependency.fetchReferenced();
+                    if (keys != null) key = keys;
+                    final Object value = dependency.fetchReferenced(key);
                     feature.setPropertyValue(associationNames[i], value);
                 }
             }
             action.accept(feature);
-            if (!all) {
-                return true;
-            }
+            if (!all) return true;
         }
         return false;
     }
 
     /**
      * Executes the current {@link #statement} and stores all features in a list.
-     * Returns {@code null} if there is no feature, the feature instance if there
-     * is only one, and a list of features otherwise.
+     * Returns {@code null} if there is no feature, or returns the feature instance
+     * if there is only one such instance, or returns a list of features otherwise.
      */
-    private Object fetchReferenced() throws SQLException {
-        final List<Feature> instances = new ArrayList<>();
+    private Object fetchReferenced(final Object key) throws SQLException {
+        if (key != null) {
+            Object existing = instances.get(key);
+            if (existing != null) {
+                return existing;
+            }
+        }
+        final List<Feature> features = new ArrayList<>();
         try (ResultSet r = statement.executeQuery()) {
             result = r;
-            fetch(instances::add, true);
+            fetch(features::add, true);
+        } finally {
+            result = null;
         }
-        switch (instances.size()) {
-            case 0:  return null;
-            case 1:  return instances.get(0);
-            default: return instances;
+        Object feature;
+        switch (features.size()) {
+            case 0:  feature = null; break;
+            case 1:  feature = features.get(0); break;
+            default: feature = features; break;
         }
+        if (key != null) {
+            @SuppressWarnings("unchecked")          // Check is performed by putIfAbsent(…).
+            final Object previous = ((WeakValueHashMap) instances).putIfAbsent(key, feature);
+            if (previous != null) {
+                feature = previous;
+            }
+        }
+        return feature;
     }
 
     /**
-     * Closes the (pooled) connection.
+     * Closes the (pooled) connection, including the statements of all dependencies.
      */
-    @Override
-    public void run() {
-        try {
-            final Statement s = result.getStatement();
+    private void close() throws SQLException {
+        /*
+         * Only one of 'statement' and 'result' should be non-null. The connection should be closed
+         * by the 'Features' instance having a non-null 'result' because it is the main one created
+         * by 'Table.features(boolean)' method. The other 'Features' instances are dependencies.
+         */
+        if (statement != null) {
+            statement.close();
+        }
+        final ResultSet r = result;
+        if (r != null) {
+            result = null;
+            final Statement s = r.getStatement();
             try (Connection c = s.getConnection()) {
-                result.close();
+                r.close();      // Implied by s.close() according JDBC javadoc, but we are paranoiac.
                 s.close();
                 for (final Features dependency : importedFeatures) {
-                    dependency.statement.close();
+                    dependency.close();
                 }
-                // No need to close this.statement because it is null.
             }
+        }
+    }
+
+    /**
+     * Closes the (pooled) connection, including the statements of all dependencies.
+     * This is a handler to be invoked by {@link java.util.stream.Stream#close()}.
+     */
+    @Override
+    public void run() {
+        try {
+            close();
         } catch (SQLException e) {
             throw new BackingStoreException(e);
         }
diff --git a/storage/sis-sqlstore/src/main/java/org/apache/sis/internal/sql/feature/Relation.java b/storage/sis-sqlstore/src/main/java/org/apache/sis/internal/sql/feature/Relation.java
index 8bed2c1..22f6290 100644
--- a/storage/sis-sqlstore/src/main/java/org/apache/sis/internal/sql/feature/Relation.java
+++ b/storage/sis-sqlstore/src/main/java/org/apache/sis/internal/sql/feature/Relation.java
@@ -16,9 +16,12 @@
  */
 package org.apache.sis.internal.sql.feature;
 
+import java.util.Arrays;
 import java.util.Map;
+import java.util.HashMap;
 import java.util.LinkedHashMap;
 import java.util.Collection;
+import java.util.Iterator;
 import java.util.Objects;
 import java.sql.ResultSet;
 import java.sql.SQLException;
@@ -77,9 +80,11 @@ final class Relation extends TableReference {
 
         /*
          * Note: another possible type of relation is the one provided by getCrossReference(…).
-         * Inconvenient is that it requires to know the tables on both side of the relation.
-         * But advantage is that it work with any set of columns having unique values
-         * (not necessarily the primary key).
+         * Inconvenient is that we need to specify the table names on both sides of the relation.
+         * But advantage is that it works with any set of columns having unique values, not only
+         * the primary keys. However if we use getCrossReference(…), then we need to keep in mind
+         * that Table.instances is only for instances identified by their primary keys and shall
+         * not be used for instances identified by the columns returned by getCrossReference(…).
          */
 
         /**
@@ -117,21 +122,24 @@ final class Relation extends TableReference {
     private final Map<String,String> columns;
 
     /**
-     * Whether entries in foreigner table will be deleted if the primary keys in the referenced table is deleted.
-     * This is used as a hint for detecting what may be the "main" table in a relation.
+     * The other table identified by {@link #catalog}, {@link #schema} and {@link #table} names.
+     * This is set during {@link Table} construction and should not be modified after that point.
      */
-    final boolean cascadeOnDelete;
+    private Table searchTable;
 
     /**
-     * The other table which is related to the table containing this relation.
-     * This is set during {@link Table} construction and should not be modified after that point.
+     * The name of the feature property where the association to {@link #searchTable} table will be stored.
      */
-    private Table related;
+    private String propertyName;
 
     /**
-     * The name of the feature property where the association to {@link #related} table will be stored.
+     * Whether the {@link #columns} map include all primary key columns. This field is set to {@code false}
+     * if the foreigner key uses only a subset of the primary key columns, in which case the referenced rows
+     * may not be unique.
+     *
+     * @see #isFullKey()
      */
-    private String propertyName;
+    private boolean isFullKey;
 
     /**
      * Creates a new relation for an imported key. The given {@code ResultSet} must be positioned
@@ -151,21 +159,18 @@ final class Relation extends TableReference {
      * or be closed if the last row has been reached. This constructor always moves the given result set by at
      * least one row, unless an exception occurs.</p>
      */
-    Relation(final Direction dir, final ResultSet reflect) throws SQLException, DataStoreContentException {
-        super(reflect.getString(dir.catalog),
-              reflect.getString(dir.schema),
-              reflect.getString(dir.table),
-              reflect.getString(Reflection.FK_NAME));
+    Relation(final Analyzer analyzer, final Direction dir, final ResultSet reflect) throws SQLException, DataStoreContentException {
+        super(analyzer.getUniqueString(reflect, dir.catalog),
+              analyzer.getUniqueString(reflect, dir.schema),
+              analyzer.getUniqueString(reflect, dir.table),
+              analyzer.getUniqueString(reflect, Reflection.FK_NAME));
 
         final Map<String,String> m = new LinkedHashMap<>();
-        boolean cascade = false;
         do {
-            final String column = reflect.getString(dir.column);
-            if (m.put(column, reflect.getString(dir.containerColumn)) != null) {
-                throw new DataStoreContentException(Resources.format(Resources.Keys.DuplicatedEntity_2, "Column", column));
-            }
-            if (!cascade) {
-                cascade = reflect.getInt(Reflection.DELETE_RULE) == DatabaseMetaData.importedKeyCascade;
+            final String column = analyzer.getUniqueString(reflect, dir.column);
+            if (m.put(column, analyzer.getUniqueString(reflect, dir.containerColumn)) != null) {
+                throw new DataStoreContentException(Resources.forLocale(analyzer.locale)
+                        .getString(Resources.Keys.DuplicatedColumn_1, column));
             }
             if (!reflect.next()) {
                 reflect.close();
@@ -176,50 +181,144 @@ final class Relation extends TableReference {
                  Objects.equals(catalog, reflect.getString(dir.catalog)));
 
         columns = CollectionsExt.compact(m);
-        cascadeOnDelete = cascade;
     }
 
     /**
-     * Invoked after construction for setting the target table.
-     * Shall be invoked exactly once.
+     * Invoked after construction for setting the table identified by {@link #catalog}, {@link #schema}
+     * and {@link #table} names. Shall be invoked exactly once.
+     *
+     * @param  search       the other table containing the primary key ({@link Direction#IMPORT})
+     *                      or the foreigner key ({@link Direction#EXPORT}) of this relation.
+     * @param  property     name of the property of the enclosing {@code Table.featureType}
+     *                      where to store the feature instances read from the other table.
+     * @param  primaryKeys  the primary key columns of the relation. May be the primary key columns of this table
+     *                      or the primary key columns of the other table, depending on {@link Direction}.
+     * @param  direction    the direction of this {@code Relation}.
      */
-    final void setRelatedTable(final Table table, final String property) throws DataStoreException {
-        if (related != null) {
-            throw new DataStoreException(Resources.format(Resources.Keys.InternalError));     // Should never happen.
+    final void setSearchTable(final Analyzer analyzer, final Table search, final String property,
+                              final String[] primaryKeys, final Direction direction) throws DataStoreException
+    {
+        /*
+         * Sanity check (could be assertion): name of the given table
+         * must match the name expected by this relation.
+         */
+        String expected, actual;    // Identity comparison okay because of Analyzer.getUniqueString(…)
+        if ((expected = table)  != (actual = search.table)  ||
+            (expected = schema) != (actual = search.schema) || searchTable != null)
+        {
+            String message = analyzer.internalError();                                  // Should never happen.
+            if (expected != actual) message += ' ' + actual + " ≠ " + expected;
+            throw new DataStoreException(message);
         }
-        related = table;
+        /*
+         * Store the specified table and property name, and verify if this relation
+         * contains all columns required by the primary key.
+         */
+        searchTable  = search;
         propertyName = property;
+        final Collection<String> referenced;        // Primary key components referenced by this relation.
+        switch (direction) {
+            case IMPORT: referenced = columns.keySet(); break;
+            case EXPORT: referenced = columns.values(); break;
+            default: throw new AssertionError(direction);
+        }
+        isFullKey = referenced.containsAll(Arrays.asList(primaryKeys));
+        if (isFullKey && getKeySize() >= 2) {
+            /*
+             * Sort the columns in the order expected by the primary key.  This is required because we need
+             * a consistent order in the cache provided by Table.instanceForPrimaryKeys() even if different
+             * relations declare different column order in their foreigner key. Note that the 'columns' map
+             * is unmodifiable if its size is less than 2, and modifiable otherwise.
+             */
+            final Map<String,String> copy = new HashMap<>(columns);
+            columns.clear();
+            for (String key : primaryKeys) {
+                String value = key;
+                switch (direction) {
+                    case IMPORT: {                                      // Primary keys are 'columns' keys.
+                        value = copy.remove(key);
+                        break;
+                    }
+                    case EXPORT: {                                      // Primary keys are 'columns' values.
+                        key = null;
+                        for (final Iterator<Map.Entry<String,String>> it = copy.entrySet().iterator(); it.hasNext();) {
+                            final Map.Entry<String,String> e = it.next();
+                            if (value.equals(e.getValue())) {
+                                key = e.getKey();
+                                it.remove();
+                                break;
+                            }
+                        }
+                        break;
+                    }
+                }
+                if (key == null || value == null || columns.put(key, value) != null) {
+                    throw new DataStoreException(analyzer.internalError());
+                }
+            }
+            if (!copy.isEmpty()) {
+                throw new DataStoreContentException(Resources.forLocale(analyzer.locale)
+                            .getString(Resources.Keys.MalformedForeignerKey_2, freeText,
+                                       CollectionsExt.first(copy.keySet())));
+            }
+        }
+    }
+
+    /**
+     * Returns the other table identified by {@link #catalog}, {@link #schema} and {@link #table} names.
+     * This is the table where the search operations will be performed. In other words, this is part of
+     * the following pseudo-query:
+     *
+     * <pre>{@code SELECT * FROM <search table> WHERE <search columns> = ...}</pre>
+     */
+    final Table getSearchTable() {
+        if (searchTable != null) {
+            return searchTable;
+        }
+        throw new IllegalStateException(super.toString());              // Should not happen.
     }
 
     /**
-     * Returns the other table which is related to the table containing this relation.
-     * See {@link Direction} for more details about the relation.
+     * Returns the columns of the {@linkplain #getSearchTable() search table}
+     * which will need to appear in the {@code WHERE} clause.
+     * For {@link Direction#IMPORT}, they are primary key columns.
+     * For {@link Direction#EXPORT}, they are foreigner key columns.
      */
-    final Table getRelatedTable() {
-        return related;
+    final Collection<String> getSearchColumns() {
+        return columns.keySet();
     }
 
     /**
-     * Returns the name of the feature property where the association to {@link #related} table will be stored.
+     * Returns the foreigner key columns of the table that contains this relation.
+     * This method should be invoked only for {@link Direction#IMPORT} (otherwise the method name is wrong).
+     * This method returns only the foreigner key columns known to this relation; this is not necessarily
+     * all the enclosing table foreigner keys. Some columns may be used in more than one relation.
      */
-    final String getPropertyName() {
-        return propertyName;
+    final Collection<String> getForeignerKeys() {
+        return columns.values();
     }
 
     /**
-     * Returns the primary keys of the table related by the foreigner keys.
+     * Returns the number of columns used by the foreigner key.
      */
-    final Collection<String> getPrimaryKeys() {
-        return columns.keySet();
+    final int getKeySize() {
+        return columns.size();
     }
 
     /**
-     * Returns the foreigner keys of the table that contains this relation. This method returns only
-     * the foreigner keys known to this relation; this is not necessarily all the table foreigner keys.
-     * Some columns may be used in more than one relation.
+     * Returns {@code true} if this relation includes all required primary key columns. Returns {@code false}
+     * if the foreigner key uses only a subset of the primary key columns, in which case the referenced rows
+     * may not be unique.
      */
-    final Collection<String> getForeignerKeys() {
-        return columns.values();
+    final boolean isFullKey() {
+        return isFullKey;
+    }
+
+    /**
+     * Returns the name of the feature property where the association to {@link #searchTable} table will be stored.
+     */
+    final String getPropertyName() {
+        return propertyName;
     }
 
     /**
diff --git a/storage/sis-sqlstore/src/main/java/org/apache/sis/internal/sql/feature/Resources.java b/storage/sis-sqlstore/src/main/java/org/apache/sis/internal/sql/feature/Resources.java
index e6d6358..e2ef0c5 100644
--- a/storage/sis-sqlstore/src/main/java/org/apache/sis/internal/sql/feature/Resources.java
+++ b/storage/sis-sqlstore/src/main/java/org/apache/sis/internal/sql/feature/Resources.java
@@ -66,9 +66,9 @@ public final class Resources extends IndexedResourceBundle {
         public static final short DataSource = 1;
 
         /**
-         * Unexpected duplication of “{0}” entity named “{1}”.
+         * Unexpected duplication of column named “{0}”.
          */
-        public static final short DuplicatedEntity_2 = 5;
+        public static final short DuplicatedColumn_1 = 5;
 
         /**
          * “{0}” is not a valid qualified name for a table.
@@ -81,6 +81,11 @@ public final class Resources extends IndexedResourceBundle {
         public static final short InternalError = 6;
 
         /**
+         * Unexpected column “{1}” in the “{0}” foreigner key.
+         */
+        public static final short MalformedForeignerKey_2 = 7;
+
+        /**
          * Table names, optionally with their schemas and catalogs.
          */
         public static final short QualifiedTableNames = 2;
diff --git a/storage/sis-sqlstore/src/main/java/org/apache/sis/internal/sql/feature/Resources.properties b/storage/sis-sqlstore/src/main/java/org/apache/sis/internal/sql/feature/Resources.properties
index ff78d5a..7981c36 100644
--- a/storage/sis-sqlstore/src/main/java/org/apache/sis/internal/sql/feature/Resources.properties
+++ b/storage/sis-sqlstore/src/main/java/org/apache/sis/internal/sql/feature/Resources.properties
@@ -20,8 +20,9 @@
 # For resources shared by all modules in the Apache SIS project, see "org.apache.sis.util.resources" package.
 #
 DataSource                        = Provider of connections to the database.
-DuplicatedEntity_2                = Unexpected duplication of \u201c{0}\u201d entity named \u201c{1}\u201d.
+DuplicatedColumn_1                = Unexpected duplication of column named \u201c{0}\u201d.
 IllegalQualifiedName_1            = \u201c{0}\u201d is not a valid qualified name for a table.
 InternalError                     = Unexpected error while analyzing the database schema.
+MalformedForeignerKey_2           = Unexpected column \u201c{1}\u201d in the \u201c{0}\u201d foreigner key.
 QualifiedTableNames               = Table names, optionally with their schemas and catalogs.
 UnknownType_1                     = No mapping from SQL type \u201c{0}\u201d to a Java class.
diff --git a/storage/sis-sqlstore/src/main/java/org/apache/sis/internal/sql/feature/Resources_fr.properties b/storage/sis-sqlstore/src/main/java/org/apache/sis/internal/sql/feature/Resources_fr.properties
index a9e2f83..287784a 100644
--- a/storage/sis-sqlstore/src/main/java/org/apache/sis/internal/sql/feature/Resources_fr.properties
+++ b/storage/sis-sqlstore/src/main/java/org/apache/sis/internal/sql/feature/Resources_fr.properties
@@ -25,8 +25,9 @@
 #   U+00A0 NO-BREAK SPACE         before  :
 #
 DataSource                        = Fournisseur de connexions \u00e0 la base de donn\u00e9es.
-DuplicatedEntity_2                = Doublon inattendu d\u2019une entit\u00e9 \u00ab\u202f{0}\u202f\u00bb nomm\u00e9e \u00ab\u202f{1}\u202f\u00bb.
+DuplicatedColumn_1                = Doublon inattendu d\u2019une colonne nomm\u00e9e \u00ab\u202f{0}\u202f\u00bb.
 IllegalQualifiedName_1            = \u00ab\u202f{0}\u202f\u00bb n\u2019est pas un nom qualifi\u00e9 de table valide.
 InternalError                     = Erreur inattendue pendant l\u2019analyse du sch\u00e9ma de la base de donn\u00e9es.
+MalformedForeignerKey_2           = Colonne \u00ab\u202f{1}\u202f\u00bb inattendue dans la cl\u00e9 \u00e9trang\u00e8re \u00ab\u202f{0}\u202f\u00bb.
 QualifiedTableNames               = Noms de tables, optionnellemment avec leurs noms de sch\u00e9mas et catalogues.
 UnknownType_1                     = Pas de correspondance entre le type SQL \u00ab\u202f{0}\u202f\u00bb et une classe Java.
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 b150d6a..2a73fef 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
@@ -43,9 +43,12 @@ import org.apache.sis.internal.metadata.sql.SQLUtilities;
 import org.apache.sis.internal.storage.AbstractFeatureSet;
 import org.apache.sis.internal.util.CollectionsExt;
 import org.apache.sis.storage.DataStoreException;
+import org.apache.sis.util.collection.WeakValueHashMap;
 import org.apache.sis.util.collection.TreeTable;
 import org.apache.sis.util.CharSequences;
 import org.apache.sis.util.Exceptions;
+import org.apache.sis.util.Classes;
+import org.apache.sis.util.Numbers;
 import org.apache.sis.util.Debug;
 
 // Branch-dependent imports
@@ -81,7 +84,7 @@ final class Table extends AbstractFeatureSet {
     final FeatureType featureType;
 
     /**
-     * The name in the database of this {@code Table} object, together with its schema.
+     * The name in the database of this {@code Table} object, as a table name and a schema name.
      */
     final String schema, table;
 
@@ -100,6 +103,11 @@ final class Table extends AbstractFeatureSet {
     private final String[] attributeColumns;
 
     /**
+     * The columns that constitute the primary key, or {@code null} if there is no primary key.
+     */
+    private final String[] primaryKeys;
+
+    /**
      * The primary keys of other tables that are referenced by this table foreign key columns.
      * They are 0:1 relations. May be {@code null} if there is no imported keys.
      */
@@ -112,6 +120,24 @@ final class Table extends AbstractFeatureSet {
     private final Relation[] exportedKeys;
 
     /**
+     * The class of primary key values, or {@code null} if there is no primary keys.
+     * If the primary keys use more than one column, then this field is the class of
+     * an array; it may be an array of primitive type.
+     */
+    final Class<?> primaryKeyClass;
+
+    /**
+     * Feature instances already created for given primary keys. This map is used only when requesting feature
+     * instances by identifiers (not for iterating over all features) and those identifiers are primary keys.
+     * We create this map only for tables referenced by foreigner keys of other tables as enumerated by the
+     * {@link Relation.Direction#IMPORT} and {@link Relation.Direction#EXPORT} cases; not for arbitrary
+     * cross-reference cases. Values are usually {@code Feature} instances, but may also be {@code Collection<Feature>}.
+     *
+     * @see #instanceForPrimaryKeys()
+     */
+    private WeakValueHashMap<?,Object> instanceForPrimaryKeys;
+
+    /**
      * {@code true} if this table contains at least one geometry column.
      */
     final boolean hasGeometry;
@@ -149,10 +175,11 @@ final class Table extends AbstractFeatureSet {
         final Map<String,Boolean> primaryKeys = new LinkedHashMap<>();
         try (ResultSet reflect = analyzer.metadata.getPrimaryKeys(id.catalog, id.schema, id.table)) {
             while (reflect.next()) {
-                primaryKeys.put(reflect.getString(Reflection.COLUMN_NAME), null);
+                primaryKeys.put(analyzer.getUniqueString(reflect, Reflection.COLUMN_NAME), null);
                 // The actual Boolean value will be fetched in the loop on columns later.
             }
         }
+        this.primaryKeys = primaryKeys.isEmpty() ? null : primaryKeys.keySet().toArray(new String[primaryKeys.size()]);
         /*
          * Creates a list of associations between the table read by this method and other tables.
          * The associations are defined by the foreigner keys referencing primary keys. Note that
@@ -168,7 +195,7 @@ final class Table extends AbstractFeatureSet {
         final Map<String, List<Relation>> foreignerKeys = new HashMap<>();
         try (ResultSet reflect = analyzer.metadata.getImportedKeys(id.catalog, id.schema, id.table)) {
             if (reflect.next()) do {
-                Relation relation = new Relation(Relation.Direction.IMPORT, reflect);
+                Relation relation = new Relation(analyzer, Relation.Direction.IMPORT, reflect);
                 importedKeys.add(relation);
                 for (final String column : relation.getForeignerKeys()) {
                     CollectionsExt.addToMultiValuesMap(foreignerKeys, column, relation);
@@ -183,7 +210,7 @@ final class Table extends AbstractFeatureSet {
             exportedKeys = new ArrayList<>();
             try (ResultSet reflect = analyzer.metadata.getExportedKeys(id.catalog, id.schema, id.table)) {
                 if (reflect.next()) do {
-                    exportedKeys.add(new Relation(Relation.Direction.EXPORT, reflect));
+                    exportedKeys.add(new Relation(analyzer, Relation.Direction.EXPORT, reflect));
                 } while (!reflect.isClosed());
             }
         }
@@ -193,14 +220,16 @@ final class Table extends AbstractFeatureSet {
          * nullability. Attribute names are added in the 'attributeNames' and 'attributeColumns' list. Those
          * names are usually the same, except when a column is used both as a primary key and as foreigner key.
          */
-        boolean hasGeometry = false;
-        int startWithLowerCase = 0;
+        Class<?> primaryKeyClass   = null;
+        boolean  primaryKeyNonNull = true;
+        boolean  hasGeometry       = false;
+        int startWithLowerCase     = 0;
         final List<String> attributeNames = new ArrayList<>();
         final List<String> attributeColumns = new ArrayList<>();
         final FeatureTypeBuilder feature = new FeatureTypeBuilder(analyzer.nameFactory, analyzer.functions.library, analyzer.locale);
         try (ResultSet reflect = analyzer.metadata.getColumns(id.catalog, schemaEsc, tableEsc, null)) {
             while (reflect.next()) {
-                final String         column       = reflect.getString(Reflection.COLUMN_NAME);
+                final String         column       = analyzer.getUniqueString(reflect, Reflection.COLUMN_NAME);
                 final boolean        mandatory    = Boolean.FALSE.equals(SQLUtilities.parseBoolean(reflect.getString(Reflection.IS_NULLABLE)));
                 final boolean        isPrimaryKey = primaryKeys.containsKey(column);
                 final List<Relation> dependencies = foreignerKeys.get(column);
@@ -248,8 +277,11 @@ final class Table extends AbstractFeatureSet {
                      */
                     if (isPrimaryKey) {
                         attribute.addRole(AttributeRole.IDENTIFIER_COMPONENT);
+                        primaryKeyNonNull &= mandatory;
+                        primaryKeyClass = Classes.findCommonClass(primaryKeyClass, type);
                         if (primaryKeys.put(column, SQLUtilities.parseBoolean(reflect.getString(Reflection.IS_AUTOINCREMENT))) != null) {
-                            throw new DataStoreContentException(Resources.format(Resources.Keys.DuplicatedEntity_2, "Column", column));
+                            throw new DataStoreContentException(Resources.forLocale(analyzer.locale)
+                                    .getString(Resources.Keys.DuplicatedColumn_1, column));
                         }
                     }
                     if (Geometries.isKnownType(type)) {
@@ -275,10 +307,22 @@ final class Table extends AbstractFeatureSet {
                         if (dependency != null) {
                             final GenericName typeName = dependency.getName(analyzer);
                             final Table table = analyzer.table(dependency, typeName, true);
-                            final String propertyName = (count++ == 0) ? column : column + '-' + count;
+                            /*
+                             * Use the column name as the association name, provided that the foreigner key
+                             * use only that column. If the foreigner key use more than one column, then we
+                             * do not know which column describes better the association (often there is none).
+                             * In such case we use the foreigner key name as a fallback.
+                             */
+                            String propertyName = null;
+                            if (dependency.getKeySize() > 1) {
+                                propertyName = dependency.freeText;             // Foreigner key name (may be null).
+                            }
+                            if (propertyName == null) {
+                                propertyName = (count++ == 0) ? column : column + '-' + count;
+                            }
                             final AssociationRoleBuilder association;
                             if (table != null) {
-                                dependency.setRelatedTable(table, propertyName);
+                                dependency.setSearchTable(analyzer, table, propertyName, table.primaryKeys, Relation.Direction.IMPORT);
                                 association = feature.addAssociation(table.featureType);
                             } else {
                                 association = feature.addAssociation(typeName);     // May happen in case of cyclic dependency.
@@ -328,7 +372,7 @@ final class Table extends AbstractFeatureSet {
                 final Table table = analyzer.table(dependency, typeName, true);
                 final AssociationRoleBuilder association;
                 if (table != null) {
-                    dependency.setRelatedTable(table, propertyName);
+                    dependency.setSearchTable(analyzer, table, propertyName, this.primaryKeys, Relation.Direction.EXPORT);
                     association = feature.addAssociation(table.featureType);
                 } else {
                     association = feature.addAssociation(typeName);     // May happen in case of cyclic dependency.
@@ -339,6 +383,16 @@ final class Table extends AbstractFeatureSet {
             }
         }
         /*
+         * If the primary keys uses more than one column, we will need an array to store it.
+         * If all columns are non-null numbers, use primitive arrays instead than array of wrappers.
+         */
+        if (primaryKeys.size() > 1) {
+            if (primaryKeyNonNull) {
+                primaryKeyClass = Numbers.wrapperToPrimitive(primaryKeyClass);
+            }
+            primaryKeyClass = Classes.changeArrayDimension(primaryKeyClass, 1);
+        }
+        /*
          * Global information on the feature type (name, remarks).
          * The remarks are opportunistically stored in id.freeText if known by the caller.
          */
@@ -347,7 +401,7 @@ final class Table extends AbstractFeatureSet {
         if (id instanceof Relation) {
             try (ResultSet reflect = analyzer.metadata.getTables(id.catalog, schemaEsc, tableEsc, null)) {
                 while (reflect.next()) {
-                    remarks = reflect.getString(Reflection.REMARKS);
+                    remarks = analyzer.getUniqueString(reflect, Reflection.REMARKS);
                     if (remarks != null) {
                         remarks = remarks.trim();
                         if (remarks.isEmpty()) {
@@ -363,6 +417,7 @@ final class Table extends AbstractFeatureSet {
         this.featureType      = feature.build();
         this.importedKeys     = toArray(importedKeys);
         this.exportedKeys     = toArray(exportedKeys);
+        this.primaryKeyClass  = primaryKeyClass;
         this.hasGeometry      = hasGeometry;
         this.attributeNames   = attributeNames.toArray(new String[attributeNames.size()]);
         this.attributeColumns = attributeColumns.equals(attributeNames) ? this.attributeNames
@@ -423,11 +478,25 @@ final class Table extends AbstractFeatureSet {
      * Returns the feature type inferred from the database structure analysis.
      */
     @Override
-    public FeatureType getType() {
+    public final FeatureType getType() {
         return featureType;
     }
 
     /**
+     * Returns a cache for fetching feature instances by identifier. The map is created when this method is
+     * first invoked. Keys are primary key values, typically as {@code String} or {@code Integer} instances
+     * or arrays of those if the keys use more than one column. Values are usually {@code Feature} instances,
+     * but may also be {@code Collection<Feature>}.
+     */
+    @SuppressWarnings("ReturnOfCollectionOrArrayField")
+    final synchronized WeakValueHashMap<?,Object> instanceForPrimaryKeys() {
+        if (instanceForPrimaryKeys == null) {
+            instanceForPrimaryKeys = new WeakValueHashMap<>(primaryKeyClass);
+        }
+        return instanceForPrimaryKeys;
+    }
+
+    /**
      * Returns the number of rows, or -1 if unknown. Note that some database drivers returns 0,
      * so it is better to consider 0 as "unknown" too. We do not cache this count because it may
      * change at any time.
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 e77a309..48aa069 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
@@ -16,6 +16,8 @@
  */
 package org.apache.sis.storage.sql;
 
+import java.util.Map;
+import java.util.HashMap;
 import java.util.stream.Stream;
 import org.apache.sis.storage.FeatureSet;
 import org.apache.sis.storage.StorageConnector;
@@ -43,6 +45,18 @@ import org.opengis.feature.FeatureAssociationRole;
  */
 public final strictfp class SQLStoreTest extends TestCase {
     /**
+     * Number of time that the each country has been seen while iterating over the cities.
+     */
+    private final Map<String,Integer> countryCount = new HashMap<>();
+
+    /**
+     * The {@code Country} value for Canada, or {@code null} if not yet visited.
+     * This feature should appear twice, and all those occurrences should use the exact same instance.
+     * We use that for verifying the {@code Table.instanceForPrimaryKeys} caching.
+     */
+    private Feature canada;
+
+    /**
      * Tests reading an existing schema. The schema is created and populated by the {@code Features.sql} script.
      *
      * @throws Exception if an error occurred while testing the database.
@@ -55,22 +69,35 @@ public final strictfp class SQLStoreTest extends TestCase {
                     SQLStoreProvider.createTableName(null, "features", "Cities")))
             {
                 final FeatureSet cities = (FeatureSet) store.findResource("Cities");
-                verifyCityType(cities.getType());
+                verifyFeatureType(cities.getType(),
+                        new String[] {"sis:identifier", "pk:country", "country",   "native_name", "translation", "population",  "parks"},
+                        new Object[] {null,             String.class, "Countries", String.class,  String.class,  Integer.class, "Parks"});
+
+                verifyFeatureType(((FeatureSet) store.findResource("Countries")).getType(),
+                        new String[] {"sis:identifier", "code",       "native_name"},
+                        new Object[] {null,             String.class, String.class});
+
+                verifyFeatureType(((FeatureSet) store.findResource("Parks")).getType(),
+                        new String[] {"sis:identifier", "pk:country", "FK_City", "city",       "native_name", "translation"},
+                        new Object[] {null,             String.class, "Cities",  String.class, String.class,  String.class});
+
                 try (Stream<Feature> features = cities.features(false)) {
-                    features.forEach(SQLStoreTest::verifyContent);
+                    features.forEach((f) -> verifyContent(f));
                 }
             }
         }
+        assertEquals(Integer.valueOf(2), countryCount.remove("CAN"));
+        assertEquals(Integer.valueOf(1), countryCount.remove("FRA"));
+        assertEquals(Integer.valueOf(1), countryCount.remove("JPN"));
+        assertTrue  (countryCount.isEmpty());
     }
 
     /**
      * Verifies the result of analyzing the structure of the {@code "Cities"} table.
      */
-    private static void verifyCityType(final FeatureType cities) {
-        final String[] expectedNames = {"sis:identifier", "pk:country", "country",   "native_name", "translation", "population",  "parks"};
-        final Object[] expectedTypes = {null,             String.class, "Countries", String.class,  String.class,  Integer.class, "Parks"};
+    private static void verifyFeatureType(final FeatureType type, final String[] expectedNames, final Object[] expectedTypes) {
         int i = 0;
-        for (PropertyType pt : cities.getProperties(false)) {
+        for (PropertyType pt : type.getProperties(false)) {
             assertEquals("name", expectedNames[i], pt.getName().toString());
             final Object expectedType = expectedTypes[i];
             if (expectedType != null) {
@@ -94,10 +121,11 @@ public final strictfp class SQLStoreTest extends TestCase {
      * Verifies the content of the {@code Cities} table.
      * The features are in no particular order.
      */
-    private static void verifyContent(final Feature feature) {
+    private void verifyContent(final Feature feature) {
         final String city = feature.getPropertyValue("native_name").toString();
         final String country, countryName, cityLatin;
         final int population;
+        boolean isCanada = false;
         switch (city) {
             case "東京": {
                 cityLatin   = "Tōkyō";
@@ -118,6 +146,7 @@ public final strictfp class SQLStoreTest extends TestCase {
                 country     = "CAN";
                 countryName = "Canada";
                 population  = 1704694;          // In 2016.
+                isCanada    = true;
                 break;
             }
             case "Québec": {
@@ -125,6 +154,7 @@ public final strictfp class SQLStoreTest extends TestCase {
                 country     = "CAN";
                 countryName = "Canada";
                 population  = 531902;           // In 2016.
+                isCanada    = true;
                 break;
             }
             default: {
@@ -140,6 +170,17 @@ public final strictfp class SQLStoreTest extends TestCase {
 
         // Associations
         assertEquals("country", countryName, getIndirectPropertyValue(feature, "country", "native_name"));
+
+        // Caching
+        if (isCanada) {
+            final Feature f = (Feature) feature.getPropertyValue("country");
+            if (canada == null) {
+                canada = f;
+            } else {
+                assertSame(canada, f);              // Want exact same feature instance, not just equal.
+            }
+        }
+        countryCount.merge(country, 1, (o, n) -> n+1);
     }
 
     /**


Mime
View raw message