Add Fixed, Uuid support to arrow-avro #7557

nathaniel-d-ef · 2025-05-28T15:26:12Z

Which issue does this PR close?

Part of 4886

Related to 6965

Rationale for this change

This change expands upon the Avro reader logic by adding full support for the Fixed and Uuid types (Uuid relies on Fixed). It builds out the Fixed path currently stubbed out.

What changes are included in this PR?

Adds Fixed and Uuid support to the arrow-avro crate with changes to the following:

arrow-avro/src/codec.rs

Adds support for Uuid type
Adds test

arrow-avro/src/reader/cursor.rs:

Adds a get_fixed helper method to read the specified bytes into a buffer.

arrow-avro/src/reader/record.rs:

Introduces the Fixed decoding path, building out the nyi Codec::Fixed in the Decoder.
Introduces the Uuid decoding path, building off of Fixed
Adds tests.

Are there any user-facing changes?

n/a

klion26 · 2025-06-17T01:20:17Z

arrow-avro/src/codec.rs

    /// Represents Avro fixed type, maps to Arrow's FixedSizeBinary data type
    /// The i32 parameter indicates the fixed binary size
    Fixed(i32),
+    /// Represents Avro Uuid type, a FixedSizeBinary with a length of 16


In the rfc link, it says that A UUID is 128 bits long, and requires no central registration process, does the length 16 here suffice for that?

for the uuid crate, the length for the example 67e55044-10b1-426f-9247-bb680e5fe0c8 is 36

It should; 16 bytes == 128 bits. It looks to me like the UUID crate specifies 16 & 128 as well. Do you mean character count? That's independent of byte size.

got it, thanks

klion26 · 2025-06-17T02:51:31Z

arrow-avro/src/reader/record.rs

    }
+
+    #[test]
+    fn test_fixed_decoding() {


Thanks for these tests. Do we need to add a test for the UUID type?

It seems unnecessary to me, as it would be nearly identical to fixed but with a hard-coded length. I can add one though, if necessary.

I'm not entirely sure about this; we could involve a maintainer for a double check.
just noticed that we already have tests for other types.

I think a test explicitly for reading uuids is important (maybe it is covered by end to end coverage somewhere else however)

I think it is important to ensure we don't accidentally break UUID handling in the future\

Perhaps we can add one in a follow on PR

@alamb We don't have an E2E test covering UUID. I'll be sure to create an Avro UUID test file (unless there's already one you'd want us to use) and include an E2E UUID test along with the other E2E tests in our second to last PR. Our final PR would then contain the completed arrow-avro implementations of the ReaderBuilder and Reader.

(unless there's already one you'd want us to use)

Not that i know of -- but I am not familar with apache avro -- if there is some sort of standardized test corpus it would be great to use that

I took a look around quickly but couldn't find anything like https://github.com/apache/parquet-testing for avro 🤔

Introduced handling for Avro `fixed` type in the decoder, enabling proper initialization, decoding, and array construction. Added corresponding tests to validate decoding functionality and edge cases like empty arrays. Implemented `get_fixed` method in `AvroCursor` for precise byte reading.

…d minimize diffs with earlier iteration

Introduce a new `Codec::Uuid` variant to handle Avro Uuid logical types, mapping it to a `FixedSizeBinary` of length 16 in Arrow. This includes updates to codec handling, relevant match branches, and a unit test to validate functionality.

klion26

Thank you for the contribution, LGTM % whether to need to add a test for uuid type.

klion26 · 2025-06-19T06:22:58Z

arrow-avro/src/codec.rs

    /// Represents Avro fixed type, maps to Arrow's FixedSizeBinary data type
    /// The i32 parameter indicates the fixed binary size
    Fixed(i32),
+    /// Represents Avro Uuid type, a FixedSizeBinary with a length of 16


got it, thanks

klion26 · 2025-06-19T06:27:24Z

arrow-avro/src/reader/record.rs

    }
