laskoviymishka commented on code in PR #2845:
URL: https://github.com/apache/iceberg-rust/pull/2845#discussion_r3623828882
##########
crates/iceberg/src/scan/context.rs:
##########
@@ -175,9 +175,23 @@ 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>> {
let partition_spec_id = manifest_file.partition_spec_id;
+ // Historical specs may reference source columns that were later
dropped from the
+ // schema, in which case the partition type cannot be resolved. A
missing spec is
+ // reported by the partition filter cache instead.
+ let resolvable = self
+ .table_metadata
+ .partition_spec_by_id(partition_spec_id)
+ .is_none_or(|spec|
spec.partition_type(&self.snapshot_schema).is_ok());
Review Comment:
Nit (non-blocking, efficiency): this resolvability check computes
`partition_type(&self.snapshot_schema)` once per manifest file, but
`PartitionFilterCache::get` already recomputes `partition_type` internally on a
cache miss. For a scan over many manifests sharing one spec, resolvable
manifests pay for `partition_type` twice on the first manifest and re-run the
check for every subsequent manifest even though the final filter is cached by
`spec_id`. Cheap enough for a stopgap, but caching resolvability by `spec_id`
(or folding the check into the cache-miss path) would avoid the O(#manifests)
extra calls.
##########
crates/iceberg/src/scan/context.rs:
##########
@@ -175,9 +175,23 @@ 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>> {
let partition_spec_id = manifest_file.partition_spec_id;
+ // Historical specs may reference source columns that were later
dropped from the
+ // schema, in which case the partition type cannot be resolved. A
missing spec is
+ // reported by the partition filter cache instead.
+ let resolvable = self
+ .table_metadata
+ .partition_spec_by_id(partition_spec_id)
+ .is_none_or(|spec|
spec.partition_type(&self.snapshot_schema).is_ok());
+ if !resolvable {
+ return Ok(Arc::new(BoundPredicate::AlwaysTrue));
Review Comment:
Nit (non-blocking): a `// TODO(#2844)` breadcrumb here would make the
stopgap nature discoverable from the code itself — the PR body notes the
longer-term fix (deriving partition types from transforms, à la
apache/iceberg#17262, which would restore pruning on a historical spec's
still-live partition fields), but that context lives only in the PR.
##########
crates/iceberg/src/arrow/record_batch_transformer.rs:
##########
@@ -59,11 +59,11 @@ fn constants_map(
for (pos, field) in partition_spec.fields().iter().enumerate() {
// Only identity transforms should use constant values from partition
metadata
if matches!(field.transform, Transform::Identity) {
- // Get the field from schema to extract its type
- let iceberg_field =
schema.field_by_id(field.source_id).ok_or(Error::new(
- ErrorKind::Unexpected,
- format!("Field {} not found in schema", field.source_id),
- ))?;
+ // The source column may have been dropped from the schema after
the spec was
+ // created. It cannot be projected in that case, so no constant is
needed.
+ let Some(iceberg_field) = schema.field_by_id(field.source_id) else
{
Review Comment:
Follow-up (non-blocking, coverage): this `constants_map` branch isn't
reached by the native scan flow today — `FileScanTask` is built with
`with_partition_spec(None)` (`scan/context.rs`, with a `TODO: Pass actual
PartitionSpec through context chain`), and `pipeline.rs` only calls
`with_partition` when the spec is `Some`. So this change is a reasonable
forward-looking guard, but it's currently unexercised (the new scan test uses
`None`). A direct unit test here — mirroring the existing `with_partition`
tests in this file — would prevent a future refactor that threads the spec
through from silently reintroducing the panic.
--
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]
