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
