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 afa573ef65 Detect name collision in the fields reported by GDAL. It
happens for example when using the Shapefile driver, because field names are
truncated to 10 characters.
afa573ef65 is described below
commit afa573ef6527f34a752e8aa47902664706106ce4
Author: Martin Desruisseaux <[email protected]>
AuthorDate: Sat Dec 28 16:08:42 2024 +0100
Detect name collision in the fields reported by GDAL.
It happens for example when using the Shapefile driver, because field names
are truncated to 10 characters.
---
.../org/apache/sis/feature/DefaultFeatureType.java | 19 ++----
.../sis/feature/builder/FeatureTypeBuilder.java | 14 ++--
.../apache/sis/feature/privy/FeatureUtilities.java | 6 ++
.../apache/sis/storage/gdal/FeatureIterator.java | 2 +-
.../org/apache/sis/storage/gdal/FeatureLayer.java | 76 ++++++++++++++++------
.../org/apache/sis/storage/gdal/FieldAccessor.java | 33 +++++++++-
6 files changed, 107 insertions(+), 43 deletions(-)
diff --git
a/endorsed/src/org.apache.sis.feature/main/org/apache/sis/feature/DefaultFeatureType.java
b/endorsed/src/org.apache.sis.feature/main/org/apache/sis/feature/DefaultFeatureType.java
index d9110ab7c3..5984c258a3 100644
---
a/endorsed/src/org.apache.sis.feature/main/org/apache/sis/feature/DefaultFeatureType.java
+++
b/endorsed/src/org.apache.sis.feature/main/org/apache/sis/feature/DefaultFeatureType.java
@@ -221,28 +221,23 @@ public class DefaultFeatureType extends
AbstractIdentifiedType implements Featur
* <th>Map key</th>
* <th>Value type</th>
* <th>Returned by</th>
- * </tr>
- * <tr>
+ * </tr><tr>
* <td>{@value
org.apache.sis.feature.AbstractIdentifiedType#NAME_KEY}</td>
* <td>{@link GenericName} or {@link String}</td>
* <td>{@link #getName()}</td>
- * </tr>
- * <tr>
+ * </tr><tr>
* <td>{@value
org.apache.sis.feature.AbstractIdentifiedType#DEFINITION_KEY}</td>
* <td>{@link InternationalString} or {@link String}</td>
* <td>{@link #getDefinition()}</td>
- * </tr>
- * <tr>
+ * </tr><tr>
* <td>{@value
org.apache.sis.feature.AbstractIdentifiedType#DESIGNATION_KEY}</td>
* <td>{@link InternationalString} or {@link String}</td>
* <td>{@link #getDesignation()}</td>
- * </tr>
- * <tr>
+ * </tr><tr>
* <td>{@value
org.apache.sis.feature.AbstractIdentifiedType#DESCRIPTION_KEY}</td>
* <td>{@link InternationalString} or {@link String}</td>
* <td>{@link #getDescription()}</td>
- * </tr>
- * <tr>
+ * </tr><tr>
* <td>{@value
org.apache.sis.feature.AbstractIdentifiedType#DEPRECATED_KEY}</td>
* <td>{@link Boolean}</td>
* <td>{@link #isDeprecated()}</td>
@@ -396,7 +391,7 @@ public class DefaultFeatureType extends
AbstractIdentifiedType implements Featur
*
* In the `aliases` map below, null values will be assigned to
ambiguous short names.
*/
- final Map<String, PropertyType> aliases = new LinkedHashMap<>();
+ final var aliases = new LinkedHashMap<String, PropertyType>();
for (final PropertyType property : allProperties) {
GenericName name = property.getName();
while (name instanceof ScopedName) {
@@ -531,7 +526,7 @@ public class DefaultFeatureType extends
AbstractIdentifiedType implements Featur
* change the result.
*/
if (feature instanceof DefaultFeatureType) {
- final DefaultFeatureType dt = (DefaultFeatureType) feature;
+ final var dt = (DefaultFeatureType) feature;
return dt.isResolved = resolve(dt, dt.properties, previous,
dt.isResolved);
} else {
return resolve(feature, feature.getProperties(false), previous,
feature.isSimple());
diff --git
a/endorsed/src/org.apache.sis.feature/main/org/apache/sis/feature/builder/FeatureTypeBuilder.java
b/endorsed/src/org.apache.sis.feature/main/org/apache/sis/feature/builder/FeatureTypeBuilder.java
index ba638a793e..22287ece14 100644
---
a/endorsed/src/org.apache.sis.feature/main/org/apache/sis/feature/builder/FeatureTypeBuilder.java
+++
b/endorsed/src/org.apache.sis.feature/main/org/apache/sis/feature/builder/FeatureTypeBuilder.java
@@ -298,7 +298,7 @@ public class FeatureTypeBuilder extends TypeBuilder {
* For each operation, wrap them in pseudo-builder only if the
operation
* is not one of the operations automatically generated by this
builder.
*/
- final Map<String,Set<AttributeRole>> propertyRoles = new HashMap<>();
+ final var propertyRoles = new HashMap<String,Set<AttributeRole>>();
for (final PropertyType property : template.getProperties(false)) {
PropertyTypeBuilder builder;
if (property instanceof AttributeType<?>) {
@@ -682,7 +682,7 @@ public class FeatureTypeBuilder extends TypeBuilder {
// We disallow Feature.class because that type shall be handled as
association instead of attribute.
throw new
IllegalArgumentException(errors().getString(Errors.Keys.IllegalArgumentValue_2,
"valueClass", valueClass));
}
- final AttributeTypeBuilder<V> property = new
AttributeTypeBuilder<>(this, Numbers.primitiveToWrapper(valueClass));
+ final var property = new AttributeTypeBuilder<V>(this,
Numbers.primitiveToWrapper(valueClass));
properties.add(property);
clearCache();
return property;
@@ -701,7 +701,7 @@ public class FeatureTypeBuilder extends TypeBuilder {
*/
public <V> AttributeTypeBuilder<V> addAttribute(final AttributeType<V>
template) {
ensureNonNull("template", template);
- final AttributeTypeBuilder<V> property = new
AttributeTypeBuilder<>(this, template);
+ final var property = new AttributeTypeBuilder<V>(this, template);
properties.add(property);
clearCache();
return property;
@@ -764,7 +764,7 @@ public class FeatureTypeBuilder extends TypeBuilder {
*/
public AssociationRoleBuilder addAssociation(final FeatureType type) {
ensureNonNull("type", type);
- final AssociationRoleBuilder property = new
AssociationRoleBuilder(this, type, type.getName());
+ final var property = new AssociationRoleBuilder(this, type,
type.getName());
properties.add(property);
clearCache();
return property;
@@ -782,7 +782,7 @@ public class FeatureTypeBuilder extends TypeBuilder {
*/
public AssociationRoleBuilder addAssociation(final GenericName type) {
ensureNonNull("type", type);
- final AssociationRoleBuilder property = new
AssociationRoleBuilder(this, null, type);
+ final var property = new AssociationRoleBuilder(this, null, type);
properties.add(property);
clearCache();
return property;
@@ -801,7 +801,7 @@ public class FeatureTypeBuilder extends TypeBuilder {
*/
public AssociationRoleBuilder addAssociation(final FeatureAssociationRole
template) {
ensureNonNull("template", template);
- final AssociationRoleBuilder property = new
AssociationRoleBuilder(this, template);
+ final var property = new AssociationRoleBuilder(this, template);
properties.add(property);
clearCache();
return property;
@@ -918,7 +918,7 @@ public class FeatureTypeBuilder extends TypeBuilder {
geometryIndex = numSynthetic++;
}
}
- final PropertyType[] propertyTypes = new PropertyType[numSynthetic
+ numSpecified];
+ final var propertyTypes = new PropertyType[numSynthetic +
numSpecified];
int propertyCursor = numSynthetic;
int identifierCursor = 0;
for (int i=0; i<numSpecified; i++) {
diff --git
a/endorsed/src/org.apache.sis.feature/main/org/apache/sis/feature/privy/FeatureUtilities.java
b/endorsed/src/org.apache.sis.feature/main/org/apache/sis/feature/privy/FeatureUtilities.java
index b089754d1a..c34d9b00e6 100644
---
a/endorsed/src/org.apache.sis.feature/main/org/apache/sis/feature/privy/FeatureUtilities.java
+++
b/endorsed/src/org.apache.sis.feature/main/org/apache/sis/feature/privy/FeatureUtilities.java
@@ -40,6 +40,12 @@ import org.opengis.feature.PropertyType;
* @author Martin Desruisseaux (Geomatys)
*/
public final class FeatureUtilities extends Static {
+ /**
+ * Prefix to insert before sequential number for name disambiguation.
+ * This is used when attribute name collisions are detected in a file.
+ */
+ public static final String DISAMBIGUATION_SEQUENTIAL_NUMBER_PREFIX = " #";
+
/**
* Do not allow instantiation of this class.
*/
diff --git
a/optional/src/org.apache.sis.storage.gdal/main/org/apache/sis/storage/gdal/FeatureIterator.java
b/optional/src/org.apache.sis.storage.gdal/main/org/apache/sis/storage/gdal/FeatureIterator.java
index dba0932520..13db2caf4e 100644
---
a/optional/src/org.apache.sis.storage.gdal/main/org/apache/sis/storage/gdal/FeatureIterator.java
+++
b/optional/src/org.apache.sis.storage.gdal/main/org/apache/sis/storage/gdal/FeatureIterator.java
@@ -274,7 +274,7 @@ final class FeatureIterator implements
Spliterator<Feature>, Runnable {
try {
feature = layer.type.newInstance();
for (FieldAccessor<?> field : layer.fields) {
- feature.setPropertyValue(field.name, field.getValue(this, ogr,
handle));
+ feature.setPropertyValue(field.name(), field.getValue(this,
ogr, handle));
}
} finally {
ogr.destroyFeature.invokeExact(handle);
diff --git
a/optional/src/org.apache.sis.storage.gdal/main/org/apache/sis/storage/gdal/FeatureLayer.java
b/optional/src/org.apache.sis.storage.gdal/main/org/apache/sis/storage/gdal/FeatureLayer.java
index f8a8cd338f..0d5b87aa56 100644
---
a/optional/src/org.apache.sis.storage.gdal/main/org/apache/sis/storage/gdal/FeatureLayer.java
+++
b/optional/src/org.apache.sis.storage.gdal/main/org/apache/sis/storage/gdal/FeatureLayer.java
@@ -17,6 +17,7 @@
package org.apache.sis.storage.gdal;
import java.util.Locale;
+import java.util.HashMap;
import java.util.Optional;
import java.util.stream.Stream;
import java.util.stream.StreamSupport;
@@ -36,6 +37,7 @@ import org.apache.sis.referencing.IdentifiedObjects;
import org.apache.sis.storage.AbstractFeatureSet;
import org.apache.sis.storage.DataStoreException;
import org.apache.sis.util.ArraysExt;
+import org.apache.sis.util.Workaround;
import org.apache.sis.util.resources.Errors;
import org.apache.sis.util.privy.Strings;
@@ -110,14 +112,24 @@ final class FeatureLayer extends AbstractFeatureSet {
final int fieldCount = (int) ogr.getFieldCount.invokeExact(definition);
final int geomCount = (int)
ogr.getGeomFieldCount.invokeExact(definition);
/*
- * Fetch the non-geometry fields.
+ * Fetch the non-geometry fields. The field names at this point are
temporary and may be modified later
+ * if we detect name collisions. The `HashMap` will be used for
detecting those collisions. The initial
+ * set of keys contains only the names declared by GDAL. Value is the
index of the field using the name.
*/
@SuppressWarnings("LocalVariableHidesMemberVariable")
final var fields = new
FieldAccessor<?>[FieldAccessor.NUM_ADDITIONAL_FIELDS + fieldCount + geomCount];
- int fieldIndex = 0;
- fields[fieldIndex++] = addAttribute(builder,
FieldAccessor.Identifier.INSTANCE);
+ final var names = HashMap.<String,Integer>newHashMap(fields.length);
+ final var props = new AttributeTypeBuilder<?>[fields.length];
+ props[0] = builder.addAttribute((fields[0] =
FieldAccessor.Identifier.INSTANCE).getJavaClass());
+ int fieldIndex = 1;
for (int i=0; i<fieldCount; i++) {
- fields[fieldIndex++] = addAttribute(builder,
FieldAccessor.create(ogr, definition, i));
+ final FieldAccessor<?> field = FieldAccessor.create(ogr,
definition, i);
+ fields[fieldIndex] = field;
+ names.putIfAbsent(field.name(), fieldIndex);
+ final Class<?> element = field.getElementClass();
+ props[fieldIndex++] = (element != null)
+ ?
builder.addAttribute(element).setMaximumOccurs(Integer.MAX_VALUE)
+ : builder.addAttribute(field.getJavaClass());
}
/*
* Fetch the geometry fields with the first one taken as the default
geometry.
@@ -125,12 +137,9 @@ final class FeatureLayer extends AbstractFeatureSet {
* The declared geometry type may be conservatively generalized to
`GEOMETRY`
* if the information provided by the driver is not reliable.
*/
- final String driverName = store.getDriverName(gdal);
- final boolean generalize = (driverName != null) &&
driverName.toLowerCase(Locale.US).contains("shapefile");
-
@SuppressWarnings("LocalVariableHidesMemberVariable")
CoordinateReferenceSystem defaultCRS = null;
-
+ final boolean generalize =
forceGeometryCollection(store.getDriverName(gdal));
boolean isFirstGeometry = true;
for (int i=0; i<geomCount; i++) {
final var geomField = (MemorySegment)
ogr.getGeomFieldDefinition.invokeExact(definition, i);
@@ -141,6 +150,8 @@ final class FeatureLayer extends AbstractFeatureSet {
if (name == null || name.isBlank()) {
name = setAsDefault ? AttributeConvention.GEOMETRY :
"geometry" + i;
setAsDefault = false; // Because already has the default
name.
+ } else {
+ names.putIfAbsent(name, fieldIndex);
}
/*
* OGR Hack: ShapeFile geometry type is not correctly detected
because the OGR driver does
@@ -160,35 +171,58 @@ final class FeatureLayer extends AbstractFeatureSet {
final CoordinateReferenceSystem crs = SpatialRef.parseCRS(store,
gdal,
(MemorySegment)
ogr.getGeomFieldSpatialRef.invokeExact(geomField));
- final AttributeTypeBuilder<?> attribute =
builder.addAttribute(geomClass).setName(name).setCRS(crs);
+ final AttributeTypeBuilder<?> attribute =
builder.addAttribute(geomClass).setCRS(crs);
if (setAsDefault) { // First geometry as default.
attribute.addRole(AttributeRole.DEFAULT_GEOMETRY);
}
+ props[fieldIndex] = attribute;
fields[fieldIndex++] = new FieldAccessor.Geometry<>(name, i,
geomClass, crs);
if (isFirstGeometry) {
isFirstGeometry = false;
defaultCRS = crs; // May still be null.
}
}
- this.type = builder.setName(GDAL.toString((MemorySegment)
ogr.getLayerName.invokeExact(handle))).build();
+ /*
+ * Verify if two fields have the same name. It may happen with formats
such as Shapefile,
+ * which truncate the names to 10 characters. If a collision is
detected, we rename both
+ * fields for avoiding confusion. We perform this check only after we
collected the names
+ * of all fields for avoiding to accidentally use the name of another
field.
+ */
+ for (int i=0; i<fieldIndex; i++) {
+ String name = fields[i].name();
+ Integer previous = names.putIfAbsent(name, i);
+ if (previous != null && previous != i) {
+ if (previous >= 0) {
+ Integer value = ~previous;
+ names.put(name, value); // Negative value as a flag
meaning "already renamed".
+ fields[previous].rename(names, value);
+ }
+ fields[i].rename(names, ~i);
+ }
+ }
+ for (int i=0; i<fieldIndex; i++) {
+ props[i].setName(fields[i].name());
+ }
+ String name = GDAL.toString((MemorySegment)
ogr.getLayerName.invokeExact(handle));
+ this.type = builder.setName(name).build();
this.fields = ArraysExt.resize(fields, fieldIndex);
this.defaultCRS = defaultCRS;
}
/**
- * Adds a non-geometry attribute to a feature type. This is a helper
method for the constructor.
- * The returned {@link FieldAccessor} instance encapsulates the code for
fetching the attribute value.
+ * Returns whether the type of the geometry field should be generalized
from a single type to a collection.
+ * This is a workaround for the geometry type not correctly detected by
the OGR Shapefile driver,
+ * because it does not distinguish betwen polygon and multi-polygon.
+ *
+ * @see <a href="https://code.djangoproject.com/ticket/7218";>GIS:
ogrinspect sometimes gets field types wrong</a>
*/
- private static FieldAccessor<?> addAttribute(final FeatureTypeBuilder
builder, final FieldAccessor<?> field) {
- final AttributeTypeBuilder<?> attribute;
- Class<?> ft = field.getElementClass();
- if (ft != null) {
- attribute =
builder.addAttribute(ft).setMaximumOccurs(Integer.MAX_VALUE);
- } else {
- attribute = builder.addAttribute(field.getJavaClass());
+ @Workaround(library="GDAL", version="3.9.3")
+ private static boolean forceGeometryCollection(String driverName) {
+ if (driverName != null) {
+ driverName = driverName.toLowerCase(Locale.US);
+ return driverName.contains("shapefile");
}
- attribute.setName(field.name);
- return field;
+ return false;
}
/**
diff --git
a/optional/src/org.apache.sis.storage.gdal/main/org/apache/sis/storage/gdal/FieldAccessor.java
b/optional/src/org.apache.sis.storage.gdal/main/org/apache/sis/storage/gdal/FieldAccessor.java
index 3023fa997c..58f79a4419 100644
---
a/optional/src/org.apache.sis.storage.gdal/main/org/apache/sis/storage/gdal/FieldAccessor.java
+++
b/optional/src/org.apache.sis.storage.gdal/main/org/apache/sis/storage/gdal/FieldAccessor.java
@@ -16,6 +16,7 @@
*/
package org.apache.sis.storage.gdal;
+import java.util.Map;
import java.util.List;
import java.time.LocalDate;
import java.time.LocalDateTime;
@@ -29,6 +30,7 @@ import java.lang.foreign.ValueLayout;
import java.nio.ByteBuffer;
import org.opengis.referencing.crs.CoordinateReferenceSystem;
import org.apache.sis.feature.privy.AttributeConvention;
+import org.apache.sis.feature.privy.FeatureUtilities;
import org.apache.sis.util.privy.Strings;
@@ -62,9 +64,13 @@ abstract class FieldAccessor<V> {
private static final int NUM_DATE_COMPONENTS = 7;
/**
- * Name of this field.
+ * Name of this field. Each name in a {@code FeatureType} shall be unique.
+ * This name may differ from the name declared by <abbr>GDAL</abbr> if
name collisions were detected.
+ *
+ * @see #name()
+ * @see #rename(Map, Integer)
*/
- public final String name;
+ private String name;
/**
* Index of this field in each {@code OGRFeatureH} instance.
@@ -84,6 +90,29 @@ abstract class FieldAccessor<V> {
this.index = index;
}
+ /**
+ * Renames this field for avoiding name collision.
+ * The algorithm in this method is not verify efficient, but should be
okay in the common
+ * case where there is only a few fields (typically only two) in conflict
for a given name.
+ *
+ * @param names names that are already used.
+ * @param value value to assign to the new name in the map.
+ */
+ final void rename(final Map<String,Integer> names, final Integer value) {
+ final String base = name;
+ int counter = 0;
+ do name = base +
FeatureUtilities.DISAMBIGUATION_SEQUENTIAL_NUMBER_PREFIX + (++counter);
+ while (names.putIfAbsent(name, value) != null);
+ }
+
+ /**
+ * Returns the name of this field. Each name in a {@code FeatureType} is
unique.
+ * This name may differ from the name declared by <abbr>GDAL</abbr> if
name collisions were detected.
+ */
+ public final String name() {
+ return name;
+ }
+
/**
* Returns the minimal size of the native buffer that this field will need
for fetching values.
* A buffer is needed when invoking <abbr>GDAL</abbr> functions having
arguments that are pointers
