blackmwk commented on code in PR #3131:
URL: https://github.com/apache/iceberg-rust/pull/3131#discussion_r3902413749
##########
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:
Updated validation so a missing partition is accepted only when the
partition spec is absent or unpartitioned. Added regression coverage for both
partitioned and unpartitioned specs.
##########
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:
Added the symmetric check: a missing spec is accepted only for absent or
empty partition data; non-empty partition data still returns DataInvalid.
##########
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:
Removed the per-value schema, transform, and type validation loop, including
its source-column lookup. Validation now checks only presence consistency and
tuple arity.
--
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]
