OGR OFTDateTime needs more precision

[Daniel Morissette]

While working on ticket #2585, we have discovered that OGR's OFTDateTime carries only seconds but not milliseconds. This means that if we switch MITAB to use OFTDateTime instead of the current implementation that carries time as strings internally then we'll lose the milliseconds and users of MITAB outside of the OGR environment will complain.

In an exchange on IRC, Even also mentioned that GPX for example uses XML datetime that supports fractionnal seconds, so we could benefit from more precision in OFTDateTime there too.

It's too late for the 1.6 release, but we should consider adding fractional seconds to OFTDateTime, or perhaps just store the seconds value as a float instead of a GByte?

[Even Rouault]

After discussing with Daniel on IRC, it appears that it will not be obvious to extend the date, because the OGRField union (ogr_core.h) is part of C API - and used by OGR_F_SetFieldRaw() and OGRParseDate(), without any explicit allocating/desallocating function. We cannot directly add a new field to the Date struct inside OGRField, otherwise the size of the whole union would be extended...

Possible way of dealing with that:

typedef union {
    int         Integer;
    double      Real;
    char       *String;

 [...]
 
    struct {
        GInt16  Year;
        GByte   Month;
        GByte   Day;
        GByte   Hour;
        GByte   Minute;
        GByte   Second;
        GByte   TZFlag; /* 0=unknown, 1=localtime(ambiguous), 
                           100=GMT, 104=GMT+1, 80=GMT-5, etc */
    } Date;
 
// Add new ComplexField type here that is a ref to a OGRComplexField
    struct {
         OGRComplexFieldType eOCFT; /* EvenR: maybe we don't need the type here, but just extends OGRFieldType */
         OGRComplexField     *poCF;
    } ComplexField;
 
} OGRField;
 
 
typedef union {
   // ... define complex field types here, avoiding the ABI issues
 
    struct {
        GInt16  Year;
        GByte   Month;
        GByte   Day;
        GByte   Hour;
        GByte   Minute;
        GByte   Second;
        int     Millisecond; /* EvenR: or make it a float or double that represents the fractional part of second, so that people wanting to deal with micro or nanoseconds can do it... */
        GByte   TZFlag; /* 0=unknown, 1=localtime(ambiguous), 
                           100=GMT, 104=GMT+1, 80=GMT-5, etc */
    } DateEx;
} OGRComplexField;

[aboudreault]

After a discussion with earlier today, we noticed that there is a lot of bits unused in the Date struct and that we could simply remove unused bits from the struct and then we'll be able to store milliseconds in it without use this method. Currently, the Date struct is 8 bytes. Month, Day, Hour, Minute and Second are all stored in 8 bits. Letting Year to 16 bits... Month needs only 4 bits, Day: 5 bits, Hour: 5 bits, Minute: 6 bits and Second: 6 bits. We just reduced the Date struct size from 64 bits to 50 bits. Then, We can add milliseconds (10 bits) to get a total of 60 bits for the struct. By storing Year, Month, Day, Hour, Minute, Second and Millisecond and TZFlag in a double (and handle them with bitwise operation), we could add millisecond and preserve ABI compatibility.

[Even Rouault]

Alan,

I suppose you are talking about a new stucture, and not the existing Date structure in the union ? Because you can't reduce the number of bits of month, day, hour etc. Their size belong to the ABI.

[warmerdam]

Note that the layout of the OGRField structure is part of the ABI so compressing dates into field structures will result in a chnage to the ABI from 1.6. This could be acceptable but should be discussed some before proceeding.

However, I would also note that I'm not keen on using bitfields in OGRField as they often cause odd compatability problems on different compilers. And I don't want to pack stuff into multi-part integers as this would make it quite a bit more complex to use.

[aboudreault]

ok, i tought that the OGRField structure was only used internally.

[Daniel Morissette]

For the time being, we'll solve ticket #2585 by using #ifdef'd code that changes behavior depending on whether the MITAB code is used standalone or as part of OGR.

• When MITAB is used standalone then we'll store MapInfo DateTime as strings and we won't lose the milliseconds and that will keep users of the standalone MITAB happy.

• When the MITAB code is built as part of the OGR driver, then the #ifdef will switch the code to handle dates internally as OFTDateTime and the milliseconds will be lost.

With respect to extending the OGR DateTime support, that is less urgent once we have the solution above in place, but we may have to consider breaking ABI compatibility in 1.7 as Frank seems to suggest if we read between the lines. While we're at it we should set things up so that there is a function to alloc/free OGRField structs instead of letting callers allocate OGRFields on the stack directly... that seems to be the root problem that is blocking us here based on previous discussions with Even.

[Even Rouault]

We should be very prudent in deciding whether to break or not C ABI, even for 1.7. I'm pretty sure that some Linux distributions won't be very happy at all with that (I'm thinking to Debian for example). They are probably already not very happy with the fact that C++ ABI changes between minor versions, but that's not an uncommon pattern (I'm thinking to libgeos.so and libgeos_c.so, where the latter has C ABI stability).

Breaking C ABI is something more serious. I can't find a formal document in GDAL project management that clearly states the rules (maybe http://trac.osgeo.org/gdal/wiki/ReleaseManagementPrelim), but my understanding while discussing about that a few times with Frank has always been that C ABI stability is to be always preserved. For example, that's the reason for QGIS to have switched recently to using GDAL C API, instead of the C++ API.

Of course I doubt very much that many people directly use OGRField, OGR_F_SetFieldRaw() and OGRParseDate(). But IMHO, we should study solutions in the spirit of what Daniel proposed with OGRComplexField. In general, I can see very few goods reasons to break C API/ABI :

• current API/ABI is completely broken (= could not be used at all in the past releases, e.g. due to fundamental design flows, etc)
• no technical way to add otherwise a new major & needed functionnality

[Jukka Rahkonen]

Still no support for milliseconds. Best time to break C API/ABI is right now when changing into GDAL 2.0.

[Jukka Rahkonen]

There seems to be now OGR_F_GetFieldAsDateTimeEx which defines seconds with millisecond accuracy:

pfSecond (0-59 with millisecond accuracy)

BTW, page ​http://www.gdal.org/classOGRFeature.html is listing GetFieldAsDateTime twice while the other should probably be GetFieldAsDateTimeEx (checked 2015-07-26).

Am I right that this ticket can be closed as resolved?

[Even Rouault]

Doxygen is OK. The C++ method has the same name with 2 different signatures to accept integer or floating point seconds. The C functions must have different name because of limitations of the C language.

Closing that one.

[Even Rouault]

Milestone renamed