Bug 121978 - Problem with new function SECOND
Summary: Problem with new function SECOND
Status: VERIFIED FIXED
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: Calc (show other bugs)
Version:
(earliest affected)
6.0.6.2 release
Hardware: All All
: medium normal
Assignee: Winfried Donkers (retired)
URL:
Whiteboard: target:6.3.0 target:6.2.0
Keywords: bibisected, bisected, regression
Depends on: 122991
Blocks: Calc-Function
  Show dependency treegraph
 
Reported: 2018-12-08 07:21 UTC by gmolleda
Modified: 2019-02-07 10:24 UTC (History)
6 users (show)

See Also:
Crash report or crash signature:


Attachments
SECOND(...) return second-1. (124.20 KB, application/vnd.oasis.opendocument.spreadsheet)
2018-12-08 07:22 UTC, gmolleda
Details

Note You need to log in before you can comment on or make changes to this bug.
Description gmolleda 2018-12-08 07:21:21 UTC
When fixing the 118800 bug (https://bugs.documentfoundation.org/show_bug.cgi?id=118800), the developers changed the SECOND function to not round. All right.

But now, by subtracting two dates with hours, the result is a little less than the correct one and as a consequence the SECOND function fails to give the second of the correct difference.

Now if you write in Calc:

A2: 00:03:09
A3: 2018-10-22 11:31:44
A4: 2018-10-22 11:34:53
A5: =A4-A3 --> result 00:03:09
A6: =SECOND(A5) --> result in Calc 6.1.3.2 is 8, BAD. In Calc 6.0.6.2 and Excel 2016 is 9 (RIGHT). 

The internal number in A2 is 0,0021875 that is 60*3 minutes + 9 seconds * 1/(24 hours*60 minutes*60 seconds) and in A5 is 0,002187499995. Now SECOND function not round.

8 seconds, 99 hundredths is still the second 8 as the year of 12/31/2018 is 2018 and not 2019. You can wait 2018 if you write =YEAR("12/31/2018") and 8 if you write =SECOND("00:00:08,99"). In 6.0.6.2 and Excel 2016 this function results 9 and in Calc 6.1.3.2 the result is 8.

The problem is if you subtract two dates and the result goes wrong narrowly. Before nothing happened and now offers a second less than it should.
Comment 1 gmolleda 2018-12-08 07:22:48 UTC
Created attachment 147373 [details]
SECOND(...) return second-1.
Comment 2 Oliver Brinzing 2018-12-08 10:34:16 UTC
confirming with lo 6.1.4.1, 6.0.7.3: result is 8
Comment 3 Winfried Donkers (retired) 2018-12-08 11:03:32 UTC
With version 6.3 (current master, Linux), I get 00:03:08 in cell A5, and 8 in cell C5.
I will investigate further once I have finished working on another bug.
Comment 4 gmolleda 2018-12-08 11:17:21 UTC
I think of two possible solutions, or have two functions: SECOND("00:00:08,99") with result 9 and SECONDNOTROUNDED("00:00:08,99") that return 8. Or a function with a second parameter optional rounding: =SECOND("00:00:08,99" [, BOOLEAN ROUND]), default BOOLEAN ROUND is TRUE.

=SECOND("00:00:08,99") = SECOND("00:00:08,99",TRUE) = 9
=SECOND("00:00:08,99", FALSE) = 8

The good thing would be that computer doing the subtraction correctly, but I imagine that they are intrinsic problems to the floating-point calculations of the processors.
Comment 5 Oliver Brinzing 2018-12-08 15:49:17 UTC
seems to be a regression from:

tdf#118800 fix rounding error in Calc function HOUR, MINUTE, SECOND.

Change-Id: I7a875b172493112b66fca8f70d2061371a05486c
Reviewed-on: https://gerrit.libreoffice.org/57721
Tested-by: Jenkins
Reviewed-by: Eike Rathke <erack@redhat.com>
(cherry picked from commit c69e7266916ac1b8917477fb4eccdb9098da5792)
Reviewed-on: https://gerrit.libreoffice.org/57728

https://gerrit.libreoffice.org/plugins/gitiles/core/+/bda9288ffee552b55eed9dbf02e1204957bd4513

$ git bisect log
# bad: [1d66cc00ca6fd2e562cbed88704051b2f5d989e3] source sha:8d2abb388b0a2423c9b7e1f52373e1b06dd9786f
# good: [29d08f54c2f71ffee4fe12dbb24c5f5cbedecfd2] source sha:6eeac3539ea4cac32d126c5e24141f262eb5a4d9
git bisect start 'master' 'oldest'
# good: [3ac46f6c41b5044f162a451b10af0dc5afdcc113] source sha:22c7c3f54dbb93f856190c561b2540064c5a767d
git bisect good 3ac46f6c41b5044f162a451b10af0dc5afdcc113
# good: [63fc3e0d41dd91f9fb3fe9891e009451285d9619] source sha:13a1bc409d9b2f0d14f4d316b7977b1fc2eb3c8a
git bisect good 63fc3e0d41dd91f9fb3fe9891e009451285d9619
# good: [6a75149c2bee2fa1c01b36aef4b734ceee7bc025] source sha:91d8af2c5cf4e8ec0f1ce0e532e0c896de77750b
git bisect good 6a75149c2bee2fa1c01b36aef4b734ceee7bc025
# good: [6440df548810a3adf5f7d125fe738467a5db7891] source sha:76c0b3c516f6b0d43136522b4d476eb60211cec1
git bisect good 6440df548810a3adf5f7d125fe738467a5db7891
# bad: [809628c3decd16dacc705bda20efc603037667bf] source sha:3c20597ada7f74a4a96dec841264593fdbf0bcd5
git bisect bad 809628c3decd16dacc705bda20efc603037667bf
# good: [da141926f04f873160a6b3c3b9c9a2e5ef754543] source sha:7196aa63832c9d7fe59b31f98cd64696795d2841
git bisect good da141926f04f873160a6b3c3b9c9a2e5ef754543
# bad: [11d929709827f777669545d6c45744ee4857808a] source sha:51c7a6f68ef25d7c292dc988400a2c59ef479017
git bisect bad 11d929709827f777669545d6c45744ee4857808a
# good: [dfe687583b5b2c81fa4a7a49aa17e81391043354] source sha:337012970f62a8a40da2a02806363b560cf295df
git bisect good dfe687583b5b2c81fa4a7a49aa17e81391043354
# bad: [eabc85c658401f6e8953da678a71229e1281713b] source sha:7b62c5266e62c3fb0ce1285949d51020075a3f81
git bisect bad eabc85c658401f6e8953da678a71229e1281713b
# good: [80d46d9ed0972d75f5c338220d74750e4dae7383] source sha:076cb54472d5bba8916df9ee50074ac74433d694
git bisect good 80d46d9ed0972d75f5c338220d74750e4dae7383
# bad: [64716bbf3fab04a4d6300700c9d7ee205a8e1aef] source sha:e6eb1644f84dd1fd48bbbf5418acdcc750f723bc
git bisect bad 64716bbf3fab04a4d6300700c9d7ee205a8e1aef
# bad: [6996350c78b99fc62232b3638fb7b3b9392e81ae] source sha:bda9288ffee552b55eed9dbf02e1204957bd4513
git bisect bad 6996350c78b99fc62232b3638fb7b3b9392e81ae
# bad: [6996350c78b99fc62232b3638fb7b3b9392e81ae] source sha:bda9288ffee552b55eed9dbf02e1204957bd4513
git bisect bad 6996350c78b99fc62232b3638fb7b3b9392e81ae
# good: [6a205cee7c62db8e04fe65ff241c7b3285e53458] source sha:896b7e69a3182a0142a323ba5f76a2d8a811091a
git bisect good 6a205cee7c62db8e04fe65ff241c7b3285e53458
# first bad commit: [6996350c78b99fc62232b3638fb7b3b9392e81ae] source sha:bda9288ffee552b55eed9dbf02e1204957bd4513
Comment 6 gmolleda 2018-12-08 16:13:28 UTC
Beware that they have corrected the failure of which =SECOND("00:00:08.99") = 9 and not 8, same as YEAR ("12/31/2018") <> 2019.

Another solution would be to round up the later unit:
=SECOND("00:00:08.990") -> SECOND("00:00:08.99") = 8, but =SECOND("00:00:08.999") -> SECOND("00:00:09.00") = 9.

Units?
DATE (dd/mm/yyyy)
HOUR
MINUTE
SECOND
HUNDREDTH or THOUSANDTHS
Comment 7 Winfried Donkers (retired) 2018-12-10 14:30:54 UTC
I'll take it.

Bug 118800 was meant to fix rounding 59.9 seconds to 60, which is not a valid result for SECOND, which must return integer values between 0 and 59 (inclusive).

This bug shows that the fix for tdf118800 is not correct, as it returns the integer part of the second value, instead of rounding the value.

ODF1.2 Part 2 ยง6.10.16 states that the second value is to be rounded.

That means that SECOND(11:22:59.9) should return 0 (11:23:00).
Comment 8 Eike Rathke 2018-12-10 18:18:06 UTC
Which is somewhat funny though because with A1: 11:22:59.9 a formula =HOUR(A1)&":"&MINUTE(A1)&":"&SECOND(A1) then yields "11:22:00".
Note that ODFF 6.10.10 HOUR and 6.10.12 MINUTE do not specify any rounding, otherwise even an over-roll to another day could occur.
Shrug, if that's what is specified and people expect from Excel..
Bad me, who didn't look each function up before doing the change with commit 7f3c6efd859050c8f376b6820710e91fa9077ac4.

@Winfried:
You can probably easily fix that in ScInterpreter::ScGetSec() by changing the call to GetClock() to 

  tools::Time::GetClock( GetDouble(), nHour, nMinute, nSecond, fFractionOfSecond, 1);

and then compare fFractionOfSecond to >= 0.5 and if so then

  nSecond = (nSecond+1) % 60;
Comment 9 Eike Rathke 2018-12-10 18:26:08 UTC
(In reply to Eike Rathke from comment #8)
> Bad me, who didn't look each function up before doing the change with commit
> 7f3c6efd859050c8f376b6820710e91fa9077ac4.
Or rather commit 273b3e10eab70ebc084cb62568bd699fddfb376e as a follow-up of c69e7266916ac1b8917477fb4eccdb9098da5792, but the earlier behaviour of rounding also HOUR and MINUTE was equally wrong..
Comment 10 Winfried Donkers (retired) 2018-12-11 07:21:19 UTC
(In reply to Eike Rathke from comment #9)
> (In reply to Eike Rathke from comment #8)
> > Bad me, who didn't look each function up before doing the change with commit
> > 7f3c6efd859050c8f376b6820710e91fa9077ac4.
> Or rather commit 273b3e10eab70ebc084cb62568bd699fddfb376e as a follow-up of
> c69e7266916ac1b8917477fb4eccdb9098da5792, but the earlier behaviour of
> rounding also HOUR and MINUTE was equally wrong..

@Eike: I have the 'fix' for SECOND ready (yesterday afternoon), but I stumbled across inconsistenties between ODF-standard, Calc and Excel in SECOND, MINUTE, HOUR (and possibly DAY, MONTH, YEAR), just as you did in comment #8.
So I intend to make a comparison between ODF1.2 Part2, Calc, Excel, MariaDB, Transact-SQL for the(date)time parts and for the presentation (cell format)of (date)time (e.g. how is 23:59:59.9 presented with cell format HH:MM).
I want to withhold my fix till the extend of the behavioural problem is clear to me.

I think the cause for these problems are that a time can be regarded mathematically (i.e. use rounding) and treshold-wise (e.g. New year only starts when 60 seconds are reached at 23:59, or your time does not count when you stop 1mm from the finish in a 100m sprint contest). And these two approaches don't mix.
Comment 11 Winfried Donkers (retired) 2018-12-11 10:16:04 UTC
(In reply to Winfried Donkers from comment #10)
> So I intend to make a comparison between ODF1.2 Part2, Calc, Excel, MariaDB,
> Transact-SQL for the(date)time parts and for the presentation (cell
> format)of (date)time (e.g. how is 23:59:59.9 presented with cell format
> HH:MM).

I added attachment #147433 [details] to bug #118800.
Comment 12 gmolleda 2018-12-22 05:35:24 UTC
The correct behavior was the previous (6.0.7.3), rounding, not as in 6.1.3.2 without rounding.
If someone wants the exact second without rounding you can use the functions:
10:50:59,99	
0	=SECOND(A1)
59	=VALUE(MID(TEXT(A1;"hh:mm:ss,0000");7;2))

With LibreOffice 6.0.7.3
Comment 13 Commit Notification 2019-01-10 13:04:39 UTC
Winfried Donkers committed a patch related to this issue.
It has been pushed to "master":

https://git.libreoffice.org/core/+/1692034d53ce28b2c5d1c63ab429232a92b2f861%5E%21

tdf#121978 use round again for Calc function SECOND.

It will be available in 6.3.0.

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

Affected users are encouraged to test the fix and report feedback.
Comment 14 Commit Notification 2019-01-11 00:10:09 UTC
Winfried Donkers committed a patch related to this issue.
It has been pushed to "libreoffice-6-2":

https://git.libreoffice.org/core/+/02817ec9f22fc5845d2b2686cac4573cda3b4062%5E%21

tdf#121978 use round again for Calc function SECOND.

It will be available in 6.2.1.

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

Affected users are encouraged to test the fix and report feedback.
Comment 15 Commit Notification 2019-01-11 12:57:55 UTC
Winfried Donkers committed a patch related to this issue.
It has been pushed to "libreoffice-6-2-0":

https://git.libreoffice.org/core/+/e443c49f59e4962d667570c23f82a3390fd604fe%5E%21

tdf#121978 use round again for Calc function SECOND.

It will be available in 6.2.0.

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

Affected users are encouraged to test the fix and report feedback.
Comment 16 BogdanB 2019-01-17 20:49:33 UTC
Tested for 6.3
It's ok now, with example from Description I get at A6 the value 9, which si right.

Version: 6.3.0.0.alpha0+
Build ID: afbbdcc216a84b59fb263777659b044c4a7cf6f0
CPU threads: 4; OS: Linux 4.15; UI render: default; VCL: gtk3; 
TinderBox: Linux-rpm_deb-x86_64@86-TDF, Branch:master, Time: 2019-01-13_03:54:12
Locale: ro-RO (ro_RO.UTF-8); UI-Language: en-US
Calc: threaded