tsreaper commented on code in PR #3866:
URL: https://github.com/apache/paimon/pull/3866#discussion_r1705013220
##########
paimon-core/src/main/java/org/apache/paimon/iceberg/manifest/IcebergConversions.java:
##########
@@ -74,8 +79,32 @@ public static ByteBuffer toByteBuffer(DataType type, Object
value) {
case DECIMAL:
Decimal decimal = (Decimal) value;
return ByteBuffer.wrap((decimal.toUnscaledBytes()));
+ case TIMESTAMP_WITHOUT_TIME_ZONE:
+ TimestampType timestampType = (TimestampType) type;
+ precision = timestampType.getPrecision();
+ timestamp = (Timestamp) value;
+ return convertTimestampWithPrecisionToBuffer(timestamp,
precision);
+ case TIMESTAMP_WITH_LOCAL_TIME_ZONE:
+ LocalZonedTimestampType localTimestampType =
(LocalZonedTimestampType) type;
+ precision = localTimestampType.getPrecision();
+ timestamp = (Timestamp) value;
+ return convertTimestampWithPrecisionToBuffer(timestamp,
precision);
+ case TIME_WITHOUT_TIME_ZONE:
+ Long time = ((Integer) value).longValue();
+ return
ByteBuffer.allocate(8).order(ByteOrder.LITTLE_ENDIAN).putLong(0, time);
default:
throw new UnsupportedOperationException("Cannot serialize
type: " + type);
}
}
+
+ private static ByteBuffer convertTimestampWithPrecisionToBuffer(
+ Timestamp timestamp, int precision) {
+ long timestampValue;
+ if (precision <= 6) {
+ timestampValue = timestamp.getMillisecond();
Review Comment:
According to Iceberg's document: Stores **microseconds** from 1970-01-01
00:00:00.000000 in an 8-byte little-endian long.
`timestamp.getMillisecond()` only gives you millisecond. Also don't forget
there might still be values in `getNanoOfMillisecond`.
##########
paimon-core/src/main/java/org/apache/paimon/iceberg/manifest/IcebergConversions.java:
##########
@@ -74,8 +79,32 @@ public static ByteBuffer toByteBuffer(DataType type, Object
value) {
case DECIMAL:
Decimal decimal = (Decimal) value;
return ByteBuffer.wrap((decimal.toUnscaledBytes()));
+ case TIMESTAMP_WITHOUT_TIME_ZONE:
+ TimestampType timestampType = (TimestampType) type;
+ precision = timestampType.getPrecision();
+ timestamp = (Timestamp) value;
+ return convertTimestampWithPrecisionToBuffer(timestamp,
precision);
+ case TIMESTAMP_WITH_LOCAL_TIME_ZONE:
+ LocalZonedTimestampType localTimestampType =
(LocalZonedTimestampType) type;
+ precision = localTimestampType.getPrecision();
+ timestamp = (Timestamp) value;
+ return convertTimestampWithPrecisionToBuffer(timestamp,
precision);
+ case TIME_WITHOUT_TIME_ZONE:
+ Long time = ((Integer) value).longValue();
+ return
ByteBuffer.allocate(8).order(ByteOrder.LITTLE_ENDIAN).putLong(0, time);
Review Comment:
Why do you cast it to an `Integer`?
From Paimon's `TimeType`:
Data type of a time WITHOUT time zone consisting of {@code
hour:minute:second[.fractional]} with up to **nanosecond** precision.
From Iceberg's `time` type:
Stores **microseconds** from midnight in an 8-byte little-endian long.
Please read the docs very carefully. You cannot use this long value directly.
##########
paimon-core/src/main/java/org/apache/paimon/iceberg/manifest/IcebergConversions.java:
##########
@@ -74,8 +79,32 @@ public static ByteBuffer toByteBuffer(DataType type, Object
value) {
case DECIMAL:
Decimal decimal = (Decimal) value;
return ByteBuffer.wrap((decimal.toUnscaledBytes()));
+ case TIMESTAMP_WITHOUT_TIME_ZONE:
+ TimestampType timestampType = (TimestampType) type;
+ precision = timestampType.getPrecision();
+ timestamp = (Timestamp) value;
+ return convertTimestampWithPrecisionToBuffer(timestamp,
precision);
+ case TIMESTAMP_WITH_LOCAL_TIME_ZONE:
+ LocalZonedTimestampType localTimestampType =
(LocalZonedTimestampType) type;
+ precision = localTimestampType.getPrecision();
+ timestamp = (Timestamp) value;
+ return convertTimestampWithPrecisionToBuffer(timestamp,
precision);
+ case TIME_WITHOUT_TIME_ZONE:
+ Long time = ((Integer) value).longValue();
+ return
ByteBuffer.allocate(8).order(ByteOrder.LITTLE_ENDIAN).putLong(0, time);
default:
throw new UnsupportedOperationException("Cannot serialize
type: " + type);
}
}
+
+ private static ByteBuffer convertTimestampWithPrecisionToBuffer(
+ Timestamp timestamp, int precision) {
+ long timestampValue;
+ if (precision <= 6) {
+ timestampValue = timestamp.getMillisecond();
+ } else {
+ timestampValue = timestamp.getNanoOfMillisecond();
Review Comment:
What about the milli-second part? You throw it away?
##########
paimon-core/src/test/java/org/apache/paimon/iceberg/IcebergCompatibilityTest.java:
##########
@@ -271,6 +274,101 @@ public void testAppendOnlyTableWithAllTypes() throws
Exception {
r -> "");
}
+ @Test
+ public void testPartitionedPrimaryKeyTable_Timestamp() throws Exception {
Review Comment:
I suggest that you only test on `TIMESTAMP(6)` Paimon type.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@paimon.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
