Bug 40575 - Maximum date value in Base is 27/06/9957
Summary: Maximum date value in Base is 27/06/9957
Status: NEW
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: Base (show other bugs)
Version:
(earliest affected)
Inherited From OOo
Hardware: All All
: medium minor
Assignee: Not Assigned
URL:
Whiteboard:
Keywords:
Depends on:
Blocks: Database
  Show dependency treegraph
 
Reported: 2011-09-02 03:49 UTC by Alex Thurgood
Modified: 2018-10-14 06:18 UTC (History)
6 users (show)

See Also:
Crash report or crash signature:


Attachments
testdb to reproduce behaviour (3.54 KB, application/vnd.oasis.opendocument.database)
2011-09-02 03:49 UTC, Alex Thurgood
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Alex Thurgood 2011-09-02 03:49:24 UTC
Created attachment 50839 [details]
testdb to reproduce behaviour

Steps to reproduce :



1) Create a simple HSQLDB database containing 2 fields :

ID : INT autovalue = YES
bday : DATE (DD/MM/YYYY) formatting

save.

2) Open table for entering data. In the date field enter 01/06/9957. Displayed date is 01/06/9957.

3) Now in next tuple, enter 01/06/9958 :
- expected display is 01/06/9958
- actual display is 31/12/9999

4) Wrong behaviour : all dates with years from 9958 and onwards are displayed as 31/12/9999 irrespective of the date entered.


This is a quite significant reduction in the number of years available for date management within a database.


Alex
Comment 1 Alex Thurgood 2011-09-02 04:03:08 UTC Comment hidden (obsolete)
Comment 2 Alex Thurgood 2011-09-02 04:12:26 UTC

After some testing, it appears that the maximu date value is 27/06/9957. Any date value entered which is higher than that triggers automatic conversion to 31/12/9999.

So Base incorrectly refuses to register any date comprised between 28/06/9957 and 30/12/9999.


Alex
Comment 3 Zoltán Reizinger 2011-10-05 06:59:35 UTC Comment hidden (no-value)
Comment 4 Julien Nabet 2011-12-04 12:53:01 UTC
I reproduced exactly this behavior with 3.4.4 and on master. (pc Debian x86-32 testing updated today).
Could a dev give some pointers on the code to find the cause ?
Comment 5 Eike Rathke 2011-12-20 16:18:18 UTC
Assuming that Base uses tools' class Date,
tools/source/datetime/tdate.cxx line 49 has a magic

#define MAX_DAYS    3636532

that's ~9957 years. Apparently the algorithm underlying in DaysToDate() can't cope with higher values, didn't check, at least all operators adding days to dates calling that method check for MAX_DAYS before doing so.

Though I wouldn't go as far as calling that "a quite significant reduction in the number of years available", really, which star gate does one try to calculate with an office suite..
Comment 6 Julien Nabet 2011-12-22 14:57:28 UTC
Just for information, a research for "DaysToDate" in Opengrok shows that it's implemented 3 times :
- scaddins/source/datefunc/datefunc.cxx
- tools/source/datetime/tdate.cxx
- scaddins/source/analysis/analysishelper.cxx
But if DaysToDate function appears called from tools/source/datetime/date.cxx,
MAX_DAYS is also present in connectivity/source/commontools/dbconversion.cpp
which calls implBuildFromRelative

The 3 DaysToDate functions + implBuildFromRelative function seem to want to do the same thing.
Comment 7 Björn Michaelsen 2011-12-23 13:24:38 UTC Comment hidden (obsolete)
Comment 8 Alex Thurgood 2012-02-26 06:27:48 UTC
(In reply to comment #5)
> 
> Though I wouldn't go as far as calling that "a quite significant reduction in
> the number of years available", really, which star gate does one try to
> calculate with an office suite..

Hmmm, well, maybe if I want to turn my LO into a control interface for my next hyperspace trip, I might end up landing on a Replicator planet and being destroyed by robots ;-)

