Thanks Frank, I think the original TreeMap implementation just sorted Strings also (i.e. by FeatureID ) - we were not expecting them to maintain the order added.
-- Jody Garnett On 13 September 2015 at 06:27, Frank Gasdorf <[email protected]> wrote: > See comments inline .. > > 2015-09-11 0:02 GMT+02:00 Jody Garnett <[email protected]>: > >> Going to take a bit longer to respond, sorry for my earlier terse email >> from phone. >> -- >> Jody Garnett >> >> On 10 September 2015 at 14:43, Frank Gasdorf <[email protected] >> > wrote: >>> >>> 2015-09-10 23:13 GMT+02:00 Jody Garnett <[email protected]>: >>> >>>> Since its not an "Ordered" DataStore that should preserve order I'd >>>>> suggest to remove these tests. Opinions? >>>>> >>>> >>>> If you change the functionality, change the test to match. >>>> >>> >> I am agreeing with you - if MemoryDataStore no longer has a natural order >> (due to change to concurrent hash map) then we can remove those tests. >> >> > Can I suggest ConcurrentSkipListMap (see >> http://stackoverflow.com/questions/1904439/when-is-a-concurrentskiplistset-useful) >> as a suggested thread safe sorted map. >> > > ConcurrentSkipListMap has a natural ordering, see javadoc comment: > > "A scalable concurrent {@link ConcurrentNavigableMap} implementation. > > The map is sorted according to the {@linkplain Comparable natural > > ordering} of its keys, or by a {@link Comparator} provided at map > > creation time, depending on which constructor is used." > > > I expect to get features not really the same order we put it in since > Simple Features doesnt implement Compareable (and the tess gave the same > feedback ;)). > > I rewrote the test and added explicit keys for Features in array. Key is a > String, which implements Comparable. That means that MemoryDataStore has a > Simple Feature Key ordering by default and depends on the generated / used > key. > > >> This should allow us to fix the bug, and preserve expected behaviour. If >> that works for you let me know and I can keep the RC doors open for the fix >> :) >> >> >> There should now be several feature collection implementations to choose >>>> from so code that requires a TreeSet based functionality has a migration >>>> path. >>>> >>> >>> Not sure about I understood you correctly. >>> >> >> There are three feature collection implementations (to replace >> DefaultFeatureCollections factory class that nobody used). >> >> * TreeSetFeatureCollection - traditional TreeSet implementation used by >> MemoryDataStore >> * ListFetureCollection - just a wrapper, it is on the honour system not >> to put the same feature in twice >> * SpatialIndexFeatureCollection - uses STRtree so the collection cannot >> really be modified once set up >> >> These can be wrapped up by DataUtilities.soruce( collection ) for a >> useful FeatureSource. See performance options >> <http://docs.geotools.org/latest/userguide/library/main/collection.html#performance-options> >> for more information. >> >> >>> Do users of MemoryDataStore expect to get the features the same order >>> they wrote it before? >>> >> >> Yes - but this is only because I wrote it as a teaching aid when defining >> the API. It helped to have a "natural order" to allow the example to more >> closely mimic shapefile. >> >> The downside is generations of GeoTools users have assume MemoryDataStore >> would be faster than a Shapefile (it is not for some workflows due to lack >> of a spatial index). >> >> >>> I thought writers/readers have to provide a Query, if there is no order >>> criteria in it's kind of unpredictable what order the result is. >>> >> >> That is correct, we did not guarantee result order specifically to make >> it easy for an implementation to have multiple indexes and swap between the >> most appropriate one as required for the query. >> >> A "SortBy" query option is available, but it is not universally >> implemented, there is some kind of QueryCapabilities data structure you can >> check to see if sorting is available. >> >> For Shapefile Andrea has gone to a lot of work to write results out to >> disk for sorting and then traverse of the those results... >> >> I would compare it to a database, its kind of internally implementation >>> details (see >>> http://stackoverflow.com/questions/899514/default-row-ordering-for-select-query-in-oracle/900106#900106 >>> ). >>> >>> Oh, now I read the following comment : >>> https://github.com/fgdrf/geotools/blob/0ca687939b17ccb34d3dbbb8df4c838d9d02e86f/modules/library/data/src/main/java/org/geotools/data/memory/MemoryDataStore.java#L131 >>> >>> >> The comment is from when we assumed we would be able to implement order >> everywhere. >> > > Sounds like we should remove the comment to avoid confusions in the future > ;) > >> >> >>> and wondering even more ;) that the whole rendering system depends on >>> internal behaviors. I understand the requirement but at the moment not >>> really the place of implementation to match it. I guessed the renderer >>> should take care of, if required (Is there a hint/flag for it?) >>> >> >> There is in QueryCapabilities, and as indicated Andrea takes some heroic >> measures to try and provide sorting for implementations that do not have >> the ability natively. >> >> >>> Possible workaround is to store feature id in the STRtree, and then look >>>> up current feature value by feature id. This however makes me wish for a >>>> graph database. >>>> >>> >>> Actually, I used a modified MemoryDataStore for years to work with >>> features from 0 to 20000 per featureType and never hat problems >>> accessing/quering features from ConcurrentHashMap. However, optimizing >>> performance I would move to a later point of time sind the current >>> Implementation is at it is fast enough. In addition Concurrent classes act >>> the same "temporary out of date informations" >>> >> >> Understood. >> >> Interesting discussion, thanks for your feedback :D >>> >> >> Yep, more appropriate for a code sprint than our current code freeze. >> > > Let me check the timetable when the Codesprint is taking place. Maybe I > can join from Europe. However. I created a pull to get Feedback from travis > builds https://github.com/geotools/geotools/pull/960 > > Thanks again > > - Frank > > >> -- >> Jody >> > >
------------------------------------------------------------------------------
_______________________________________________ GeoTools-Devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/geotools-devel
