Skip to content

Conversation

@wgtmac
Copy link
Member

@wgtmac wgtmac commented May 16, 2025

No description provided.

@wgtmac wgtmac marked this pull request as ready for review May 17, 2025 16:24
*node = std::make_shared<::avro::NodePrimitive>(::avro::AVRO_FIXED);
(*node)->setLogicalType(::avro::LogicalType{::avro::LogicalType::UUID});
(*node)->setFixedSize(16);
(*node)->setName(::avro::Name("uuid_fixed"));
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm wondering if the _fixed suffix is needed.

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

Okay then. Will the name be serialized to the Avro file?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes

Status Visit(const StructType& type, ::avro::NodePtr* node);
Status Visit(const ListType& type, ::avro::NodePtr* node);
Status Visit(const MapType& type, ::avro::NodePtr* node);
Status Visit(const SchemaField& field, ::avro::NodePtr* node);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we make this one private, since SchemaField is not a Iceberg type?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think this is fine because the file is also internal. We don't want to expose any avro:: and arrow:: namespace.

@Fokko Fokko merged commit e8a4050 into apache:main May 19, 2025
6 checks passed
@Fokko
Copy link
Contributor

Fokko commented May 19, 2025

Thanks @wgtmac for working on this, and thanks @lidavidm and @zhjwpku for the review 🙌

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.

4 participants