At the least, a nice little error message that the maximum limit has been reached would be nice ;-)

Alex
Comment 9 Jochen 2012-08-24 22:06:14 UTC
Hi Alex

exists your problem still using LO 3.6.0?
Comment 10 Alex Thurgood 2012-08-27 06:29:22 UTC
Yes.
Tested on Version 3.7.0.0.alpha0+ (Build ID: 3e9f9e5)
Comment 11 Alex Thurgood 2012-08-27 06:37:38 UTC Comment hidden (off-topic)
Comment 12 Julien Nabet 2014-07-24 05:22:14 UTC
Eike: what about removing MAX_DAYS and use:
- sal_Int32 everywhere (instead of long and sal_uInt16)
- sal_IntPtr instead of sal_uIntPtr
- SAL_MAX_INT32 (0x7FFFFFFF -> 4294967295 so far more than 3636532)
?
If ok, I could give a try to propose a patch.
Comment 13 Eike Rathke 2014-07-24 09:01:52 UTC
@Julien:
No objection in general, but bear in mind that the Date::nDate value is stored in a decimal coded way to hold YYYYMMDD so the maximum year value in a sal_Int32 would be 214748 (0x7fffffff is 2147483647 not 4294967295 ;-)  As the maximum number of normalized days must be representable by that coded sal_Int32, the resulting MAX_DAYS would be 78383020 (214748*365), do not drop those checks. Instead, verify that they do sane things, i.e. some currently set 9999-12-31 in such a case. Places may also check for year values of 9999 or 9957 or some such, introduce a DATE_MAX_YEAR constant for the 214748 maximum year value and eliminate those hard coded values.

