feat: support ser/deser of value #82

ZENOTME · 2023-10-18T16:35:24Z

This PR:

modify to store i128 for Value::Decimal
It make more easy to process Value::Decimal and avoid type consisent problem if we use Decimal.
support support ser/deser of value
add related test and fix some wrong in schema

ZENOTME · 2023-10-18T16:37:24Z

cc @JanKaul @Fokko @Xuanwo @liurenjie1024

liurenjie1024

Thanks @ZENOTME Left some comments.

crates/iceberg/src/avro/schema.rs

crates/iceberg/src/spec/values.rs

liurenjie1024

Thanks, left some small comments.

crates/iceberg/src/avro/schema.rs

crates/iceberg/src/spec/values.rs

liurenjie1024 · 2023-10-19T08:03:28Z

crates/iceberg/src/spec/values.rs

+}
+
+impl Iterator for StructIntoIter {
+    type Item = (i32, Option<Literal>, String);


I think the item should be Option<Literal>. According to this discussion, we will remove struct types in Struct, and we can't return field name then.

Should it be (i32,Option<Literal>)?🤔 Seems we still need to store field id in Struct

If we remove struct types in Struct, we also have no filed_id.

crates/iceberg/src/spec/values.rs

liurenjie1024

Generally LGTM, thanks!

liurenjie1024 · 2023-10-23T06:40:51Z

crates/iceberg/src/avro/schema.rs

+                        )
+                    })?;
+                    match logical_type {
+                        "uuid" => Type::Primitive(PrimitiveType::Uuid),


Suggested change

"uuid" => Type::Primitive(PrimitiveType::Uuid),

UUID_LOGICAL_TYPE => Type::Primitive(PrimitiveType::Uuid),

liurenjie1024 · 2023-10-23T06:43:08Z

crates/iceberg/src/spec/values.rs

+}
+
+impl Iterator for StructIntoIter {
+    type Item = (i32, Option<Literal>, String);


If we remove struct types in Struct, we also have no filed_id.

liurenjie1024 · 2023-10-23T06:48:46Z

crates/iceberg/src/spec/values.rs

+    }
+
+    #[derive(Clone)]
+    struct Record {


I think Record should contains a StructType:

struct Record<'a> { r#type: &'a StructType, values: Vec<Option<RawLiteralEnum>> }

This way we can avoid copying field names every time. But we can leave it as an optimization.

liurenjie1024 · 2023-10-23T07:11:45Z

crates/iceberg/src/spec/values.rs

