laskoviymishka commented on code in PR #2869:
URL: https://github.com/apache/iceberg-rust/pull/2869#discussion_r3626299779
##########
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:
the `Err(_) => AlwaysTrue` here catches every failure `partition_type()` can
return, not just the dropped-column case the comment is about. A
schema-corruption bug or a future error path would get silently rewritten to
always-true — and since we cache it by spec id, it's permanent for the rest of
the scan, so there's no retry and nothing in the logs to explain why pruning
quietly stopped.
I'd at minimum log a `tracing::warn!` with the `spec_id` and the error
before falling back. Better still, narrow the arm to the known case — either a
distinct `ErrorKind`, or an explicit `field_by_id(source_id).is_none()` check
up front — and let genuinely unexpected errors keep propagating. wdyt?
##########
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:
good regression guard — and since #2695 threads the real spec through the
scan flow, the `constants_map` path this covers is live, not hypothetical. The
gap is just that the change this PR actually makes — the `AlwaysTrue` fallback
in `PartitionFilterCache::get` (cache.rs:97) — has no direct coverage. Nothing
here builds a cache, calls `get()` with a spec whose source column was dropped,
and asserts we get `AlwaysTrue` back and that it's cached.
Could we add a test that hits the cache path directly, or an integration
test that scans a table with a predicate over a dropped partition column and
asserts `plan_files()` succeeds? That's what we'd regress if someone reverted
the `match`.
##########
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:
not blocking, and I see the TODO already flags this — just want to name the
parity gap explicitly. Falling back to `AlwaysTrue` for the whole spec is
coarser than Java/PyIceberg: they substitute `UnknownType` when a partition
source field is missing, so `partition_type()` still resolves and
`InclusiveProjection` returns always-true for only the dropped field's terms
while live fields keep pruning. A spec like `bucket(N, dropped)`,
`year(active_date)` still prunes on `active_date` in Java but loses all pruning
here.
The real fix lives one level down in `partition_type()` (substitute the
transform's fixed result type instead of erroring, which also covers the
void-transform tombstone case), and that makes the fallback per-term for free.
Fine to keep the spec-level stopgap for this PR — just worth making sure #2844
captures the per-term parity as the target.
##########
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:
small thing — the doc comment describing the always-true fallback came off
with the pre-check, so `get_partition_filter` is now undocumented and the
contract only lives in cache.rs. Could we leave a one-liner here pointing at
`PartitionFilterCache::get` for the fallback behavior?
##########
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:
one thing worth a line of comment here: caching `AlwaysTrue` keyed by
`spec_id` alone is only safe because this cache lives per-scan in `PlanContext`
with a fixed schema and predicate. If it ever got hoisted to table or catalog
scope, a spec cached as always-true couldn't be re-resolved by a later scan
that would succeed. Could we note that per-scan assumption right where we
cache, so a future refactor doesn't trip on 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]
