[Gdal-dev] RFC 6: OGR support for querying and selection by geometry and feature style

[Tamas Szekeres]

2006/10/25, Frank Warmerdam <warmerdam at pobox.com>:
> Tamas,
>
> I'm quite pleased with RFC-6.  Am I correct in understanding that
> your existing patch addresses fetching the special fields, and using
> them in WHERE clauses?

Frank,

Both of them were addressed according to your previous notes in
http://mapserver.gis.umn.edu/bugs/show_bug.cgi?id=1438
Special fields can now appear in the select list, the where
expression and even in the orderby clause as well.

>
> Some things I'd like to see in the proposal:
>
>   1) I think we should have constants for the special fields!  All
>      these case statements with "1", "2", etc in them scare me!
>

No problem.

>   2) I'd like to be able to register a function to fetch the special
>      fields so we don't have to have the special fetch code appearing
>      in so many places.  At the very least a way of registering a function
>      in an array parallel to the SpecialFieldNames[] and SpecialFieldTypes[]
>      arrays. But perhaps it would be better to actually have methods on the
>      OGRFeature like:
>         const char *GetSpecialFieldAsString( int nSpecialFieldCode );
>         int         GetSpecialFieldAsInteger( int nSpecialFieldCode );
>

No problem about adding the field value accessor function for every special
field. I have already been thinking of it but would not want to
disturb the existing
code so much. Anyway it would make the things more clear with the overhead
of an extra function call.

>   3) I'd like the RFC to include a section on "how to add a new special
>      field".  Hopefully if the special field accessor on the OGRFeature
>      idea works, it won't be too complicated.
>

Well, that was the only area where the code should be significantly
changed when addig a new field. However I haven't dealt with this
problem so far, because the ownership of the returned values was not
handled uniformly. (With regard to 4)

>   4) I'm a bit concerned about the lifespan for dynamically allocated
>      return strings.  How carefully have you analysed the lifespan for
>      the strings for which you propose the StringFinalizer class?  If
>      the lifespan is appropriate, I was thinking that perhaps we could
>      hold them in the OGRFeature in a temporary string value that would
>      last till the next Get*AsString() request on that particular
>      OGRFeature.  Would that be a long enough life span, even if there
>      were multiple special fields that required dynamically allocated
>      values?  Actually, I'd like to use the current approach for regular
>      non-string values requested on OGRFeature::GetFieldAsString().
>      Currently these are held in a static buffer, causing a substantial
>      thread-safety issue.

It causes much of pain figuring out how GDAL/OGR handles the ownership
of the function parameters and return values. In this case the difference of
the usage between exportToWkt and GetStyleString/getGeometryName scared
me highly. I would be the happiest if we eliminated the need of the
StringFinalizer class. The current solution preserves the returned wkt
within the
scope of the function. However it will not cause problems if the subsequent
Get*AsString() overrides the value because we are using the returned value just
after the funcion call.
At this point I will leave you the task to consider how it will affect the other
part of the OGR library. But I like this idea as being the warrior
against unnecessary
allocations and static variables.

>
>   5) I'd like to see a section in the RFC describing the test suite items
>      you will add to the GDAL autotest.
>

I'm afraid I'm not too familiar with GDAL autotest but I'm keen to dig into it
more deeply if i have time. But now I can provide only the data (in
mapinfo format) and
the SQL query samples if needed. Currently I am testing the code using
step by step execution of ogrinfo in debug mode.

>   6) I think the backward compatibility section of the RFC should mention
>      that the new special fields will potentially conflict with regard
>      fields with the given names, and to describe which will take precidence
>      when there is a conflict.
>

No problem with this either. I would suggest a preceding control
character like '@' but it
will break my existing mapfiles (~500)  ;-)

> I think the feature this RFC describes will be a fantastic addition to the
> OGR SQLish engine.  If you wish, I'd be willing to implement items (4) and
> (5) above if you can take care of the rest.
>

I would be helpful if you can do (4) and (5) notify me when you get
ready with it.

Best Regards,

Tamas