Furthermore not all code may be able to cope with a year value of more than 4 digits, either in input or display formats, field lengths, ... so there may be some follow-up work to be done.
Comment 14 Lionel Elie Mamane 2014-07-24 09:38:54 UTC
Something more disruptive, but arguably cleaner, would be to stop (let's say in Base / form elements) (ab)using integers to mean dates, and to use com::sun::star::util::Date.

Unless we also "improve" formatters (in ways that other components of LibreOffice, especially Calc, will maybe not like...), I'm afraid this we'd have to go back to integer to format dates... So maybe this route would become quite a "big piece", but nice dream.
Comment 15 Lionel Elie Mamane 2014-07-24 09:40:43 UTC
(In reply to comment #12)
> Eike: what about removing MAX_DAYS and use:
> - sal_Int32 everywhere (instead of long and sal_uInt16)
> - sal_IntPtr instead of sal_uIntPtr

I very much doubt any date-handling code has any good reason to use either sal_IntPtr or sal_uIntPtr which are "an integer big enough to contain a pointer". The uses of that should very most probably be some sal_(u)Int16/32/64.
Comment 16 Julien Nabet 2014-07-24 10:35:44 UTC
Forget my last comment, we can't blindly change all this with sal_Int32 since it's called in several parts.
Moreover, following Lionel's comments, it seems it's not at all an easyhack, hope someone will find a way.
Comment 17 Eike Rathke 2014-07-24 11:45:53 UTC
I think changing the internal representation in tools' class Date wouldn't be that hard. To inspect places that might rely on the internal layout of Date::nDate and do calculations on their own and if so fix them to use proper class Date methods, I'd make the following methods private and global make to catch where it breaks:

Date( sal_uInt32 _nDate )
void SetDate( sal_uInt32 nNewDate )
sal_uInt32 GetDate() const


Using css::util::Date would only have advantage over improving tools' Date internals for that it would obsolete the internal back/forth conversion to/from coded integer. Its year field is a short, thus sal_Int16 with a max year value of 32765 which would be less than the 214748 years. (not that I don't doubt any use if such year values anyway ;-)

How you actually represent a date in an application is a different goal, and the spreadsheet applications' way (and not only those) to express a date/time as a double since a null date is yet something different. This is also independent of how you format a date, and the formatter doesn't have to care what the Date class uses internally.

I agree that the sal_uIntPtr should be sal_Int64 instead.
Comment 18 Alex Thurgood 2015-01-03 17:40:11 UTC Comment hidden (no-value)
Comment 19 QA Administrators 2016-01-17 20:02:20 UTC Comment hidden (obsolete)
Comment 20 Alex Thurgood 2016-01-19 12:24:25 UTC
Still present in 

Version: 4.4.7.2
Build ID: f3153a8b245191196a4b6b9abd1d0da16eead600
Locale: fr.UTF-8
Comment 21 Alex Thurgood 2016-01-19 12:36:07 UTC
Confirming also on 

Version: 5.2.0.0.alpha0+
Build ID: ac91a81b7b47fc9eba24455d34e2168309ea51d1
CPU Threads: 2; OS Version: -; UI Render: default; 
Locale: fr-FR (fr.UTF-8)
Comment 22 QA Administrators 2017-03-06 13:52:23 UTC Comment hidden (obsolete)
Comment 23 Eike Rathke 2017-03-06 15:04:12 UTC
Internal tools' class Date representation is now (since 5.2) capable to handle dates between -32768-01-01 and 32767-12-31, including conversion from/to css::util::Date

However, connectivity/source/commontools/dbconversion.cxx still has its own date implementation that probably interferes here. Someone familiar with Base should check what the consequences were if that was eliminated and the common tools Date routines be used. My guess is that various database related things can't cope with years > 9999 because year might be stored using 4 characters, so some extra handling will still be needed there.
Comment 24 Lionel Elie Mamane 2017-03-06 16:46:14 UTC
(In reply to Eike Rathke from comment #23)

> However, connectivity/source/commontools/dbconversion.cxx still has its own
> date implementation that probably interferes here.

For full clarity, it contains a (re?)implementation of some date utilities, but no date (data structure) implementation per se.

It uses the same MAX_DAYS mentioned in comment 5 in addDays and subDays, but frankly I have no idea why. I just ripped out the MAX_DAYS test and it seems to work fine.

I expect most, if not all, of these utilities would be advantageously replaced by a call to common tools Date routines.

> My guess is that various database
> related things can't cope with years > 9999 because year might be stored
> using 4 characters, so some extra handling will still be needed there.

LibreOffice doesn't know, and doesn't care, how the database engine stores the year. It should just throw the value at the database and if the database it unhappy it will raise an error or truncate the value. Or *maybe* the SDBC driver (that is the code in connectivity/source/drivers/*). Any code outside of that (that is generic to "all of Base" or "all database engines") has no good reason to arbitrarily enforce such a limitation, and I officially give my blessing to anyone that volunteers to rip it out.

The only thing is that if we construct SQL expressions with dates in them (as is done e.g. with calls to DBTypeConversion::toDateString, such as in connectivity/source/commontools/DateConversion.cxx function  DBTypeConversion::toSQLString), then the date MUST be in ISO order YEAR-MONTH-DAY, and year MUST have at least 4 digits to avoid ambiguity. That is "AT LEAST", not necssarily "AT MOST". 


WORK ON C++-LEVEL INFRASTRUCTURE

1) Remove the MAX_DAYS check from addDays and subDays
   (I'm doing that now)
2) Remove the negative value check from addDays and subDays,
   unless it makes sense.
3) Replace utilities in dbconversion.cxx by their tools counterpart;
   implBuildFromRelative and implIsLeapYear are prime targets.
   OTOH toDateString, toTimeString, toDateTimeString probably
   belong dbconversion.cxx
   (any volunteer?)
4) Keep in dbconversion.cxx only those that have no counterpart

WORK ON FORM CONTROLS

There are two kinds of form controls that can show dates:

1) Formatted field: this should be able to handle the same date range as Calc,
   that should be the number of days that fits in C++ "double". If not, that is
   to be fixed. One reason they now don't is the MAX_DAYS check,
   which I'm removing

2) Date Fields: testing shows the maximum value of the "Date max" property is
   01/01/9999. Find out why. Arbitrary limit or actual code limit?
   Raise the limit.
Comment 25 Commit Notification 2017-03-06 16:56:57 UTC
Lionel Elie Mamane committed a patch related to this issue.
It has been pushed to "master":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=57909620b4493ae7c8fe950c47e2c826b3c164aa

tdf#40575 remove obsolete MAX_DAYS check

It will be available in 5.4.0.

The patch should be included in the daily builds available at
http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
http://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.
Comment 26 Eike Rathke 2017-03-06 17:17:46 UTC
Thanks Lionel :-)
Comment 27 Alex Thurgood 2017-09-15 14:48:50 UTC
Testing with 

Version: 6.0.0.0.alpha0+
Build ID: 003c2cfaa258e204402a366a105366da74e220a2
CPU threads: 4; OS: Mac OS X 10.12.6; UI render: default; 
Locale: fr-FR (fr_FR.UTF-8); Calc: group

shows than I can successfully enter 9999 as the year, so this part is fixed :-)


For the record, if I try and enter any date beyond that in data entry mode in a table, I get a :

Error inserting the new record

java.lang.IllegalArgumentException

presumably because dates in hsqldb are ISO and the year can only have 4 digits ?
Comment 28 Alex Thurgood 2017-11-17 09:35:36 UTC
(In reply to Eike Rathke from comment #23)
> Internal tools' class Date representation is now (since 5.2) capable to
> handle dates between -32768-01-01 and 32767-12-31, including conversion
> from/to css::util::Date
> 
> However, connectivity/source/commontools/dbconversion.cxx still has its own
> date implementation that probably interferes here. Someone familiar with
> Base should check what the consequences were if that was eliminated and the
> common tools Date routines be used. My guess is that various database
> related things can't cope with years > 9999 because year might be stored
> using 4 characters, so some extra handling will still be needed there.

@Eike :

Does any of this affect the DateAdd() function in LOBasic ?
Someone has just reported on the German M-L that the help is incorrect when describing how to enter the string for the date (indeed, it does seem wrong, but that is tangential to what is to follow):

Sub example_dateadd
MsgBox DateAdd("m", 1, 31012004) &" - "& DateAdd("M", 1, 31012005)
End Sub

Using the above:

- if use double quote characters around the date string, I get other error messages, such as "unsuported type", or "unsupported operation" depending on which way I type the string separators (dots or slashes);

- if I use as in the above example, i.e. no quotes around what is essentially an integer, I get a MsgBox that displays "31/0/1/32767 - 31/01/32767" which is obviously incorrect.

Is this behaviour related to the changes in date representation support of css::util::Date or does DateAdd() rely on the Database date functions ?

Should I open a separate report for this ?
Comment 29 Alex Thurgood 2017-11-17 09:37:33 UTC
(In reply to Alex Thurgood from comment #28)


> - if I use as in the above example, i.e. no quotes around what is
> essentially an integer, I get a MsgBox that displays "31/0/1/32767 -
> 31/01/32767" which is obviously incorrect.
> 


I accidentally inserted an extra slash into the first date string above when typing out what is displayed on screen "31/01/32767" - I meant to point out that the year calculation is obviously incorrectly displayed.
Comment 30 Eike Rathke 2017-11-20 18:11:36 UTC
(In reply to Alex Thurgood from comment #28)
> Does any of this affect the DateAdd() function in LOBasic ?
Basic date math would be unrelated to Base dates.

> Should I open a separate report for this ?
Yes please.
Comment 31 Alex Thurgood 2017-11-23 14:32:08 UTC
(In reply to Eike Rathke from comment #30)
> (In reply to Alex Thurgood from comment #28)
> > Does any of this affect the DateAdd() function in LOBasic ?
> Basic date math would be unrelated to Base dates.
> 
> > Should I open a separate report for this ?
> Yes please.

OKay, now known as bug 114011