Opened 15 years ago
Closed 8 years ago
Last modified 8 years ago
#2680 closed defect (fixed)
OGR OFTDateTime needs more precision
|Reported by:||Daniel Morissette||Owned by:||warmerdam|
|Cc:||Even Rouault, aboudreault|
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?
Change History (11)
comment:1 by , 15 years ago
comment:2 by , 15 years ago
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.
comment:3 by , 15 years ago
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.
comment:4 by , 15 years ago
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.
comment:5 by , 15 years ago
ok, i tought that the OGRField structure was only used internally.
comment:6 by , 15 years ago
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.
comment:7 by , 15 years ago
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
comment:8 by , 8 years ago
Still no support for milliseconds. Best time to break C API/ABI is right now when changing into GDAL 2.0.
comment:9 by , 8 years ago
|Cc:||rouault,aboudreault → rouault, aboudreault|
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?
comment:10 by , 8 years ago
|Milestone:||1.8.1 → 2.0|
|Status:||new → closed|
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.
comment:11 by , 8 years ago
|Milestone:||2.0 → 2.0.0|
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: