AVRO-3892: [Rust] Support to resolve fixed from bytes and deserialize bytes in deserialize_any #2567

ZENOTME · 2023-10-24T16:17:09Z

What is the purpose of the change

(For example: This pull request improves file read performance by buffering data, fixing AVRO-XXXX.)

fixing AVRO-3892

This PR:

support to resolve fixed from bytes
support to deserialize bytes, fixed, decimal in deserialize_any

Verifying this change

For resolving fixed from bytes, I have add related test.
For deserializing bytes, fixed, decimal in deserialize_any, seems it don't have test for deserialize_any. If it required, we can consider to add test for whole deserialize_any. And actually I have test it in feat: support ser/deser of value iceberg-rust#82 locally.

Documentation

Does this pull request introduce a new feature? (yes / no)
If yes, how is the feature documented? (not applicable / docs / JavaDocs / not documented)

martin-g · 2023-10-25T06:47:34Z

lang/rust/avro/src/de.rs

@@ -350,6 +354,7 @@ impl<'a, 'de> de::Deserializer<'de> for &'a Deserializer<'de> {
            Value::String(ref s) => visitor.visit_bytes(s.as_bytes()),
            Value::Bytes(ref bytes) | Value::Fixed(_, ref bytes) => visitor.visit_bytes(bytes),
            Value::Uuid(ref u) => visitor.visit_bytes(u.as_bytes()),
+            Value::Decimal(ref d) => visitor.visit_bytes(&d.to_vec()?),


It'd be nice to add new unit tests for the changes above.

Thanks for review! I have added the test for it.

Signed-off-by: Martin Tzvetanov Grigorov <mgrigorov@apache.org>

martin-g · 2023-10-25T07:12:36Z

lang/rust/avro/src/de.rs

@@ -350,6 +354,7 @@ impl<'a, 'de> de::Deserializer<'de> for &'a Deserializer<'de> {
            Value::String(ref s) => visitor.visit_bytes(s.as_bytes()),
            Value::Bytes(ref bytes) | Value::Fixed(_, ref bytes) => visitor.visit_bytes(bytes),
            Value::Uuid(ref u) => visitor.visit_bytes(u.as_bytes()),
