Add parametric TIMESTAMP(p) support with long timestamps#87
Draft
Copilot wants to merge 9 commits into
Draft
Conversation
Copilot created this pull request from a session on behalf of
Joe-Abraham
April 2, 2026 11:32
View session
Reviewer's GuidePorts Trino’s parametric TIMESTAMP(p) infrastructure into PrestoDB: introduces short/long timestamp type hierarchy with a TIMESTAMP(p) parametric type, new 12‑byte block storage (Fixed12Block/Builder/Encoding) and LongTimestamp value type, wires these into metadata, operators, and existing connectors (e.g., Iceberg and plan printer), and adds comprehensive tests plus bug fixes for timestamp rescaling/formatting and equality/hash semantics. Class diagram for the new timestamp type hierarchyclassDiagram
direction LR
class TimestampType {
<<abstract>>
-int precision
+static int MAX_PRECISION
+static int MAX_SHORT_PRECISION
+static TimestampType TIMESTAMP_SECONDS
+static TimestampType TIMESTAMP_MILLIS
+static TimestampType TIMESTAMP_MICROS
+static TimestampType TIMESTAMP_NANOS
+static TimestampType TIMESTAMP_PICOS
+static TimestampType TIMESTAMP
+static TimestampType TIMESTAMP_MICROSECONDS
+TimestampType createTimestampType(int precision)
+int getPrecision()
+boolean isShort()
+boolean isLong()
+TimeUnit getStorageUnit()
+boolean isComparable()
+boolean isOrderable()
+Object getObjectValue(SqlFunctionProperties properties, Block block, int position)
+TypeSignature getTypeSignature()
+boolean equals(Object other)
+int hashCode()
+long getEpochSecond(long timestamp)
+int getNanos(long timestamp)
}
class ShortTimestampType {
+ShortTimestampType(int precision)
+int getFixedSize()
+Object getObjectValue(SqlFunctionProperties properties, Block block, int position)
+long getLong(Block block, int position)
+long getLongUnchecked(UncheckedBlock block, int internalPosition)
+Slice getSlice(Block block, int position)
+void writeLong(BlockBuilder blockBuilder, long value)
+void appendTo(Block block, int position, BlockBuilder blockBuilder)
+boolean equalTo(Block leftBlock, int leftPosition, Block rightBlock, int rightPosition)
+long hash(Block block, int position)
+int compareTo(Block leftBlock, int leftPosition, Block rightBlock, int rightPosition)
+BlockBuilder createBlockBuilder(BlockBuilderStatus status, int expectedEntries, int expectedBytesPerEntry)
+BlockBuilder createBlockBuilder(BlockBuilderStatus status, int expectedEntries)
+BlockBuilder createFixedSizeBlockBuilder(int positionCount)
+static long hash(long value)
}
class LongTimestampType {
+LongTimestampType(int precision)
+int getFixedSize()
+Object getObjectValue(SqlFunctionProperties properties, Block block, int position)
+LongTimestamp getObject(Block block, int position)
+void writeObject(BlockBuilder blockBuilder, Object value)
+void appendTo(Block block, int position, BlockBuilder blockBuilder)
+boolean equalTo(Block leftBlock, int leftPosition, Block rightBlock, int rightPosition)
+long hash(Block block, int position)
+int compareTo(Block leftBlock, int leftPosition, Block rightBlock, int rightPosition)
+BlockBuilder createBlockBuilder(BlockBuilderStatus status, int expectedEntries, int expectedBytesPerEntry)
+BlockBuilder createBlockBuilder(BlockBuilderStatus status, int expectedEntries)
+BlockBuilder createFixedSizeBlockBuilder(int positionCount)
}
class LongTimestamp {
-long epochMicros
-int picosOfMicro
+LongTimestamp(long epochMicros, int picosOfMicro)
+long getEpochMicros()
+int getPicosOfMicro()
+boolean equals(Object other)
+int hashCode()
+int compareTo(LongTimestamp other)
+String toString()
}
class Timestamps {
<<utility>>
+static int MAX_PRECISION
+static int MAX_SHORT_PRECISION
+static int DEFAULT_PRECISION
+static long[] POWERS_OF_TEN
+static long MILLISECONDS_PER_SECOND
+static long MICROSECONDS_PER_SECOND
+static long NANOSECONDS_PER_SECOND
+static long PICOSECONDS_PER_SECOND
+static long MILLISECONDS_PER_DAY
+static long rescale(long value, int fromPrecision, int toPrecision)
+static long roundDiv(long value, long divisor)
+static long round(long value, int precision)
+static long truncateToMicros(long epochMicros, int picosOfMicro)
+static boolean isShortTimestamp(int precision)
+static String formatTimestamp(int precision, long epochMicros, int picosOfMicro)
+static long millisToMicros(long millis)
+static long microsToMillis(long micros)
+static long microsToNanos(long micros)
+static long nanosToMicros(long nanos)
}
TimestampType <|-- ShortTimestampType
TimestampType <|-- LongTimestampType
LongTimestampType ..> LongTimestamp
LongTimestampType ..> Fixed12Block
LongTimestampType ..> Fixed12BlockBuilder
ShortTimestampType ..> LongArrayBlockBuilder
LongTimestamp ..> Timestamps
TimestampType ..> Timestamps
ShortTimestampType ..> SqlTimestamp
class Block
class BlockBuilder
class BlockBuilderStatus
class UncheckedBlock
class LongArrayBlockBuilder
class Fixed12Block
class Fixed12BlockBuilder
class SqlTimestamp
class SqlFunctionProperties
class TimeUnit
class TypeSignature
Class diagram for Fixed12Block storage and encodingclassDiagram
direction LR
class Fixed12Block {
+static int FIXED12_BYTES
+static int SIZE_IN_BYTES_PER_POSITION
+static int INT_LONGS_PER_ENTRY
-int positionOffset
-int positionCount
-boolean[] valueIsNull
-int[] values
-long retainedSizeInBytes
+Fixed12Block(int positionCount, Optional~boolean[]~ valueIsNull, int[] values)
+Fixed12Block(int positionOffset, int positionCount, boolean[] valueIsNull, int[] values)
+long getFixed12First(int position)
+int getFixed12Second(int position)
+static long getFixed12FirstUnchecked(int[] values, int internalPosition)
+static int getFixed12SecondUnchecked(int[] values, int internalPosition)
+static void encodeFixed12(long first, int second, int[] target, int position)
+long getSizeInBytes()
+OptionalInt fixedSizeInBytesPerPosition()
+long getRegionSizeInBytes(int position, int length)
+long getPositionsSizeInBytes(boolean[] usedPositions, int usedPositionCount)
+long getRetainedSizeInBytes()
+long getEstimatedDataSizeForStats(int position)
+void retainedBytesForEachPart(ObjLongConsumer~Object~ consumer)
+int getPositionCount()
+long getLong(int position, int offset)
+int getInt(int position)
+boolean mayHaveNull()
+boolean isNull(int position)
+void writePositionTo(int position, BlockBuilder blockBuilder)
+void writePositionTo(int position, SliceOutput output)
+Block getSingleValueBlock(int position)
+Block copyPositions(int[] positions, int offset, int length)
+Block getRegion(int positionOffset, int length)
+Block copyRegion(int positionOffset, int length)
+String getEncodingName()
+Block appendNull()
+long getLongUnchecked(int internalPosition, int offset)
+int getOffsetBase()
+boolean isNullUnchecked(int internalPosition)
+boolean equals(Object other)
+int hashCode()
}
class Fixed12BlockBuilder {
-BlockBuilderStatus blockBuilderStatus
-boolean initialized
-int initialEntryCount
-int positionCount
-boolean hasNullValue
-boolean hasNonNullValue
-boolean[] valueIsNull
-int[] values
-long retainedSizeInBytes
+Fixed12BlockBuilder(BlockBuilderStatus status, int expectedEntries)
+Fixed12BlockBuilder writeFixed12(long first, int second)
+BlockBuilder writeLong(long value)
+BlockBuilder writeInt(int value)
+BlockBuilder closeEntry()
+BlockBuilder appendNull()
+Block build()
+BlockBuilder newBlockBuilderLike(BlockBuilderStatus status)
+BlockBuilder newBlockBuilderLike(BlockBuilderStatus status, int expectedEntries)
+long getSizeInBytes()
+OptionalInt fixedSizeInBytesPerPosition()
+long getRegionSizeInBytes(int position, int length)
+long getPositionsSizeInBytes(boolean[] usedPositions, int usedPositionCount)
+long getRetainedSizeInBytes()
+long getEstimatedDataSizeForStats(int position)
+void retainedBytesForEachPart(ObjLongConsumer~Object~ consumer)
+int getPositionCount()
+long getLong(int position, int offset)
+int getInt(int position)
+boolean mayHaveNull()
+boolean isNull(int position)
+void writePositionTo(int position, BlockBuilder blockBuilder)
+void writePositionTo(int position, SliceOutput output)
+BlockBuilder readPositionFrom(SliceInput input)
+Block getSingleValueBlock(int position)
+Block copyPositions(int[] positions, int offset, int length)
+Block getRegion(int positionOffset, int length)
+Block copyRegion(int positionOffset, int length)
+String getEncodingName()
+long getLongUnchecked(int internalPosition, int offset)
+boolean isNullUnchecked(int internalPosition)
+int getOffsetBase()
}
class Fixed12BlockEncoding {
+static String NAME
+String getName()
+void writeBlock(BlockEncodingSerde serde, SliceOutput sliceOutput, Block block)
+Block readBlock(BlockEncodingSerde serde, SliceInput sliceInput)
}
class BlockEncodingManager {
+BlockEncodingManager()
+void addBlockEncoding(BlockEncoding encoding)
}
class Block {
<<interface>>
}
class BlockBuilder {
<<interface>>
}
class BlockEncoding {
<<interface>>
}
class BlockEncodingSerde
class BlockBuilderStatus
class RunLengthEncodedBlock
class SliceOutput
class SliceInput
Fixed12Block ..|> Block
Fixed12BlockBuilder ..|> BlockBuilder
Fixed12BlockEncoding ..|> BlockEncoding
Fixed12BlockEncoding ..> Fixed12Block : creates
Fixed12BlockEncoding ..> Fixed12BlockBuilder : uses encodeFixed12
Fixed12BlockBuilder ..> Fixed12Block : build()
Fixed12BlockBuilder ..> RunLengthEncodedBlock
Fixed12Block ..> SliceOutput
Fixed12BlockBuilder ..> SliceOutput
Fixed12BlockBuilder ..> SliceInput
BlockEncodingManager o--> Fixed12BlockEncoding
Class diagram for TIMESTAMP(p) parametric type and operatorsclassDiagram
direction LR
class TimestampParametricType {
+static TimestampParametricType TIMESTAMP
+String getName()
+Type createType(List~TypeParameter~ parameters)
}
class ParametricType {
<<interface>>
+String getName()
+Type createType(List~TypeParameter~ parameters)
}
class TypeParameter {
+boolean isLongLiteral()
+long getLongLiteral()
}
class Type {
<<interface>>
}
class StandardTypes {
<<utility>>
+static String TIMESTAMP
+static String TIMESTAMP_MICROSECONDS
}
class BuiltInTypeAndFunctionNamespaceManager {
-void registerBuiltInTypes(FunctionsConfig config)
+void addType(Type type)
+void addParametricType(ParametricType type)
+List~SqlFunction~ getBuiltInFunctions(FunctionsConfig config)
}
class TimestampMicrosecondsOperators {
<<utility>>
+Boolean equal(long left, long right)
+Boolean notEqual(long left, long right)
+boolean lessThan(long left, long right)
+boolean lessThanOrEqual(long left, long right)
+boolean greaterThan(long left, long right)
+boolean greaterThanOrEqual(long left, long right)
+boolean between(long value, long min, long max)
+long hashCode(long value)
+long xxHash64(long value)
+boolean indeterminate(long value, boolean isNull)
}
class TimestampMicrosecondsDistinctFromOperator {
+boolean isDistinctFrom(long left, boolean leftNull, long right, boolean rightNull)
+boolean isDistinctFrom(Block left, int leftPosition, Block right, int rightPosition)
}
class Block
class TimestampType
TimestampParametricType ..|> ParametricType
TimestampParametricType ..> TimestampType : returns
TimestampParametricType ..> StandardTypes
TimestampParametricType ..> TypeParameter
BuiltInTypeAndFunctionNamespaceManager ..> TimestampType : addType(TIMESTAMP)
BuiltInTypeAndFunctionNamespaceManager ..> TimestampParametricType : addParametricType(TimestampParametricType.TIMESTAMP)
BuiltInTypeAndFunctionNamespaceManager ..> TimestampType : addType(TIMESTAMP_MICROSECONDS)
BuiltInTypeAndFunctionNamespaceManager ..> TimestampMicrosecondsOperators
TimestampMicrosecondsOperators ..> TimestampMicrosecondsDistinctFromOperator
TimestampMicrosecondsDistinctFromOperator ..> Block
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Owner
|
@copilot the default precision should be 3 and make this change backward compatible |
Author
... Fixed in b8ca248. The root cause was that The fix registers TIMESTAMP both as a named type ( |
Copilot
AI
changed the title
Port parametric TIMESTAMP(p) type infrastructure from Trino to PrestoDB
Port parametric TIMESTAMP(p) type infrastructure and tests from Trino to PrestoDB
Apr 10, 2026
Owner
|
/sourcery-ai review |
Owner
|
@sourcery-ai summary |
Owner
|
@sourcery-ai review |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 6 issues, and left some high level feedback:
- The HASH_CODE operator for TIMESTAMP_MICROSECONDS uses AbstractLongType.hash while ShortTimestampType.hash(Block, ...) uses a different hash implementation, so the type-level hash and the scalar operator hash can diverge; consider wiring the operator to use the same hash function as ShortTimestampType (or aligning ShortTimestampType.hash with AbstractLongType.hash) to satisfy the comparable/orderable contract consistently.
- In Timestamps.rescale(), the downscale path uses roundDiv which is defined as simple truncating division (value / divisor) instead of a true rounding or floorDiv; if negative timestamps or exact rounding semantics matter, it may be clearer either to rename this helper to reflect truncation or to use floorDiv for consistency with other conversion helpers like microsToMillis/nanosToMicros.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The HASH_CODE operator for TIMESTAMP_MICROSECONDS uses AbstractLongType.hash while ShortTimestampType.hash(Block, ...) uses a different hash implementation, so the type-level hash and the scalar operator hash can diverge; consider wiring the operator to use the same hash function as ShortTimestampType (or aligning ShortTimestampType.hash with AbstractLongType.hash) to satisfy the comparable/orderable contract consistently.
- In Timestamps.rescale(), the downscale path uses roundDiv which is defined as simple truncating division (value / divisor) instead of a true rounding or floorDiv; if negative timestamps or exact rounding semantics matter, it may be clearer either to rename this helper to reflect truncation or to use floorDiv for consistency with other conversion helpers like microsToMillis/nanosToMicros.
## Individual Comments
### Comment 1
<location path="presto-common/src/main/java/com/facebook/presto/common/type/Timestamps.java" line_range="77-86" />
<code_context>
+ * For example, rescaling 123456 from precision 6 (microseconds) to precision 3 (milliseconds)
+ * produces 123 (truncation). Rescaling 123 from precision 3 to precision 6 produces 123000.
+ */
+ public static long rescale(long value, int fromPrecision, int toPrecision)
+ {
+ if (fromPrecision == toPrecision) {
+ return value;
+ }
+ if (fromPrecision < toPrecision) {
+ // Scale up
+ int scaleFactor = toPrecision - fromPrecision;
+ return value * POWERS_OF_TEN[scaleFactor];
+ }
+ else {
+ // Scale down (truncation)
+ int scaleFactor = fromPrecision - toPrecision;
+ return roundDiv(value, POWERS_OF_TEN[scaleFactor]);
+ }
+ }
</code_context>
<issue_to_address>
**issue (bug_risk):** Downscaling timestamps should likely use floor-based division for negative values instead of truncation toward zero.
Using `roundDiv` (which currently uses `/`) changes semantics for negative timestamps: Java integer division truncates toward zero, so rescaling `-1500` from precision 3 to 0 yields `-1` instead of `-2`, breaking monotonic, floor-like behavior across the epoch. Existing Presto rescale helpers use `floorDiv`/`floorMod` to keep ordering and arithmetic consistent for pre-epoch values. Please switch to `floorDiv` here (or reuse an existing helper) when `fromPrecision > toPrecision`.
</issue_to_address>
### Comment 2
<location path="presto-common/src/main/java/com/facebook/presto/common/block/Fixed12Block.java" line_range="365-390" />
<code_context>
+ }
+ }
+
+ @Override
+ public boolean equals(Object obj)
+ {
+ if (this == obj) {
+ return true;
+ }
+ if (obj == null || getClass() != obj.getClass()) {
+ return false;
+ }
+ Fixed12Block other = (Fixed12Block) obj;
+ return this.positionOffset == other.positionOffset &&
+ this.positionCount == other.positionCount &&
+ Arrays.equals(this.valueIsNull, other.valueIsNull) &&
+ Arrays.equals(this.values, other.values) &&
+ this.retainedSizeInBytes == other.retainedSizeInBytes;
+ }
+
+ @Override
+ public int hashCode()
+ {
+ return Objects.hash(positionOffset,
+ positionCount,
+ Arrays.hashCode(valueIsNull),
+ Arrays.hashCode(values),
+ retainedSizeInBytes);
+ }
+
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Including retainedSizeInBytes and backing array identity in equals/hashCode makes Fixed12Block equality depend on layout, not just logical contents.
As implemented, two blocks with the same logical contents but different compaction/backing arrays or retained-size accounting will compare unequal and produce different hashes. If these methods are meant to reflect logical equality, they should ignore `retainedSizeInBytes` and compare `valueIsNull`/`values` only over the visible range (`positionOffset .. positionOffset + positionCount`). If they are not meant for logical equality (e.g., only for debugging), consider documenting that explicitly or reverting to identity-based equality to prevent misuse in sets/maps.
```suggestion
@Override
public boolean equals(Object obj)
{
if (this == obj) {
return true;
}
if (obj == null || getClass() != obj.getClass()) {
return false;
}
Fixed12Block other = (Fixed12Block) obj;
if (this.positionCount != other.positionCount) {
return false;
}
int thisStart = this.positionOffset;
int thisEnd = thisStart + this.positionCount;
int otherStart = other.positionOffset;
int otherEnd = otherStart + other.positionCount;
if (!Arrays.equals(this.valueIsNull, thisStart, thisEnd,
other.valueIsNull, otherStart, otherEnd)) {
return false;
}
int thisValuesStart = this.positionOffset * INT_LONGS_PER_ENTRY;
int thisValuesEnd = thisValuesStart + this.positionCount * INT_LONGS_PER_ENTRY;
int otherValuesStart = other.positionOffset * INT_LONGS_PER_ENTRY;
int otherValuesEnd = otherValuesStart + other.positionCount * INT_LONGS_PER_ENTRY;
return Arrays.equals(this.values, thisValuesStart, thisValuesEnd,
other.values, otherValuesStart, otherValuesEnd);
}
@Override
public int hashCode()
{
int start = positionOffset;
int end = start + positionCount;
int valuesStart = positionOffset * INT_LONGS_PER_ENTRY;
int valuesEnd = valuesStart + positionCount * INT_LONGS_PER_ENTRY;
int result = positionCount;
result = 31 * result + Arrays.hashCode(valueIsNull, start, end);
result = 31 * result + Arrays.hashCode(values, valuesStart, valuesEnd);
return result;
}
```
</issue_to_address>
### Comment 3
<location path="presto-common/src/test/java/com/facebook/presto/common/block/TestFixed12Block.java" line_range="186" />
<code_context>
+ }
+
+ @Test
+ public void testWritePositionTo()
+ {
+ Fixed12BlockBuilder builder = new Fixed12BlockBuilder(null, 5);
</code_context>
<issue_to_address>
**suggestion (testing):** Consider adding tests for Slice-based serialization round-trip of Fixed12Block and Fixed12BlockBuilder
The Slice-based paths are currently untested:
- `Fixed12BlockBuilder.writePositionTo(int, SliceOutput)` / `readPositionFrom(SliceInput)`
- `Fixed12BlockEncoding.writeBlock` / `readBlock`
Please add a round-trip test that writes several values (including nulls) via `writePositionTo(SliceOutput)` and reads them back via `readPositionFrom(SliceInput)` or through `Fixed12BlockEncoding` + `BlockEncodingSerde` to validate the wire format and null handling.
Suggested implementation:
```java
assertFalse(block.isNull(2));
}
@Test
public void testSliceRoundTripWithWritePositionTo()
{
Fixed12BlockBuilder builder = new Fixed12BlockBuilder(null, 5);
builder.writeFixed12(10L, 1);
builder.appendNull();
builder.writeFixed12(Long.MAX_VALUE, Integer.MIN_VALUE);
builder.writeFixed12(0L, 0);
builder.appendNull();
Block original = builder.build();
// serialize positions individually using writePositionTo/readPositionFrom
DynamicSliceOutput sliceOutput = new DynamicSliceOutput(1024);
for (int position = 0; position < original.getPositionCount(); position++) {
((Fixed12Block) original).writePositionTo(position, sliceOutput);
}
Slice slice = sliceOutput.slice();
SliceInput sliceInput = slice.getInput();
Fixed12BlockBuilder roundTripBuilder = new Fixed12BlockBuilder(null, original.getPositionCount());
for (int i = 0; i < original.getPositionCount(); i++) {
roundTripBuilder.readPositionFrom(sliceInput);
}
Block roundTripped = roundTripBuilder.build();
assertBlockEquals(roundTripped, original);
}
@Test
public void testSliceRoundTripWithBlockEncoding()
{
Fixed12BlockBuilder builder = new Fixed12BlockBuilder(null, 5);
builder.writeFixed12(10L, 1);
builder.appendNull();
builder.writeFixed12(Long.MIN_VALUE, Integer.MAX_VALUE);
builder.writeFixed12(0L, 0);
builder.appendNull();
Block original = builder.build();
BlockEncodingSerde blockEncodingSerde = new BlockEncodingManager();
SliceOutput sliceOutput = new DynamicSliceOutput(1024);
blockEncodingSerde.writeBlock(sliceOutput, original);
Slice slice = sliceOutput.slice();
SliceInput sliceInput = slice.getInput();
Block roundTripped = blockEncodingSerde.readBlock(sliceInput);
assertBlockEquals(roundTripped, original);
```
To compile and run, the following may also be required depending on existing imports in `TestFixed12Block.java`:
1. Ensure these imports are present (or equivalent, matching your package structure):
- `import com.facebook.presto.common.block.Block;`
- `import com.facebook.presto.common.block.BlockEncodingManager;`
- `import com.facebook.presto.common.block.BlockEncodingSerde;`
- `import com.facebook.presto.common.block.Fixed12Block;`
- `import com.facebook.presto.common.block.Fixed12BlockBuilder;`
- `import io.airlift.slice.Slice;`
- `import io.airlift.slice.SliceInput;`
- `import io.airlift.slice.SliceOutput;`
- `import io.airlift.slice.DynamicSliceOutput;`
2. Ensure there is a static import for `assertBlockEquals` if not already present:
- `import static com.facebook.presto.common.block.BlockAssertions.assertBlockEquals;`
3. If your project uses a different concrete implementation of `BlockEncodingSerde` than `BlockEncodingManager`, adjust the instantiation accordingly.
</issue_to_address>
### Comment 4
<location path="presto-common/src/test/java/com/facebook/presto/common/type/TestTimestamps.java" line_range="132" />
<code_context>
+ }
+
+ @Test
+ public void testRound()
+ {
+ // At max precision, no rounding
</code_context>
<issue_to_address>
**suggestion (testing):** Extend timestamp rounding/rescaling tests to cover negative values and precision boundaries more thoroughly
Current tests only exercise positive values, and negative inputs near precision boundaries aren’t covered (aside from a small `rescale` case). Please add:
- A `testRoundNegative()` covering multiple precisions (e.g., `round(-123456789012L, 9)` and `round(-123456789012L, 0)`), and explicitly asserting the intended rounding policy for negatives (truncate toward zero vs. other behavior).
- Additional `rescale` tests for negative values at extreme `fromPrecision`/`toPrecision` pairs (0 and 12).
This will clarify the contract for negatives and protect against regressions in sign/rounding behavior.
Suggested implementation:
```java
@Test
public void testRound()
{
// At max precision, no rounding
assertEquals(round(123456789012L, 12), 123456789012L);
// Round to precision 9 (truncate last 3 digits of picos)
assertEquals(round(123456789012L, 9), 123456789000L);
// Round to precision 6
assertEquals(round(123456789012L, 6), 123456000000L);
// Round to precision 3
assertEquals(round(123456789012L, 3), 123000000000L);
// Round to precision 0 (truncate all fractional digits)
assertEquals(round(123456789012L, 0), 0L);
}
@Test
public void testRoundNegative()
{
// At max precision, negative values are preserved as-is
assertEquals(round(-123456789012L, 12), -123456789012L);
// Truncate toward zero for negative values at precision 9
assertEquals(round(-123456789012L, 9), -123456789000L);
// Truncate toward zero for negative values at precision 0
assertEquals(round(-123456789012L, 0), 0L);
```
`.
Here are the concrete edits:
<file_operations>
<file_operation operation="edit" file_path="presto-common/src/test/java/com/facebook/presto/common/type/TestTimestamps.java">
<<<<<<< SEARCH
@Test
public void testRound()
{
// At max precision, no rounding
assertEquals(round(123456789012L, 12), 123456789012L);
// Round to precision 9 (truncate last 3 digits of picos)
assertEquals(round(123456789012L, 9), 123456789000L);
// Round to precision 6
assertEquals(round(123456789012L, 6), 123456000000L);
// Round to precision 3
assertEquals(round(123456789012L, 3), 123000000000L);
=======
@Test
public void testRound()
{
// At max precision, no rounding
assertEquals(round(123456789012L, 12), 123456789012L);
// Round to precision 9 (truncate last 3 digits of picos)
assertEquals(round(123456789012L, 9), 123456789000L);
// Round to precision 6
assertEquals(round(123456789012L, 6), 123456000000L);
// Round to precision 3
assertEquals(round(123456789012L, 3), 123000000000L);
// Round to precision 0 (truncate all fractional digits)
assertEquals(round(123456789012L, 0), 0L);
}
@Test
public void testRoundNegative()
{
// At max precision, negative values are preserved as-is
assertEquals(round(-123456789012L, 12), -123456789012L);
// Truncate toward zero for negative values at precision 9
assertEquals(round(-123456789012L, 9), -123456789000L);
// Truncate toward zero for negative values at precision 0
assertEquals(round(-123456789012L, 0), 0L);
>>>>>>> REPLACE
</file_operation>
</file_operations>
<additional_changes>
To complete your requested coverage for `rescale` with negative values at extreme precision pairs (0 and 12), you’ll need to extend the existing rescale tests (likely a `testRescale()` method) as follows:
1. Add assertions that mirror the existing positive rescale tests, but with negative inputs. For example (adjust values to match how the positive cases are structured in your file):
- From highest to lowest precision, truncating toward zero:
```java
assertEquals(rescale(-123456789012L, 12, 0), 0L);
assertEquals(rescale(-123456789000L, 12, 0), -123L); // if this matches the positive test case structure
```
- From lowest to highest precision (padding with zeros):
```java
assertEquals(rescale(-123L, 0, 12), -123000000000000L);
```
2. Place these new negative-rescale assertions adjacent to the corresponding positive ones (same `fromPrecision`/`toPrecision` pairs) so the contract for sign handling and truncation policy is clear and easy to compare.
3. Ensure the expectations align with the actual implementation of `rescale` (truncate toward zero vs. another rounding rule); adjust the expected values above to match the existing positive tests’ behavior for the same precisions.
Because the `testRescale` implementation isn’t visible in the snippet, you’ll need to integrate these additional assertions into the correct test method(s) and potentially adjust the literal values so they are consistent with the existing positive test coverage pattern.
</issue_to_address>
### Comment 5
<location path="presto-main-base/src/test/java/com/facebook/presto/type/TestTimestampParametricType.java" line_range="49" />
<code_context>
+ }
+
+ @Test
+ public void testDefaultPrecision()
+ {
+ Type type = parametricType.createType(Collections.emptyList());
</code_context>
<issue_to_address>
**suggestion (testing):** Add assertions around TimestampType type signatures and alias identity to better lock in backward compatibility
These tests already cover precision mapping and the short/long split. To better protect backward compatibility, consider also asserting that:
- `TimestampType.TIMESTAMP.getTypeSignature().toString()` is exactly `"timestamp"` (and likewise for `TIMESTAMP_MICROSECONDS`), to lock in the special-casing in `TimestampType.getTypeSignature()`.
- `parametricType.createType(Collections.emptyList())` returns the same instance as `TimestampType.TIMESTAMP` (identity, not just equality), so future changes can’t silently start producing new instances.
This will more fully pin down the expected behavior of the parametric type vs. the predefined constants.
Suggested implementation:
```java
@Test
public void testDefaultPrecision()
{
Type type = parametricType.createType(Collections.emptyList());
assertTrue(type instanceof TimestampType);
assertEquals(((TimestampType) type).getPrecision(), 3);
// Default parametric type should return the predefined constant instance
assertSame(type, TimestampType.TIMESTAMP);
// Lock in special-cased type signatures and aliases
assertEquals(TimestampType.TIMESTAMP.getTypeSignature().toString(), "timestamp");
assertEquals(TimestampType.TIMESTAMP_MICROSECONDS.getTypeSignature().toString(), "timestamp(6)");
}
```
If `assertSame` or the static `TimestampType` constants are not yet statically imported or available, ensure:
1. `import static org.testng.Assert.assertSame;` is present at the top of the file.
2. `TimestampType` and `TimestampType.TIMESTAMP_MICROSECONDS` are imported or referenced with their fully qualified names.
</issue_to_address>
### Comment 6
<location path="presto-common/src/test/java/com/facebook/presto/common/type/TestLongTimestamp.java" line_range="126" />
<code_context>
+ }
+
+ @Test
+ public void testToString()
+ {
+ // LongTimestamp.toString() uses Timestamps.formatTimestamp(12, epochMicros, picosOfMicro)
</code_context>
<issue_to_address>
**suggestion (testing):** Strengthen LongTimestamp.toString tests by asserting the exact formatted representation and fractional length
Currently the test only checks that the output contains certain substrings. Since `toString` delegates to `Timestamps.formatTimestamp(12, ...)`, please also:
- Assert the exact full string for a few key cases (epoch, positive with non-zero `picosOfMicro`, negative), including 12 fractional digits.
- Assert that the fractional part is always length 12, matching the `MAX_PRECISION` contract.
This will help catch subtle formatting regressions (e.g., changes in fraction width or padding).
Suggested implementation:
```java
@Test
public void testToString()
{
// LongTimestamp.toString() uses Timestamps.formatTimestamp(12, epochMicros, picosOfMicro)
// Epoch timestamp: 1970-01-01 00:00:00.000000000000
LongTimestamp epoch = new LongTimestamp(0L, 0);
String epochStr = epoch.toString();
assertEquals(epochStr, "1970-01-01 00:00:00.000000000000");
assertEquals(epochStr.substring(epochStr.indexOf('.') + 1).length(), 12, "Expected 12 fractional digits for epoch timestamp");
// Positive timestamp with non-zero picosOfMicro:
// 2020-01-01 00:00:00.000000123456 = 1577836800 seconds = 1577836800000000 micros, picosOfMicro = 123456
LongTimestamp positive = new LongTimestamp(1577836800000000L, 123456);
String positiveStr = positive.toString();
assertEquals(positiveStr, "2020-01-01 00:00:00.000000123456");
assertEquals(positiveStr.substring(positiveStr.indexOf('.') + 1).length(), 12, "Expected 12 fractional digits for positive timestamp");
// Negative timestamp just before the epoch:
// -1 micro = 1969-12-31 23:59:59.999999000000
LongTimestamp negative = new LongTimestamp(-1L, 0);
String negativeStr = negative.toString();
assertEquals(negativeStr, "1969-12-31 23:59:59.999999000000");
assertEquals(negativeStr.substring(negativeStr.indexOf('.') + 1).length(), 12, "Expected 12 fractional digits for negative timestamp");
}
```
1. Ensure that `org.testng.Assert.assertEquals` is imported in this test file (it likely already is, since other tests typically use it). If it is not, add:
`import static org.testng.Assert.assertEquals;`
2. The existing `testToStringWithPositiveTimestamp` can remain unchanged; the strengthened `testToString` now covers:
- Exact full string representation for epoch, positive with non-zero `picosOfMicro`, and negative timestamps.
- Verification that the fractional part is always 12 digits long, matching the `MAX_PRECISION` contract.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+77
to
+86
| public static long rescale(long value, int fromPrecision, int toPrecision) | ||
| { | ||
| if (fromPrecision == toPrecision) { | ||
| return value; | ||
| } | ||
| if (fromPrecision < toPrecision) { | ||
| // Scale up | ||
| int scaleFactor = toPrecision - fromPrecision; | ||
| return value * POWERS_OF_TEN[scaleFactor]; | ||
| } |
There was a problem hiding this comment.
issue (bug_risk): Downscaling timestamps should likely use floor-based division for negative values instead of truncation toward zero.
Using roundDiv (which currently uses /) changes semantics for negative timestamps: Java integer division truncates toward zero, so rescaling -1500 from precision 3 to 0 yields -1 instead of -2, breaking monotonic, floor-like behavior across the epoch. Existing Presto rescale helpers use floorDiv/floorMod to keep ordering and arithmetic consistent for pre-epoch values. Please switch to floorDiv here (or reuse an existing helper) when fromPrecision > toPrecision.
Comment on lines
+365
to
+390
| @Override | ||
| public boolean equals(Object obj) | ||
| { | ||
| if (this == obj) { | ||
| return true; | ||
| } | ||
| if (obj == null || getClass() != obj.getClass()) { | ||
| return false; | ||
| } | ||
| Fixed12Block other = (Fixed12Block) obj; | ||
| return this.positionOffset == other.positionOffset && | ||
| this.positionCount == other.positionCount && | ||
| Arrays.equals(this.valueIsNull, other.valueIsNull) && | ||
| Arrays.equals(this.values, other.values) && | ||
| this.retainedSizeInBytes == other.retainedSizeInBytes; | ||
| } | ||
|
|
||
| @Override | ||
| public int hashCode() | ||
| { | ||
| return Objects.hash(positionOffset, | ||
| positionCount, | ||
| Arrays.hashCode(valueIsNull), | ||
| Arrays.hashCode(values), | ||
| retainedSizeInBytes); | ||
| } |
There was a problem hiding this comment.
suggestion (bug_risk): Including retainedSizeInBytes and backing array identity in equals/hashCode makes Fixed12Block equality depend on layout, not just logical contents.
As implemented, two blocks with the same logical contents but different compaction/backing arrays or retained-size accounting will compare unequal and produce different hashes. If these methods are meant to reflect logical equality, they should ignore retainedSizeInBytes and compare valueIsNull/values only over the visible range (positionOffset .. positionOffset + positionCount). If they are not meant for logical equality (e.g., only for debugging), consider documenting that explicitly or reverting to identity-based equality to prevent misuse in sets/maps.
Suggested change
| @Override | |
| public boolean equals(Object obj) | |
| { | |
| if (this == obj) { | |
| return true; | |
| } | |
| if (obj == null || getClass() != obj.getClass()) { | |
| return false; | |
| } | |
| Fixed12Block other = (Fixed12Block) obj; | |
| return this.positionOffset == other.positionOffset && | |
| this.positionCount == other.positionCount && | |
| Arrays.equals(this.valueIsNull, other.valueIsNull) && | |
| Arrays.equals(this.values, other.values) && | |
| this.retainedSizeInBytes == other.retainedSizeInBytes; | |
| } | |
| @Override | |
| public int hashCode() | |
| { | |
| return Objects.hash(positionOffset, | |
| positionCount, | |
| Arrays.hashCode(valueIsNull), | |
| Arrays.hashCode(values), | |
| retainedSizeInBytes); | |
| } | |
| @Override | |
| public boolean equals(Object obj) | |
| { | |
| if (this == obj) { | |
| return true; | |
| } | |
| if (obj == null || getClass() != obj.getClass()) { | |
| return false; | |
| } | |
| Fixed12Block other = (Fixed12Block) obj; | |
| if (this.positionCount != other.positionCount) { | |
| return false; | |
| } | |
| int thisStart = this.positionOffset; | |
| int thisEnd = thisStart + this.positionCount; | |
| int otherStart = other.positionOffset; | |
| int otherEnd = otherStart + other.positionCount; | |
| if (!Arrays.equals(this.valueIsNull, thisStart, thisEnd, | |
| other.valueIsNull, otherStart, otherEnd)) { | |
| return false; | |
| } | |
| int thisValuesStart = this.positionOffset * INT_LONGS_PER_ENTRY; | |
| int thisValuesEnd = thisValuesStart + this.positionCount * INT_LONGS_PER_ENTRY; | |
| int otherValuesStart = other.positionOffset * INT_LONGS_PER_ENTRY; | |
| int otherValuesEnd = otherValuesStart + other.positionCount * INT_LONGS_PER_ENTRY; | |
| return Arrays.equals(this.values, thisValuesStart, thisValuesEnd, | |
| other.values, otherValuesStart, otherValuesEnd); | |
| } | |
| @Override | |
| public int hashCode() | |
| { | |
| int start = positionOffset; | |
| int end = start + positionCount; | |
| int valuesStart = positionOffset * INT_LONGS_PER_ENTRY; | |
| int valuesEnd = valuesStart + positionCount * INT_LONGS_PER_ENTRY; | |
| int result = positionCount; | |
| result = 31 * result + Arrays.hashCode(valueIsNull, start, end); | |
| result = 31 * result + Arrays.hashCode(values, valuesStart, valuesEnd); | |
| return result; | |
| } |
| } | ||
|
|
||
| @Test | ||
| public void testWritePositionTo() |
There was a problem hiding this comment.
suggestion (testing): Consider adding tests for Slice-based serialization round-trip of Fixed12Block and Fixed12BlockBuilder
The Slice-based paths are currently untested:
Fixed12BlockBuilder.writePositionTo(int, SliceOutput)/readPositionFrom(SliceInput)Fixed12BlockEncoding.writeBlock/readBlock
Please add a round-trip test that writes several values (including nulls) via writePositionTo(SliceOutput) and reads them back via readPositionFrom(SliceInput) or through Fixed12BlockEncoding + BlockEncodingSerde to validate the wire format and null handling.
Suggested implementation:
assertFalse(block.isNull(2));
}
@Test
public void testSliceRoundTripWithWritePositionTo()
{
Fixed12BlockBuilder builder = new Fixed12BlockBuilder(null, 5);
builder.writeFixed12(10L, 1);
builder.appendNull();
builder.writeFixed12(Long.MAX_VALUE, Integer.MIN_VALUE);
builder.writeFixed12(0L, 0);
builder.appendNull();
Block original = builder.build();
// serialize positions individually using writePositionTo/readPositionFrom
DynamicSliceOutput sliceOutput = new DynamicSliceOutput(1024);
for (int position = 0; position < original.getPositionCount(); position++) {
((Fixed12Block) original).writePositionTo(position, sliceOutput);
}
Slice slice = sliceOutput.slice();
SliceInput sliceInput = slice.getInput();
Fixed12BlockBuilder roundTripBuilder = new Fixed12BlockBuilder(null, original.getPositionCount());
for (int i = 0; i < original.getPositionCount(); i++) {
roundTripBuilder.readPositionFrom(sliceInput);
}
Block roundTripped = roundTripBuilder.build();
assertBlockEquals(roundTripped, original);
}
@Test
public void testSliceRoundTripWithBlockEncoding()
{
Fixed12BlockBuilder builder = new Fixed12BlockBuilder(null, 5);
builder.writeFixed12(10L, 1);
builder.appendNull();
builder.writeFixed12(Long.MIN_VALUE, Integer.MAX_VALUE);
builder.writeFixed12(0L, 0);
builder.appendNull();
Block original = builder.build();
BlockEncodingSerde blockEncodingSerde = new BlockEncodingManager();
SliceOutput sliceOutput = new DynamicSliceOutput(1024);
blockEncodingSerde.writeBlock(sliceOutput, original);
Slice slice = sliceOutput.slice();
SliceInput sliceInput = slice.getInput();
Block roundTripped = blockEncodingSerde.readBlock(sliceInput);
assertBlockEquals(roundTripped, original);To compile and run, the following may also be required depending on existing imports in TestFixed12Block.java:
-
Ensure these imports are present (or equivalent, matching your package structure):
import com.facebook.presto.common.block.Block;import com.facebook.presto.common.block.BlockEncodingManager;import com.facebook.presto.common.block.BlockEncodingSerde;import com.facebook.presto.common.block.Fixed12Block;import com.facebook.presto.common.block.Fixed12BlockBuilder;import io.airlift.slice.Slice;import io.airlift.slice.SliceInput;import io.airlift.slice.SliceOutput;import io.airlift.slice.DynamicSliceOutput;
-
Ensure there is a static import for
assertBlockEqualsif not already present:import static com.facebook.presto.common.block.BlockAssertions.assertBlockEquals;
-
If your project uses a different concrete implementation of
BlockEncodingSerdethanBlockEncodingManager, adjust the instantiation accordingly.
| } | ||
|
|
||
| @Test | ||
| public void testRound() |
There was a problem hiding this comment.
suggestion (testing): Extend timestamp rounding/rescaling tests to cover negative values and precision boundaries more thoroughly
Current tests only exercise positive values, and negative inputs near precision boundaries aren’t covered (aside from a small rescale case). Please add:
- A
testRoundNegative()covering multiple precisions (e.g.,round(-123456789012L, 9)andround(-123456789012L, 0)), and explicitly asserting the intended rounding policy for negatives (truncate toward zero vs. other behavior). - Additional
rescaletests for negative values at extremefromPrecision/toPrecisionpairs (0 and 12).
This will clarify the contract for negatives and protect against regressions in sign/rounding behavior.
Suggested implementation:
@Test
public void testRound()
{
// At max precision, no rounding
assertEquals(round(123456789012L, 12), 123456789012L);
// Round to precision 9 (truncate last 3 digits of picos)
assertEquals(round(123456789012L, 9), 123456789000L);
// Round to precision 6
assertEquals(round(123456789012L, 6), 123456000000L);
// Round to precision 3
assertEquals(round(123456789012L, 3), 123000000000L);
// Round to precision 0 (truncate all fractional digits)
assertEquals(round(123456789012L, 0), 0L);
}
@Test
public void testRoundNegative()
{
// At max precision, negative values are preserved as-is
assertEquals(round(-123456789012L, 12), -123456789012L);
// Truncate toward zero for negative values at precision 9
assertEquals(round(-123456789012L, 9), -123456789000L);
// Truncate toward zero for negative values at precision 0
assertEquals(round(-123456789012L, 0), 0L);`.
Here are the concrete edits:
<file_operations>
<file_operation operation="edit" file_path="presto-common/src/test/java/com/facebook/presto/common/type/TestTimestamps.java">
<<<<<<< SEARCH
@test
public void testRound()
{
// At max precision, no rounding
assertEquals(round(123456789012L, 12), 123456789012L);
// Round to precision 9 (truncate last 3 digits of picos)
assertEquals(round(123456789012L, 9), 123456789000L);
// Round to precision 6
assertEquals(round(123456789012L, 6), 123456000000L);
// Round to precision 3
assertEquals(round(123456789012L, 3), 123000000000L);
=======
@test
public void testRound()
{
// At max precision, no rounding
assertEquals(round(123456789012L, 12), 123456789012L);
// Round to precision 9 (truncate last 3 digits of picos)
assertEquals(round(123456789012L, 9), 123456789000L);
// Round to precision 6
assertEquals(round(123456789012L, 6), 123456000000L);
// Round to precision 3
assertEquals(round(123456789012L, 3), 123000000000L);
// Round to precision 0 (truncate all fractional digits)
assertEquals(round(123456789012L, 0), 0L);
}
@Test
public void testRoundNegative()
{
// At max precision, negative values are preserved as-is
assertEquals(round(-123456789012L, 12), -123456789012L);
// Truncate toward zero for negative values at precision 9
assertEquals(round(-123456789012L, 9), -123456789000L);
// Truncate toward zero for negative values at precision 0
assertEquals(round(-123456789012L, 0), 0L);
REPLACE
</file_operation>
</file_operations>
<additional_changes>
To complete your requested coverage for rescale with negative values at extreme precision pairs (0 and 12), you’ll need to extend the existing rescale tests (likely a testRescale() method) as follows:
- Add assertions that mirror the existing positive rescale tests, but with negative inputs. For example (adjust values to match how the positive cases are structured in your file):
- From highest to lowest precision, truncating toward zero:
assertEquals(rescale(-123456789012L, 12, 0), 0L); assertEquals(rescale(-123456789000L, 12, 0), -123L); // if this matches the positive test case structure
- From lowest to highest precision (padding with zeros):
assertEquals(rescale(-123L, 0, 12), -123000000000000L);
- From highest to lowest precision, truncating toward zero:
- Place these new negative-rescale assertions adjacent to the corresponding positive ones (same
fromPrecision/toPrecisionpairs) so the contract for sign handling and truncation policy is clear and easy to compare. - Ensure the expectations align with the actual implementation of
rescale(truncate toward zero vs. another rounding rule); adjust the expected values above to match the existing positive tests’ behavior for the same precisions.
Because the testRescale implementation isn’t visible in the snippet, you’ll need to integrate these additional assertions into the correct test method(s) and potentially adjust the literal values so they are consistent with the existing positive test coverage pattern.
| } | ||
|
|
||
| @Test | ||
| public void testDefaultPrecision() |
There was a problem hiding this comment.
suggestion (testing): Add assertions around TimestampType type signatures and alias identity to better lock in backward compatibility
These tests already cover precision mapping and the short/long split. To better protect backward compatibility, consider also asserting that:
TimestampType.TIMESTAMP.getTypeSignature().toString()is exactly"timestamp"(and likewise forTIMESTAMP_MICROSECONDS), to lock in the special-casing inTimestampType.getTypeSignature().parametricType.createType(Collections.emptyList())returns the same instance asTimestampType.TIMESTAMP(identity, not just equality), so future changes can’t silently start producing new instances.
This will more fully pin down the expected behavior of the parametric type vs. the predefined constants.
Suggested implementation:
@Test
public void testDefaultPrecision()
{
Type type = parametricType.createType(Collections.emptyList());
assertTrue(type instanceof TimestampType);
assertEquals(((TimestampType) type).getPrecision(), 3);
// Default parametric type should return the predefined constant instance
assertSame(type, TimestampType.TIMESTAMP);
// Lock in special-cased type signatures and aliases
assertEquals(TimestampType.TIMESTAMP.getTypeSignature().toString(), "timestamp");
assertEquals(TimestampType.TIMESTAMP_MICROSECONDS.getTypeSignature().toString(), "timestamp(6)");
}If assertSame or the static TimestampType constants are not yet statically imported or available, ensure:
import static org.testng.Assert.assertSame;is present at the top of the file.TimestampTypeandTimestampType.TIMESTAMP_MICROSECONDSare imported or referenced with their fully qualified names.
| } | ||
|
|
||
| @Test | ||
| public void testToString() |
There was a problem hiding this comment.
suggestion (testing): Strengthen LongTimestamp.toString tests by asserting the exact formatted representation and fractional length
Currently the test only checks that the output contains certain substrings. Since toString delegates to Timestamps.formatTimestamp(12, ...), please also:
- Assert the exact full string for a few key cases (epoch, positive with non-zero
picosOfMicro, negative), including 12 fractional digits. - Assert that the fractional part is always length 12, matching the
MAX_PRECISIONcontract.
This will help catch subtle formatting regressions (e.g., changes in fraction width or padding).
Suggested implementation:
@Test
public void testToString()
{
// LongTimestamp.toString() uses Timestamps.formatTimestamp(12, epochMicros, picosOfMicro)
// Epoch timestamp: 1970-01-01 00:00:00.000000000000
LongTimestamp epoch = new LongTimestamp(0L, 0);
String epochStr = epoch.toString();
assertEquals(epochStr, "1970-01-01 00:00:00.000000000000");
assertEquals(epochStr.substring(epochStr.indexOf('.') + 1).length(), 12, "Expected 12 fractional digits for epoch timestamp");
// Positive timestamp with non-zero picosOfMicro:
// 2020-01-01 00:00:00.000000123456 = 1577836800 seconds = 1577836800000000 micros, picosOfMicro = 123456
LongTimestamp positive = new LongTimestamp(1577836800000000L, 123456);
String positiveStr = positive.toString();
assertEquals(positiveStr, "2020-01-01 00:00:00.000000123456");
assertEquals(positiveStr.substring(positiveStr.indexOf('.') + 1).length(), 12, "Expected 12 fractional digits for positive timestamp");
// Negative timestamp just before the epoch:
// -1 micro = 1969-12-31 23:59:59.999999000000
LongTimestamp negative = new LongTimestamp(-1L, 0);
String negativeStr = negative.toString();
assertEquals(negativeStr, "1969-12-31 23:59:59.999999000000");
assertEquals(negativeStr.substring(negativeStr.indexOf('.') + 1).length(), 12, "Expected 12 fractional digits for negative timestamp");
}- Ensure that
org.testng.Assert.assertEqualsis imported in this test file (it likely already is, since other tests typically use it). If it is not, add:
import static org.testng.Assert.assertEquals; - The existing
testToStringWithPositiveTimestampcan remain unchanged; the strengthenedtestToStringnow covers:- Exact full string representation for epoch, positive with non-zero
picosOfMicro, and negative timestamps. - Verification that the fractional part is always 12 digits long, matching the
MAX_PRECISIONcontract.
- Exact full string representation for epoch, positive with non-zero
Owner
|
@copilot fix the review points |
Author
Fixed in 0d22274. Addressed the following review points:
Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
Joe-Abraham
force-pushed
the
copilot/port-parametric-timestamps
branch
from
0d22274 to
6699611
Compare
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
Author
All 6 Sourcery AI review points addressed in 3e2c869 and 841e8f2:
Also clarified |
Owner
|
@sourcery-ai review |
Owner
|
@sourcery-ai title |
sourcery-ai
Bot
changed the title
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The new TimestampType.getStorageUnit() throws for long (precision > 6) timestamps, but Iceberg code paths (e.g., IcebergPageSink.getIcebergValue, adjustTimestampForPartitionTransform, IcebergAbstractMetadata, PartitionTable, IOPlanPrinter) now unconditionally call getStorageUnit() for any TimestampType; consider either guarding with isShort()/isLong() or enforcing that only short timestamps reach these code paths to avoid unexpected UnsupportedOperationException at runtime.
- LongTimestamp.toString() always formats with 12 fractional digits regardless of the declared precision of the associated LongTimestampType; if lower-precision long timestamps (e.g., precision 7–11) are exposed to users, you may want to consider aligning the string formatting with the type’s precision rather than hard-coding 12 digits.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new TimestampType.getStorageUnit() throws for long (precision > 6) timestamps, but Iceberg code paths (e.g., IcebergPageSink.getIcebergValue, adjustTimestampForPartitionTransform, IcebergAbstractMetadata, PartitionTable, IOPlanPrinter) now unconditionally call getStorageUnit() for any TimestampType; consider either guarding with isShort()/isLong() or enforcing that only short timestamps reach these code paths to avoid unexpected UnsupportedOperationException at runtime.
- LongTimestamp.toString() always formats with 12 fractional digits regardless of the declared precision of the associated LongTimestampType; if lower-precision long timestamps (e.g., precision 7–11) are exposed to users, you may want to consider aligning the string formatting with the type’s precision rather than hard-coding 12 digits.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Owner
|
@copilot i want to test the changes manually, write sample queries and which connector should i use hive/iceberg |
Author
Use Hive for basic TIMESTAMP (millisecond) testing and Iceberg for microsecond precision testing. Here are sample queries for both: Hive Connector (default TIMESTAMP = millisecond precision)-- 1. Verify bare TIMESTAMP resolves to TIMESTAMP(3) with default precision
SELECT CAST(orderdate AS TIMESTAMP) FROM orders LIMIT 5;
-- 2. Verify approx_distinct works with TIMESTAMP (the query that was failing)
SELECT approx_distinct(CAST(orderdate AS TIMESTAMP)) FROM orders;
-- 3. Check typeof() to confirm default precision
SELECT typeof(CAST(orderdate AS TIMESTAMP));
-- Expected: timestamp
-- 4. Explicit TIMESTAMP(p) precision syntax
SELECT CAST(orderdate AS TIMESTAMP(0)) FROM orders LIMIT 5;
SELECT CAST(orderdate AS TIMESTAMP(3)) FROM orders LIMIT 5;
SELECT CAST(orderdate AS TIMESTAMP(6)) FROM orders LIMIT 5;
-- 5. Timestamp literals (millisecond)
SELECT TIMESTAMP '2024-01-15 10:30:45.123';
-- 6. Timestamp literals (microsecond - triggers TIMESTAMP_MICROSECONDS)
SELECT TIMESTAMP '2024-01-15 10:30:45.123456';
-- 7. Comparisons and ordering
SELECT CAST(orderdate AS TIMESTAMP) AS ts FROM orders ORDER BY ts LIMIT 10;
-- 8. Aggregations with TIMESTAMP
SELECT COUNT(DISTINCT CAST(orderdate AS TIMESTAMP)) FROM orders;
SELECT MIN(CAST(orderdate AS TIMESTAMP)), MAX(CAST(orderdate AS TIMESTAMP)) FROM orders;Iceberg Connector (native microsecond precision)-- 9. Create an Iceberg table with timestamp column (stores as µs internally)
CREATE TABLE iceberg_ts_test (id INTEGER, ts TIMESTAMP) WITH (format = 'PARQUET');
-- 10. Insert and query
INSERT INTO iceberg_ts_test VALUES (1, TIMESTAMP '2024-06-15 12:30:45.123');
INSERT INTO iceberg_ts_test VALUES (2, TIMESTAMP '2024-06-15 12:30:45.123456');
SELECT * FROM iceberg_ts_test;
-- 11. Verify type resolution
SELECT typeof(ts) FROM iceberg_ts_test LIMIT 1;
-- Expected: timestamp microseconds (Iceberg maps to TIMESTAMP_MICROSECONDS)
-- Cleanup
DROP TABLE iceberg_ts_test;Quick Smoke Test (TPC-H with Hive)The fastest way to validate backward compatibility is the TPC-H -- This is the exact query that was previously failing
SELECT approx_distinct(CAST(orderdate AS TIMESTAMP)) FROM orders;
-- Additional backward-compat checks
SELECT CAST(orderdate AS TIMESTAMP) FROM orders WHERE orderkey = 1;
SELECT COUNT(*) FROM orders WHERE CAST(orderdate AS TIMESTAMP) > TIMESTAMP '1995-01-01 00:00:00';Start with the Hive/TPC-H queries (1–8) to verify backward compatibility, then use Iceberg (9–11) for microsecond precision validation. |
Owner
|
@copilot rebase with latest master |
Copilot stopped work on behalf of
Joe-Abraham due to an error
April 23, 2026 07:41
Joe-Abraham
force-pushed
the
copilot/port-parametric-timestamps
branch
from
5f899b2 to
8b038bb
Compare
Owner
|
@copilot the below test in failing after the change java.lang.AssertionError: Execution of 'actual' query failed: SELECT array_top_n(ARRAY[orderkey], 25, (x, y) -> if (x < y, 1, if (x > y, -1, 0))) from orders |
Author
This test failure is not caused by our TIMESTAMP changes. Here's the root cause: The test
The test now requires the newer Velox binary (
To fix: rebuild your Velox binary from |
…d related operators
…adability and performance
Joe-Abraham
force-pushed
the
copilot/port-parametric-timestamps
branch
from
8b038bb to
06e0101
Compare
Create TimestampParametricOperators.java implementing comparison, hash, and equality operators for the parametric timestamp(p) type covering all precisions: - Short timestamps (p=0..6, Java type = long): uses direct long comparison - Long timestamps (p=7..12, Java type = LongTimestamp): uses LongTimestamp.compareTo() and reads epochMicros/picosOfMicro from blocks The PolymorphicScalarFunction machinery automatically dispatches to the correct method variant based on the resolved Java type of the argument. Operators implemented: EQUAL, NOT_EQUAL, LESS_THAN, LESS_THAN_OR_EQUAL, GREATER_THAN, GREATER_THAN_OR_EQUAL, BETWEEN, HASH_CODE, XX_HASH_64, IS_DISTINCT_FROM, INDETERMINATE. Register the new operators in BuiltInTypeAndFunctionNamespaceManager alongside the existing TimestampOperators (precision 3) and TimestampMicrosecondsOperators (precision 6) which are kept for backward compatibility with named types. Also add ClientCapabilities enum with PARAMETRIC_DATETIME capability constant and X-Presto-Client-Capabilities header to PrestoHeaders. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: Joe-Abraham <53977252+Joe-Abraham@users.noreply.github.com>
- Add LongTimestampWithTimeZone value class for sub-millisecond precision (4-12)
- Stores epoch millis + packed fraction (picosOfMilli << 12 | timeZoneKey)
- Uses Fixed12Block for storage (first 8 bytes = epoch millis, last 4 = packed)
- Implements Comparable<LongTimestampWithTimeZone>
- Rewrite TimestampWithTimeZoneType as abstract parametric class (precision 0-12)
- Short precision (0-3): stored as packed long (existing DateTimeEncoding format)
- Long precision (4-12): stored as Fixed12Block
- Preserves TIMESTAMP_WITH_TIME_ZONE = createTimestampWithTimeZoneType(3) for
backward compatibility
- Add ShortTimestampWithTimeZoneType (precision 0-3): packed long storage
- Add LongTimestampWithTimeZoneType (precision 4-12): Fixed12Block storage
- Add TimestampWithTimeZoneParametricType for TIMESTAMP WITH TIME ZONE(p) parsing
- Register TimestampWithTimeZoneParametricType in BuiltInTypeAndFunctionNamespaceManager
- Add TIMESTAMP_WITH_TIME_ZONE to StandardTypes.PARAMETRIC_TYPES
- Fix pre-existing checkstyle import order violation (DecimalSaturatedFloorCasts and
TimestampParametricOperators were mis-ordered in BuiltInTypeAndFunctionNamespaceManager)
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Joe-Abraham <53977252+Joe-Abraham@users.noreply.github.com>
The Fixed12Block packing scheme uses 20 bits for sub-millisecond precision (32 bits minus 12 bits for the timezone key), which supports values up to 1,048,575 — only sufficient for nanoseconds within a millisecond (0 to 999,999), not picoseconds. Rename the field and methods to accurately reflect this and remove the divide-by-1000 in toString(). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: Joe-Abraham <53977252+Joe-Abraham@users.noreply.github.com>
…e infrastructure Agent-Logs-Url: https://github.com/Joe-Abraham/presto/sessions/5e3e7a97-5d3f-451d-be3f-ed2f5b49d8e1 Co-authored-by: Joe-Abraham <53977252+Joe-Abraham@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Step 1: Core TIMESTAMP type hierarchy (
presto-common)TimestampTypemade abstract withShortTimestampType(p=0-6,long) andLongTimestampType(p=7-12,Fixed12Block)LongTimestampvalue class (epochMicros + picosOfMicro)Timestampsutility class (rescale, format helpers)Fixed12Block/Fixed12BlockBuilderblock infrastructure for 12-byte valuesTimestampParametricTypeinpresto-main-baseBuiltInTypeAndFunctionNamespaceManagerStep 1: Parametric TIMESTAMP operators (
presto-main-base)TimestampParametricOperators— polymorphic operators for all precisions 0-12 (equal, notEqual, lessThan, lessThanOrEqual, greaterThan, greaterThanOrEqual, between, hashCode, xxHash64, isDistinctFrom, indeterminate)BuiltInTypeAndFunctionNamespaceManagerPARAMETRIC_DATETIME client capability (
presto-client)ClientCapabilitiesenum withPARAMETRIC_DATETIMEX-Presto-Client-Capabilitiesheader inPrestoHeadersStep 2: Core TIMESTAMP WITH TIME ZONE type hierarchy (
presto-common)TimestampWithTimeZoneTypemade abstract withShortTimestampWithTimeZoneType(p=0-3,long) andLongTimestampWithTimeZoneType(p=4-12,Fixed12Block)LongTimestampWithTimeZonevalue class (epochMillis + nanosOfMilli + timeZoneKey)TimestampWithTimeZoneParametricTypeinpresto-main-baseBuiltInTypeAndFunctionNamespaceManagerStep 2: Parametric TIMESTAMP WITH TIME ZONE operators (
presto-main-base)TimestampWithTimeZoneParametricOperators— polymorphic operators for all precisions 0-12BuiltInTypeAndFunctionNamespaceManagerBuild passes (
presto-common,presto-spi,presto-main-base)Scalar function rewrites (DateTimeFunctions, etc.) for all precisions — deferred
Phase 2 client compatibility fixes (Steps 3-5)
Phase 3 semantic correctness (Steps 6-7)
Phase 4 Hive connector (Steps 9-10)