+                                let mut key = None;
+                                let mut value = None;
+                                required.into_iter().for_each(|(k, v)| {
+                                    if k == "key" {


Suggested change

if k == "key" {

if k == MAP_KEY_FIELD_NAME {

liurenjie1024 · 2023-10-23T07:12:03Z

crates/iceberg/src/spec/values.rs

+                                required.into_iter().for_each(|(k, v)| {
+                                    if k == "key" {
+                                        key = Some(v);
+                                    } else if k == "value" {


Suggested change

} else if k == "value" {

} else if k == MAP_VALUE_FIELD_NAME {

liurenjie1024 · 2023-10-23T07:14:09Z

crates/iceberg/src/spec/values.rs

+                                            .ok_or_else(|| invalid_err("list"))?;
+                                        let value = v.try_into(value_ty)?;
+                                        if map_ty.value_field.required && value.is_none() {
+                                            return Err(invalid_err("list"));


I think we should make the error here more clear.

crates/iceberg/src/spec/values.rs

liurenjie1024 · 2023-10-23T07:23:21Z

crates/iceberg/src/spec/values.rs

+    // - binary
+    // - fixed
+    // - decimal
+    // - uuid


I think we can still test them? They are encoded in bytes when converting from literal to raw literal?

Did you means we can test serialize?

liurenjie1024 · 2023-10-24T01:31:31Z

crates/iceberg/src/spec/values.rs

+                },
+                // # TODO
+                // rust avro don't support deserialize any bytes representation now.
+                RawLiteralEnum::Bytes(_) => Err(invalid_err_with_reason(


Sorry, I don't quite get your points, I think avro has byte type: https://docs.rs/apache-avro/0.16.0/apache_avro/types/enum.Value.html#variant.Bytes

https://github.com/apache/avro/blob/2b1955947ab446ad437f152ec2f3310ea399a015/lang/rust/avro/src/de.rs#L279 But it can't support to deserialize bytes.

And it also can't support to serialize fixed type now. https://issues.apache.org/jira/browse/AVRO-3892?filter=-2

I will try to send a PR to fix them later.

OK, I see. I'll create an issue to track this.

liurenjie1024

Others LGTM

liurenjie1024 · 2023-10-24T02:02:42Z

cc @Fokko @Xuanwo @JanKaul PTAL

Xuanwo

Mostly LGTM, only some suggestions.

Xuanwo · 2023-10-27T02:43:48Z

crates/iceberg/src/avro/schema.rs

+const UUID_BYTES: usize = 16;
+const UUID_LOGICAL_TYPE: &str = "uuid";
+// # TODO
+// This const may better to maintain in avro-rs.


We can create an issue for avro-rs and comment the issue link here.

Have added a #86 to track them

Xuanwo · 2023-10-27T02:50:05Z

crates/iceberg/src/spec/values.rs

+    }
+
+    // # TODO
+    // rust avro don't support deserialize any bytes representation now:


How about create an issue and comment the link here?

2. modify representation of Deciaml to i128

2. fix to pass test

ZENOTME · 2023-11-01T08:48:59Z

My bad, have fixed the typos now.🥵

ZENOTME · 2023-11-04T08:54:15Z

@Fokko Hi, we can run the check again

liurenjie1024 · 2023-11-08T11:26:47Z

cc @Xuanwo @Fokko Any other comments?

Fokko · 2023-11-13T08:44:25Z

crates/iceberg/src/avro/schema.rs

+        BTreeMap::from([(
+            LOGICAL_TYPE.to_string(),
+            Value::String(logical_type.to_string()),
+        )])


Just out of curiosity. Is BTreeMap the default in Rust? Trees tend to have many pointers and, therefore have faster lookups in exchange for a larger memory footprint (compared to a HashMap).

No, it's not default. For here it's just because avro::FixedSchema use BTreeMap. And indeed HashMap can save more memory. But I find that seems avro prefer to use BTreeMap. (I'm not sure that maybe they need the sorted order when iterate it the attributes in serialization?

This sometimes comes up if you want the value to be hashable. BTreeMap is hashable while HashMap is not hashable. I think it has something to do with requiring an order to be hashable.

Fokko · 2023-11-13T08:51:05Z

crates/iceberg/src/avro/schema.rs

+                    })?;
+                    match logical_type {
+                        UUID_LOGICAL_TYPE => Type::Primitive(PrimitiveType::Uuid),
+                        ty => {


For another PR, logical_type could also be a decimal: https://github.com/apache/iceberg-python/blob/main/pyiceberg/utils/schema_conversion.py#L571-L579

Thanks! We should add more checks here I think. I will do it later.

Fokko · 2023-11-13T09:13:43Z

crates/iceberg/src/spec/values.rs

+                PrimitiveLiteral::Decimal(_) => Type::Primitive(PrimitiveType::Decimal {
+                    precision: MAX_DECIMAL_PRECISION,
+                    scale: 0,


Are we ignoring the scale?

How are making PrimitiveLiteral::Decimal as (i128,i64) to store the value in the first one and the scale in the later one? 🤔

When will this be used? I think inferring type from literal is not feasible?

I think inferring type from literal is not feasible?

Sounds reasonable. In most of case we can get the type from the according schema. cc @JanKaul

Initially I thought it would be useful to directly get the type from the value. But in case of the decimal you would need to store the Decimal with the scale like @ZENOTME suggested. It might make more sense to delete this method entirely and use the schema to get the types.

Fokko · 2023-11-13T09:17:58Z

I'll move this forward since it has been pending for a while (Sorry for that!). I have one comment about ignoring the scale, which looks incorrect to me. Thanks @ZENOTME for working on this, and @liurenjie1024 and @Xuanwo for the review 🙌

ZENOTME mentioned this pull request Oct 18, 2023

feat: suport read/write Manifest #79

Merged

ZENOTME force-pushed the value_ser branch from 40faa94 to d91db5b Compare October 18, 2023 16:49

liurenjie1024 reviewed Oct 19, 2023

View reviewed changes

ZENOTME force-pushed the value_ser branch from 9377fc7 to 308dd32 Compare October 19, 2023 06:29

liurenjie1024 reviewed Oct 19, 2023

View reviewed changes

liurenjie1024 mentioned this pull request Oct 23, 2023

fix(avro-schema): fix avro map schema generation #83

Closed

liurenjie1024 reviewed Oct 23, 2023

View reviewed changes

liurenjie1024 reviewed Oct 24, 2023

View reviewed changes

liurenjie1024 mentioned this pull request Oct 24, 2023

Add test cases for literal ser/de of decimal, uuid, fixed, binary. #85

Open

liurenjie1024 approved these changes Oct 24, 2023

View reviewed changes

liurenjie1024 requested review from Fokko, Xuanwo and JanKaul October 24, 2023 02:22

ZENOTME mentioned this pull request Oct 24, 2023

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

Merged

Xuanwo reviewed Oct 27, 2023

View reviewed changes

ZENOTME added 7 commits October 27, 2023 18:44

1. add RawLiteral to support ser/deser Literal

b7635ce

2. modify representation of Deciaml to i128

fix avro type for uuid type

5802ef8

refine comment

a3d42f9

1. add convert with avro value test

f8eb9a9

2. fix to pass test

refine code

e9573b2

fix

613b409

refine code and add more test

efeb59d

ZENOTME force-pushed the value_ser branch from 82eccae to efeb59d Compare October 27, 2023 10:53

ZENOTME added 2 commits October 27, 2023 19:35

add todo link

00efe2d

fix typos

68e89c7

Xuanwo approved these changes Nov 8, 2023

View reviewed changes

Fokko approved these changes Nov 13, 2023

View reviewed changes

Fokko merged commit 28d7006 into apache:main Nov 13, 2023
6 checks passed

ZENOTME deleted the value_ser branch November 13, 2023 10:58

liurenjie1024 mentioned this pull request Nov 16, 2023

refactor: Remove datatype() method in Literal. #95

Closed

	"uuid" => Type::Primitive(PrimitiveType::Uuid),
	UUID_LOGICAL_TYPE => Type::Primitive(PrimitiveType::Uuid),

	} else if k == "value" {
	} else if k == MAP_VALUE_FIELD_NAME {

feat: support ser/deser of value #82

feat: support ser/deser of value #82

Conversation

ZENOTME commented Oct 18, 2023

ZENOTME commented Oct 18, 2023

liurenjie1024 left a comment

Choose a reason for hiding this comment

liurenjie1024 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liurenjie1024 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liurenjie1024 left a comment

Choose a reason for hiding this comment

liurenjie1024 commented Oct 24, 2023

Xuanwo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ZENOTME commented Nov 1, 2023

ZENOTME commented Nov 4, 2023

liurenjie1024 commented Nov 8, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ZENOTME Nov 13, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fokko commented Nov 13, 2023

ZENOTME Nov 13, 2023 •

edited