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 02f7f3f  Tune the meaning of "0 occurrence" of features in metadata. 
Better metadata about features in netCDF metadata.
02f7f3f is described below

commit 02f7f3ffa17b23a0f7b747fb7cd5b609210d6d0d
Author: Martin Desruisseaux <martin.desruisse...@geomatys.com>
AuthorDate: Sun Feb 27 18:38:42 2022 +0100

    Tune the meaning of "0 occurrence" of features in metadata.
    Better metadata about features in netCDF metadata.
---
 .../org/apache/sis/internal/netcdf/Convention.java |  8 ++--
 .../org/apache/sis/internal/netcdf/FeatureSet.java |  2 +-
 .../apache/sis/internal/netcdf/VariableRole.java   |  2 +-
 .../apache/sis/storage/netcdf/MetadataReader.java  | 54 ++++++++++++++--------
 .../org/apache/sis/internal/sql/feature/Table.java |  4 +-
 .../sis/internal/storage/MetadataBuilder.java      | 51 +++++++++++---------
 .../sis/internal/storage/MetadataBuilderTest.java  | 18 +++++---
 7 files changed, 83 insertions(+), 56 deletions(-)

diff --git 
a/storage/sis-netcdf/src/main/java/org/apache/sis/internal/netcdf/Convention.java
 
