Conversation
kevinjqliu
left a comment
kevinjqliu
left a comment
There was a problem hiding this comment.
Great catch! I have a question on time-millis
| ("time-millis", "int"): TimeType(), | ||
| ("time-micros", "long"): TimeType(), | ||
| ("timestamp-millis", "int"): TimestampType(), | ||
| ("timestamp-millis", "long"): TimestampType(), |
There was a problem hiding this comment.
good catch! heres the corresponding type conversion for timestamp-millis in java https://github.com/apache/iceberg/blob/d3d5662aee19dd1799fd12740c64eadcb7f8da8b/core/src/main/java/org/apache/iceberg/avro/GenericAvroReader.java#L184-L187
I see time-micros but I dont see time-millis in here though
There was a problem hiding this comment.
Interesting. I was just looking into the Avro Spec for logicalTypes and not in the iceberg Java Code.
There was a problem hiding this comment.
This was intentional since Iceberg does not support millis, just micros, and nanos in V3+
| def test_convert_time_millis_type() -> None: | ||
| avro_logical_type = {"type": "int", "logicalType": "time-millis"} | ||
| actual = AvroSchemaConversion()._convert_logical_type(avro_logical_type) | ||
| assert actual == TimeType() |
There was a problem hiding this comment.
i wonder if theres a better way to test these type conversions..
There was a problem hiding this comment.
I could put them in 1 test and parameterize it with pytest
|
looks like the linter failed, could you run |
3 participants
Rationale for this change
timestamp-millistime-millisFollowing up on feat: add schema conversion from avro
timestamp-millisanduuid#2173 thetimestamp-millisis actually stored arelongnotint. I also noted, thattime-milliswas missing as well.https://avro.apache.org/docs/1.12.0/specification/#time_ms
Are these changes tested?
yes
Are there any user-facing changes?
no