maltesander commented on PR #11391: URL: https://github.com/apache/nifi/pull/11391#issuecomment-5100881777
I pushed some improvements @exceptionfactory: - pre-allocated LinkedHashMap in convertMap https://github.com/apache/nifi/pull/11391/commits/48a9f1e74498f97f17b26161b5fbc31e606839ca - added `RecordFieldType.CHOICE` to the required type conversions list + test https://github.com/apache/nifi/pull/11391/commits/c5ade0837f1f20ca68c489dec4405e48b06456af And one more thing / question: Iceberg supports TimestampTz which we cannot differ via `RecordFieldType`: ``` TZPROBE timestamptz <- LocalDateTime => ClassCastException: class java.time.LocalDateTime cannot be cast to class java.time.OffsetDateTime TZPROBE timestamptz <- OffsetDateTime => WROTE OK ``` Without something like (timezones vary, needs to be decided): ```java private static Object convertTimestamp(final Timestamp timestamp, final Type icebergType) { return shouldAdjustToUtc(icebergType) ? timestamp.toInstant().atOffset(ZoneOffset.UTC) : timestamp.toLocalDateTime(); } private static boolean shouldAdjustToUtc(final Type icebergType) { return switch (icebergType) { case Types.TimestampType timestampType -> timestampType.shouldAdjustToUTC(); case Types.TimestampNanoType timestampNanoType -> timestampNanoType.shouldAdjustToUTC(); case null, default -> false; }; } ``` The first 3 tests fail without explicit conversion, so we are still missing (at least) one more fix: ```java @Test void testConvertTimestampWithoutZone() { final Timestamp timestamp = Timestamp.valueOf(CREATED_LOCAL_DATE_TIME); final Object converted = RecordConverter.convertValue(timestamp, Types.TimestampType.withoutZone()); assertEquals(CREATED_LOCAL_DATE_TIME, converted); } /** * Iceberg timestamptz columns require an OffsetDateTime rather than a LocalDateTime. A Timestamp identifies an * instant, so the converted value must describe that same instant expressed at UTC. */ @Test void testConvertTimestampWithZone() { final Timestamp timestamp = Timestamp.valueOf(CREATED_LOCAL_DATE_TIME); final Object converted = RecordConverter.convertValue(timestamp, Types.TimestampType.withZone()); final OffsetDateTime offsetDateTime = assertInstanceOf(OffsetDateTime.class, converted); assertEquals(ZoneOffset.UTC, offsetDateTime.getOffset()); assertEquals(timestamp.toInstant(), offsetDateTime.toInstant()); } @Test void testConvertTimestampNanoWithZone() { final Timestamp timestamp = Timestamp.valueOf(CREATED_LOCAL_DATE_TIME); final Object converted = RecordConverter.convertValue(timestamp, Types.TimestampNanoType.withZone()); final OffsetDateTime offsetDateTime = assertInstanceOf(OffsetDateTime.class, converted); assertEquals(timestamp.toInstant(), offsetDateTime.toInstant()); } /** * The Iceberg type is not known for every field, so an unresolved type must retain the LocalDateTime conversion. */ @Test void testConvertTimestampUnknownIcebergType() { final Timestamp timestamp = Timestamp.valueOf(CREATED_LOCAL_DATE_TIME); final Object converted = RecordConverter.convertValue(timestamp, null); assertEquals(CREATED_LOCAL_DATE_TIME, converted); } /** * A timestamptz column nested inside a struct must be converted through the recursive path, matching the * positional access Iceberg uses when writing. */ @Test void testGetConvertedRecordNestedTimestampWithZone() { final Types.StructType structType = Types.StructType.of( Types.NestedField.optional(1, CREATED_FIELD_NAME, Types.TimestampType.withZone()) ); final RecordSchema nestedSchema = new SimpleRecordSchema(List.of( new RecordField(CREATED_FIELD_NAME, RecordFieldType.TIMESTAMP.getDataType()) )); final Timestamp timestamp = Timestamp.valueOf(CREATED_LOCAL_DATE_TIME); final Map<String, Object> nestedValues = new LinkedHashMap<>(); nestedValues.put(CREATED_FIELD_NAME, timestamp); final Record nestedRecord = new MapRecord(nestedSchema, nestedValues); final Object converted = RecordConverter.convertValue(nestedRecord, structType); final StructLike struct = assertInstanceOf(StructLike.class, converted); assertEquals(timestamp.toInstant(), struct.get(0, OffsetDateTime.class).toInstant()); } ``` This is not mentioned in the Jira ticket or this PR so i would defer fixing this in this PR? -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
