On Wed, Jan 23, 2013 at 11:10 AM, Niels Charlier <ni...@scitus.be> wrote:
> Hello Andrea,
>
> Sorry for my late reponses.
>
>
> On 01/19/2013 06:54 PM, Andrea Aime wrote:
>
> Here is some more I've found during the review.
>
> There's a change in DefaultCatalogFacade.java that I'm not sure about:
>
> if (null != sortByList) {
> for (int i = sortByList.length - 1; i >=0 ; i--) {
> SortBy sortBy = sortByList[i];
> Ordering<Object> ordering =
> Ordering.from(comparator(sortBy));
> if (SortOrder.DESCENDING.equals(sortBy.getSortOrder())) {
> ordering = ordering.reverse();
> }
> all = ordering.sortedCopy(all);
> }
> }
>
> What guarantees are there that sorting the same list multiple times
> we'll get
> to a result that is ordered on sortBy1, then sortBy2, and so on?
> What I'm trying to say is, the second time we sort the algorithm might as
> well
> ruin the partial ordering established by the first sort... the Java 6
> algorithm might
> work, but Java 7 has a different sorting algorithm (TimSort), not sure if
> the
> multiple sorts are going to be preserved there.
>
> I see your point. I remember I needed to include support for sorting with
> more than one sortby, which wasn't yet supported previously.
> But perhaps we have to revise that algorithm and see if there is a more
> appropriate solution.
>
>
>
> Anyways, no big deal, I guess it can be handled as a bug later, if it
> turns
> out to misbehave.
>
> What's a bit more troublesome is changes in the CatalogStore interface.
> Mind, I'm not against them per se, but in order to change a interface that
> was
> discussed openly on the ml I would have expected the same opennes, or at
> least some explanation on why they happened, instead I'm looking at the
> diff trying to understand them.
> The changes propagate in other portions as well, it seems bits of the CSW
> service code have been refactored, again, without providing any clue as to
> why.
>
> I'm not saying the changes are right or wrong (on the contraty, I'm
> confident
> they have been done for good reasons), but they should have been
> discussed: since the patch is out already, can we have some rationale
> about them?
>
> It would make reviewing the patch a lot easier.
>
>
>
> First my apologies for not explaining the changes in advance. I usually
> try to make sure I am certain that particular changes are necessary or
> useful before I propose them, perhaps that is why I didn't do it earlier.
>
> Then I also have to apologise about something else, I did my best cleaning
> up all the code and filling in the javadoc before I did my pull request but
> I seemed to have missed this file. The javadoc for translateProperty wasn't
> filled in, and on top of that one of the changes had to be removed before
> committing. The extra parameter elementSet was supposed to be removed
> again, this was a change I made at some point but became unnecessary later.
> So please remove this change all together, this can be done in the
> interface and all its implementations without a problem. Again, sorry about
> that, that was not my intention.
>
So if I understand this correctly this means two additional changes.
1. Remove the ElementSet argument from CatalogStore.getRecords() method.
2. Remove CatalogStore.translateProperty()
(1) is trivial, more or less all calls to it passed in null. Made that
change and tests are happy.
(2) is not so clear to me. At the moment there is code calling this method,
and multiple implementations of of it, one from AbstractCatalogStore and
one from InternalCatalogStore which appear to be doing different things. So
it's not quite obvious to me what should be done here.
> The other two changes however were for a good reasons and have both to do
> with making CSW more generic.
>
> A lot of the generic stuff relies on using the RecordDescriptor interface.
> At points where the old code simply calls CSWRecordDescriptor.SOME_CONSTANT
> the new code often uses an instance of the RecordDescriptor interface and a
> method to get this information, for obvious reasons. Another thing is that
> as a consequence of parsing .xsd files the name of the record type is not
> any more in the FeatureType, but in an AttributeDescriptor. For example,
> the name of the CSW DC type is not "Record" but "RecordType", there is a
> descriptor with the name "Record" that points to "RecordType". This is good
> because it corresponds with how this is coded in the .xsd file. This is
> just one of the reasons why I don't pass on FeatureType objects any more,
> but rather RecordDescriptor objects. From here all sorts of information can
> be retrieved, like a namespace set which is also frequently used.
>
> Considering the translateProperty method, this method converts a Name to
> PropertyName. This is used for property selection. This happens differently
> with the two Catalog Stores. The Abstract Catalog Store selects properties
> from a finished feature, so the PropertyName applies to a finished feature.
> The Internal Catalog Store selects properties earlier in the process, in
> the mapping, so it needs property names that the mapping understands (so
> for example, with '/value' included).
>
> I hope this helps, if there are any more questions, please ask.
>
> Kind Regards
> Niels Charlier
>
--
Justin Deoliveira
OpenGeo - http://opengeo.org
Enterprise support for open source geospatial.
------------------------------------------------------------------------------
Master Visual Studio, SharePoint, SQL, ASP.NET, C# 2012, HTML5, CSS,
MVC, Windows 8 Apps, JavaScript and much more. Keep your skills current
with LearnDevNow - 3,200 step-by-step video tutorials by Microsoft
MVPs and experts. ON SALE this month only -- learn more at:
http://p.sf.net/sfu/learnnow-d2d
_______________________________________________
Geoserver-devel mailing list
Geoserver-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/geoserver-devel
