blackmwk commented on code in PR #3131:
URL: https://github.com/apache/iceberg-rust/pull/3131#discussion_r3902315720
##########
crates/iceberg/src/scan/task.rs:
##########
@@ -183,14 +231,116 @@ impl FileScanTask {
self.predicate.as_ref()
}
- /// Returns the schema of this file scan task as a reference
- pub fn schema(&self) -> &Schema {
- &self.schema
+ /// Returns the delete files that may need to be applied to the data file.
+ pub fn deletes(&self) -> &[FileScanTaskDeleteFile] {
+ &self.deletes
}
- /// Returns the schema of this file scan task as a SchemaRef
- pub fn schema_ref(&self) -> SchemaRef {
- self.schema.clone()
+ /// Returns the partition data from the manifest entry.
+ pub fn partition(&self) -> Option<&Struct> {
+ self.partition.as_ref()
+ }
+
+ /// Returns the partition spec for the data file.
+ pub fn partition_spec(&self) -> Option<&Arc<PartitionSpec>> {
+ self.partition_spec.as_ref()
+ }
+
+ /// Returns the name mapping used to resolve field ids.
+ pub fn name_mapping(&self) -> Option<&Arc<NameMapping>> {
+ self.name_mapping.as_ref()
+ }
+
+ /// Returns the unified partition type across all table partition specs.
+ pub fn unified_partition_type(&self) -> Option<&Arc<StructType>> {
+ self.unified_partition_type.as_ref()
+ }
+
+ /// Returns whether names are treated as case-sensitive.
+ pub fn case_sensitive(&self) -> bool {
+ self.case_sensitive
+ }
+
+ /// Returns the key metadata for the encrypted data file.
+ pub fn key_metadata(&self) -> Option<&[u8]> {
+ self.key_metadata.as_deref()
+ }
+
+ fn validate(&self) -> Result<()> {
+ let Some(partition) = self.partition.as_ref() else {
Review Comment:
If partition is non, then it should be an partitioned or None.
##########
crates/iceberg/src/scan/task.rs:
##########
@@ -183,14 +231,116 @@ impl FileScanTask {
self.predicate.as_ref()
}
- /// Returns the schema of this file scan task as a reference
- pub fn schema(&self) -> &Schema {
- &self.schema
+ /// Returns the delete files that may need to be applied to the data file.
+ pub fn deletes(&self) -> &[FileScanTaskDeleteFile] {
+ &self.deletes
}
- /// Returns the schema of this file scan task as a SchemaRef
- pub fn schema_ref(&self) -> SchemaRef {
- self.schema.clone()
+ /// Returns the partition data from the manifest entry.
+ pub fn partition(&self) -> Option<&Struct> {
+ self.partition.as_ref()
+ }
+
+ /// Returns the partition spec for the data file.
+ pub fn partition_spec(&self) -> Option<&Arc<PartitionSpec>> {
+ self.partition_spec.as_ref()
+ }
+
+ /// Returns the name mapping used to resolve field ids.
+ pub fn name_mapping(&self) -> Option<&Arc<NameMapping>> {
+ self.name_mapping.as_ref()
+ }
+
+ /// Returns the unified partition type across all table partition specs.
+ pub fn unified_partition_type(&self) -> Option<&Arc<StructType>> {
+ self.unified_partition_type.as_ref()
+ }
+
+ /// Returns whether names are treated as case-sensitive.
+ pub fn case_sensitive(&self) -> bool {
+ self.case_sensitive
+ }
+
+ /// Returns the key metadata for the encrypted data file.
+ pub fn key_metadata(&self) -> Option<&[u8]> {
+ self.key_metadata.as_deref()
+ }
+
+ fn validate(&self) -> Result<()> {
+ let Some(partition) = self.partition.as_ref() else {
+ return Ok(());
+ };
+ let Some(partition_spec) = self.partition_spec.as_deref() else {
Review Comment:
Similarly, you should also check partition value.
##########
crates/iceberg/src/scan/task.rs:
##########
@@ -183,14 +231,116 @@ impl FileScanTask {
self.predicate.as_ref()
}
- /// Returns the schema of this file scan task as a reference
- pub fn schema(&self) -> &Schema {
- &self.schema
+ /// Returns the delete files that may need to be applied to the data file.
+ pub fn deletes(&self) -> &[FileScanTaskDeleteFile] {
+ &self.deletes
}
- /// Returns the schema of this file scan task as a SchemaRef
- pub fn schema_ref(&self) -> SchemaRef {
- self.schema.clone()
+ /// Returns the partition data from the manifest entry.
+ pub fn partition(&self) -> Option<&Struct> {
+ self.partition.as_ref()
+ }
+
+ /// Returns the partition spec for the data file.
+ pub fn partition_spec(&self) -> Option<&Arc<PartitionSpec>> {
+ self.partition_spec.as_ref()
+ }
+
+ /// Returns the name mapping used to resolve field ids.
+ pub fn name_mapping(&self) -> Option<&Arc<NameMapping>> {
+ self.name_mapping.as_ref()
+ }
+
+ /// Returns the unified partition type across all table partition specs.
+ pub fn unified_partition_type(&self) -> Option<&Arc<StructType>> {
+ self.unified_partition_type.as_ref()
+ }
+
+ /// Returns whether names are treated as case-sensitive.
+ pub fn case_sensitive(&self) -> bool {
+ self.case_sensitive
+ }
+
+ /// Returns the key metadata for the encrypted data file.
+ pub fn key_metadata(&self) -> Option<&[u8]> {
+ self.key_metadata.as_deref()
+ }
+
+ fn validate(&self) -> Result<()> {
+ let Some(partition) = self.partition.as_ref() else {
+ return Ok(());
+ };
+ let Some(partition_spec) = self.partition_spec.as_deref() else {
+ return if partition.fields().is_empty() {
+ Ok(())
+ } else {
+ Err(Error::new(
+ ErrorKind::DataInvalid,
+ "Non-empty FileScanTask partition requires a partition
spec",
+ ))
+ };
+ };
+
+ if partition.fields().len() != partition_spec.fields().len() {
+ return Err(Error::new(
+ ErrorKind::DataInvalid,
+ format!(
+ "FileScanTask partition has {} fields but partition spec
has {} fields",
+ partition.fields().len(),
+ partition_spec.fields().len()
+ ),
+ ));
+ }
+
+ if partition_spec
+ .fields()
+ .iter()
+ .any(|field| self.schema.field_by_id(field.source_id).is_none())
+ {
+ // A historical partition spec may legally reference a source
column that has
+ // since been dropped. Without its type information, the partition
values cannot
+ // be validated, but they are still valid advisory metadata for
the scan task.
+ return Ok(());
+ }
+
+ for (partition_value, partition_field) in
Review Comment:
I don't want to do this check here, it may have performance impact, let
remove 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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