+
+    #[test]
+    fn test_fixed_decoding() {


I'm not entirely sure about this; we could involve a maintainer for a double check.
just noticed that we already have tests for other types.

nathaniel-d-ef · 2025-06-20T16:29:01Z

@klion26 Thank you for your review; I went ahead and added the test.

jecsand838 · 2025-06-27T02:30:41Z

@alamb @tustvold Any ideas when you'd be able to give this one a final review? The next set of PRs I was going to open depend on this one getting approved and merged in.

alamb

Thank you @nathaniel-d-ef and @klion26 -- this looks like an improvement to me

@klion26 once you are happy that a PR is looking good please feel free to @ mention me for a final stamp of approval / merge

alamb · 2025-06-27T14:44:15Z

arrow-avro/src/reader/record.rs

    }
+
+    #[test]
+    fn test_fixed_decoding() {


I think a test explicitly for reading uuids is important (maybe it is covered by end to end coverage somewhere else however)

I think it is important to ensure we don't accidentally break UUID handling in the future\

Perhaps we can add one in a follow on PR

alamb · 2025-06-27T14:45:13Z

arrow-avro/src/reader/record.rs

+        let mut decoder = Decoder::try_new(&avro_type).expect("Failed to create decoder");
+
+        let data1 = [1u8, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16];
+        let mut cursor1 = AvroCursor::new(&data1);


It would be nice to extend this test, perhaps in a follow on PR, to ensure the UUID was read correctly

alamb · 2025-06-27T14:48:28Z

@nathaniel-d-ef can. you please merge main into this PR and run cargo fmt so we can get a clean CI run ? Then I'll merge it

@alamb @tustvold Any ideas when you'd be able to give this one a final review? The next set of PRs I was going to open depend on this one getting approved and merged in.

@jecsand838 sorry for the delay

jecsand838 · 2025-06-27T19:43:31Z

@jecsand838 sorry for the delay

@alamb No worries at all! Thank you so much for responding and getting back so quickly.

alamb · 2025-06-28T10:21:49Z

Thanks again everyone

klion26 · 2025-06-29T05:43:57Z

Thank you @nathaniel-d-ef and @klion26 -- this looks like an improvement to me

@klion26 once you are happy that a PR is looking good please feel free to @ mention me for a final stamp of approval / merge

@alamb got it, will ping you in the future

github-actions bot added the arrow Changes to the arrow crate label May 28, 2025

nathaniel-d-ef changed the title ~~Add Fixed support to arrow-avro~~ Add Fixed, Uuid support to arrow-avro May 28, 2025

klion26 reviewed Jun 17, 2025

View reviewed changes

nathaniel-d-ef added 5 commits June 17, 2025 16:40

Remove redundant comments in test_fixed_decoding function

dfb0222

Refactor Fixed type handling to use try_into for size conversion an…

e94e488

…d minimize diffs with earlier iteration

Add support for Avro Uuid logical type

6863908

Introduce a new `Codec::Uuid` variant to handle Avro Uuid logical types, mapping it to a `FixedSizeBinary` of length 16 in Arrow. This includes updates to codec handling, relevant match branches, and a unit test to validate functionality.

Refactor test formatting and assertions for clarity and consistency

23cc74d

nathaniel-d-ef force-pushed the avro-codec-fixed branch from 6de7cc7 to 23cc74d Compare June 17, 2025 21:45

klion26 approved these changes Jun 19, 2025

View reviewed changes

Add test for Uuid decoding in Avro reader

91f8442

alamb approved these changes Jun 27, 2025

View reviewed changes

jecsand838 and others added 2 commits June 27, 2025 14:21

Merge branch 'apache:main' into avro-codec-fixed

a1d8882

Improve formatting in test_uuid_decoding for readability

e605642

alamb merged commit 674dc17 into apache:main Jun 28, 2025
23 checks passed

jecsand838 deleted the avro-codec-fixed branch June 30, 2025 21:31

Add Fixed, Uuid support to arrow-avro #7557

Add Fixed, Uuid support to arrow-avro #7557

Uh oh!

Conversation

nathaniel-d-ef commented May 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

klion26 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nathaniel-d-ef commented Jun 20, 2025

Uh oh!

jecsand838 commented Jun 27, 2025

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alamb commented Jun 27, 2025

Uh oh!

jecsand838 commented Jun 27, 2025

Uh oh!

Uh oh!

alamb commented Jun 28, 2025

Uh oh!

klion26 commented Jun 29, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

nathaniel-d-ef commented May 28, 2025 •

edited

Loading