+            Value::Decimal(ref d) => visitor.visit_bytes(&d.to_vec()?),
            _ => Err(de::Error::custom(format!(
                "Expected a String|Bytes|Fixed|Uuid, but got {:?}",


Suggested change

"Expected a String|Bytes|Fixed|Uuid, but got {:?}",

"Expected a String|Bytes|Fixed|Uuid|Decimal, but got {:?}",

…alue::Bytes The tests are not really related to AVRO-3892. They do not cover the new changes in deserialize_any() Signed-off-by: Martin Tzvetanov Grigorov <mgrigorov@apache.org>

ZENOTME · 2023-10-25T15:08:14Z

lang/rust/avro/src/de.rs

+    #[test]
+    fn test_avro_3892_deserialize_bytes_from_uuid() -> TestResult {
+        let uuid_str = "10101010-2020-2020-2020-101010101010";
+        let expected_bytes = Uuid::parse_str(uuid_str)?.as_bytes().to_vec();


One thing may need to notice: I find that the behaviour of deserialize_str and deserialize_bytes for Value::Uuid is different. We can see that in deserialize_bytes for Value::Uuid, the bytes representation is Uuid::parse_str(uuid_str)?.as_bytes() rathern than uuid_str.as_bytes(). But for deserialize_str, the result will be uuid_str.as_bytes() I think. Is this behaviour is expect? cc @martin-g

Dunno!
I have to check what Uuid::as_bytes() does. I'll check tomorrow!
But feel free to propose a PR with a fix if you think it is wrong!

martin-g · 2023-10-25T17:12:49Z

lang/rust/avro/src/de.rs

+
+    #[test]
+    fn test_avro_3892_deserialize_bytes_from_decimal() -> TestResult {
+        let expeced_bytes = BigInt::from(123456789).to_signed_bytes_be();


Suggested change

let expeced_bytes = BigInt::from(123456789).to_signed_bytes_be();

let expected_bytes = BigInt::from(123456789).to_signed_bytes_be();

lang/rust/avro/src/de.rs

martin-g · 2023-10-25T17:15:51Z

lang/rust/avro/src/de.rs

+    #[test]
+    fn test_avro_3892_deserialize_bytes_from_uuid() -> TestResult {
+        let uuid_str = "10101010-2020-2020-2020-101010101010";
+        let expected_bytes = Uuid::parse_str(uuid_str)?.as_bytes().to_vec();


Dunno!
I have to check what Uuid::as_bytes() does. I'll check tomorrow!
But feel free to propose a PR with a fix if you think it is wrong!

martin-g · 2023-10-26T19:47:35Z

Thank you, @ZENOTME !

… bytes in deserialize_any (#2567) * support to resolve fixed from bytes * support to deserialize bytes, fixed, decimal. * fix clippy * AVRO-3892: Rename test method Signed-off-by: Martin Tzvetanov Grigorov <mgrigorov@apache.org> * AVRO-3892: [Rust] Add unit tests for deserializing &str/String from Value::Bytes The tests are not really related to AVRO-3892. They do not cover the new changes in deserialize_any() Signed-off-by: Martin Tzvetanov Grigorov <mgrigorov@apache.org> * add unit test for deserialize bytes from decimal and uuid * add more test --------- Signed-off-by: Martin Tzvetanov Grigorov <mgrigorov@apache.org> Co-authored-by: ZENOTME <st810918843@gmail.com> Co-authored-by: Martin Tzvetanov Grigorov <mgrigorov@apache.org> (cherry picked from commit 8073145)

… bytes in deserialize_any (apache#2567) * support to resolve fixed from bytes * support to deserialize bytes, fixed, decimal. * fix clippy * AVRO-3892: Rename test method Signed-off-by: Martin Tzvetanov Grigorov <mgrigorov@apache.org> * AVRO-3892: [Rust] Add unit tests for deserializing &str/String from Value::Bytes The tests are not really related to AVRO-3892. They do not cover the new changes in deserialize_any() Signed-off-by: Martin Tzvetanov Grigorov <mgrigorov@apache.org> * add unit test for deserialize bytes from decimal and uuid * add more test --------- Signed-off-by: Martin Tzvetanov Grigorov <mgrigorov@apache.org> Co-authored-by: ZENOTME <st810918843@gmail.com> Co-authored-by: Martin Tzvetanov Grigorov <mgrigorov@apache.org>

ZENOTME added 2 commits October 24, 2023 23:43

support to resolve fixed from bytes

8b331fd

support to deserialize bytes, fixed, decimal.

80283f4

github-actions bot added the Rust label Oct 24, 2023

fix clippy

c8ad266

martin-g requested changes Oct 25, 2023

View reviewed changes

AVRO-3892: Rename test method

61eeaa9

Signed-off-by: Martin Tzvetanov Grigorov <mgrigorov@apache.org>

martin-g reviewed Oct 25, 2023

View reviewed changes

martin-g and others added 2 commits October 25, 2023 10:16

AVRO-3892: [Rust] Add unit tests for deserializing &str/String from V…

4d93020

…alue::Bytes The tests are not really related to AVRO-3892. They do not cover the new changes in deserialize_any() Signed-off-by: Martin Tzvetanov Grigorov <mgrigorov@apache.org>

add unit test for deserialize bytes from decimal and uuid

40bc2d7

ZENOTME commented Oct 25, 2023

View reviewed changes

martin-g reviewed Oct 25, 2023

View reviewed changes

add more test

1fb832e

martin-g approved these changes Oct 26, 2023

View reviewed changes

martin-g merged commit 8073145 into apache:main Oct 26, 2023
15 checks passed

ZENOTME deleted the bytes branch October 27, 2023 01:10

ZENOTME mentioned this pull request Oct 27, 2023

Tracking: ser/se for bytes-like type apache/iceberg-rust#86

Open

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

AVRO-3892: [Rust] Support to resolve fixed from bytes and deserialize bytes in deserialize_any #2567

AVRO-3892: [Rust] Support to resolve fixed from bytes and deserialize bytes in deserialize_any #2567

ZENOTME commented Oct 24, 2023

martin-g Oct 25, 2023

ZENOTME Oct 25, 2023

martin-g Oct 25, 2023

ZENOTME Oct 25, 2023

martin-g Oct 25, 2023

martin-g Oct 25, 2023

martin-g Oct 25, 2023

martin-g commented Oct 26, 2023

	"Expected a String\|Bytes\|Fixed\|Uuid, but got {:?}",
	"Expected a String\|Bytes\|Fixed\|Uuid\|Decimal, but got {:?}",

	let expeced_bytes = BigInt::from(123456789).to_signed_bytes_be();
	let expected_bytes = BigInt::from(123456789).to_signed_bytes_be();

AVRO-3892: [Rust] Support to resolve fixed from bytes and deserialize bytes in deserialize_any #2567

AVRO-3892: [Rust] Support to resolve fixed from bytes and deserialize bytes in deserialize_any #2567

Conversation

ZENOTME commented Oct 24, 2023

What is the purpose of the change

Verifying this change

Documentation

martin-g Oct 25, 2023

Choose a reason for hiding this comment

ZENOTME Oct 25, 2023

Choose a reason for hiding this comment

martin-g Oct 25, 2023

Choose a reason for hiding this comment

ZENOTME Oct 25, 2023

Choose a reason for hiding this comment

martin-g Oct 25, 2023

Choose a reason for hiding this comment

martin-g Oct 25, 2023

Choose a reason for hiding this comment

martin-g Oct 25, 2023

Choose a reason for hiding this comment

martin-g commented Oct 26, 2023