Skip to content

Commit

Permalink
Make construction of SqlTimestamp and SqlTimestampWithTimeZone stricter
Browse files Browse the repository at this point in the history 
Previously, it was applying rounding rules, which ended up masking potential
problems in operations that are expected to do proper rounding.
  • Loading branch information
@martint
martint committed Nov 2, 2020
1 parent 4aa3fe8 commit ba17097
Show file tree
Hide file tree
Showing 6 changed files with 55 additions and 88 deletions.
2 changes: 1 addition & 1 deletion presto-main/src/main/java/io/prestosql/testing/DateTimeTestingUtils.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ public static SqlTimestampWithTimeZone sqlTimestampWithTimeZoneOf(int precision,
ZonedDateTime base = ZonedDateTime.of(year, month, day, hour, minute, second, 0, timeZoneKey.getZoneId());

long epochMillis = base.toEpochSecond() * MILLISECONDS_PER_SECOND + nanoOfSecond / NANOSECONDS_PER_MILLISECOND;
int picosOfMilli = (int) scaleNanosToPicos(nanoOfSecond);
int picosOfMilli = (nanoOfSecond % NANOSECONDS_PER_MILLISECOND) * PICOSECONDS_PER_NANOSECOND;

return SqlTimestampWithTimeZone.newInstance(precision, epochMillis, picosOfMilli, timeZoneKey);
}
Expand Down
3 changes: 2 additions & 1 deletion presto-main/src/test/java/io/prestosql/block/BlockAssertions.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@
import static io.prestosql.spi.type.RealType.REAL;
import static io.prestosql.spi.type.TimestampType.TIMESTAMP_MILLIS;
import static io.prestosql.spi.type.TimestampWithTimeZoneType.TIMESTAMP_WITH_TIME_ZONE;
import static io.prestosql.spi.type.Timestamps.MICROSECONDS_PER_MILLISECOND;
import static io.prestosql.spi.type.VarbinaryType.VARBINARY;
import static io.prestosql.spi.type.VarcharType.VARCHAR;
import static io.prestosql.testing.TestingConnectorSession.SESSION;
Expand Down Expand Up @@ -532,7 +533,7 @@ public static Block createTimestampSequenceBlock(int start, int end)
BlockBuilder builder = TIMESTAMP_MILLIS.createFixedSizeBlockBuilder(end - start);

for (int i = start; i < end; i++) {
TIMESTAMP_MILLIS.writeLong(builder, i);
TIMESTAMP_MILLIS.writeLong(builder, i * MICROSECONDS_PER_MILLISECOND);
}

return builder.build();
Expand Down
22 changes: 11 additions & 11 deletions presto-main/src/test/java/io/prestosql/type/TestTimestampType.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,17 +30,17 @@ public TestTimestampType()
public static Block createTestBlock()
{
BlockBuilder blockBuilder = TIMESTAMP_MILLIS.createBlockBuilder(null, 15);
TIMESTAMP_MILLIS.writeLong(blockBuilder, 1111);
TIMESTAMP_MILLIS.writeLong(blockBuilder, 1111);
TIMESTAMP_MILLIS.writeLong(blockBuilder, 1111);
TIMESTAMP_MILLIS.writeLong(blockBuilder, 2222);
TIMESTAMP_MILLIS.writeLong(blockBuilder, 2222);
TIMESTAMP_MILLIS.writeLong(blockBuilder, 2222);
TIMESTAMP_MILLIS.writeLong(blockBuilder, 2222);
TIMESTAMP_MILLIS.writeLong(blockBuilder, 2222);
TIMESTAMP_MILLIS.writeLong(blockBuilder, 3333);
TIMESTAMP_MILLIS.writeLong(blockBuilder, 3333);
TIMESTAMP_MILLIS.writeLong(blockBuilder, 4444);
TIMESTAMP_MILLIS.writeLong(blockBuilder, 1111_000);
TIMESTAMP_MILLIS.writeLong(blockBuilder, 1111_000);
TIMESTAMP_MILLIS.writeLong(blockBuilder, 1111_000);
TIMESTAMP_MILLIS.writeLong(blockBuilder, 2222_000);
TIMESTAMP_MILLIS.writeLong(blockBuilder, 2222_000);
TIMESTAMP_MILLIS.writeLong(blockBuilder, 2222_000);
TIMESTAMP_MILLIS.writeLong(blockBuilder, 2222_000);
TIMESTAMP_MILLIS.writeLong(blockBuilder, 2222_000);
TIMESTAMP_MILLIS.writeLong(blockBuilder, 3333_000);
TIMESTAMP_MILLIS.writeLong(blockBuilder, 3333_000);
TIMESTAMP_MILLIS.writeLong(blockBuilder, 4444_000);
return blockBuilder.build();
}

