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: Add tests and fix missed sharing of WKT values.
Date Wed, 18 Nov 2020 16:03:11 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 31a6086  Add tests and fix missed sharing of WKT values.
31a6086 is described below

commit 31a6086b2f8c97dc8143791b5326dc68eac38faa
Author: Martin Desruisseaux <martin.desruisseaux@geomatys.com>
AuthorDate: Wed Nov 18 17:02:07 2020 +0100

    Add tests and fix missed sharing of WKT values.
---
 .../java/org/apache/sis/io/wkt/StoredTree.java     | 57 +++++++++++---
 .../java/org/apache/sis/io/wkt/WKTDictionary.java  | 77 ++++++++++++++++--
 .../org/apache/sis/io/wkt/WKTDictionaryTest.java   | 91 +++++++++++++++++++++-
 3 files changed, 204 insertions(+), 21 deletions(-)

diff --git a/core/sis-referencing/src/main/java/org/apache/sis/io/wkt/StoredTree.java b/core/sis-referencing/src/main/java/org/apache/sis/io/wkt/StoredTree.java
index 5800f5b..d29c13c 100644
--- a/core/sis-referencing/src/main/java/org/apache/sis/io/wkt/StoredTree.java
+++ b/core/sis-referencing/src/main/java/org/apache/sis/io/wkt/StoredTree.java
@@ -21,6 +21,7 @@ import java.util.LinkedList;
 import java.util.Locale;
 import java.util.Map;
 import java.util.stream.Stream;
+import java.util.function.Consumer;
 import java.io.Serializable;
 import org.apache.sis.util.ArraysExt;
 import org.apache.sis.internal.referencing.WKTKeywords;
