Skip to content

Commit

Permalink
Fix precision loss in from_unixtime(double) function
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
hainenber authored and spershin committed Mar 12, 2024
1 parent 6804b44 commit 6a9f3cd
Show file tree
Hide file tree
Showing 2 changed files with 13 additions and 1 deletion.
Original file line number Diff line number Diff line change
Expand Up @@ -182,7 +182,13 @@ public static long localTimestamp(SqlFunctionProperties properties)
@SqlType(StandardTypes.TIMESTAMP)
public static long fromUnixTime(@SqlType(StandardTypes.DOUBLE) double unixTime)
{
return Math.round(unixTime * 1000);
// This implementation fixes previous issue of precision loss when it comes for some edge cases.
// For example, 1.7041507095805E9 would correctly yield "2024-01-01 15:11:49.580"
// Machine-representable double for the 1.7041507095805E9 is 1704150709.58049988746643066406.
// 1704150709 goes to seconds and 0.580499887466 should be rounded to 580 milliseconds.
// Previous implementation would wrongly result in 581 milliseconds.
// Reference: https://github.com/prestodb/presto/issues/21891#issue-2126580070
return Math.round(Math.floor(unixTime) * 1000 + Math.round((unixTime - Math.floor(unixTime)) * 1000));
}

@ScalarFunction("from_unixtime")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
import com.facebook.presto.common.type.SqlDate;
import com.facebook.presto.common.type.SqlTime;
import com.facebook.presto.common.type.SqlTimeWithTimeZone;
import com.facebook.presto.common.type.SqlTimestamp;
import com.facebook.presto.common.type.SqlTimestampWithTimeZone;
import com.facebook.presto.common.type.TimeType;
import com.facebook.presto.common.type.TimeZoneKey;
Expand Down Expand Up @@ -78,6 +79,7 @@
import static java.util.concurrent.TimeUnit.NANOSECONDS;
import static java.util.concurrent.TimeUnit.SECONDS;
import static org.joda.time.DateTimeUtils.getInstantChronology;
import static org.joda.time.DateTimeZone.UTC;
import static org.joda.time.Days.daysBetween;
import static org.joda.time.DurationFieldType.millis;
import static org.joda.time.Months.monthsBetween;
Expand Down Expand Up @@ -193,6 +195,10 @@ public void testFromUnixTime()
dateTime = new DateTime(2001, 1, 22, 3, 4, 5, 888, DATE_TIME_ZONE);
seconds = dateTime.getMillis() / 1000.0;
assertFunction("from_unixtime(" + seconds + ")", TimestampType.TIMESTAMP, sqlTimestampOf(dateTime, session));

// The particular double, 1.7041507095805E9, was causing loss of precision in the function before the fix #21899
SqlTimestamp expected = sqlTimestampOf(2024, 1, 1, 23, 11, 49, 580, UTC, session.getTimeZoneKey(), session.toConnectorSession());
assertFunction("from_unixtime(1.7041507095805E9)", TimestampType.TIMESTAMP, expected);
}

@Test
Expand Down

0 comments on commit 6a9f3cd

Please # to comment.