Skip to content

NIFI-16069 - PutIcebergRecord fails with ClassCastException when writing complex types (arrays, maps, nested records) - #11391

Open
maltesander wants to merge 7 commits into
apache:mainfrom
maltesander:NIFI-16069
Open

NIFI-16069 - PutIcebergRecord fails with ClassCastException when writing complex types (arrays, maps, nested records)#11391
maltesander wants to merge 7 commits into
apache:mainfrom
maltesander:NIFI-16069

Conversation

@maltesander

Copy link
Copy Markdown
Contributor

Summary

NIFI-16069 - PutIcebergRecord fails with ClassCastException when writing complex types (arrays, maps, nested records)

PutIcebergRecord fails to write FlowFiles whose schema contains complex/nested types like Iceberg list, map, or struct columns. RecordConverter only translates top-level scalar values (java.sql timestamp/date/time -> java.time) and passes complex values through unchanged.
As a result, values reach Iceberg's Parquet writer in NiFi's native representation, which is incompatible with what Iceberg expects:

  • Nested records arrive as org.apache.nifi.serialization.record.MapRecord but Iceberg requires org.apache.iceberg.StructLike.
  • Array fields arrive as Object[] but Iceberg's writer requires a java.util.Collection.
  • Maps and elements/values nested inside these types are likewise not converted (e.g. a date inside an array or map value).

Because conversion is gated on scalar field types only, records consisting solely of complex fields skip conversion entirely.

Tracking

Please complete the following tracking steps prior to pull request creation.

Issue Tracking

Pull Request Tracking

  • Pull Request title starts with Apache NiFi Jira issue number, such as NIFI-00000
  • Pull Request commit message starts with Apache NiFi Jira issue number, as such NIFI-00000
  • Pull request contains commits signed with a registered key indicating Verified status

Pull Request Formatting

  • Pull Request based on current revision of the main branch
  • Pull Request refers to a feature branch with one commit containing changes

Verification

Please indicate the verification steps performed prior to pull request creation.

Build

  • Build completed using ./mvnw clean install -P contrib-check
    • JDK 21
    • JDK 25

Licensing

  • New dependencies are compatible with the Apache License 2.0 according to the License Policy
  • New dependencies are documented in applicable LICENSE and NOTICE files

Documentation

  • Documentation formatting appears as expected in rendered files

…ing complex types (arrays, maps, nested records)

@exceptionfactory exceptionfactory left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for proposing this improvement @maltesander. The initial version of Iceberg integration did not include supported for nested and complex types, so this is an important area of improvement. The general approach looks good, and the tests are helpful. I plan on taking a closer look at the conversion details, I noted a few minor recommendations for now.

Comment on lines +82 to +89
case Object[] array when icebergType != null && icebergType.isListType() ->
convertList(Arrays.asList(array), icebergType.asListType().elementType());
case Collection<?> collection when icebergType != null && icebergType.isListType() ->
convertList(collection, icebergType.asListType().elementType());
case Record nestedRecord when icebergType != null && icebergType.isStructType() ->
new DelegatedRecord(nestedRecord, icebergType.asStructType());
case Map<?, ?> map when icebergType != null && icebergType.isMapType() ->
convertMap(map, icebergType.asMapType());

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The nested conditionals are difficult to read, I recommend breaking this out in more detail to make it easier to follow

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the feedback @exceptionfactory. I tried to simplify here d174fd3.
I introduced a convertComplexValue method to handle this.

Comment on lines +206 to +213
final Schema schema = new Schema(
Types.NestedField.required(1, "id", Types.StringType.get()),
Types.NestedField.optional(2, "tags",
Types.ListType.ofOptional(3, Types.StringType.get())),
Types.NestedField.optional(4, "address", nestedStruct),
Types.NestedField.optional(5, "attributes",
Types.MapType.ofOptional(6, 7, Types.StringType.get(), Types.StringType.get()))
);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The field names should be declared statically and reused in both the input Schema and output row.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved hardcoded constants to static ones: ef3e162

@maltesander

Copy link
Copy Markdown
Contributor Author

I pushed some improvements @exceptionfactory:

  • pre-allocated LinkedHashMap in convertMap 48a9f1e
  • added RecordFieldType.CHOICE to the required type conversions list + test c5ade08

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):

    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:

    @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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants