laskoviymishka commented on code in PR #2869:
URL: https://github.com/apache/iceberg-rust/pull/2869#discussion_r3631569514
##########
crates/iceberg/src/scan/cache.rs:
##########
@@ -69,21 +69,46 @@ 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()?,
- );
+ // A historical spec may reference a source column that was later
dropped from the
+ // schema, which is a legitimate v2+ state. Such a spec cannot be
resolved to a
+ // partition type, so it falls back to an always-true filter: files
under the spec
+ // are not partition-pruned but still receive the row filter. Any
other resolution
+ // failure is unexpected and propagates. The fallback is cached by
spec id like any
+ // other filter; this is safe only because the cache lives per-scan in
`PlanContext`
+ // with a fixed schema and predicate. Hoisting it to table or catalog
scope would
+ // pin a spec to always-true even for a later scan whose schema could
resolve it.
+ // 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,
+ // which also makes the fallback per-term (dropped fields only)
instead of per-spec.
+ let dropped_source_column = partition_spec
Review Comment:
tiny thing — `dropped_source_column` reads as singular, but it's true when
*any* field's source is dropped. `has_dropped_source_column` or
`any_source_column_dropped` matches the `.any()` a bit better. Not blocking.
##########
crates/iceberg/src/scan/cache.rs:
##########
@@ -235,3 +260,81 @@ impl ExpressionEvaluatorCache {
Ok(read.get(&spec_id).unwrap().clone())
}
}
+
+#[cfg(test)]
+mod tests {
+ use std::collections::HashMap;
+ use std::sync::Arc;
+
+ use super::PartitionFilterCache;
+ use crate::expr::{Bind, BoundPredicate, Reference};
+ use crate::spec::{
+ Datum, FormatVersion, NestedField, PrimitiveType, Schema, SortOrder,
TableMetadataBuilder,
+ Transform, Type, UnboundPartitionSpec,
+ };
+
+ /// A historical spec whose source column was dropped from the current
schema resolves to an
+ /// always-true filter, and the fallback is cached under the spec id.
+ #[test]
+ fn dropped_partition_source_column_falls_back_to_always_true() {
Review Comment:
This exercises `cache.get` directly, which is exactly right for the caching
assertion — but nothing drives the full `plan_files` → `get_partition_filter` →
`get` path that the removed context.rs pre-check used to live on.
So if the plumbing in `get_partition_filter` regressed — wrong schema
threaded through to `get`, say — this test would still pass. Could we add one
scan-level test that scans a table with a historical dropped-column spec plus a
predicate and asserts it completes and returns all rows? That's the path that
actually changed here. wdyt?
##########
crates/iceberg/src/scan/cache.rs:
##########
@@ -69,21 +69,46 @@ 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()?,
- );
+ // A historical spec may reference a source column that was later
dropped from the
+ // schema, which is a legitimate v2+ state. Such a spec cannot be
resolved to a
+ // partition type, so it falls back to an always-true filter: files
under the spec
+ // are not partition-pruned but still receive the row filter. Any
other resolution
+ // failure is unexpected and propagates. The fallback is cached by
spec id like any
+ // other filter; this is safe only because the cache lives per-scan in
`PlanContext`
+ // with a fixed schema and predicate. Hoisting it to table or catalog
scope would
+ // pin a spec to always-true even for a later scan whose schema could
resolve it.
+ // 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,
+ // which also makes the fallback per-term (dropped fields only)
instead of per-spec.
+ let dropped_source_column = partition_spec
+ .fields()
+ .iter()
+ .any(|field| schema.field_by_id(field.source_id).is_none());
- let mut inclusive_projection =
InclusiveProjection::new(partition_spec.clone());
+ let partition_filter = if dropped_source_column {
+ tracing::warn!(
Review Comment:
This fires before the write lock, so under the cache's benign double-check
race (two threads missing the same `spec_id` at once) we'd emit the warn twice
and insert AlwaysTrue twice. The insert is idempotent so it's harmless, but the
duplicate log line is a new observable side effect that wasn't there before.
I'd move the warn onto the actual insert path so it only fires when we
populate the entry. wdyt?
--
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]
