kevinjqliu commented on code in PR #1369:
URL: https://github.com/apache/iceberg-rust/pull/1369#discussion_r2105691302
##########
crates/iceberg/src/spec/manifest_list.rs:
##########
@@ -844,83 +812,10 @@ pub(super) mod _serde {
pub key_metadata: Option<ByteBuf>,
}
- #[derive(Debug, Serialize, Deserialize, PartialEq, Eq)]
- pub(super) struct FieldSummary {
- pub contains_null: bool,
- pub contains_nan: Option<bool>,
- pub lower_bound: Option<ByteBuf>,
- pub upper_bound: Option<ByteBuf>,
- }
-
- impl FieldSummary {
- /// Converts the [FieldSummary] into a [super::FieldSummary].
- /// [lower_bound] and [upper_bound] are converted into [Literal]s need
the type info so use
- /// this function instead of [std::TryFrom] trait.
- pub(crate) fn try_into(self, r#type: &PrimitiveType) ->
Result<super::FieldSummary> {
- Ok(super::FieldSummary {
- contains_null: self.contains_null,
- contains_nan: self.contains_nan,
- lower_bound: self
- .lower_bound
- .as_ref()
- .map(|v| Datum::try_from_bytes(v, r#type.clone()))
- .transpose()
- .map_err(|err| err.with_context("type", format!("{:?}",
r#type)))?,
- upper_bound: self
- .upper_bound
- .as_ref()
- .map(|v| Datum::try_from_bytes(v, r#type.clone()))
- .transpose()
- .map_err(|err| err.with_context("type", format!("{:?}",
r#type)))?,
- })
- }
- }
-
- fn try_convert_to_field_summary(
- partitions: Option<Vec<FieldSummary>>,
- partition_type: Option<&StructType>,
- ) -> Result<Vec<super::FieldSummary>> {
- if let Some(partitions) = partitions {
- if let Some(partition_type) = partition_type {
- let partition_types = partition_type.fields();
- if partitions.len() != partition_types.len() {
- return Err(Error::new(
- crate::ErrorKind::DataInvalid,
- format!(
- "Invalid partition spec. Expected {} fields, got
{}",
- partition_types.len(),
- partitions.len()
- ),
- ));
- }
- partitions
- .into_iter()
- .zip(partition_types)
- .map(|(v, field)| {
-
v.try_into(field.field_type.as_primitive_type().ok_or_else(|| {
- Error::new(
- crate::ErrorKind::DataInvalid,
- "Invalid partition spec. Field type is not
primitive",
- )
- })?)
- })
- .collect::<Result<Vec<_>>>()
- } else {
- Err(Error::new(
- crate::ErrorKind::DataInvalid,
- "Invalid partition spec. Partition type is required",
- ))
- }
- } else {
- Ok(Vec::new())
- }
- }
-
impl ManifestFileV2 {
/// Converts the [ManifestFileV2] into a [ManifestFile].
/// The convert of [partitions] need the partition_type info so use
this function instead of [std::TryFrom] trait.
Review Comment:
`partition_type` is removed
```suggestion
```
##########
crates/iceberg/src/expr/visitors/manifest_evaluator.rs:
##########
@@ -414,6 +444,11 @@ impl ManifestFilterVisitor<'_> {
};
Ok(bound)
}
+
+ fn bytes_to_datum(bytes: &ByteBuf, t: Type) -> Datum {
Review Comment:
nit should this be in `Datum` alongside `try_from_bytes`?
##########
crates/iceberg/src/spec/manifest_list.rs:
##########
@@ -577,7 +575,7 @@ pub struct ManifestFile {
/// A list of field summaries for each partition field in the spec. Each
/// field in the list corresponds to a field in the manifest file’s
/// partition spec.
- pub partitions: Vec<FieldSummary>,
+ pub partitions: Option<Vec<FieldSummary>>,
Review Comment:
i like this change, it aligns with the spec definition
--
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
