You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "jecsand838 (via GitHub)" <gi...@apache.org> on 2025/07/10 00:27:13 UTC
[PR] Add arrow-avro support for Duration type and minor fixes for UUID decoding [arrow-rs]
jecsand838 opened a new pull request, #7889:
URL: https://github.com/apache/arrow-rs/pull/7889
# Which issue does this PR close?
- Part of https://github.com/apache/arrow-rs/issues/4886
- Related to https://github.com/apache/arrow-rs/pull/6965
# Rationale for this change
The `arrow-avro` crate currently lacks support for the Avro `duration` type, which is a standard and commonly used type in Avro schemas. This omission prevents users from reading Avro files containing duration types, limiting the crate's utility.
This change introduces support for decoding Avro duration types by mapping them to the Arrow `Interval` type. This is a logical and efficient representation. Implementing this feature brings the `arrow-avro` crate closer to full Avro specification compliance and makes it more robust for real-world use cases.
# What changes are included in this PR?
This PR contains:
* arrow-avro decoder support for Duration types.
* Minor fixes UUID decoding. UUID types now map to `utf8` type to better align with the [Avro specification](https://avro.apache.org/docs/1.11.1/specification/#uuid)
* New integration test using a temporary `duration_uuid.avro` file crate using this python script: https://gist.github.com/jecsand838/cbdaaf581af78f357778bf87d2f3cf15
# Are these changes tested?
Yes, this PR includes for integration and unit tests covering these modifications.
# Are there any user-facing changes?
N/A
# Follow-Up PRs
1. PR to update `test_duration_uuid` once https://github.com/apache/arrow-testing/pull/108 is merged in.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add arrow-avro support for Duration type and minor fixes for UUID decoding [arrow-rs]
Posted by "mbrobbel (via GitHub)" <gi...@apache.org>.
mbrobbel commented on code in PR #7889:
URL: https://github.com/apache/arrow-rs/pull/7889#discussion_r2196795383
##########
arrow-avro/Cargo.toml:
##########
@@ -56,6 +56,7 @@ rand = { version = "0.9", default-features = false, features = ["std", "std_rng"
criterion = { version = "0.6.0", default-features = false }
tempfile = "3.3"
arrow = { workspace = true }
+uuid = { version = "1.17.0", features = ["v4"] }
Review Comment:
It looks like we don't need the `v4` feature here?
```suggestion
uuid = "1.17.0"
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add arrow-avro support for Duration type and minor fixes for UUID decoding [arrow-rs]
Posted by "jecsand838 (via GitHub)" <gi...@apache.org>.
jecsand838 commented on code in PR #7889:
URL: https://github.com/apache/arrow-rs/pull/7889#discussion_r2198812060
##########
arrow-avro/src/codec.rs:
##########
@@ -201,7 +201,7 @@ pub enum Codec {
/// - `scale` (`Option<usize>`): Number of fractional digits.
/// - `fixed_size` (`Option<usize>`): Size in bytes if backed by a `fixed` type, otherwise `None`.
Decimal(usize, Option<usize>, Option<usize>),
- /// Represents Avro Uuid type, a FixedSizeBinary with a length of 16
+ /// Represents Avro Uuid type, a logicalType of String data represented as UTF-8 encoded bytes.
Review Comment:
@mbrobbel I just pushed up those `UUID` changes. Let me know what you think and if you identify any remaining gaps. Thank you again for calling this out.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add arrow-avro support for Duration type and minor fixes for UUID decoding [arrow-rs]
Posted by "jecsand838 (via GitHub)" <gi...@apache.org>.
jecsand838 commented on code in PR #7889:
URL: https://github.com/apache/arrow-rs/pull/7889#discussion_r2198650005
##########
arrow-avro/src/codec.rs:
##########
@@ -201,7 +201,7 @@ pub enum Codec {
/// - `scale` (`Option<usize>`): Number of fractional digits.
/// - `fixed_size` (`Option<usize>`): Size in bytes if backed by a `fixed` type, otherwise `None`.
Decimal(usize, Option<usize>, Option<usize>),
- /// Represents Avro Uuid type, a FixedSizeBinary with a length of 16
+ /// Represents Avro Uuid type, a logicalType of String data represented as UTF-8 encoded bytes.
Review Comment:
@mbrobbel Thank you for correcting me here! I'll change it back to being a `FixedSizeBinary` with a length of 16 then. Since I didn't see a `DataType::Uuid` in `arrow-schema/src/datatype.rs` and was unfamiliar with Arrow Uuids, I made an incorrect assumption. I apologize for not catching that in the arrow documentation.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add arrow-avro support for Duration type and minor fixes for UUID decoding [arrow-rs]
Posted by "mbrobbel (via GitHub)" <gi...@apache.org>.
mbrobbel commented on code in PR #7889:
URL: https://github.com/apache/arrow-rs/pull/7889#discussion_r2200283854
##########
arrow-avro/src/codec.rs:
##########
@@ -61,9 +61,25 @@ impl AvroDataType {
}
/// Returns an arrow [`Field`] with the given name
+ #[cfg(feature = "canonical_extension_types")]
pub fn field_with_name(&self, name: &str) -> Field {
- let d = self.codec.data_type();
- Field::new(name, d, self.nullability.is_some()).with_metadata(self.metadata.clone())
+ let nullable = self.nullability.is_some();
+ let data_type = self.codec.data_type();
+ let field = match self.codec {
+ Codec::Uuid => Field::new(name, data_type, nullable)
+ .with_extension_type(arrow_schema::extension::Uuid),
+ _ => Field::new(name, data_type, nullable),
+ };
+ field.with_metadata(self.metadata.clone())
+ }
+
+ /// Returns an arrow [`Field`] with the given name
+ #[cfg(not(feature = "canonical_extension_types"))]
Review Comment:
Nit: instead of duplicating the method (and overlapping logic) you could also create a separate method and call that only if the feature is enabled:
```rust
#[cfg(feature = "canonical_extension_types")]
fn with_extension_type(codec: &Codec, field: Field) -> Field {
match codec {
Codec::Uuid => Field::new(name, data_type, nullable)
.with_extension_type(arrow_schema::extension::Uuid),
_ => field,
}
}
```
```rust
pub fn field_with_name(&self, name: &str) -> Field {
let field = ...;
#[cfg(feature = "canonical_extension_types")]
with_extension_type(&self.codec, field)
#[cfg(not(feature = "canonical_extension_types"))]
field
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add arrow-avro support for Duration type and minor fixes for UUID decoding [arrow-rs]
Posted by "jecsand838 (via GitHub)" <gi...@apache.org>.
jecsand838 commented on PR #7889:
URL: https://github.com/apache/arrow-rs/pull/7889#issuecomment-3063264632
> Thanks @jecsand838. I think it would be good to get at least one more review, because I'm not familiar with this crate.
@mbrobbel Thank you for the solid review and great suggestions.
@alamb @scovich Would either of you be able to provide the additional review(s) if you get a chance?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add arrow-avro support for Duration type and minor fixes for UUID decoding [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #7889:
URL: https://github.com/apache/arrow-rs/pull/7889
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add arrow-avro support for Duration type and minor fixes for UUID decoding [arrow-rs]
Posted by "jecsand838 (via GitHub)" <gi...@apache.org>.
jecsand838 commented on code in PR #7889:
URL: https://github.com/apache/arrow-rs/pull/7889#discussion_r2200256247
##########
arrow-avro/src/codec.rs:
##########
@@ -201,7 +201,7 @@ pub enum Codec {
/// - `scale` (`Option<usize>`): Number of fractional digits.
/// - `fixed_size` (`Option<usize>`): Size in bytes if backed by a `fixed` type, otherwise `None`.
Decimal(usize, Option<usize>, Option<usize>),
- /// Represents Avro Uuid type, a FixedSizeBinary with a length of 16
+ /// Represents Avro Uuid type, a FixedSizeBinary with a length of 16.
Review Comment:
I went ahead and made this change. Let me know what you think.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add arrow-avro support for Duration type and minor fixes for UUID decoding [arrow-rs]
Posted by "jecsand838 (via GitHub)" <gi...@apache.org>.
jecsand838 commented on code in PR #7889:
URL: https://github.com/apache/arrow-rs/pull/7889#discussion_r2198161005
##########
arrow-avro/Cargo.toml:
##########
@@ -56,6 +56,7 @@ rand = { version = "0.9", default-features = false, features = ["std", "std_rng"
criterion = { version = "0.6.0", default-features = false }
tempfile = "3.3"
arrow = { workspace = true }
+uuid = { version = "1.17.0", features = ["v4"] }
Review Comment:
ty!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add arrow-avro support for Duration type and minor fixes for UUID decoding [arrow-rs]
Posted by "mbrobbel (via GitHub)" <gi...@apache.org>.
mbrobbel commented on code in PR #7889:
URL: https://github.com/apache/arrow-rs/pull/7889#discussion_r2199862261
##########
arrow-avro/src/reader/record.rs:
##########
@@ -273,6 +276,9 @@ impl Decoder {
Self::Binary(offsets, _) | Self::String(offsets, _) | Self::StringView(offsets, _) => {
offsets.push_length(0);
}
+ Self::Uuid(v) => {
+ v.push(0);
Review Comment:
I think we should push 16 zero bytes here:
```suggestion
v.extend([0; 16]);
```
##########
arrow-avro/src/reader/record.rs:
##########
@@ -611,6 +619,46 @@ fn sign_extend_to<const N: usize>(raw: &[u8]) -> Result<[u8; N], ArrowError> {
Ok(arr)
}
+#[inline]
+fn hex_char_to_u8(c: u8) -> Result<u8, ArrowError> {
+ match c {
+ b'0'..=b'9' => Ok(c - b'0'),
+ b'a'..=b'f' => Ok(c - b'a' + 10),
+ b'A'..=b'F' => Ok(c - b'A' + 10),
+ _ => Err(ArrowError::ParseError(format!(
+ "Invalid hex character '{c}' in UUID string",
+ ))),
+ }
+}
+
+#[inline]
+fn parse_uuid_bytes(s_bytes: &[u8]) -> Result<[u8; 16], ArrowError> {
Review Comment:
I would suggest using https://docs.rs/uuid/latest/uuid/struct.Uuid.html#method.try_parse
##########
arrow-avro/src/codec.rs:
##########
@@ -201,7 +201,7 @@ pub enum Codec {
/// - `scale` (`Option<usize>`): Number of fractional digits.
/// - `fixed_size` (`Option<usize>`): Size in bytes if backed by a `fixed` type, otherwise `None`.
Decimal(usize, Option<usize>, Option<usize>),
- /// Represents Avro Uuid type, a FixedSizeBinary with a length of 16
+ /// Represents Avro Uuid type, a FixedSizeBinary with a length of 16.
Review Comment:
Maybe you can change `AvroDataType::field_with_name` to match on the `Codec` and for `Codec::Uuid` add the [`Uuid`](https://docs.rs/arrow-schema/latest/arrow_schema/extension/struct.Uuid.html) canonical extension type via [`Field::with_extension_type`](https://docs.rs/arrow-schema/latest/arrow_schema/struct.Field.html#method.with_extension_type)?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add arrow-avro support for Duration type and minor fixes for UUID decoding [arrow-rs]
Posted by "jecsand838 (via GitHub)" <gi...@apache.org>.
jecsand838 commented on code in PR #7889:
URL: https://github.com/apache/arrow-rs/pull/7889#discussion_r2200253393
##########
arrow-avro/src/reader/record.rs:
##########
@@ -611,6 +619,46 @@ fn sign_extend_to<const N: usize>(raw: &[u8]) -> Result<[u8; N], ArrowError> {
Ok(arr)
}
+#[inline]
+fn hex_char_to_u8(c: u8) -> Result<u8, ArrowError> {
+ match c {
+ b'0'..=b'9' => Ok(c - b'0'),
+ b'a'..=b'f' => Ok(c - b'a' + 10),
+ b'A'..=b'F' => Ok(c - b'A' + 10),
+ _ => Err(ArrowError::ParseError(format!(
+ "Invalid hex character '{c}' in UUID string",
+ ))),
+ }
+}
+
+#[inline]
+fn parse_uuid_bytes(s_bytes: &[u8]) -> Result<[u8; 16], ArrowError> {
Review Comment:
I got that pushed up.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add arrow-avro support for Duration type and minor fixes for UUID decoding [arrow-rs]
Posted by "jecsand838 (via GitHub)" <gi...@apache.org>.
jecsand838 commented on code in PR #7889:
URL: https://github.com/apache/arrow-rs/pull/7889#discussion_r2200978216
##########
arrow-avro/src/codec.rs:
##########
@@ -61,9 +61,25 @@ impl AvroDataType {
}
/// Returns an arrow [`Field`] with the given name
+ #[cfg(feature = "canonical_extension_types")]
pub fn field_with_name(&self, name: &str) -> Field {
- let d = self.codec.data_type();
- Field::new(name, d, self.nullability.is_some()).with_metadata(self.metadata.clone())
+ let nullable = self.nullability.is_some();
+ let data_type = self.codec.data_type();
+ let field = match self.codec {
+ Codec::Uuid => Field::new(name, data_type, nullable)
+ .with_extension_type(arrow_schema::extension::Uuid),
+ _ => Field::new(name, data_type, nullable),
+ };
+ field.with_metadata(self.metadata.clone())
+ }
+
+ /// Returns an arrow [`Field`] with the given name
+ #[cfg(not(feature = "canonical_extension_types"))]
Review Comment:
That's a much cleaner approach. I went ahead and pushed those changes as well.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add arrow-avro support for Duration type and minor fixes for UUID decoding [arrow-rs]
Posted by "mbrobbel (via GitHub)" <gi...@apache.org>.
mbrobbel commented on code in PR #7889:
URL: https://github.com/apache/arrow-rs/pull/7889#discussion_r2198542115
##########
arrow-avro/src/codec.rs:
##########
@@ -201,7 +201,7 @@ pub enum Codec {
/// - `scale` (`Option<usize>`): Number of fractional digits.
/// - `fixed_size` (`Option<usize>`): Size in bytes if backed by a `fixed` type, otherwise `None`.
Decimal(usize, Option<usize>, Option<usize>),
- /// Represents Avro Uuid type, a FixedSizeBinary with a length of 16
+ /// Represents Avro Uuid type, a logicalType of String data represented as UTF-8 encoded bytes.
Review Comment:
I would expect Avro Uuids to map to Arrow Uuids (https://arrow.apache.org/docs/format/CanonicalExtensions.html#uuid)?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org