FeatureCollection is not a java.util.Collection
remove methods that assume in-memory model
This page represents the current plan; for discussion please check the tracker link above.
- History and Motivation
- Bring back FeatureResult
History and Motivation
FeatureCollection needs to lighten up to be happy; the original FeatureCollection implementation was a java.util.Collection offering iterator based access to features stored in memory. When the data access layer was rewritten for DataStore the concept of a FeatureResults was introduced wich could be thought of as a "prepared statement" (able to fetch results each time it was asked). This is a nice light weight "streaming" model for data access that does not force us to load the data into memory.
The intension was to treat it a bit more like a Java ResultSet (so several iterators could be open against the same paged results). But the road to hell is paved with good intensions.
And the FeatureResults interface was in for some hell: 2.1.x Bring Back FeatureCollection was a last ditch community lead request to bring back the FeatureCollection api and allow iterators to be used (thus not existing current code). The compromise that was reached was to ask users to FeatureCollection.close( Iterator ) each iterator after use; so that we could still offer a nice streaming based approach to data access.
For GeoTools 2.5.x we made the migration to Java 5 and we could no longer allow FeatureCollection to implement java.util.Collection because of the new "for each" syntax did not offer a callback for us to close our iterators. Justin started Focus FeatureSource around FeatureReader and FeatureWriter, however all we had time for was the bare minimum to make Java 5 not a fatal opportunity for client code to leak memory.
Bring back FeatureResult
In the spirit of Bring Back FeatureCollection here is our last minuet 2.7.x request to complete the journey; formally recognise that FeatureCollection is not a java.util.Collection and turf Iterator access (and other concepts that mirror in memory storage).
Remove the CollectionListener
First up is the add/remove listeners; a recent code review shows that they are only implemented by the new JDBCNG FeatureCollection (because justin is apparently very responsible). Due to everyone else not being responsible we can safely remove these since no client code could of depended on being sent an event.
Note: These listeners have been deprecated as part of Clean up FeatureEvents since this proposal was written.
- Deprecated in 8.0.x
- Remove in 9.0.x
Asking for your feature collection to be "loaded" into a DefaultFeatureCollection is the currently documented way of avoiding all this streaming nonsense and loading your data into memory.
The result is documented to be a java.util.Collection that also implements FeatureCollection. This is the current migration path for those wishing to have an in memory collection.
We can offer this same migration path to anyone having trouble making the switch off iterator.
We may also wish to consider a different implementation for DefaultFeatureCollection:
- TreeSetFeatureCollection - is used currently and is slow
- ListFeatureCollection - is new and untested, but may be faster in day to day use?
- SpatialIndexFeatureCollection - cannot be modified once made; not a suitable replacement
Remove Iterator Methods
The following methods cover the care and feeding of Iterators:
We are removing the close(FeatureIterator) method as well as it already has a serviceable FeatureIterator.close() method (the method was only provided to ease transition to FeatureIterator).
- Deprecated in 8.0.x
- Remove in 9.0.x
Remove Collection Methods
The following methods are used to update and modify the contents of an in memory Collection and do not fit with the "Prepaired Statement" model of a FeatureCollection.
- Deprecated in 8.0.x
- Remove in 9.0.x
getSchema and getDescriptor()
This is a long standing hole in the API, proposal written in response to questions from Ben: FeatureCollection Descriptor for FeatureMembers
It sounds like this is just an additional method:
Consider making the API consistent with a getType() method? Does not seem worth the bother.
- Added in GeoTools 9.0.x
Here is what SimpleFeatureCollection looks like after the change:
Points raised during discussion:
- toArray methods are there on the off chance the implementation can provide a quick implementation? Is this another good intension? Probably. Could be replaced with a DataUtilities method
- subCollection & sort functionality have been covered by Query these days; and could be removed.
Alternative: subCollection( Query ) would be more useful.
- contains and containsAll are interesting; would almost expect look up by FeatureId instead
- getID() is left from a time when FeatureCollection implemented Feature; it is still used when encoding
- getDescriptor is technically accurate; however we are used to working with FeatureType
- isEmpty() earns its keep; size() == 0 is not a replacement as you need to count the whole set
- with that in mind containsAll falls into the same idea; how to prevent a multi pass operation
- accepts earns its keep for Jody Garnett,
Andrea Aime wants the javadocs cleaned up so it is obvious that the feature being visited is a Flyweight (ie the same object with just the values changing inside each time)
- Minimal baseline: minimum that is functional (if we want more we need to justify it):
This proposal is the subject of a grass roots revolution on the geotools-devel list; indeed the revolution is on going and will not be televised.
Voting has not started yet:
- Andrea Aime +0
- Ben Caradoc-Davies +0
- Christian Mueller +0
- Ian Turton +0
- Justin Deoliveira +1
- Jody Garnett +1
- Michael Bedward +0
- Simone Giannecchini +0
- JG: Deprecate methods for 8.0 release
- FeatureCollection Deprecate Collection methods
- FeatureCollection Deprecate CollectionListener (and methods)
- FeatureCollection Deprecate FeatureCollection Iterator methods
- JG: Changes for 9.0 master
- FeatureCollection Remove Collection methods
- FeatureCollection Remove CollectionListener (and methods)
- FeatureCollection Remove FeatureCollection Iterator methods
- Deprecate FeatureCollections factory
- Fix support classes ( used to mark anything that did not go smoothly)
- DataFeatureCollection remains based on FeatureReader
- AbstractFeatureCollection will remain based on iteartor()
- BaseFeatureCollection created to be based on features()
- SubCollection migrated to BaseFeatureCollection
- SortFeatureList migrated to BaseFeatureCollection
- AdaptorFeatureCollection - base class used by uDig
- FilteringSimpleFeatureCollection: migrated to FilteringFeatureIterator (from FilteringIterator)
- MaxSimpleFeatureCollection: migrated to MaxFeaturesSimpleFeatureIterator (from MaxFeaturesIterator)
- ReprojectingFeatureCollection: migrated to ReprojectingFeatureIterator (from ReprojectingIterator)
- ReTypingFeatureCollection: migrated to ReTypingFeatureIterator (from ReTypingIterator)
- DefaultFeatureCollection - remains based on TreeSet
- GMLFeatureCollection: subclass of DefaultFeatureCollection
- MemoryFeatureCollection: copy of DefaultFeatureCollection
- TreeSetFeatureCollection: copy of DefaultFeatureCollection changed to no longer implement Collection
- DefaultVersionedFeatureCollection: add is now protected and use during init to populate the collection
- PropertyValueCollection - this is not a FeatureCollection it was being used to return Collection<Attribute> I have changed it to return a Feature (containing the expected Attribute) but cannot untangle the Encoder well enough to make the test pass.
- Fix geotools modules (notes on any thing interesting, also check commit logs)
- In general pretty smooth transition
- app-schema makes "dangerous" use of feature collection support classes (that internally assume SimpleFeature)
- StreamingRenderer required code duplication to handle Collection.iterator() and FeatureCollection.features()
- Many cases where try/finally was used to ensure featureIterator.close() was called
- PropertyValueCollection and ValueCollectionTypeBindingTest is broken!
- Documentation - see Documentation Changes below
- Integration party
- JG: Stage work on branch: https://github.com/geotools/geotools/tree/featurecollection_cleanup
- JG: Submit pull request:
- JD: GeoServer pull request:
- JG: uDig pull request:
The following documentation needs to be updated:
- Fix up the upgrade instructions
- Update feature tutorial to use ListFeatureCollection (to preserve order)
- Update feature collection examples
- add( Feature )
- Use DefaultFeatureCollection so Collection.add( feature ) is visible
- Change FeatureCollections.newCollection() → new DefaultFeatureCollection()
- FeatureCollection.close( Iterator )
- Preferred: use FeatureIterator.close()
- Use: ((Closeable)iterator).close()
- Handy FeatureCollections that implement java.util.Collection
- ListFeatureCollection (delegates to List)
- DefaultFeatureCollection (the original based on a TreeSet sorted by FeatureId)
- MemoryFeatureCollection (a copy of DefaultFeatureCollection for use by MemoryDataStore)
With the FeatureCollection.add method being removed, you will need to use an explicit instance that supports adding content.
The deprecated FeatureCollection.iterator() method is no longer available, please use FeatureCollection.features() as shown below.
FeatureCollection close method
We have made FeatureCollection implement closable (for Java 7 try-with-resource compatibility). This also provides an excellent replacement for FeatureCollection.close( Iterator ).