Expand Down
23 changes: 21 additions & 2 deletions presto-spi/src/main/java/io/prestosql/spi/type/SqlTimestamp.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
import static java.lang.Math.floorDiv;
import static java.lang.Math.floorMod;
import static java.lang.Math.toIntExact;
import static java.lang.String.format;

public final class SqlTimestamp
{
Expand All @@ -38,12 +39,30 @@ public final class SqlTimestamp

public static SqlTimestamp fromMillis(int precision, long millis)
{
return newInstance(precision, millis * 1000, 0);
return newInstanceWithRounding(precision, millis * 1000, 0);
}

public static SqlTimestamp newInstance(int precision, long epochMicros, int picosOfMicro)
{
return newInstanceWithRounding(precision, epochMicros, picosOfMicro);
if (precision <= 6) {
if (picosOfMicro != 0) {
throw new IllegalArgumentException(format("Expected picosOfMicro to be 0 for precision %s: %s", precision, picosOfMicro));
}
if (round(epochMicros, 6 - precision) != epochMicros) {
throw new IllegalArgumentException(format("Expected 0s for digits beyond precision %s: epochMicros = %s", precision, epochMicros));
}
}
else {
if (round(picosOfMicro, 12 - precision) != picosOfMicro) {
throw new IllegalArgumentException(format("Expected 0s for digits beyond precision %s: picosOfMicro = %s", precision, picosOfMicro));
}
}

if (picosOfMicro < 0 || picosOfMicro > PICOSECONDS_PER_MICROSECOND) {
throw new IllegalArgumentException("picosOfMicro is out of range: " + picosOfMicro);
}

return new SqlTimestamp(precision, epochMicros, picosOfMicro);
}

private static SqlTimestamp newInstanceWithRounding(int precision, long epochMicros, int picosOfMicro)
Expand Down
21 changes: 20 additions & 1 deletion presto-spi/src/main/java/io/prestosql/spi/type/SqlTimestampWithTimeZone.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
import static java.lang.Math.floorDiv;
import static java.lang.Math.floorMod;
import static java.lang.Math.toIntExact;
import static java.lang.String.format;
import static java.util.Objects.requireNonNull;

public final class SqlTimestampWithTimeZone
Expand All @@ -47,7 +48,25 @@ public static SqlTimestampWithTimeZone fromInstant(int precision, Instant instan

public static SqlTimestampWithTimeZone newInstance(int precision, long epochMillis, int picosOfMilli, TimeZoneKey timeZoneKey)
{
return newInstanceWithRounding(precision, epochMillis, picosOfMilli, timeZoneKey);
if (precision <= 3) {
if (picosOfMilli != 0) {
throw new IllegalArgumentException(format("Expected picosOfMilli to be 0 for precision %s: %s", precision, picosOfMilli));
}
if (round(epochMillis, 3 - precision) != epochMillis) {
throw new IllegalArgumentException(format("Expected 0s for digits beyond precision %s: epochMicros = %s", precision, epochMillis));
}
}
else {
if (round(picosOfMilli, 12 - precision) != picosOfMilli) {
throw new IllegalArgumentException(format("Expected 0s for digits beyond precision %s: picosOfMilli = %s", precision, picosOfMilli));
}
}

if (picosOfMilli < 0 || picosOfMilli > PICOSECONDS_PER_MILLISECOND) {
throw new IllegalArgumentException("picosOfMilli is out of range: " + picosOfMilli);
}

return new SqlTimestampWithTimeZone(precision, epochMillis, picosOfMilli, timeZoneKey);
}

private static SqlTimestampWithTimeZone newInstanceWithRounding(int precision, long epochMillis, int picosOfMilli, TimeZoneKey sessionTimeZoneKey)
Expand Down
72 changes: 0 additions & 72 deletions presto-spi/src/test/java/io/prestosql/spi/type/TestSqlTimestamp.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -40,78 +40,6 @@ public void testBaseline()
assertThat(newInstance(12, 0, 0).toString()).isEqualTo("1970-01-01 00:00:00.000000000000");
}

@Test
public void testPositiveEpoch()
{
// round down
// represents a timestamp of 1970-01-01 00:00:00.111111111111
assertThat(newInstance(0, 111111, 111111).toString()).isEqualTo("1970-01-01 00:00:00");
assertThat(newInstance(1, 111111, 111111).toString()).isEqualTo("1970-01-01 00:00:00.1");
assertThat(newInstance(2, 111111, 111111).toString()).isEqualTo("1970-01-01 00:00:00.11");
assertThat(newInstance(3, 111111, 111111).toString()).isEqualTo("1970-01-01 00:00:00.111");
assertThat(newInstance(4, 111111, 111111).toString()).isEqualTo("1970-01-01 00:00:00.1111");
assertThat(newInstance(5, 111111, 111111).toString()).isEqualTo("1970-01-01 00:00:00.11111");
assertThat(newInstance(6, 111111, 111111).toString()).isEqualTo("1970-01-01 00:00:00.111111");
assertThat(newInstance(7, 111111, 111111).toString()).isEqualTo("1970-01-01 00:00:00.1111111");
assertThat(newInstance(8, 111111, 111111).toString()).isEqualTo("1970-01-01 00:00:00.11111111");
assertThat(newInstance(9, 111111, 111111).toString()).isEqualTo("1970-01-01 00:00:00.111111111");
assertThat(newInstance(10, 111111, 111111).toString()).isEqualTo("1970-01-01 00:00:00.1111111111");
assertThat(newInstance(11, 111111, 111111).toString()).isEqualTo("1970-01-01 00:00:00.11111111111");
assertThat(newInstance(12, 111111, 111111).toString()).isEqualTo("1970-01-01 00:00:00.111111111111");

// round up
// represents a timestamp of 1970-01-01 00:00:00.555555555555
assertThat(newInstance(0, 555555, 555555).toString()).isEqualTo("1970-01-01 00:00:01");
assertThat(newInstance(1, 555555, 555555).toString()).isEqualTo("1970-01-01 00:00:00.6");
assertThat(newInstance(2, 555555, 555555).toString()).isEqualTo("1970-01-01 00:00:00.56");
assertThat(newInstance(3, 555555, 555555).toString()).isEqualTo("1970-01-01 00:00:00.556");
assertThat(newInstance(4, 555555, 555555).toString()).isEqualTo("1970-01-01 00:00:00.5556");
assertThat(newInstance(5, 555555, 555555).toString()).isEqualTo("1970-01-01 00:00:00.55556");
assertThat(newInstance(6, 555555, 555555).toString()).isEqualTo("1970-01-01 00:00:00.555556");
assertThat(newInstance(7, 555555, 555555).toString()).isEqualTo("1970-01-01 00:00:00.5555556");
assertThat(newInstance(8, 555555, 555555).toString()).isEqualTo("1970-01-01 00:00:00.55555556");
assertThat(newInstance(9, 555555, 555555).toString()).isEqualTo("1970-01-01 00:00:00.555555556");
assertThat(newInstance(10, 555555, 555555).toString()).isEqualTo("1970-01-01 00:00:00.5555555556");
assertThat(newInstance(11, 555555, 555555).toString()).isEqualTo("1970-01-01 00:00:00.55555555556");
assertThat(newInstance(12, 555555, 555555).toString()).isEqualTo("1970-01-01 00:00:00.555555555555");
}

@Test
public void testNegativeEpoch()
{
// round down
// represents a timestamp of 1969-12-31 23:59:59.111111111111
assertThat(newInstance(0, -888889, 111111).toString()).isEqualTo("1969-12-31 23:59:59");
assertThat(newInstance(1, -888889, 111111).toString()).isEqualTo("1969-12-31 23:59:59.1");
assertThat(newInstance(2, -888889, 111111).toString()).isEqualTo("1969-12-31 23:59:59.11");
assertThat(newInstance(3, -888889, 111111).toString()).isEqualTo("1969-12-31 23:59:59.111");
assertThat(newInstance(4, -888889, 111111).toString()).isEqualTo("1969-12-31 23:59:59.1111");
assertThat(newInstance(5, -888889, 111111).toString()).isEqualTo("1969-12-31 23:59:59.11111");
assertThat(newInstance(6, -888889, 111111).toString()).isEqualTo("1969-12-31 23:59:59.111111");
assertThat(newInstance(7, -888889, 111111).toString()).isEqualTo("1969-12-31 23:59:59.1111111");
assertThat(newInstance(8, -888889, 111111).toString()).isEqualTo("1969-12-31 23:59:59.11111111");
assertThat(newInstance(9, -888889, 111111).toString()).isEqualTo("1969-12-31 23:59:59.111111111");
assertThat(newInstance(10, -888889, 111111).toString()).isEqualTo("1969-12-31 23:59:59.1111111111");
assertThat(newInstance(11, -888889, 111111).toString()).isEqualTo("1969-12-31 23:59:59.11111111111");
assertThat(newInstance(12, -888889, 111111).toString()).isEqualTo("1969-12-31 23:59:59.111111111111");

// round up
// represents a timestamp of 1969-12-31 23:59:59.555555555555
assertThat(newInstance(0, -444445, 555555).toString()).isEqualTo("1970-01-01 00:00:00");
assertThat(newInstance(1, -444445, 555555).toString()).isEqualTo("1969-12-31 23:59:59.6");
assertThat(newInstance(2, -444445, 555555).toString()).isEqualTo("1969-12-31 23:59:59.56");
assertThat(newInstance(3, -444445, 555555).toString()).isEqualTo("1969-12-31 23:59:59.556");
assertThat(newInstance(4, -444445, 555555).toString()).isEqualTo("1969-12-31 23:59:59.5556");
assertThat(newInstance(5, -444445, 555555).toString()).isEqualTo("1969-12-31 23:59:59.55556");
assertThat(newInstance(6, -444445, 555555).toString()).isEqualTo("1969-12-31 23:59:59.555556");
assertThat(newInstance(7, -444445, 555555).toString()).isEqualTo("1969-12-31 23:59:59.5555556");
assertThat(newInstance(8, -444445, 555555).toString()).isEqualTo("1969-12-31 23:59:59.55555556");
assertThat(newInstance(9, -444445, 555555).toString()).isEqualTo("1969-12-31 23:59:59.555555556");
assertThat(newInstance(10, -444445, 555555).toString()).isEqualTo("1969-12-31 23:59:59.5555555556");
assertThat(newInstance(11, -444445, 555555).toString()).isEqualTo("1969-12-31 23:59:59.55555555556");
assertThat(newInstance(12, -444445, 555555).toString()).isEqualTo("1969-12-31 23:59:59.555555555555");
}

@Test
public void testRoundTo()
{
Expand Down

0 comments on commit ba17097

Please sign in to comment.