@@ -90,14 +91,15 @@ final class StoredTree implements Serializable {
          * separated array for making possible to share {@code Node} instances.
          */
         Node(final Deflater deflater, final Element element) {
-            keyword  = element.keyword;
+            keyword  = (String) deflater.unique(element.keyword);
             children = element.getChildren();
             if (children != null) {
                 for (int i=0; i<children.length; i++) {
-                    final Object child = children[i];
+                    Object child = children[i];
                     if (child instanceof Element) {
-                        children[i] = deflater.unique(new Node(deflater, (Element) child));
+                        child = new Node(deflater, (Element) child);
                     }
+                    children[i] = deflater.unique(child);
                 }
             }
             deflater.addOffset(element);
@@ -137,9 +139,9 @@ final class StoredTree implements Serializable {
         final Node peekLastElement(final String... keys) {
             if (children != null) {
                 for (int i = children.length; --i >= 0;) {
-                    final Object value = children[i];
-                    if (value instanceof Node) {
-                        final Node node = (Node) value;
+                    final Object object = children[i];
+                    if (object instanceof Node) {
+                        final Node node = (Node) object;
                         if (node.children != null) {
                             for (final String key : keys) {
                                 if (node.keyword.equalsIgnoreCase(key)) {
@@ -171,6 +173,25 @@ final class StoredTree implements Serializable {
         }
 
         /**
+         * Adds keyword and children to the given supplier.
+         * Children of children are added recursively.
+         * This method is for testing purposes only.
+         *
+         * @see StoredTree#forEachValue(Consumer)
+         */
+        final void forEachValue(final Consumer<Object> addTo) {
+            addTo.accept(keyword);
+            if (children != null) {
+                for (final Object child : children) {
+                    addTo.accept(child);
+                    if (child instanceof Node) {
+                        ((Node) child).forEachValue(addTo);
+                    }
+                }
+            }
+        }
+
+        /**
          * Returns the string representation of the first value, which is usually the element
name.
          * For example in {@code DATUM["WGS 84", …]} this is "WGS 84". If there is no children
then
          * this method returns the keyword, which is usually an enumeration value (for example
"NORTH"}).
@@ -259,7 +280,7 @@ final class StoredTree implements Serializable {
      */
     StoredTree(final Element root, final Map<Object,Object> sharedValues) {
         final Deflater deflater = new Deflater(sharedValues);
-        this.root = deflater.unique(new Node(deflater, root));
+        this.root = (Node) deflater.unique(new Node(deflater, root));
         offsets = deflater.offsets();
     }
 
@@ -322,9 +343,9 @@ final class StoredTree implements Serializable {
          * @see Node#hashCode()
          * @see Node#equals(Object)
          */
-        final Node unique(final Node node) {
+        final Object unique(final Object node) {
             final Object existing = sharedValues.putIfAbsent(node, node);
-            return (existing != null) ? (Node) existing : node;
+            return (existing != null) ? existing : node;
         }
 
         /**
@@ -336,7 +357,7 @@ final class StoredTree implements Serializable {
             if (count >= offsets.length) {
                 offsets = Arrays.copyOf(offsets, count * 2);
             }
-            int offset = Math.min(Short.MAX_VALUE, element.offset);
+            int offset = Math.min(Short.MAX_VALUE, Math.max(0, element.offset));
             if (element.isFragment) offset = ~offset;
             offsets[count++] = (short) offset;
         }
@@ -348,10 +369,10 @@ final class StoredTree implements Serializable {
         @SuppressWarnings("ReturnOfCollectionOrArrayField")
         final short[] offsets() {
             offsets = ArraysExt.resize(offsets, count);
-            final Deflater other = (Deflater) sharedValues.putIfAbsent(this, this);
+            final short[] other = (short[]) sharedValues.putIfAbsent(this, offsets);
             sharedValues = null;
             if (other != null) {
-                this.offsets = other.offsets;
+                offsets = other;
             }
             return offsets;
         }
@@ -363,14 +384,17 @@ final class StoredTree implements Serializable {
          */
         @Override
         public boolean equals(final Object other) {
+            assert offsets.length == count;
             return (other instanceof Deflater) && Arrays.equals(offsets, ((Deflater)
other).offsets);
         }
 
         /**
          * Computes a hash code value based only on the {@link #offsets} array.
+         * See {@link #equals(Object)} for rational.
          */
         @Override
         public int hashCode() {
+            assert offsets.length == count;
             return Arrays.hashCode(offsets);
         }
     }
@@ -465,6 +489,15 @@ final class StoredTree implements Serializable {
     }
 
     /**
+     * Adds keywords and children to the given supplier. This is for testing purposes only.
+     *
+     * @see WKTDictionary#forEachValue(Consumer)
+     */
+    final void forEachValue(final Consumer<Object> addTo) {
+        root.forEachValue(addTo);
+    }
+
+    /**
      * Returns the keyword of the root element.
      */
     final String keyword() {
diff --git a/core/sis-referencing/src/main/java/org/apache/sis/io/wkt/WKTDictionary.java b/core/sis-referencing/src/main/java/org/apache/sis/io/wkt/WKTDictionary.java
index a20ff85..7548254 100644
--- a/core/sis-referencing/src/main/java/org/apache/sis/io/wkt/WKTDictionary.java
+++ b/core/sis-referencing/src/main/java/org/apache/sis/io/wkt/WKTDictionary.java
@@ -32,7 +32,9 @@ import java.io.BufferedReader;
 import java.io.IOException;
 import java.text.ParseException;
 import java.text.ParsePosition;
+import java.util.function.Consumer;
 import org.opengis.util.FactoryException;
+import org.opengis.util.InternationalString;
 import org.opengis.metadata.Identifier;
 import org.opengis.metadata.citation.Citation;
 import org.opengis.referencing.IdentifiedObject;
@@ -53,6 +55,7 @@ import org.apache.sis.util.ArraysExt;
 import org.apache.sis.util.Exceptions;
 import org.apache.sis.util.resources.Errors;
 import org.apache.sis.util.collection.FrequencySortedSet;
+import org.apache.sis.util.iso.SimpleInternationalString;
 
 
 /**
@@ -99,7 +102,7 @@ public class WKTDictionary extends GeodeticAuthorityFactory {
      * @see #updateAuthority()
      * @see #getAuthority()
      */
-    private Citation authority;
+    private volatile Citation authority;
 
     /**
      * Authorities declared in all {@code "ID[CITATION[…]]"} elements found in WKT definitions.
@@ -714,6 +717,26 @@ public class WKTDictionary extends GeodeticAuthorityFactory {
     }
 
     /**
+     * Adds all definition values to the given supplier. This is for testing purposes only.
+     * This method performs no locking because it is not needed for current JUnit tests.
+     *
+     * @see StoredTree#forEachValue(Consumer)
+     */
+    final void forEachValue(final Consumer<Object> addTo) {
+        for (final Object value : definitions.values()) {
+            if (value instanceof Disambiguation) {
+                Disambiguation choices = (Disambiguation) value;
+                do {
+                    addTo.accept(choices.value);
+                    choices = choices.previous;
+                } while (choices != null);
+            } else {
+                addTo.accept(value);
+            }
+        }
+    }
+
+    /**
      * Returns the authority or specification that defines the codes recognized by this factory.
      * This is the first of the following values, in preference order:
      *
@@ -748,6 +771,30 @@ public class WKTDictionary extends GeodeticAuthorityFactory {
     }
 
     /**
+     * Gets a description of the object corresponding to a code.
+     *
+     * @param  code  value allocated by authority.
+     * @return a description of the object, or {@code null} if {@code null} if none.
+     * @throws NoSuchAuthorityCodeException if the specified {@code code} was not found.
+     * @throws FactoryException if the query failed for some other reason.
+     */
+    @Override
+    public InternationalString getDescriptionText(final String code) throws FactoryException
{
+        final String text;
+        final Object value = getOrCreate(code, false);
+        if (value instanceof IdentifiedObject) {
+            text = ((IdentifiedObject) value).getName().getCode();
+        } else {
+            text = String.valueOf(value);
+            if (!(value instanceof StoredTree)) {
+                // Exception message saved in a previous invocation of `getOrCreate(…)`.
+                throw new FactoryException(text);
+            }
+        }
+        return new SimpleInternationalString(text);
+    }
+
+    /**
      * Returns an arbitrary object from a code.
      *
      * @param  code  value allocated by authority.
@@ -757,6 +804,25 @@ public class WKTDictionary extends GeodeticAuthorityFactory {
      */
     @Override
     public IdentifiedObject createObject(final String code) throws FactoryException {
+        final Object value = getOrCreate(code, true);
+        if (value instanceof IdentifiedObject) {
+            return (IdentifiedObject) value;
+        } else {
+            // Exception message saved in a previous invocation of `getOrCreate(…)`.
+            throw new FactoryException(String.valueOf(value));
+        }
+    }
+
+    /**
+     * Returns the object associated to the given code.
+     *
+     * @param  code    value allocated by authority.
+     * @param  create  whether to create {@link IdentifiedObject} from {@link StoredTree}.
+     * @return the object for the given code, possibly as a {@link StoredTree} if {@code
create} is {@code false}.
+     * @throws NoSuchAuthorityCodeException if the specified {@code code} was not found.
+     * @throws FactoryException if the object creation failed for some other reason.
+     */
+    private Object getOrCreate(final String code, final boolean create) throws FactoryException
{
         /*
          * Separate the authority from the rest of the code. The CharSequences.skipWhitespaces(…)
          * methods are robust to negative index and will work even if code.indexOf(…) returned
-1.
@@ -825,7 +891,7 @@ public class WKTDictionary extends GeodeticAuthorityFactory {
          * If `StoredTree`, try to replace that value by an `IdentifiedObject` (on success)
or `String` (on failure).
          * Must be done under write lock because `parser` is not thread-safe.
          */
-        if (value instanceof StoredTree) {
+        if (create && value instanceof StoredTree) {
             lock.writeLock().lock();
             try {
                 if (choices != null) {
@@ -859,12 +925,7 @@ public class WKTDictionary extends GeodeticAuthorityFactory {
                 lock.writeLock().unlock();
             }
         }
-        if (value instanceof IdentifiedObject) {
-            return (IdentifiedObject) value;
-        } else {
-            // Exception message saved in a previous invocation of this method.
-            throw new FactoryException(String.valueOf(value));
-        }
+        return value;
     }
 
     /**
diff --git a/core/sis-referencing/src/test/java/org/apache/sis/io/wkt/WKTDictionaryTest.java
b/core/sis-referencing/src/test/java/org/apache/sis/io/wkt/WKTDictionaryTest.java
index 56001f0..76c6dae 100644
--- a/core/sis-referencing/src/test/java/org/apache/sis/io/wkt/WKTDictionaryTest.java
+++ b/core/sis-referencing/src/test/java/org/apache/sis/io/wkt/WKTDictionaryTest.java
@@ -17,7 +17,12 @@
 package org.apache.sis.io.wkt;
 
 import java.util.Set;
+import java.util.Map;
 import java.util.Arrays;
+import java.util.HashSet;
+import java.util.IdentityHashMap;
+import java.util.function.Consumer;
+import java.util.function.BiFunction;
 import java.io.BufferedReader;
 import java.io.InputStreamReader;
 import java.io.IOException;
@@ -60,7 +65,13 @@ public final strictfp class WKTDictionaryTest extends TestCase {
             factory.load(source);
         }
         /*
-         * TEST code space should be fist because it is the most frequently used
+         * The `load(…)` method should detect duplicated WKT elements and use references
+         * to unique instances of nodes such as "AngleUnit["Degree", 0.0174532925199433]]".
+         * Following line verifies if the trees of WKT elements indeed share common nodes.
+         */
+        new SharedValuesCheck().verify(factory);
+        /*
+         * "TEST" code space should be fist because it is the most frequently used
          * in the test file. The authority should be "TEST" for the same reason.
          * Codes can be in any order. Code spaces are omitted when there is no ambiguity.
          */
@@ -76,6 +87,12 @@ public final strictfp class WKTDictionaryTest extends TestCase {
         assertSame(codes, factory.getAuthorityCodes(GeodeticCRS.class));            // Test
sharing.
         assertSame(codes, factory.getAuthorityCodes(GeographicCRS.class));          // Test
caching.
         /*
+         * Test descriptions before CRS creation.
+         * Implementation fetches them from `StoredTree` instances.
+         */
+        assertEquals("North_Pole_Stereographic", factory.getDescriptionText("ESRI::102018").toString());
+        assertEquals("South_Pole_Stereographic", factory.getDescriptionText("ESRI::102021").toString());
+        /*
          * Tests CRS creation.
          */
         verifyCRS(factory.createProjectedCRS (        "102018"), "North_Pole_Stereographic",
+90);
@@ -83,6 +100,12 @@ public final strictfp class WKTDictionaryTest extends TestCase {
         verifyCRS(factory.createGeographicCRS("TEST:  :102021"), "Anguilla 1957");
         verifyCRS(factory.createGeographicCRS("TEST:v2:102021"), "Anguilla 1957 (bis)");
         /*
+         * Test descriptions after CRS creation.
+         * Implementation fetches them from `IdentifiedObject` instances.
+         */
+        assertEquals("North_Pole_Stereographic", factory.getDescriptionText("ESRI::102018").toString());
+        assertEquals("South_Pole_Stereographic", factory.getDescriptionText("ESRI::102021").toString());
+        /*
          * Test creation of CRS having errors.
          *   - Verify error index.
          */
@@ -91,6 +114,72 @@ public final strictfp class WKTDictionaryTest extends TestCase {
     }
 
     /**
+     * Verifies that there is no duplicated nodes in the {@link StoredTree}s.
+     * When a WKT element is repeated often (e.g. "AngleUnit["Degree", 0.0174532925199433]]"),
+     * only one {@link org.apache.sis.io.wkt.StoredTree.Node} instance should be created
and shared by all trees.
+     */
+    private static final class SharedValuesCheck implements Consumer<Object>, BiFunction<Integer,Integer,Integer>
{
+        /**
+         * Counter of number of occurrences of each instance. Keys may be {@link String},
+         * {@link Long}, {@link Double} or {@code StoredTree.Node} instances among others.
+         * Values are number of occurrences.
+         */
+        private final Map<Object,Integer> counts = new IdentityHashMap<>(90);
+
+        /**
+         * Verifies all trees in the given factory.
+         */
+        final void verify(final WKTDictionary factory) {
+            factory.forEachValue(this);
+            assertEquals("Some values are equal but distinct instances. A single instance
should be shared.",
+                         new HashSet<>(counts.keySet()).size(), counts.size());
+            /*
+             * Verify the number of occurrences of a few values. Note that the same string
representation of keys
+             * value may appear twice: once because the value was already a `String`, and
once because the value
+             * was a `StoredTree.Node` with the same string representation.
+             *
+             * The `expected` values below are empirical values and may need to be updated
if the content of
+             * `ExtraCRS.txt` test file is modified.
+             */
+            for (final Map.Entry<Object,Integer> entry : counts.entrySet()) {
+                final String key = entry.getKey().toString();
+                final int expected;
+                switch (key) {
+                    case "Cartesian": expected = 2; break;
+                    case "north":     expected = 6; break;
+                    case "Degree":    expected = 6; break;
+                    case "Latitude of natural origin": {
+                        /*
+                         * There is 2 parameters with that string value, but those two parameters
are
+                         * distinct instances because they have different parameter values
(90° and -90°).
+                         */
+                        expected = (entry.getKey() instanceof String) ? 2 : 1;
+                        break;
+                    }
+                    default: continue;
+                }
+                assertEquals(key, expected, entry.getValue().intValue());
+            }
+        }
+
+        /**
+         * Invoked for each value in a WKT element. This method counts the number of occurrences
of each
+         * distinct instance, separated by identity comparison (not by {@link Object#equals(Object)}).
+         */
+        @Override public void accept(final Object value) {
+            if (value instanceof StoredTree) {
+                ((StoredTree) value).forEachValue(this);
+            }
+            counts.merge(value, 1, this);
+        }
+
+        /** Invoked for incrementing a value in the {@link #counts} map. */
+        @Override public Integer apply(final Integer oldValue, final Integer value) {
+            return oldValue + value;
+        }
+    }
+
+    /**
      * Verifies a projected CRS.
      *
      * @param crs   the CRS to verify.


Mime
View raw message