You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "aihuaxu (via GitHub)" <gi...@apache.org> on 2025/03/29 04:13:04 UTC
[PR] Add timestamp_ns, time and UUID types for Variant [iceberg]
aihuaxu opened a new pull request, #12682:
URL: https://github.com/apache/iceberg/pull/12682
Variant spec added additional types including nano-timestamp, time and UUID (https://github.com/apache/parquet-format/blob/master/VariantEncoding.md). This PR adds those missing type support in Variant encoding and decoding, shredding and Variant metrics.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add timestamp_ns, time and UUID types for Variant [iceberg]
Posted by "aihuaxu (via GitHub)" <gi...@apache.org>.
aihuaxu commented on code in PR #12682:
URL: https://github.com/apache/iceberg/pull/12682#discussion_r2059168065
##########
parquet/src/main/java/org/apache/iceberg/parquet/ParquetVariantVisitor.java:
##########
@@ -282,4 +284,18 @@ private static boolean isBinary(Type type) {
return type.isPrimitive()
&& type.asPrimitiveType().getPrimitiveTypeName() == PrimitiveTypeName.BINARY;
}
+
+ protected static PhysicalType type(TimestampLogicalTypeAnnotation timestamp) {
Review Comment:
I moved to ParquetVaraintUtil class now. That should be better.
##########
parquet/src/main/java/org/apache/iceberg/parquet/VariantReaderBuilder.java:
##########
@@ -198,15 +201,41 @@ public Optional<VariantValueReader> visit(DateLogicalTypeAnnotation ignored) {
@Override
public Optional<VariantValueReader> visit(TimestampLogicalTypeAnnotation logical) {
- PhysicalType variantType =
- logical.isAdjustedToUTC() ? PhysicalType.TIMESTAMPTZ : PhysicalType.TIMESTAMPNTZ;
+ PhysicalType variantType;
+
+ switch (logical.getUnit()) {
Review Comment:
I extracted and moved to the parent class for reuse.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add timestamp_ns, time and UUID types for Variant [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #12682:
URL: https://github.com/apache/iceberg/pull/12682#discussion_r2056663085
##########
api/src/main/java/org/apache/iceberg/variants/PhysicalType.java:
##########
@@ -41,6 +41,10 @@ public enum PhysicalType {
FLOAT(LogicalType.FLOAT, Float.class),
BINARY(LogicalType.BINARY, ByteBuffer.class),
STRING(LogicalType.STRING, String.class),
+ TIMENTZ(LogicalType.TIME, Long.class),
Review Comment:
Can you also remove `NTZ` from this symbol? This is the one that matters because it is public.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add timestamp_ns, time and UUID types for Variant [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #12682:
URL: https://github.com/apache/iceberg/pull/12682#discussion_r2058807138
##########
api/src/main/java/org/apache/iceberg/variants/Primitives.java:
##########
@@ -36,6 +36,10 @@ class Primitives {
static final int TYPE_FLOAT = 14;
static final int TYPE_BINARY = 15;
static final int TYPE_STRING = 16;
+ static final int TYPE_TIMENTZ = 17;
Review Comment:
Nit: internal constant using `TIMENTZ`.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add timestamp_ns, time and UUID types for Variant [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #12682:
URL: https://github.com/apache/iceberg/pull/12682#discussion_r2052762453
##########
parquet/src/test/java/org/apache/iceberg/parquet/TestVariantMetrics.java:
##########
@@ -496,6 +506,13 @@ private static VariantValue increment(VariantValue value) {
case STRING:
return Variants.of(
value.type(), UnicodeUtil.truncateStringMax((String) primitive.get(), 5));
+ case UUID:
+ {
Review Comment:
Is this block needed? There isn't another variable called `uuid` is there?
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add timestamp_ns, time and UUID types for Variant [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #12682:
URL: https://github.com/apache/iceberg/pull/12682#discussion_r2059082950
##########
core/src/main/java/org/apache/iceberg/variants/PrimitiveWrapper.java:
##########
@@ -45,6 +47,12 @@ class PrimitiveWrapper<T> implements VariantPrimitive<T> {
VariantUtil.primitiveHeader(Primitives.TYPE_DECIMAL16);
private static final byte BINARY_HEADER = VariantUtil.primitiveHeader(Primitives.TYPE_BINARY);
private static final byte STRING_HEADER = VariantUtil.primitiveHeader(Primitives.TYPE_STRING);
+ private static final byte TIMENTZ_HEADER = VariantUtil.primitiveHeader(Primitives.TYPE_TIME);
Review Comment:
Another reference to TIME with NTZ suffix.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add timestamp_ns, time and UUID types for Variant [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue merged PR #12682:
URL: https://github.com/apache/iceberg/pull/12682
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add timestamp_ns, time and UUID types for Variant [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #12682:
URL: https://github.com/apache/iceberg/pull/12682#discussion_r2052756028
##########
core/src/main/java/org/apache/iceberg/variants/PrimitiveWrapper.java:
##########
@@ -204,6 +217,23 @@ public int writeTo(ByteBuffer outBuffer, int offset) {
outBuffer.putInt(offset + 1, buffer.remaining());
VariantUtil.writeBufferAbsolute(outBuffer, offset + 5, buffer);
return 5 + buffer.remaining();
+ case TIMENTZ:
+ outBuffer.put(offset, TIMENTZ_HEADER);
+ outBuffer.putLong(offset + 1, (Long) value);
+ return 9;
+ case TIMESTAMPTZNS:
+ outBuffer.put(offset, TIMESTAMPTZNS_HEADER);
+ outBuffer.putLong(offset + 1, (Long) value);
+ return 9;
+ case TIMESTAMPNTZNS:
+ outBuffer.put(offset, TIMESTAMPNTZNS_HEADER);
+ outBuffer.putLong(offset + 1, (Long) value);
+ return 9;
+ case UUID:
+ outBuffer.put(offset, UUID_HEADER);
+ VariantUtil.writeBufferAbsolute(
+ outBuffer, offset + 1, UUIDUtil.convertToByteBuffer((UUID) value));
+ return 1 + 16;
Review Comment:
17?
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add timestamp_ns, time and UUID types for Variant [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #12682:
URL: https://github.com/apache/iceberg/pull/12682#discussion_r2052749038
##########
api/src/main/java/org/apache/iceberg/variants/Primitives.java:
##########
@@ -36,7 +36,10 @@ class Primitives {
static final int TYPE_FLOAT = 14;
static final int TYPE_BINARY = 15;
static final int TYPE_STRING = 16;
-
Review Comment:
Why remove the space before `PRIMITIVE_TYPE_SHIFT`?
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add timestamp_ns, time and UUID types for Variant [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #12682:
URL: https://github.com/apache/iceberg/pull/12682#discussion_r2059082344
##########
api/src/test/java/org/apache/iceberg/variants/TestSerializedPrimitives.java:
##########
@@ -451,11 +453,165 @@ public void testShortString() {
assertThat(value.get()).isEqualTo("iceberg");
}
+ @Test
+ public void testTimentzNano() {
Review Comment:
Nit: another reference to time with ntz.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add timestamp_ns, time and UUID types for Variant [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #12682:
URL: https://github.com/apache/iceberg/pull/12682#discussion_r2052748279
##########
api/src/main/java/org/apache/iceberg/variants/LogicalType.java:
##########
@@ -29,6 +29,8 @@ enum LogicalType {
TIMESTAMPNTZ,
BINARY,
STRING,
+ TIMENTZ,
Review Comment:
How about just `TIME` for now? `TIMENTZ` is a bit awkward and there is no `TIMETZ`.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add timestamp_ns, time and UUID types for Variant [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #12682:
URL: https://github.com/apache/iceberg/pull/12682#discussion_r2052750931
##########
api/src/main/java/org/apache/iceberg/variants/VariantObject.java:
##########
@@ -44,9 +46,13 @@ default VariantObject asObject() {
static String asString(VariantObject object) {
StringBuilder builder = new StringBuilder();
+ // Make the string deterministic
Review Comment:
I think that the values should be in the field order from the object. Why does this need to be a different order?
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add timestamp_ns, time and UUID types for Variant [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #12682:
URL: https://github.com/apache/iceberg/pull/12682#discussion_r2052760230
##########
parquet/src/main/java/org/apache/iceberg/parquet/VariantReaderBuilder.java:
##########
@@ -198,15 +201,35 @@ public Optional<VariantValueReader> visit(DateLogicalTypeAnnotation ignored) {
@Override
public Optional<VariantValueReader> visit(TimestampLogicalTypeAnnotation logical) {
- PhysicalType variantType =
- logical.isAdjustedToUTC() ? PhysicalType.TIMESTAMPTZ : PhysicalType.TIMESTAMPNTZ;
+ PhysicalType variantType = null;
+ if (logical.getUnit() == LogicalTypeAnnotation.TimeUnit.MICROS) {
Review Comment:
Can you use a `switch` instead with the enum?
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add timestamp_ns, time and UUID types for Variant [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #12682:
URL: https://github.com/apache/iceberg/pull/12682#discussion_r2058811330
##########
parquet/src/main/java/org/apache/iceberg/parquet/VariantReaderBuilder.java:
##########
@@ -198,15 +201,41 @@ public Optional<VariantValueReader> visit(DateLogicalTypeAnnotation ignored) {
@Override
public Optional<VariantValueReader> visit(TimestampLogicalTypeAnnotation logical) {
- PhysicalType variantType =
- logical.isAdjustedToUTC() ? PhysicalType.TIMESTAMPTZ : PhysicalType.TIMESTAMPNTZ;
+ PhysicalType variantType;
+
+ switch (logical.getUnit()) {
Review Comment:
Could this reuse the method from the writer builder? It looks like the same logic, but inline here.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add timestamp_ns, time and UUID types for Variant [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #12682:
URL: https://github.com/apache/iceberg/pull/12682#discussion_r2052757722
##########
core/src/main/java/org/apache/iceberg/variants/Variants.java:
##########
@@ -200,4 +201,36 @@ public static VariantPrimitive<ByteBuffer> of(ByteBuffer value) {
public static VariantPrimitive<String> of(String value) {
return new PrimitiveWrapper<>(PhysicalType.STRING, value);
}
+
+ public static VariantPrimitive<Long> ofTimentz(long value) {
+ return new PrimitiveWrapper<>(PhysicalType.TIMENTZ, value);
+ }
+
+ public static VariantPrimitive<Long> ofIsoTimentz(String value) {
+ return ofTimentz(DateTimeUtil.isoTimeToMicros(value));
+ }
+
+ public static VariantPrimitive<Long> ofTimestamptzNano(long value) {
+ return new PrimitiveWrapper<>(PhysicalType.TIMESTAMPTZNS, value);
+ }
+
+ public static VariantPrimitive<Long> ofIsoTimestamptzNano(String value) {
+ return ofTimestamptzNano(DateTimeUtil.isoTimestamptzToNanos(value));
+ }
+
+ public static VariantPrimitive<Long> ofTimestampntzNano(long value) {
+ return new PrimitiveWrapper<>(PhysicalType.TIMESTAMPNTZNS, value);
+ }
+
+ public static VariantPrimitive<Long> ofIsoTimestampntzNano(String value) {
+ return ofTimestampntzNano(DateTimeUtil.isoTimestampToNanos(value));
+ }
+
+ public static VariantPrimitive<UUID> ofUuid(UUID uuid) {
Review Comment:
UUID should be capitalized like the class name.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add timestamp_ns, time and UUID types for Variant [iceberg]
Posted by "aihuaxu (via GitHub)" <gi...@apache.org>.
aihuaxu commented on code in PR #12682:
URL: https://github.com/apache/iceberg/pull/12682#discussion_r2052976424
##########
api/src/main/java/org/apache/iceberg/variants/PhysicalType.java:
##########
@@ -41,6 +41,10 @@ public enum PhysicalType {
FLOAT(LogicalType.FLOAT, Float.class),
BINARY(LogicalType.BINARY, ByteBuffer.class),
STRING(LogicalType.STRING, String.class),
+ TIMENTZ(LogicalType.TIMENTZ, Long.class),
+ TIMESTAMPTZNS(LogicalType.TIMESTAMPTZ, Long.class),
+ TIMESTAMPNTZNS(LogicalType.TIMESTAMPNTZ, Long.class),
Review Comment:
That sounds better. Thanks.
##########
api/src/test/java/org/apache/iceberg/variants/TestSerializedPrimitives.java:
##########
@@ -451,11 +452,143 @@ public void testShortString() {
assertThat(value.get()).isEqualTo("iceberg");
}
+ @Test
+ public void testTimentzNano() {
Review Comment:
Negative is not allowed with "Invalid value for NanoOfDay (valid values 0 - 86399999999999)". I can add one for this.
```
Invalid value for NanoOfDay (valid values 0 - 86399999999999): -1000
java.time.DateTimeException: Invalid value for NanoOfDay (valid values 0 - 86399999999999): -1000
at java.base/java.time.temporal.ValueRange.checkValidValue(ValueRange.java:319)
at java.base/java.time.temporal.ChronoField.checkValidValue(ChronoField.java:718)
at java.base/java.time.LocalTime.ofNanoOfDay(LocalTime.java:408)
at org.apache.iceberg.util.DateTimeUtil.timeFromMicros(DateTimeUtil.java:62)
at org.apache.iceberg.variants.TestSerializedPrimitives.testTimentzNanoNegative(TestSerializedPrimitives.java:493)
```
##########
api/src/main/java/org/apache/iceberg/variants/VariantObject.java:
##########
@@ -44,9 +46,13 @@ default VariantObject asObject() {
static String asString(VariantObject object) {
StringBuilder builder = new StringBuilder();
+ // Make the string deterministic
Review Comment:
This is from the test https://github.com/apache/iceberg/blob/main/core/src/test/java/org/apache/iceberg/avro/TestAvroEncoderUtil.java#L62.
In variant spec, we are sorting field ids. So we are creating a record in original order while we are reading back as a different order with field ids sorted.
`The field ids and field offsets must be in lexicographical order of the corresponding field names in the metadata dictionary. However, the actual value entries do not need to be in any particular order.
`
##########
core/src/main/java/org/apache/iceberg/variants/Variants.java:
##########
@@ -200,4 +201,36 @@ public static VariantPrimitive<ByteBuffer> of(ByteBuffer value) {
public static VariantPrimitive<String> of(String value) {
return new PrimitiveWrapper<>(PhysicalType.STRING, value);
}
+
+ public static VariantPrimitive<Long> ofTimentz(long value) {
+ return new PrimitiveWrapper<>(PhysicalType.TIMENTZ, value);
+ }
+
+ public static VariantPrimitive<Long> ofIsoTimentz(String value) {
+ return ofTimentz(DateTimeUtil.isoTimeToMicros(value));
+ }
+
+ public static VariantPrimitive<Long> ofTimestamptzNano(long value) {
+ return new PrimitiveWrapper<>(PhysicalType.TIMESTAMPTZNS, value);
+ }
+
+ public static VariantPrimitive<Long> ofIsoTimestamptzNano(String value) {
+ return ofTimestamptzNano(DateTimeUtil.isoTimestamptzToNanos(value));
+ }
+
+ public static VariantPrimitive<Long> ofTimestampntzNano(long value) {
+ return new PrimitiveWrapper<>(PhysicalType.TIMESTAMPNTZNS, value);
+ }
+
+ public static VariantPrimitive<Long> ofIsoTimestampntzNano(String value) {
+ return ofTimestampntzNano(DateTimeUtil.isoTimestampToNanos(value));
+ }
+
+ public static VariantPrimitive<UUID> ofUuid(UUID uuid) {
Review Comment:
Thanks. I think the `ofUuid(String uuid)` should too, right?
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add timestamp_ns, time and UUID types for Variant [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #12682:
URL: https://github.com/apache/iceberg/pull/12682#discussion_r2058808582
##########
core/src/main/java/org/apache/iceberg/variants/Variants.java:
##########
@@ -200,4 +201,36 @@ public static VariantPrimitive<ByteBuffer> of(ByteBuffer value) {
public static VariantPrimitive<String> of(String value) {
return new PrimitiveWrapper<>(PhysicalType.STRING, value);
}
+
+ public static VariantPrimitive<Long> ofTimentz(long value) {
Review Comment:
Public methods shouldn't use the `ntz` suffix for time.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add timestamp_ns, time and UUID types for Variant [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #12682:
URL: https://github.com/apache/iceberg/pull/12682#discussion_r2056668528
##########
parquet/src/main/java/org/apache/iceberg/parquet/VariantWriterBuilder.java:
##########
@@ -229,19 +228,32 @@ public Optional<ParquetValueWriter<?>> visit(DateLogicalTypeAnnotation ignored)
@Override
public Optional<ParquetValueWriter<?>> visit(TimeLogicalTypeAnnotation time) {
- // ParquetValueWriter<VariantValue> writer =
- // ParquetVariantWriters.primitive(ParquetValueWriters.longs(desc), PhysicalType.TIME);
- return Optional.empty();
+ ParquetValueWriter<VariantValue> writer =
+ ParquetVariantWriters.primitive(ParquetValueWriters.longs(desc), PhysicalType.TIMENTZ);
+ return Optional.of(writer);
}
@Override
public Optional<ParquetValueWriter<?>> visit(TimestampLogicalTypeAnnotation timestamp) {
- if (timestamp.getUnit() == TimeUnit.MICROS) {
- PhysicalType type =
- timestamp.isAdjustedToUTC() ? PhysicalType.TIMESTAMPTZ : PhysicalType.TIMESTAMPNTZ;
- ParquetValueWriter<?> writer =
- ParquetVariantWriters.primitive(ParquetValueWriters.longs(desc), type);
- return Optional.of(writer);
+ switch (timestamp.getUnit()) {
Review Comment:
I think this would be cleaner if you refactored the `PhysicalType` into its own method:
```java
private PhysicalType type(TimestampLogicalTypeAnnotation timestamp) {
switch (timestamp.getUnit()) {
case MICROS:
return timestamp.isAdjustedToUTC() ? PhysicalType.TIMESTAMPTZ : PhysicalType.TIMESTAMPNTZ;
case NANOS:
return timestamp.isAdjustedToUTC() ? PhysicalType.TIMESTAMPTZ_NANOS : PhysicalType.TIMESTAMPNTZ_NANOS;
default:
throw new UnsupportedOperationException("Invalid unit for shredded timestamp: " + unit);
}
}
```
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add timestamp_ns, time and UUID types for Variant [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #12682:
URL: https://github.com/apache/iceberg/pull/12682#discussion_r2052761580
##########
parquet/src/main/java/org/apache/iceberg/parquet/VariantWriterBuilder.java:
##########
@@ -242,6 +242,12 @@ public Optional<ParquetValueWriter<?>> visit(TimestampLogicalTypeAnnotation time
ParquetValueWriter<?> writer =
ParquetVariantWriters.primitive(ParquetValueWriters.longs(desc), type);
return Optional.of(writer);
+ } else if (timestamp.getUnit() == TimeUnit.NANOS) {
Review Comment:
Can you change this to a switch statement?
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add timestamp_ns, time and UUID types for Variant [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #12682:
URL: https://github.com/apache/iceberg/pull/12682#discussion_r2052757248
##########
api/src/main/java/org/apache/iceberg/variants/PhysicalType.java:
##########
@@ -41,6 +41,10 @@ public enum PhysicalType {
FLOAT(LogicalType.FLOAT, Float.class),
BINARY(LogicalType.BINARY, ByteBuffer.class),
STRING(LogicalType.STRING, String.class),
+ TIMENTZ(LogicalType.TIMENTZ, Long.class),
+ TIMESTAMPTZNS(LogicalType.TIMESTAMPTZ, Long.class),
+ TIMESTAMPNTZNS(LogicalType.TIMESTAMPNTZ, Long.class),
Review Comment:
How about `TIMESTAMPTZ_NANOS` instead? These are really hard to read.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add timestamp_ns, time and UUID types for Variant [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #12682:
URL: https://github.com/apache/iceberg/pull/12682#discussion_r2052751355
##########
api/src/test/java/org/apache/iceberg/variants/TestSerializedPrimitives.java:
##########
@@ -451,11 +452,143 @@ public void testShortString() {
assertThat(value.get()).isEqualTo("iceberg");
}
+ @Test
+ public void testTimentzNano() {
Review Comment:
Are negative values allowed? If not, what happens? Could you add a test case?
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Add timestamp_ns, time and UUID types for Variant [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #12682:
URL: https://github.com/apache/iceberg/pull/12682#discussion_r2059086175
##########
parquet/src/main/java/org/apache/iceberg/parquet/ParquetVariantVisitor.java:
##########
@@ -282,4 +284,18 @@ private static boolean isBinary(Type type) {
return type.isPrimitive()
&& type.asPrimitiveType().getPrimitiveTypeName() == PrimitiveTypeName.BINARY;
}
+
+ protected static PhysicalType type(TimestampLogicalTypeAnnotation timestamp) {
Review Comment:
This isn't a good place for a conversion method. The visitor is a superclass of both reader and writer builders, but it isn't where Parquet conversion logic should live.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org