Fix precision loss in from_unixtime(double) function

[hainenber]

Description

Apply spershin's proposed change to fix precision loss for timestamp yielded from FROM_UNIXTIME() function.

Motivation and Context

Fixes #21891

Impact

Test Plan

Contributor checklist

• Please make sure your submission complies with our development, formatting, commit message, and attribution guidelines.
• PR description addresses the issue accurately and concisely. If the change is non-trivial, a GitHub Issue is referenced.
• Documented new properties (with its default value), SQL syntax, functions, or other functionality.
• If release notes are required, they follow the release notes guidelines.
• Adequate tests were added if applicable.
• CI passed.

Release Notes

Please follow release notes guidelines and fill in the release notes below.

== RELEASE NOTES ==

General Changes
*  Fixes precision loss when timestamp yielded from `from_unixtime(double)` function.

[mbasmanova]

@hainenber Thank you for the fix.

Please, review Contributing guidelines at https://github.com/prestodb/presto/blob/master/CONTRIBUTING.md and update the PR to comply.

[mbasmanova]

Notice that function name is from_unixtime, not from_unix. Make sure to use the right name in PR title/description, commit message, Release Notes. etc.

Is TestExpressionCompiler.java the right place for this test? Shouldn't it go somewhere in com/facebook/presto/operator/scalar/TestDateTimeFunctionsBase.java

[hainenber]

Thanks, I've moved them to com/facebook/presto/operator/scalar/TestDateTimeFunctionsBase.java in latest commit

[hainenber]

Commits squashed and adhere to commit message guideline.

[mbasmanova]

Would you confirm that this test fails without the change?

[spershin]

Maybe adding a comment that this particular double was causing loss of precision in the function before the fix and hence we are testing it here.
In case anyone wondering why this number.

[hainenber]

I can confirm the test fail without the changes.

I reused many data in your GH issue to provide the details on the why for the change :D

[sdruzkin]

Is this a fix for when the timestamp presented as double has nanos? I cannot repro this issue when working with millis directly.

assertFunction("from_unixtime(1.7041507095805E9)"

My not so unit test code:

    public static double toUnixTimeTest(long timestamp)
    {
        return timestamp / 1000.0;
    }
    public static long fromUnixTimeTestOld(double unixTime)
    {
        return Math.round(unixTime * 1000);
    }

    public static long fromUnixTimeTestNew(double unixTime)
    {
        return Math.round(Math.floor(unixTime) * 1000 + Math.round((unixTime - Math.floor(unixTime)) * 1000));
    }

    public static void main(String[] args)
    {
        SqlTimestamp baselineTimestamp = sqlTimestampOf(LocalDateTime.of(2024, 1, 1, 23, 11, 49, millisToNanos(580)));
        long baselineMillis = baselineTimestamp.getMillis();

        for (int i = -1_000_000_000; i < 1_000_000_000; i++) {
            long expectedMillis = baselineMillis + i;
            double doubleValue = toUnixTimeTest(expectedMillis);
            long oldMillis = fromUnixTimeTestOld(doubleValue);
            long newMillis = fromUnixTimeTestNew(doubleValue);

            if (expectedMillis != oldMillis || oldMillis != newMillis) {
                SqlTimestamp tsOld = new SqlTimestamp(oldMillis, MILLISECONDS);
                SqlTimestamp tsNew = new SqlTimestamp(newMillis, MILLISECONDS);

                System.out.println(expectedMillis);
                System.out.println(baselineTimestamp);
                System.out.println(tsOld);
                System.out.println(tsNew);
                System.out.println();
            }
        }
    }

[sdruzkin]

@mbasmanova In my understanding this change might cause checksum mismatches when running verifier on Velox written DWRF partitions because Velox uses nano precision in timestamps.

[mbasmanova]

This is a tricky change. Future readers won't know why this is done in this particular way and 'git log' won't help much because commit message just says 'loses precision in some corner cases' without providing any details.

Would you add a comment here to explain what's going on and why the computation is done this way and also update commit message to explain clearly the problem and solution?

[spershin]

@hainenber
We could reuse some sentences from the issue and put them in comment.

[hainenber]

I've amended the change with more details in the form of comments and commit message.

[spershin]

@hainenber

Thank you for tacking ling this change.
It is good to go.
Please, add some comments to the new code as reviewers suggested.

[spershin]

@hainenber
We could reuse some sentences from the issue and put them in comment.

[spershin]

Maybe adding a comment that this particular double was causing loss of precision in the function before the fix and hence we are testing it here.
In case anyone wondering why this number.

[spershin]

@hainenber
You might want to rebase your PR on the fresh master and resubmit - there was an update to prevent e2e tests from failing all the time.

[mbasmanova]

@hainenber Thank you for iterating on this PR. Looks good % some nits and commit message needs updating.

There are typos in the commit message, the title is too long and the body has some lines that are too long. I suggest to use the following as the commit message.

Fix precision loss in from_unixtime(double) function

from_unixtime(1.7041507095805E9) used to return 1704150709 seconds and 581
milliseconds. It should return 580 milliseconds.

Before this change, the function used Math.round(unixTime * 1000), which loses
precision in some cases.

In the above case, it pushes the resulting number to 1704150709580.500000000,
which after rounding becomes 1704150709581, e.g. 581 milliseconds.

Current commit message:

[mbasmanova]

and hence we are testing it here

Drop this phrase as it is redundant.

[mbasmanova]

Let's move this comment inside the function.

[mbasmanova]

Machine-representable double for the 1.7041507095805E9 is 1704150709.58049988746643066406.

My understanding is that 1.7041507095805E9 cannot be represented in a computer. Are you saying that 1704150709.58049988746643066406 is the closest value that can be represented?

[hainenber]

Yes, that's the original sentence from the author :D

[mbasmanova]

CC: @kaikalur @aditi-pandit

[github-actions]

Codenotify: Notifying subscribers in CODENOTIFY files for diff 75ebaf1...5acdf21.

No notifications.

[spershin]

Looks good, thank you for working on this and addressing tons of comments!

[spershin]

@hainenber

Let's rebase and merge this change.
Thanks!

[spershin]

@hainenber

Let's rebase and merge this change.
Let us know, please, if you for some reason are unable to do this.
Thanks!

[hainenber]

hi there, this change can have other folks taking over. Sorry for the belated info!

[tdcmeehan]

@spershin rebased, could you please re-review?

[spershin]

Thanks for the fix!

[spershin]

@tdcmeehan

Accepted, however, I'm not a committer, so a committer needs to review this too.

cc @mbasmanova ?

[mbasmanova]

Thanks.