Skip to content

Conversation

@mbutrovich
Copy link
Collaborator

This fix enables proper interoperability with Iceberg implementations that include the type discriminator field in their JSON serialization (such as Apache Iceberg Java).

Which issue does this PR close?

Partially address #1749.

What changes are included in this PR?

Fixes a bug in the StructType deserializer that causes JSON deserialization to fail with the error expected ',' or '}' at line 1 column 8.

Root Cause:
The StructType::Deserialize implementation was not properly consuming the "type" field value when deserializing JSON like:

{"type":"struct","fields":[{"id":1,"name":"foo","required":true,"type":"string"}]}

In serde's visitor pattern, when you call map.next_key(), you must call map.next_value() to consume the corresponding value, even if you're discarding it. The deserializer was calling next_key() for the "type" field but not consuming its value, causing the parser to remain stuck at the value position and fail when encountering the next field.

Changes:

  • Modified StructTypeVisitor::visit_map to properly consume the type field value
  • Added validation that the type field equals "struct"

Are these changes tested?

Yes, two new unit tests have been added to crates/iceberg/src/spec/datatypes.rs:

  1. struct_type_with_type_field - Verifies that StructType successfully deserializes from JSON containing the "type":"struct" field. This test would fail before the fix with the error expected ',' or '}' at line 1 column 8

  2. struct_type_rejects_wrong_type - Validates that the deserializer properly rejects JSON with incorrect type values (e.g., "type":"list")

Copy link
Contributor

@liurenjie1024 liurenjie1024 left a comment

Choose a reason for hiding this comment

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

Thaks @mbutrovich for this fix!

@liurenjie1024 liurenjie1024 merged commit b3519d5 into apache:main Nov 4, 2025
34 of 36 checks passed
@mbutrovich mbutrovich deleted the struct_serde branch November 4, 2025 11:23
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