Skip to content

Fix for 'Error reading Avro files containing array of struct with 2 fields #605'#618

Merged
rdblue merged 2 commits intoapache:masterfrom
rdsr:complex_map
Nov 9, 2019
Merged

Fix for 'Error reading Avro files containing array of struct with 2 fields #605'#618
rdblue merged 2 commits intoapache:masterfrom
rdsr:complex_map

Conversation

@rdsr
Copy link
Copy Markdown
Contributor

@rdsr rdsr commented Nov 5, 2019

No description provided.


case ARRAY:
if (schema.getLogicalType() instanceof LogicalMap || AvroSchemaUtil.isKeyValueSchema(schema.getElementType())) {
if (schema.getLogicalType() instanceof LogicalMap && AvroSchemaUtil.isKeyValueSchema(schema.getElementType())) {
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.

I think the solution should be a better check in isKeyValueSchema. If the logical type is LogicalMap, then it should not need additional requirements for the element type.

Copy link
Copy Markdown
Contributor Author

@rdsr rdsr Nov 6, 2019

Choose a reason for hiding this comment

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

If the logical type is LogicalMap, then it should not need additional requirements for the element type.

I think that makes sense. In that case. Maybe I should just simplify the if condition to
f (schema.getLogicalType() instanceof LogicalMap) and maybe make the isKeyValueSchema check as part of a PreconditionCheck?

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.

Yes, that sounds good.

This was originally an or condition because we had manifest files that were written with arrays of structs for maps, but without the map logical type. We still want to support reading files like that (at least in our version) but I think the right way to support it is to use an AvroSchemaWithType visitor introduced in #601 and checking whether the Iceberg type is a map.

@rdblue
Copy link
Copy Markdown
Contributor

rdblue commented Nov 9, 2019

+1

Thanks for fixing this, @rdsr!

@rdblue rdblue merged commit f1cba90 into apache:master Nov 9, 2019
sunchao pushed a commit to sunchao/iceberg that referenced this pull request May 9, 2023
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