liurenjie1024 commented on code in PR #401:
URL: https://github.com/apache/iceberg-rust/pull/401#discussion_r1640919368
##########
Cargo.toml:
##########
@@ -65,8 +65,8 @@ log = "^0.4"
mockito = "^1"
murmur3 = "0.5.2"
once_cell = "1"
-opendal = "0.46"
-ordered-float = "4.0.0"
+opendal = { version = "0.46", features = ["services-fs"] }
Review Comment:
This is unnecessary, please merge main to solve this.
##########
crates/iceberg/src/arrow/reader.rs:
##########
@@ -74,38 +70,22 @@ impl ArrowReaderBuilder {
self
}
- /// Sets the desired column projection with a list of field ids.
- pub fn with_field_ids(mut self, field_ids: impl IntoIterator<Item =
usize>) -> Self {
- self.field_ids = field_ids.into_iter().collect();
- self
- }
-
- /// Sets the predicates to apply to the scan.
- pub fn with_predicates(mut self, predicates: BoundPredicate) -> Self {
- self.predicates = Some(predicates);
- self
- }
-
/// Build the ArrowReader.
pub fn build(self) -> ArrowReader {
ArrowReader {
batch_size: self.batch_size,
- field_ids: self.field_ids,
- schema: self.schema,
file_io: self.file_io,
- predicates: self.predicates,
+ metadata: self.table_metadata,
}
}
}
/// Reads data from Parquet files
+#[derive(Clone)]
pub struct ArrowReader {
batch_size: Option<usize>,
- field_ids: Vec<usize>,
- #[allow(dead_code)]
- schema: SchemaRef,
file_io: FileIO,
- predicates: Option<BoundPredicate>,
+ metadata: TableMetadataRef,
Review Comment:
I think this is unnecessary? For a file scan task, they should use same
schema?
##########
crates/iceberg/src/spec/values.rs:
##########
@@ -50,7 +51,7 @@ use super::datatypes::{PrimitiveType, Type};
const MAX_TIME_VALUE: i64 = 24 * 60 * 60 * 1_000_000i64 - 1;
/// Values present in iceberg type
-#[derive(Clone, Debug, PartialEq, Hash, Eq)]
+#[derive(Clone, Debug, PartialEq, Hash, Eq, Serialize, Deserialize)]
Review Comment:
We should not use derive here, we already have ser/de, see:
https://github.com/apache/iceberg-rust/blob/806840729c1dd886624f6ce3cb132d0912befb4b/crates/iceberg/src/spec/manifest.rs#L1325
##########
crates/iceberg/src/expr/predicate.rs:
##########
@@ -879,12 +905,19 @@ mod tests {
)
}
+ fn test_bound_predicate_serialize_diserialize(bound_predicate:
BoundPredicate) {
Review Comment:
Seems we don't have tests for logical expressions?
##########
crates/iceberg/src/arrow/reader.rs:
##########
Review Comment:
This maybe unrelated, but maybe it's since it's refine the scan interface. I
think we should make `ArrowReaderBuilder` crate private since it's dangerous to
allow user to construct it.
--
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
