liurenjie1024 commented on code in PR #82:
URL: https://github.com/apache/iceberg-rust/pull/82#discussion_r1364771184
##########
crates/iceberg/src/avro/schema.rs:
##########
@@ -162,7 +168,7 @@ impl SchemaVisitor for SchemaToAvroSchema {
let value_field = {
let mut field = AvroRecordField {
- name: map.key_field.name.clone(),
+ name: "value".to_string(),
Review Comment:
Same as above.
##########
crates/iceberg/src/spec/values.rs:
##########
@@ -594,7 +596,7 @@ impl From<&Literal> for JsonValue {
/// The partition struct stores the tuple of partition values for each file.
/// Its type is derived from the partition fields of the partition spec used
to write the manifest file.
/// In v2, the partition struct’s field ids must match the ids from the
partition spec.
-#[derive(Debug, Clone, PartialEq, Hash, Eq, PartialOrd, Ord)]
+#[derive(Default, Debug, Clone, PartialEq, Hash, Eq, PartialOrd, Ord)]
Review Comment:
Why we add `Default` here? It seems meaningless to me to have an empty
struct.
##########
crates/iceberg/src/spec/values.rs:
##########
@@ -996,10 +1008,552 @@ mod timestamptz {
}
}
+mod serde {
Review Comment:
```suggestion
mod _serde {
```
##########
crates/iceberg/src/avro/schema.rs:
##########
@@ -144,7 +150,7 @@ impl SchemaVisitor for SchemaToAvroSchema {
// not string type.
let key_field = {
let mut field = AvroRecordField {
- name: map.key_field.name.clone(),
+ name: "key".into(),
Review Comment:
Why we need to change this? I think it's already `key`?
##########
crates/iceberg/src/avro/schema.rs:
##########
@@ -96,16 +98,20 @@ impl SchemaVisitor for SchemaToAvroSchema {
_struct: &StructType,
results: Vec<AvroSchemaOrField>,
) -> Result<AvroSchemaOrField> {
- let avro_fields = results.into_iter().map(|r|
r.unwrap_right()).collect();
-
+ let avro_fields = results.into_iter().map(|r|
r.unwrap_right()).collect_vec();
+ let lookup = avro_fields
Review Comment:
This code seems duplicated several times, how about we create a
`RecordSchemaBuilder` to avoid this duplication? We can maintain it in iceberg
first, and contribute it to apache avro later.
##########
crates/iceberg/src/spec/values.rs:
##########
@@ -618,6 +620,16 @@ impl Struct {
(id, if *null { None } else { Some(value) }, name.as_str())
})
}
+
+ /// Create a iterator to comsume the field in order of (field_id,
field_value, field_name).
+ pub fn comsume_iter(self) -> impl Iterator<Item = (i32, Option<Literal>,
String)> {
Review Comment:
It's better to have make it `IntoIterator`
##########
crates/iceberg/src/avro/schema.rs:
##########
@@ -178,16 +184,23 @@ impl SchemaVisitor for SchemaToAvroSchema {
field
};
+ let fields = vec![key_field, value_field];
+ let lookup = fields
+ .iter()
+ .enumerate()
+ .map(|f| (f.1.name.clone(), f.0))
+ .collect();
+
let item_avro_schema = AvroSchema::Record(RecordSchema {
name: Name::from(format!("k{}_v{}", map.key_field.id,
map.value_field.id).as_str()),
aliases: None,
doc: None,
- fields: vec![key_field, value_field],
- lookup: Default::default(),
+ fields,
+ lookup,
attributes: Default::default(),
});
- Ok(Either::Left(item_avro_schema))
+ Ok(Either::Left(AvroSchema::Array(item_avro_schema.into())))
Review Comment:
Good catch!
##########
crates/iceberg/src/spec/values.rs:
##########
@@ -996,10 +1008,552 @@ mod timestamptz {
}
}
+mod serde {
+ use std::collections::BTreeMap;
+
+ use crate::{
+ spec::{PrimitiveType, Type},
+ Error, ErrorKind,
+ };
+
+ use super::{Literal, PrimitiveLiteral};
+ use serde::{
+ de::Visitor,
+ ser::{SerializeMap, SerializeSeq, SerializeStruct},
+ Deserialize, Serialize,
+ };
+ use serde_bytes::ByteBuf;
+ use serde_derive::Deserialize as DeserializeDerive;
+ use serde_derive::Serialize as SerializeDerive;
+
+ #[derive(SerializeDerive, DeserializeDerive)]
+ #[serde(transparent)]
+ /// Raw literal representation used for serde. The serialize way is used
for Avro serializer.
+ pub struct RawLiteral(RawLiteralEnum);
+
+ impl RawLiteral {
+ /// Covert literal to raw literal.
+ pub fn try_from(literal: Literal, ty: &Type) -> Result<Self, Error> {
+ Ok(Self(RawLiteralEnum::try_from(literal, ty)?))
+ }
+
+ /// Convert raw literal to literal.
+ pub fn try_into(self, ty: &Type) -> Result<Option<Literal>, Error> {
+ self.0.try_into(ty)
+ }
+ }
+
+ #[derive(SerializeDerive, Clone)]
+ #[serde(untagged)]
+ enum RawLiteralEnum {
+ Null,
+ Boolean(bool),
+ Int(i32),
+ Long(i64),
+ Float(f32),
+ Double(f64),
+ String(String),
+ Bytes(ByteBuf),
+ List(List),
+ StringMap(StringMap),
+ Record(Record),
+ }
+
+ #[derive(Clone)]
+ struct Record {
+ required: Vec<(String, RawLiteralEnum)>,
+ optional: Vec<(String, Option<RawLiteralEnum>)>,
Review Comment:
Why we need to distinguish `required` or `optional` here? I think we should
check them when converting from `Literal` to `RawLiteralEnum`? And in `Record`
we only need to simply keep `Vec<(String, Option<RawLiteralEnum)>`?
--
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: issues-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
