anoopj commented on code in PR #2869:
URL: https://github.com/apache/iceberg-rust/pull/2869#discussion_r3627000149
##########
crates/iceberg/src/scan/cache.rs:
##########
@@ -69,21 +69,33 @@ impl PartitionFilterCache {
format!("Could not find partition spec for id {spec_id}"),
))?;
- let partition_type = partition_spec.partition_type(schema)?;
- let partition_fields = partition_type.fields().to_owned();
- let partition_schema = Arc::new(
- Schema::builder()
- .with_schema_id(partition_spec.spec_id())
- .with_fields(partition_fields)
- .build()?,
- );
-
- let mut inclusive_projection =
InclusiveProjection::new(partition_spec.clone());
-
- let partition_filter = inclusive_projection
- .project(&filter)?
- .rewrite_not()
- .bind(partition_schema.clone(), case_sensitive)?;
+ // The partition type may fail to resolve against the schema. The
known case is a
+ // historical spec whose source column was dropped, which is a
legitimate v2+ state.
+ // Any resolution failure falls back to an always-true filter: files
under the spec
+ // are not partition-pruned but still receive the row filter. The
fallback is cached
+ // by spec id like any other filter.
+ // TODO(https://github.com/apache/iceberg-rust/issues/2844): derive
partition types from
+ // transforms where possible to restore pruning on a historical spec's
still-live fields,
+ // and narrow this to the dropped-column case once a distinguishable
error exists.
+ let partition_filter = match partition_spec.partition_type(schema) {
Review Comment:
sounds good. Keeping spec-level stopgap for this PR. Updated the TODO to
name the per-term parity
##########
crates/iceberg/src/arrow/record_batch_transformer.rs:
##########
@@ -1484,6 +1484,83 @@ mod test {
assert_eq!(get_string_value(result.column(2).as_ref(), 1), "Bob");
}
+ /// Test that a dropped identity partition source column is skipped rather
than erroring.
+ ///
+ /// A historical spec may use `identity(col)` where `col` was later
dropped from the
+ /// schema. `constants_map()` must skip that field instead of failing, so
the file can
+ /// still be read (the dropped column is not projected).
+ #[test]
+ fn dropped_identity_partition_source_column_is_skipped() {
+ use crate::spec::{Struct, Transform};
Review Comment:
Added dropped_partition_source_column_falls_back_to_always_true in cache.rs.
It builds a cache and calls get() with a spec whose source column was dropped
and assert we get `AlwaysTrue` and make sure if it's cached
##########
crates/iceberg/src/scan/cache.rs:
##########
@@ -69,21 +69,33 @@ impl PartitionFilterCache {
format!("Could not find partition spec for id {spec_id}"),
))?;
- let partition_type = partition_spec.partition_type(schema)?;
- let partition_fields = partition_type.fields().to_owned();
- let partition_schema = Arc::new(
- Schema::builder()
- .with_schema_id(partition_spec.spec_id())
- .with_fields(partition_fields)
- .build()?,
- );
-
- let mut inclusive_projection =
InclusiveProjection::new(partition_spec.clone());
-
- let partition_filter = inclusive_projection
- .project(&filter)?
- .rewrite_not()
- .bind(partition_schema.clone(), case_sensitive)?;
+ // The partition type may fail to resolve against the schema. The
known case is a
+ // historical spec whose source column was dropped, which is a
legitimate v2+ state.
+ // Any resolution failure falls back to an always-true filter: files
under the spec
+ // are not partition-pruned but still receive the row filter. The
fallback is cached
+ // by spec id like any other filter.
+ // TODO(https://github.com/apache/iceberg-rust/issues/2844): derive
partition types from
+ // transforms where possible to restore pruning on a historical spec's
still-live fields,
+ // and narrow this to the dropped-column case once a distinguishable
error exists.
+ let partition_filter = match partition_spec.partition_type(schema) {
+ Ok(partition_type) => {
+ let partition_fields = partition_type.fields().to_owned();
+ let partition_schema = Arc::new(
+ Schema::builder()
+ .with_schema_id(partition_spec.spec_id())
+ .with_fields(partition_fields)
+ .build()?,
+ );
+
+ let mut inclusive_projection =
InclusiveProjection::new(partition_spec.clone());
+
+ inclusive_projection
+ .project(&filter)?
+ .rewrite_not()
+ .bind(partition_schema.clone(), case_sensitive)?
+ }
+ Err(_) => BoundPredicate::AlwaysTrue,
Review Comment:
I think that was a good call. I have narrowed it. Also added a warn with
spec_id. I initially thought about a warn, but refrained because in my
experience warn is almost always a copout. ;)
##########
crates/iceberg/src/scan/context.rs:
##########
@@ -175,23 +175,9 @@ impl PlanContext {
.await
}
- /// Returns the partition filter for a manifest, or an always-true filter
when the
- /// manifest's spec cannot be resolved against the snapshot schema. Files
of such
- /// manifests are not partition-pruned but still receive the row filter.
fn get_partition_filter(&self, manifest_file: &ManifestFile) ->
Result<Arc<BoundPredicate>> {
Review Comment:
Done.
##########
crates/iceberg/src/scan/cache.rs:
##########
@@ -69,21 +69,33 @@ impl PartitionFilterCache {
format!("Could not find partition spec for id {spec_id}"),
))?;
- let partition_type = partition_spec.partition_type(schema)?;
- let partition_fields = partition_type.fields().to_owned();
- let partition_schema = Arc::new(
- Schema::builder()
- .with_schema_id(partition_spec.spec_id())
- .with_fields(partition_fields)
- .build()?,
- );
-
- let mut inclusive_projection =
InclusiveProjection::new(partition_spec.clone());
-
- let partition_filter = inclusive_projection
- .project(&filter)?
- .rewrite_not()
- .bind(partition_schema.clone(), case_sensitive)?;
+ // The partition type may fail to resolve against the schema. The
known case is a
+ // historical spec whose source column was dropped, which is a
legitimate v2+ state.
+ // Any resolution failure falls back to an always-true filter: files
under the spec
+ // are not partition-pruned but still receive the row filter. The
fallback is cached
+ // by spec id like any other filter.
Review Comment:
that is a good point. I incorporated most of your comment about into the
comment itself.
--
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]