b/storage/sis-netcdf/src/main/java/org/apache/sis/internal/netcdf/Convention.java
index b8449c2..f14865a 100644
--- 
a/storage/sis-netcdf/src/main/java/org/apache/sis/internal/netcdf/Convention.java
+++ 
b/storage/sis-netcdf/src/main/java/org/apache/sis/internal/netcdf/Convention.java
@@ -224,8 +224,8 @@ public class Convention {
      *       to confuse them with images.</li>
      * </ul>
      *
-     * <p>The default implementation returns {@link VariableRole#FEATURE} if 
the given variable may be values
-     * for one feature property of a feature set. This detection is based on 
the number of dimensions.</p>
+     * <p>The default implementation returns {@link 
VariableRole#FEATURE_PROPERTY} if the given variable may be
+     * values for one feature property of a feature set. This detection is 
based on the number of dimensions.</p>
      *
      * @param  variable  the variable for which to get the role.
      * @return role of the given variable.
@@ -236,7 +236,7 @@ public class Convention {
         }
         final int n = variable.getNumDimensions();
         if (n == 1) {
-            return VariableRole.FEATURE;
+            return VariableRole.FEATURE_PROPERTY;
         } else if (n != 0) {
             final DataType dataType = variable.getDataType();
             int numVectors = 0;                 // Number of dimension having 
more than 1 value.
@@ -251,7 +251,7 @@ public class Convention {
                 }
             }
             if (n == Variable.STRING_DIMENSION && dataType == DataType.CHAR) {
-                return VariableRole.FEATURE;
+                return VariableRole.FEATURE_PROPERTY;
             }
         }
         return VariableRole.OTHER;
diff --git 
a/storage/sis-netcdf/src/main/java/org/apache/sis/internal/netcdf/FeatureSet.java
 
b/storage/sis-netcdf/src/main/java/org/apache/sis/internal/netcdf/FeatureSet.java
index da314f7..666ffcb 100644
--- 
a/storage/sis-netcdf/src/main/java/org/apache/sis/internal/netcdf/FeatureSet.java
+++ 
b/storage/sis-netcdf/src/main/java/org/apache/sis/internal/netcdf/FeatureSet.java
@@ -304,7 +304,7 @@ final class FeatureSet extends DiscreteSampling {
         final List<FeatureSet> features = new ArrayList<>(3);     // Will 
usually contain at most one element.
         final Map<Dimension,Boolean> done = new HashMap<>();      // Whether a 
dimension has already been used.
         for (final Variable v : decoder.getVariables()) {
-            if (v.getRole() != VariableRole.FEATURE) {
+            if (v.getRole() != VariableRole.FEATURE_PROPERTY) {
                 continue;
             }
             /*
diff --git 
a/storage/sis-netcdf/src/main/java/org/apache/sis/internal/netcdf/VariableRole.java
 
b/storage/sis-netcdf/src/main/java/org/apache/sis/internal/netcdf/VariableRole.java
index 97a092d..db5764f 100644
--- 
a/storage/sis-netcdf/src/main/java/org/apache/sis/internal/netcdf/VariableRole.java
+++ 
b/storage/sis-netcdf/src/main/java/org/apache/sis/internal/netcdf/VariableRole.java
@@ -46,7 +46,7 @@ public enum VariableRole {
     /**
      * The variable is a property of a feature.
      */
-    FEATURE,
+    FEATURE_PROPERTY,
 
     /**
      * Values of the variable are bounds of values of another variable.
diff --git 
a/storage/sis-netcdf/src/main/java/org/apache/sis/storage/netcdf/MetadataReader.java
 
b/storage/sis-netcdf/src/main/java/org/apache/sis/storage/netcdf/MetadataReader.java
index 3be066d..405b518 100644
--- 
a/storage/sis-netcdf/src/main/java/org/apache/sis/storage/netcdf/MetadataReader.java
+++ 
b/storage/sis-netcdf/src/main/java/org/apache/sis/storage/netcdf/MetadataReader.java
@@ -21,9 +21,9 @@ import java.net.URISyntaxException;
 import java.util.Date;
 import java.util.List;
 import java.util.Set;
-import java.util.LinkedHashSet;
 import java.util.Map;
-import java.util.HashMap;
+import java.util.LinkedHashSet;
+import java.util.LinkedHashMap;
 import java.util.Arrays;
 import java.util.ArrayList;
 import java.util.Collection;
@@ -60,6 +60,7 @@ import org.apache.sis.internal.netcdf.Axis;
 import org.apache.sis.internal.netcdf.Decoder;
 import org.apache.sis.internal.netcdf.Variable;
 import org.apache.sis.internal.netcdf.VariableRole;
+import org.apache.sis.internal.netcdf.Dimension;
 import org.apache.sis.internal.netcdf.Grid;
 import org.apache.sis.internal.storage.io.IOUtilities;
 import org.apache.sis.internal.storage.MetadataBuilder;
@@ -616,10 +617,9 @@ split:  while ((start = 
CharSequences.skipLeadingWhitespaces(value, start, lengt
      * @param  publisher   the publisher names, built by the caller in an 
opportunist way.
      */
     private void addIdentificationInfo(final Set<InternationalString> 
publisher) throws IOException, DataStoreException {
-        boolean     hasExtent   = false;
-        Set<String> project     = null;
-        Set<String> standard    = null;
-        boolean     hasDataType = false;
+        boolean     hasExtent = false;
+        Set<String> project   = null;
+        Set<String> standard  = null;
         final Set<String> keywords = new LinkedHashSet<>();
         for (final String path : searchPath) {
             decoder.setSearchPath(path);
@@ -630,9 +630,7 @@ split:  while ((start = 
CharSequences.skipLeadingWhitespaces(value, start, lengt
                 addAccessConstraint(forCodeName(Restriction.class, keyword));
             }
             addTopicCategory(forEnumName(TopicCategory.class, 
stringValue(TOPIC_CATEGORY)));
-            SpatialRepresentationType dt = 
forCodeName(SpatialRepresentationType.class, stringValue(DATA_TYPE));
-            addSpatialRepresentation(dt);
-            hasDataType |= (dt != null);
+            
addSpatialRepresentation(forCodeName(SpatialRepresentationType.class, 
stringValue(DATA_TYPE)));
             if (!hasExtent) {
                 /*
                  * Takes only ONE extent, because a netCDF file may declare 
many time the same
@@ -643,14 +641,6 @@ split:  while ((start = 
CharSequences.skipLeadingWhitespaces(value, start, lengt
             }
         }
         /*
-         * Add spatial representation type only if it was not explicitly given 
in the metadata.
-         * The call to getGridCandidates() may be relatively costly, so we 
don't want to invoke
-         * it without necessity.
-         */
-        if (!hasDataType && decoder.getGridCandidates().length != 0) {
-            addSpatialRepresentation(SpatialRepresentationType.GRID);
-        }
-        /*
          * For the following properties, use only the first non-empty 
attribute value found on the search path.
          */
         decoder.setSearchPath(searchPath);
@@ -887,7 +877,12 @@ split:  while ((start = 
CharSequences.skipLeadingWhitespaces(value, start, lengt
      */
     @SuppressWarnings("null")
     private void addContentInfo() {
-        final Map<List<String>, List<Variable>> contents = new HashMap<>(4);
+        /*
+         * Prepare a list of features and coverages, but without writing 
metadata now.
+         * We differ metadata writing for giving us a chance to group related 
contents.
+         */
+        final Set<Dimension> features = new LinkedHashSet<>();
+        final Map<List<String>, List<Variable>> coverages = new 
LinkedHashMap<>(4);
         for (final Variable variable : decoder.getVariables()) {
             if (VariableRole.isCoverage(variable)) {
                 final List<org.apache.sis.internal.netcdf.Dimension> 
dimensions = variable.getGridDimensions();
@@ -895,12 +890,31 @@ split:  while ((start = 
CharSequences.skipLeadingWhitespaces(value, start, lengt
                 for (int i=0; i<names.length; i++) {
                     names[i] = dimensions.get(i).getName();
                 }
-                CollectionsExt.addToMultiValuesMap(contents, 
Arrays.asList(names), variable);
+                CollectionsExt.addToMultiValuesMap(coverages, 
Arrays.asList(names), variable);
                 hasGridCoverages = true;
+            } else if (variable.getRole() == VariableRole.FEATURE_PROPERTY) {
+                /*
+                 * For feature property, we should take only the first 
dimension.
+                 * If a second dimension exists, it is for character strings.
+                 */
+                features.add(variable.getGridDimensions().get(0));
+            }
+        }
+        /*
+         * Now write the metadata. Note that the spatial repersentation types 
added below are actually
+         * parts of `DataIdentification` instead of `ContentInformation`, but 
we add them here because
+         * we have the information here.
+         */
+        if (!features .isEmpty()) 
addSpatialRepresentation(SpatialRepresentationType.TEXT_TABLE);
+        if (!coverages.isEmpty()) 
addSpatialRepresentation(SpatialRepresentationType.GRID);
+        for (final Dimension feature : features) {
+            final String name = feature.getName();
+            if (name != null) {
+                
addFeatureType(decoder.nameFactory.createLocalName(decoder.namespace, name), 
feature.length());
             }
         }
         final String processingLevel = stringValue(PROCESSING_LEVEL);
-        for (final List<Variable> group : contents.values()) {
+        for (final List<Variable> group : coverages.values()) {
             /*
              * Instantiate a CoverageDescription for each distinct set of 
netCDF dimensions
              * (e.g. longitude,latitude,time). This separation is based on the 
fact that a
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 295cb01..7e5d027 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
@@ -431,8 +431,8 @@ final class Table extends AbstractFeatureSet {
 
     /**
      * 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.
+     * so it is better to consider 0 as "unknown" too (see {@link 
FeatureIterator#estimatedSize}).
+     * We do not cache this count because it may change at any time.
      *
      * @param  metadata     information about the database.
      * @param  distinct     whether to count distinct values instead of all 
values.
diff --git 
a/storage/sis-storage/src/main/java/org/apache/sis/internal/storage/MetadataBuilder.java
 
b/storage/sis-storage/src/main/java/org/apache/sis/internal/storage/MetadataBuilder.java
index c679471..ca6d322 100644
--- 
a/storage/sis-storage/src/main/java/org/apache/sis/internal/storage/MetadataBuilder.java
+++ 
b/storage/sis-storage/src/main/java/org/apache/sis/internal/storage/MetadataBuilder.java
@@ -1901,33 +1901,42 @@ parse:      for (int i = 0; i < length;) {
      * Note that the {@link FeatureCatalogBuilder} subclasses can also be used 
for that chaining.
      *
      * @param  type         the feature type to add, or {@code null} for 
no-operation.
-     * @param  occurrences  number of instances of the given feature type, or 
a negative value if unknown. Note that if
-     *                      this value is 0, it will be considered "unknown", 
because ISO-19115 consider it invalid.
-     *                      However, this is a valid case in practice (it 
represents an empty dataset at the moment).
-     * @return the name of the added feature, or {@code null} if none.
+     * @param  occurrences  number of instances of the given feature type, or 
a negative value if unknown.
+     *         Note that ISO-19115 considers 0 as an invalid value. 
Consequently if 0, the feature is not added.
+     * @return the name of the added feature (even if not added to the 
metadata), or {@code null} if none.
      *
      * @see FeatureCatalogBuilder#define(FeatureType)
      */
     public final GenericName addFeatureType(final FeatureType type, final long 
occurrences) {
-        if (type != null) {
-            final GenericName name = type.getName();
-            if (name != null) {
-                final DefaultFeatureTypeInfo info = new 
DefaultFeatureTypeInfo(name);
-                /*
-                 * Note: Exclude 0 as valid instance count.
-                 * Reason: ISO-19115 consider 0 (empty dataset) as an invalid 
count. However, in practice, it can happen
-                 * to open data sources that contain no record. They're still 
valid datasets, because as long as their
-                 * structure is valid, there's no point in raising an error 
(at our level, without any other context
-                 * information) here.
-                 */
-                if (occurrences > 0) {
-                    info.setFeatureInstanceCount(shared((int) 
Math.min(occurrences, Integer.MAX_VALUE)));
-                }
-                addIfNotPresent(featureDescription().getFeatureTypeInfo(), 
info);
-                return name;
+        if (type == null) {
+            return null;
+        }
+        final GenericName name = type.getName();
+        addFeatureType(name, occurrences);
+        return name;
+    }
+
+    /**
+     * Adds descriptions for a feature of the given name.
+     * Storage location is:
+     *
+     * <ul>
+     *   <li>{@code metadata/contentInfo/featureTypes/featureTypeName}</li>
+     *   <li>{@code 
metadata/contentInfo/featureTypes/featureInstanceCount}</li>
+     * </ul>
+     *
+     * @param  name         name of the feature type to add, or {@code null} 
for no-operation.
+     * @param  occurrences  number of instances of the given feature type, or 
a negative value if unknown.
+     *         Note that ISO-19115 considers 0 as an invalid value. 
Consequently if 0, the feature is not added.
+     */
+    public final void addFeatureType(final GenericName name, final long 
occurrences) {
+        if (name != null && occurrences != 0) {
+            final DefaultFeatureTypeInfo info = new 
DefaultFeatureTypeInfo(name);
+            if (occurrences > 0) {
+                info.setFeatureInstanceCount(shared((int) 
Math.min(occurrences, Integer.MAX_VALUE)));
             }
+            addIfNotPresent(featureDescription().getFeatureTypeInfo(), info);
         }
-        return null;
     }
 
     /**
diff --git 
a/storage/sis-storage/src/test/java/org/apache/sis/internal/storage/MetadataBuilderTest.java
 
b/storage/sis-storage/src/test/java/org/apache/sis/internal/storage/MetadataBuilderTest.java
index 9512a7e..deacdb4 100644
--- 
a/storage/sis-storage/src/test/java/org/apache/sis/internal/storage/MetadataBuilderTest.java
+++ 
b/storage/sis-storage/src/test/java/org/apache/sis/internal/storage/MetadataBuilderTest.java
@@ -42,7 +42,7 @@ import org.opengis.feature.FeatureType;
  *
  * @author  Martin Desruisseaux (Geomatys)
  * @author  Alexis Manin (Geomatys)
- * @version 1.1
+ * @version 1.2
  * @since   0.8
  * @module
  */
@@ -125,8 +125,8 @@ public final strictfp class MetadataBuilderTest extends 
TestCase {
      * Tests {@link MetadataBuilder#addFeatureType(FeatureType, long)}.
      */
     @Test
-    public void feature_count_should_be_ignored_when_it_is_zero() {
-        verifyFeatureInstanceCount("Feature count should not be written if it 
is 0", null, 0);
+    public void feature_should_be_ignored_when_count_is_zero() {
+        verifyFeatureInstanceCount("Feature should not be written if count is 
0", null, 0);
     }
 
     /**
@@ -145,9 +145,13 @@ public final strictfp class MetadataBuilderTest extends 
TestCase {
         assertNotNull(name);
 
         final DefaultMetadata metadata = builder.build(true);
-        final ContentInformation content = 
getSingleton(metadata.getContentInfo());
-        assertInstanceOf("Metadata.contentInfo", 
FeatureCatalogueDescription.class, content);
-        final FeatureTypeInfo info = 
getSingleton(((FeatureCatalogueDescription) content).getFeatureTypeInfo());
-        assertEquals(errorMessage, expected, info.getFeatureInstanceCount());
+        if (valueToInsert == 0) {
+            assertTrue(metadata.getContentInfo().isEmpty());
+        } else {
+            final ContentInformation content = 
getSingleton(metadata.getContentInfo());
+            assertInstanceOf("Metadata.contentInfo", 
FeatureCatalogueDescription.class, content);
+            final FeatureTypeInfo info = 
getSingleton(((FeatureCatalogueDescription) content).getFeatureTypeInfo());
+            assertEquals(errorMessage, expected, 
info.getFeatureInstanceCount());
+        }
     }
 }

Reply via email to