From da00da2b6a9c1869a6e57e63c905a927903b78a5 Mon Sep 17 00:00:00 2001 From: Xander Date: Fri, 14 Aug 2026 13:47:02 +0200 Subject: [PATCH 1/9] feat(scan): incremental append scan Add IncrementalAppendScanBuilder to read files appended between two snapshots. Rows written under an older schema in the range are projected onto the table's current schema (newer columns become NULL), matching the Java and PyIceberg implementations. Refactors the shared table-scan build logic into build_table_scan / ScanConfig so it can be reused by both the standard and incremental scan builders. --- crates/iceberg/public-api.txt | 18 + crates/iceberg/src/scan/context.rs | 35 +- crates/iceberg/src/scan/incremental.rs | 912 ++++++++++++++++++ crates/iceberg/src/scan/mod.rs | 519 +++++++--- crates/iceberg/src/table.rs | 46 +- ...xample_table_metadata_v2_deep_history.json | 12 +- ...e_metadata_v2_deep_history_compaction.json | 104 ++ ...metadata_v2_deep_history_stale_schema.json | 105 ++ 8 files changed, 1610 insertions(+), 141 deletions(-) create mode 100644 crates/iceberg/src/scan/incremental.rs create mode 100644 crates/iceberg/testdata/example_table_metadata_v2_deep_history_compaction.json create mode 100644 crates/iceberg/testdata/example_table_metadata_v2_deep_history_stale_schema.json diff --git a/crates/iceberg/public-api.txt b/crates/iceberg/public-api.txt index 24e276db56..2afa28b673 100644 --- a/crates/iceberg/public-api.txt +++ b/crates/iceberg/public-api.txt @@ -1328,6 +1328,20 @@ impl serde_core::ser::Serialize for iceberg::scan::FileScanTaskDeleteFile pub fn iceberg::scan::FileScanTaskDeleteFile::serialize<__S>(&self, __serializer: __S) -> core::result::Result<<__S as serde_core::ser::Serializer>::Ok, <__S as serde_core::ser::Serializer>::Error> where __S: serde_core::ser::Serializer impl<'de> serde_core::de::Deserialize<'de> for iceberg::scan::FileScanTaskDeleteFile pub fn iceberg::scan::FileScanTaskDeleteFile::deserialize<__D>(__deserializer: __D) -> core::result::Result::Error> where __D: serde_core::de::Deserializer<'de> +pub struct iceberg::scan::IncrementalAppendScanBuilder<'a> +impl<'a> iceberg::scan::IncrementalAppendScanBuilder<'a> +pub fn iceberg::scan::IncrementalAppendScanBuilder<'a>::build(self) -> iceberg::Result +pub fn iceberg::scan::IncrementalAppendScanBuilder<'a>::select(self, column_names: impl core::iter::traits::collect::IntoIterator) -> Self +pub fn iceberg::scan::IncrementalAppendScanBuilder<'a>::select_all(self) -> Self +pub fn iceberg::scan::IncrementalAppendScanBuilder<'a>::select_empty(self) -> Self +pub fn iceberg::scan::IncrementalAppendScanBuilder<'a>::with_batch_size(self, batch_size: core::option::Option) -> Self +pub fn iceberg::scan::IncrementalAppendScanBuilder<'a>::with_case_sensitive(self, case_sensitive: bool) -> Self +pub fn iceberg::scan::IncrementalAppendScanBuilder<'a>::with_concurrency_limit(self, limit: usize) -> Self +pub fn iceberg::scan::IncrementalAppendScanBuilder<'a>::with_data_file_concurrency_limit(self, limit: usize) -> Self +pub fn iceberg::scan::IncrementalAppendScanBuilder<'a>::with_filter(self, predicate: iceberg::expr::Predicate) -> Self +pub fn iceberg::scan::IncrementalAppendScanBuilder<'a>::with_manifest_entry_concurrency_limit(self, limit: usize) -> Self +pub fn iceberg::scan::IncrementalAppendScanBuilder<'a>::with_row_group_filtering_enabled(self, row_group_filtering_enabled: bool) -> Self +pub fn iceberg::scan::IncrementalAppendScanBuilder<'a>::with_row_selection_enabled(self, row_selection_enabled: bool) -> Self pub struct iceberg::scan::ScanMetrics impl iceberg::scan::ScanMetrics pub fn iceberg::scan::ScanMetrics::bytes_read(&self) -> u64 @@ -3132,6 +3146,8 @@ pub struct iceberg::table::StaticTable(_) impl iceberg::table::StaticTable pub async fn iceberg::table::StaticTable::from_metadata(metadata: iceberg::spec::TableMetadata, table_ident: iceberg::TableIdent, file_io: iceberg::io::FileIO) -> iceberg::Result pub async fn iceberg::table::StaticTable::from_metadata_file(metadata_location: &str, table_ident: iceberg::TableIdent, file_io: iceberg::io::FileIO) -> iceberg::Result +pub fn iceberg::table::StaticTable::incremental_append_scan(&self, from_snapshot_id: i64, to_snapshot_id: core::option::Option) -> iceberg::scan::IncrementalAppendScanBuilder<'_> +pub fn iceberg::table::StaticTable::incremental_append_scan_inclusive(&self, from_snapshot_id: i64, to_snapshot_id: core::option::Option) -> iceberg::scan::IncrementalAppendScanBuilder<'_> pub fn iceberg::table::StaticTable::into_table(self) -> iceberg::table::Table pub fn iceberg::table::StaticTable::metadata(&self) -> iceberg::spec::TableMetadataRef pub fn iceberg::table::StaticTable::reader_builder(&self) -> iceberg::arrow::ArrowReaderBuilder @@ -3147,6 +3163,8 @@ pub fn iceberg::table::Table::current_schema_ref(&self) -> iceberg::spec::Schema pub fn iceberg::table::Table::encryption_manager(&self) -> core::option::Option<&iceberg::encryption::EncryptionManager> pub fn iceberg::table::Table::file_io(&self) -> &iceberg::io::FileIO pub fn iceberg::table::Table::identifier(&self) -> &iceberg::TableIdent +pub fn iceberg::table::Table::incremental_append_scan(&self, from_snapshot_id: i64, to_snapshot_id: core::option::Option) -> iceberg::scan::IncrementalAppendScanBuilder<'_> +pub fn iceberg::table::Table::incremental_append_scan_inclusive(&self, from_snapshot_id: i64, to_snapshot_id: core::option::Option) -> iceberg::scan::IncrementalAppendScanBuilder<'_> pub fn iceberg::table::Table::inspect(&self) -> iceberg::inspect::MetadataTable<'_> pub fn iceberg::table::Table::manifest_list_reader(&self, snapshot: &iceberg::spec::SnapshotRef) -> iceberg::spec::ManifestListReader pub fn iceberg::table::Table::metadata(&self) -> &iceberg::spec::TableMetadata diff --git a/crates/iceberg/src/scan/context.rs b/crates/iceberg/src/scan/context.rs index 67b8cde4d8..b1d62c599b 100644 --- a/crates/iceberg/src/scan/context.rs +++ b/crates/iceberg/src/scan/context.rs @@ -33,6 +33,14 @@ use crate::spec::{ }; use crate::{Error, ErrorKind, Result}; +/// Filter applied to each [`ManifestFile`] before fetching it. +/// Returns `true` to include the manifest, `false` to skip it. +pub(crate) type ManifestFileFilter = Arc bool + Send + Sync>; + +/// Filter applied to each manifest entry after loading a manifest. +/// Returns `true` to include the entry, `false` to skip it. +pub(crate) type ManifestEntryFilter = Arc bool + Send + Sync>; + /// Wraps a [`ManifestFile`] alongside the objects that are needed /// to process it in a thread-safe manner pub(crate) struct ManifestFileContext { @@ -48,6 +56,7 @@ pub(crate) struct ManifestFileContext { delete_file_index: DeleteFileIndex, name_mapping: Option>, case_sensitive: bool, + entry_filter: Option, partition_spec: Option, unified_partition_type: Option>, } @@ -84,6 +93,7 @@ impl ManifestFileContext { delete_file_index, name_mapping, case_sensitive, + entry_filter, partition_spec, unified_partition_type, } = self; @@ -91,6 +101,12 @@ impl ManifestFileContext { let manifest = object_cache.get_manifest(&manifest_file).await?; for manifest_entry in manifest.entries() { + if let Some(ref filter) = entry_filter + && !filter(manifest_entry) + { + continue; + } + let manifest_entry_context = ManifestEntryContext { // TODO: refactor to avoid the expensive ManifestEntry clone manifest_entry: manifest_entry.clone(), @@ -156,7 +172,6 @@ impl ManifestEntryContext { /// PlanContext wraps a [`SnapshotRef`] alongside all the other /// objects that are required to perform a scan file plan. -#[derive(Debug)] pub(crate) struct PlanContext { pub snapshot: SnapshotRef, @@ -172,10 +187,21 @@ pub(crate) struct PlanContext { pub partition_filter_cache: Arc, pub manifest_evaluator_cache: Arc, pub expression_evaluator_cache: Arc, + pub manifest_file_filter: Option, + pub manifest_entry_filter: Option, pub unified_partition_type: Option>, } +impl std::fmt::Debug for PlanContext { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.debug_struct("PlanContext") + .field("snapshot", &self.snapshot) + .field("case_sensitive", &self.case_sensitive) + .finish_non_exhaustive() + } +} + impl PlanContext { pub(crate) async fn get_manifest_list(&self) -> Result> { self.object_cache @@ -229,6 +255,12 @@ impl PlanContext { // TODO: Ideally we could ditch this intermediate Vec as we return an iterator. let mut filtered_mfcs = vec![]; for manifest_file in manifest_files { + if let Some(ref filter) = self.manifest_file_filter + && !filter(manifest_file) + { + continue; + } + let tx = if manifest_file.content == ManifestContentType::Deletes { delete_file_tx.clone() } else { @@ -299,6 +331,7 @@ impl PlanContext { delete_file_index, name_mapping: self.name_mapping.clone(), case_sensitive: self.case_sensitive, + entry_filter: self.manifest_entry_filter.clone(), partition_spec: self .table_metadata .partition_spec_by_id(manifest_file.partition_spec_id) diff --git a/crates/iceberg/src/scan/incremental.rs b/crates/iceberg/src/scan/incremental.rs new file mode 100644 index 0000000000..d6d77bd184 --- /dev/null +++ b/crates/iceberg/src/scan/incremental.rs @@ -0,0 +1,912 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +//! Incremental append scan for reading only newly added data between snapshots. + +use std::collections::HashSet; +use std::sync::Arc; + +use crate::expr::Predicate; +use crate::scan::context::{ManifestEntryFilter, ManifestFileFilter}; +use crate::scan::{ScanConfig, TableScan, build_table_scan}; +use crate::spec::{ManifestContentType, ManifestStatus, Operation, TableMetadataRef}; +use crate::table::Table; +use crate::util::available_parallelism; +use crate::util::snapshot::ancestors_between; +use crate::{Error, ErrorKind, Result}; + +/// Represents a validated range of snapshots for incremental scanning. +/// +/// This struct is used to track which snapshot IDs are included in an incremental +/// scan range, allowing efficient filtering of manifest entries. +#[derive(Debug, Clone)] +pub(crate) struct AppendSnapshotSet { + /// Snapshot IDs in the range + snapshot_ids: HashSet, +} + +impl AppendSnapshotSet { + /// Build a snapshot range by walking the snapshot ancestry chain. + /// + /// Validates that `from_snapshot_id` is an ancestor of `to_snapshot_id` and + /// collects all snapshot IDs in between. Also validates that all snapshots + /// in the range have APPEND operations. + /// + /// # Arguments + /// * `table_metadata` - The table metadata containing snapshot information + /// * `from_snapshot_id` - The starting snapshot ID + /// * `to_snapshot_id` - The ending snapshot ID + /// * `from_inclusive` - Whether to include the from_snapshot in the range + pub(crate) fn build( + table_metadata: &TableMetadataRef, + from_snapshot_id: i64, + to_snapshot_id: i64, + from_inclusive: bool, + ) -> Result { + // Determine the exclusive stop point for the ancestry walk. + // For inclusive mode the from-snapshot must exist so we can look up + // its parent. For exclusive mode the snapshot may have been expired + // (the parent pointer on its child still references it), so we only + // need the ID — matching Java's BaseIncrementalScan semantics. + let oldest_exclusive = if from_inclusive { + let from_snapshot = + table_metadata + .snapshot_by_id(from_snapshot_id) + .ok_or_else(|| { + Error::new( + ErrorKind::DataInvalid, + format!("Snapshot {from_snapshot_id} not found"), + ) + })?; + from_snapshot.parent_snapshot_id() + } else { + Some(from_snapshot_id) + }; + + let snapshots: Vec<_> = + ancestors_between(table_metadata, to_snapshot_id, oldest_exclusive).collect(); + + // ancestors_between silently returns the full chain to root if + // oldest_exclusive isn't in the ancestry chain. Detect this: + // if we got snapshots but from_snapshot_id wasn't encountered as + // the stop point, the chain doesn't connect. + if from_snapshot_id == to_snapshot_id { + // Edge case: from == to. In exclusive mode, range is empty. + // In inclusive mode, we should have exactly one snapshot. + if !from_inclusive { + return Ok(Self { + snapshot_ids: HashSet::new(), + }); + } + } else if snapshots.is_empty() { + // to_snapshot_id doesn't exist + return Err(Error::new( + ErrorKind::DataInvalid, + format!( + "from_snapshot {from_snapshot_id} is not an ancestor of to_snapshot {to_snapshot_id}", + ), + )); + } else { + // Verify the oldest snapshot in our walk is actually connected + // to from_snapshot_id. The last snapshot's parent (for exclusive) + // or the last snapshot itself (for inclusive) should be from_snapshot_id. + let oldest_collected = snapshots.last().unwrap(); + let connects = if from_inclusive { + oldest_collected.snapshot_id() == from_snapshot_id + } else { + oldest_collected.parent_snapshot_id() == Some(from_snapshot_id) + }; + if !connects { + return Err(Error::new( + ErrorKind::DataInvalid, + format!( + "from_snapshot {from_snapshot_id} is not an ancestor of to_snapshot {to_snapshot_id}", + ), + )); + } + } + + // Collect only APPEND snapshot IDs, silently skipping non-APPEND + // snapshots (e.g. replace/compaction, overwrite, delete). This matches + // the Java BaseIncrementalAppendScan behavior — only append operations + // contribute new data files to an incremental append scan. + let mut snapshot_ids = HashSet::with_capacity(snapshots.len()); + for snapshot in &snapshots { + if snapshot.summary().operation == Operation::Append { + snapshot_ids.insert(snapshot.snapshot_id()); + } + } + + Ok(Self { snapshot_ids }) + } + + /// Check if a snapshot_id is within this set + pub(crate) fn contains(&self, snapshot_id: i64) -> bool { + self.snapshot_ids.contains(&snapshot_id) + } + + /// Create a manifest file filter that skips delete manifests and data + /// manifests whose `added_snapshot_id` is outside this set. + pub(crate) fn manifest_file_filter(self: &Arc) -> ManifestFileFilter { + let set = self.clone(); + Arc::new(move |manifest_file| { + manifest_file.content != ManifestContentType::Deletes + && set.contains(manifest_file.added_snapshot_id) + }) + } + + /// Create a manifest entry filter that includes only entries with + /// status ADDED and a snapshot_id within this set. + pub(crate) fn manifest_entry_filter(self: &Arc) -> ManifestEntryFilter { + let set = self.clone(); + Arc::new(move |entry| { + entry.status() == ManifestStatus::Added + && entry.snapshot_id().is_some_and(|id| set.contains(id)) + }) + } +} + +/// Builder to create an incremental append scan between two snapshots. +/// +/// An incremental append scan returns only data files that were added in +/// snapshots between `from_snapshot_id` and the target snapshot. Only +/// snapshots with APPEND operations are supported. +/// +/// This is **not** a CDC, net-changes, or changelog scan: non-append +/// snapshots in the range (overwrite, replace/compaction, delete) are +/// ignored rather than applied as net changes. The scan reads only the rows +/// added by append snapshots in the range, so its output does not represent +/// the full table state at `to_snapshot_id`, nor does it reflect rows deleted +/// or rewritten within the range. In particular, files produced by compaction +/// (`replace`) are skipped, so appended rows are never double-counted against +/// their rewritten copies. +/// +/// Use [`Table::incremental_append_scan`] or +/// [`Table::incremental_append_scan_inclusive`] to create an instance. +pub struct IncrementalAppendScanBuilder<'a> { + table: &'a Table, + from_snapshot_id: i64, + from_inclusive: bool, + to_snapshot_id: Option, + column_names: Option>, + batch_size: Option, + case_sensitive: bool, + filter: Option, + concurrency_limit_data_files: usize, + concurrency_limit_manifest_entries: usize, + concurrency_limit_manifest_files: usize, + row_group_filtering_enabled: bool, + row_selection_enabled: bool, +} + +impl<'a> IncrementalAppendScanBuilder<'a> { + pub(crate) fn new( + table: &'a Table, + from_snapshot_id: i64, + to_snapshot_id: Option, + from_inclusive: bool, + ) -> Self { + let num_cpus = available_parallelism().get(); + + Self { + table, + from_snapshot_id, + from_inclusive, + to_snapshot_id, + column_names: None, + batch_size: None, + case_sensitive: true, + filter: None, + concurrency_limit_data_files: num_cpus, + concurrency_limit_manifest_entries: num_cpus, + concurrency_limit_manifest_files: num_cpus, + row_group_filtering_enabled: true, + row_selection_enabled: false, + } + } + + /// Sets the desired size of batches in the response + /// to something other than the default + pub fn with_batch_size(mut self, batch_size: Option) -> Self { + self.batch_size = batch_size; + self + } + + /// Sets the scan's case sensitivity + pub fn with_case_sensitive(mut self, case_sensitive: bool) -> Self { + self.case_sensitive = case_sensitive; + self + } + + /// Specifies a predicate to use as a filter + pub fn with_filter(mut self, predicate: Predicate) -> Self { + self.filter = Some(predicate.rewrite_not()); + self + } + + /// Select all columns. + pub fn select_all(mut self) -> Self { + self.column_names = None; + self + } + + /// Select empty columns. + pub fn select_empty(mut self) -> Self { + self.column_names = Some(vec![]); + self + } + + /// Select some columns of the table. + pub fn select(mut self, column_names: impl IntoIterator) -> Self { + self.column_names = Some( + column_names + .into_iter() + .map(|item| item.to_string()) + .collect(), + ); + self + } + + /// Sets the concurrency limit for manifest files, manifest entries, and + /// data files for this scan + pub fn with_concurrency_limit(mut self, limit: usize) -> Self { + self.concurrency_limit_manifest_files = limit; + self.concurrency_limit_manifest_entries = limit; + self.concurrency_limit_data_files = limit; + self + } + + /// Sets the data file concurrency limit for this scan + pub fn with_data_file_concurrency_limit(mut self, limit: usize) -> Self { + self.concurrency_limit_data_files = limit; + self + } + + /// Sets the manifest entry concurrency limit for this scan + pub fn with_manifest_entry_concurrency_limit(mut self, limit: usize) -> Self { + self.concurrency_limit_manifest_entries = limit; + self + } + + /// Determines whether to enable row group filtering. + pub fn with_row_group_filtering_enabled(mut self, row_group_filtering_enabled: bool) -> Self { + self.row_group_filtering_enabled = row_group_filtering_enabled; + self + } + + /// Determines whether to enable row selection. + pub fn with_row_selection_enabled(mut self, row_selection_enabled: bool) -> Self { + self.row_selection_enabled = row_selection_enabled; + self + } + + /// Build the incremental append scan. + pub fn build(self) -> Result { + let to_snapshot = match self.to_snapshot_id { + Some(snapshot_id) => self + .table + .metadata() + .snapshot_by_id(snapshot_id) + .ok_or_else(|| { + Error::new( + ErrorKind::DataInvalid, + format!("to_snapshot with id {snapshot_id} not found"), + ) + })? + .clone(), + None => { + let Some(current_snapshot) = self.table.metadata().current_snapshot() else { + return Err(Error::new( + ErrorKind::DataInvalid, + "Cannot perform incremental scan: table has no snapshots", + )); + }; + current_snapshot.clone() + } + }; + + let append_set = Arc::new(AppendSnapshotSet::build( + &self.table.metadata_ref(), + self.from_snapshot_id, + to_snapshot.snapshot_id(), + self.from_inclusive, + )?); + + build_table_scan( + ScanConfig { + table: self.table, + column_names: self.column_names, + batch_size: self.batch_size, + case_sensitive: self.case_sensitive, + filter: self.filter, + concurrency_limit_data_files: self.concurrency_limit_data_files, + concurrency_limit_manifest_entries: self.concurrency_limit_manifest_entries, + concurrency_limit_manifest_files: self.concurrency_limit_manifest_files, + row_group_filtering_enabled: self.row_group_filtering_enabled, + row_selection_enabled: self.row_selection_enabled, + // Project onto the table's current schema (not the to-snapshot's + // schema), matching the Java and PyIceberg implementations. Rows + // written under an older schema within the range are read against + // the current schema, so newer columns become `NULL`. + schema: self.table.metadata().current_schema().clone(), + }, + to_snapshot, + Some(append_set.manifest_file_filter()), + Some(append_set.manifest_entry_filter()), + ) + } +} + +#[cfg(test)] +mod tests { + use futures::TryStreamExt; + + use super::AppendSnapshotSet; + use crate::scan::tests::TableTestFixture; + + #[test] + fn test_incremental_scan_invalid_from_snapshot_exclusive() { + let table = TableTestFixture::new().table; + + // Exclusive mode doesn't require from-snapshot to exist, but it must + // be an ancestor of the to-snapshot. 999999999 is not in the ancestry + // chain so this should fail. + let result = table.incremental_append_scan(999999999, None).build(); + + assert!(result.is_err()); + let err = result.unwrap_err(); + assert!( + err.to_string().contains("not an ancestor"), + "Expected ancestry error, got: {err}" + ); + } + + #[test] + fn test_incremental_scan_invalid_from_snapshot_inclusive() { + let table = TableTestFixture::new().table; + + // Inclusive mode requires from-snapshot to exist (we need its parent ID). + let result = table + .incremental_append_scan_inclusive(999999999, None) + .build(); + + assert!(result.is_err()); + let err = result.unwrap_err(); + assert!( + err.to_string().contains("not found"), + "Expected 'not found' error, got: {err}" + ); + } + + #[test] + fn test_incremental_scan_exclusive_from_expired_snapshot() { + // Fixture has S1 (append) -> S2 (append, current). + // Simulate S1 being expired: use S1's ID as from-snapshot in exclusive + // mode even though it wouldn't exist in metadata after expiration. + // Since exclusive mode only needs the ID (not the snapshot object), + // this should succeed — the child (S2) still has parent_snapshot_id = S1. + let table = TableTestFixture::new().table; + + let s1_id = 3051729675574597004_i64; + let s2_id = 3055729675574597004_i64; + + // Verify S2's parent is S1 (simulating the expired-parent scenario) + assert_eq!( + table + .metadata() + .snapshot_by_id(s2_id) + .unwrap() + .parent_snapshot_id(), + Some(s1_id) + ); + + let result = table.incremental_append_scan(s1_id, Some(s2_id)).build(); + + assert!( + result.is_ok(), + "Exclusive scan from an (effectively expired) parent should succeed" + ); + } + + #[test] + fn test_incremental_scan_invalid_to_snapshot() { + let table = TableTestFixture::new().table; + + let result = table + .incremental_append_scan(3051729675574597004, Some(999999999)) + .build(); + + assert!(result.is_err()); + let err = result.unwrap_err(); + assert!(err.to_string().contains("not found")); + } + + #[test] + fn test_incremental_scan_appends_after() { + // Fixture has S1 (append) -> S2 (append, current) + let table = TableTestFixture::new().table; + + let result = table + .incremental_append_scan(3051729675574597004, None) + .build(); + assert!( + result.is_ok(), + "appends_after should succeed when all snapshots are appends" + ); + + let scan = result.unwrap(); + assert!( + scan.plan_context.is_some(), + "Incremental scan should have a plan context" + ); + } + + #[test] + fn test_incremental_scan_appends_between() { + // Fixture has S1 (append) -> S2 (append, current) + let table = TableTestFixture::new().table; + + let current_snapshot_id = table.metadata().current_snapshot().unwrap().snapshot_id(); + let parent_id = table + .metadata() + .current_snapshot() + .unwrap() + .parent_snapshot_id() + .expect("Current snapshot should have a parent"); + + let result = table + .incremental_append_scan(parent_id, Some(current_snapshot_id)) + .build(); + + assert!( + result.is_ok(), + "appends_between should succeed for two append snapshots" + ); + } + + #[test] + fn test_incremental_scan_from_snapshot_inclusive() { + // Fixture has S1 (append) -> S2 (append, current) + let table = TableTestFixture::new().table; + let current_snapshot_id = table.metadata().current_snapshot().unwrap().snapshot_id(); + + // Verify the scan builds successfully + let result = table + .incremental_append_scan_inclusive(current_snapshot_id, Some(current_snapshot_id)) + .build(); + assert!( + result.is_ok(), + "Inclusive scan of a single append snapshot should succeed" + ); + + // Verify AppendSnapshotSet directly + let set = AppendSnapshotSet::build( + &table.metadata_ref(), + current_snapshot_id, + current_snapshot_id, + true, + ) + .unwrap(); + assert!( + set.contains(current_snapshot_id), + "Inclusive set should contain the from_snapshot" + ); + } + + #[test] + fn test_incremental_scan_from_snapshot_exclusive() { + // Fixture has S1 (append) -> S2 (append, current) + let table = TableTestFixture::new().table; + let current_snapshot_id = table.metadata().current_snapshot().unwrap().snapshot_id(); + + // Verify the scan builds successfully + let result = table + .incremental_append_scan(current_snapshot_id, Some(current_snapshot_id)) + .build(); + assert!( + result.is_ok(), + "Exclusive scan from=to should succeed with empty range" + ); + + // Verify AppendSnapshotSet directly + let set = AppendSnapshotSet::build( + &table.metadata_ref(), + current_snapshot_id, + current_snapshot_id, + false, + ) + .unwrap(); + assert!( + !set.contains(current_snapshot_id), + "Exclusive set should not contain the from_snapshot" + ); + } + + #[test] + fn test_incremental_scan_skips_non_append_operations() { + // Deep history fixture: S1 (append) -> S2 (append) -> S3 (append) + // -> S4 (overwrite) -> S5 (append, current) + let table = TableTestFixture::new_with_deep_history().table; + + // Scanning from S1 to S5 crosses S4 (overwrite) — should succeed + // but only include APPEND snapshots (S2, S3, S5), skipping S4 + let result = table + .incremental_append_scan(3051729675574597004, Some(3059729675574597004)) + .build(); + + assert!( + result.is_ok(), + "Should succeed, skipping non-APPEND snapshots" + ); + + let set = AppendSnapshotSet::build( + &table.metadata_ref(), + 3051729675574597004, + 3059729675574597004, + false, + ) + .unwrap(); + assert!( + !set.contains(3051729675574597004), + "S1 (from) should be excluded" + ); + assert!( + set.contains(3055729675574597004), + "S2 (append) should be in set" + ); + assert!( + set.contains(3056729675574597004), + "S3 (append) should be in set" + ); + assert!( + !set.contains(3057729675574597004), + "S4 (overwrite) should be skipped" + ); + assert!( + set.contains(3059729675574597004), + "S5 (append) should be in set" + ); + } + + #[test] + fn test_incremental_scan_append_only_range() { + // Deep history fixture: S1 (append) -> S2 (append) -> S3 (append) + // -> S4 (overwrite) -> S5 (append, current) + let table = TableTestFixture::new_with_deep_history().table; + + // Scanning from S1 to S3 (all appends) + let set = AppendSnapshotSet::build( + &table.metadata_ref(), + 3051729675574597004, + 3056729675574597004, + false, + ) + .unwrap(); + assert!( + !set.contains(3051729675574597004), + "from_snapshot should be excluded" + ); + assert!(set.contains(3055729675574597004), "S2 should be in range"); + assert!(set.contains(3056729675574597004), "S3 should be in range"); + } + + #[tokio::test] + async fn test_incremental_scan_returns_only_added_files_in_range() { + // Fixture has S1 (append) -> S2 (append, current) + // Manifest contains: + // 1.parquet: status=Added, snapshot=S2 + // 2.parquet: status=Deleted, snapshot=S1 + // 3.parquet: status=Existing, snapshot=S1 + let mut fixture = TableTestFixture::new(); + fixture.setup_manifest_files().await; + + let current_snapshot = fixture.table.metadata().current_snapshot().unwrap(); + let parent_snapshot_id = current_snapshot.parent_snapshot_id().unwrap(); + + // Incremental scan from S1 (exclusive) to S2 should return only 1.parquet + let table_scan = fixture + .table + .incremental_append_scan(parent_snapshot_id, Some(current_snapshot.snapshot_id())) + .build() + .unwrap(); + + let tasks: Vec<_> = table_scan + .plan_files() + .await + .unwrap() + .try_collect() + .await + .unwrap(); + + assert_eq!( + tasks.len(), + 1, + "Incremental scan should return exactly 1 file" + ); + assert_eq!( + tasks[0].data_file_path, + format!("{}/1.parquet", &fixture.table_location), + "Should only return the file added in S2" + ); + } + + #[tokio::test] + async fn test_incremental_scan_exclusive_same_snapshot_returns_empty() { + // Fixture has S1 (append) -> S2 (append, current) + let mut fixture = TableTestFixture::new(); + fixture.setup_manifest_files().await; + + let current_snapshot_id = fixture + .table + .metadata() + .current_snapshot() + .unwrap() + .snapshot_id(); + + // Incremental scan from S2 to S2 (exclusive) should return nothing + let table_scan = fixture + .table + .incremental_append_scan(current_snapshot_id, Some(current_snapshot_id)) + .build() + .unwrap(); + + let tasks: Vec<_> = table_scan + .plan_files() + .await + .unwrap() + .try_collect() + .await + .unwrap(); + + assert!( + tasks.is_empty(), + "Exclusive scan from=to should return no files" + ); + } + + #[tokio::test] + async fn test_incremental_scan_compaction_not_double_counted() { + // Compaction (`rewrite_data_files`) commits a `replace` snapshot whose + // rewritten file re-adds rows that were already appended earlier. An + // incremental append scan must skip that file so the appended rows are + // read exactly once — never double-counted against the rewritten copy. + // + // Deep history fixture with S4 relabeled as a `replace` (compaction): + // S1 (append) -> S2 (append) -> S3 (append) -> S4 (replace) -> S5 (append, current) + // Scanning from S1 (exclusive) to S5 should return only files from + // APPEND snapshots: s2.parquet, s3.parquet, s5.parquet. The compacted + // s4.parquet must be skipped. + let mut fixture = TableTestFixture::new_with_deep_history_compaction(); + fixture.setup_manifest_files_deep_history().await; + + let s1_id = 3051729675574597004_i64; + let s5_id = 3059729675574597004_i64; + + let table_scan = fixture + .table + .incremental_append_scan(s1_id, Some(s5_id)) + .build() + .unwrap(); + + let mut tasks: Vec<_> = table_scan + .plan_files() + .await + .unwrap() + .try_collect() + .await + .unwrap(); + + tasks.sort_by(|a, b| a.data_file_path.cmp(&b.data_file_path)); + + let file_names: Vec<&str> = tasks + .iter() + .map(|t| { + t.data_file_path + .rsplit('/') + .next() + .unwrap_or(&t.data_file_path) + }) + .collect(); + + assert_eq!( + file_names, + vec!["s2.parquet", "s3.parquet", "s5.parquet"], + "Compacted file (s4, from the replace snapshot) must be skipped" + ); + } + + #[tokio::test] + async fn test_incremental_scan_deep_history_skips_overwrite_files() { + // Deep history fixture: + // S1 (append) -> S2 (append) -> S3 (append) -> S4 (overwrite) -> S5 (append, current) + // Each snapshot adds one file: s1.parquet .. s5.parquet + // + // Incremental scan from S1 (exclusive) to S5 should return only files + // from APPEND snapshots: s2.parquet, s3.parquet, s5.parquet + // s4.parquet (added in overwrite S4) must be skipped. + let mut fixture = TableTestFixture::new_with_deep_history(); + fixture.setup_manifest_files_deep_history().await; + + let s1_id = 3051729675574597004_i64; + let s5_id = 3059729675574597004_i64; + + let table_scan = fixture + .table + .incremental_append_scan(s1_id, Some(s5_id)) + .build() + .unwrap(); + + let mut tasks: Vec<_> = table_scan + .plan_files() + .await + .unwrap() + .try_collect() + .await + .unwrap(); + + // Sort by path for deterministic assertions + tasks.sort_by(|a, b| a.data_file_path.cmp(&b.data_file_path)); + + assert_eq!( + tasks.len(), + 3, + "Should return 3 files (s2, s3, s5), skipping s4 (overwrite)" + ); + + let file_names: Vec<&str> = tasks + .iter() + .map(|t| { + t.data_file_path + .rsplit('/') + .next() + .unwrap_or(&t.data_file_path) + }) + .collect(); + + assert_eq!( + file_names, + vec!["s2.parquet", "s3.parquet", "s5.parquet"], + "Only files from APPEND snapshots should be returned" + ); + } + + #[tokio::test] + async fn test_incremental_scan_deep_history_partial_range() { + // Scan from S2 (exclusive) to S3 — both appends, should return only s3.parquet + let mut fixture = TableTestFixture::new_with_deep_history(); + fixture.setup_manifest_files_deep_history().await; + + let s2_id = 3055729675574597004_i64; + let s3_id = 3056729675574597004_i64; + + let table_scan = fixture + .table + .incremental_append_scan(s2_id, Some(s3_id)) + .build() + .unwrap(); + + let tasks: Vec<_> = table_scan + .plan_files() + .await + .unwrap() + .try_collect() + .await + .unwrap(); + + assert_eq!(tasks.len(), 1, "Should return exactly 1 file"); + assert!( + tasks[0].data_file_path.ends_with("s3.parquet"), + "Should return s3.parquet, got: {}", + tasks[0].data_file_path + ); + } + + #[test] + fn test_incremental_scan_projects_onto_current_schema() { + // The table's current schema (id 1) has three columns (x, y, z), but + // every snapshot references the older schema (id 0) with a single + // column (x). An incremental scan must project onto the *current* + // schema, matching the Java and PyIceberg implementations, so rows + // written under the older schema get NULLs for the newer columns. + let table = TableTestFixture::new_with_deep_history_stale_schema().table; + + let s1_id = 3051729675574597004_i64; + let s5_id = 3059729675574597004_i64; + + let scan = table + .incremental_append_scan(s1_id, Some(s5_id)) + .build() + .unwrap(); + + let plan_context = scan + .plan_context + .as_ref() + .expect("incremental scan should have a plan context"); + + // The scan must use the current schema (3 columns), not the + // to-snapshot's schema (1 column). + let current_schema = table.metadata().current_schema(); + assert_eq!( + plan_context.snapshot_schema.schema_id(), + current_schema.schema_id(), + "incremental scan should project onto the current schema" + ); + assert_eq!( + plan_context.snapshot_schema.as_struct().fields().len(), + 3, + "current schema has three columns (x, y, z)" + ); + + // Sanity check: the to-snapshot itself references the older schema. + let to_snapshot = table.metadata().snapshot_by_id(s5_id).unwrap(); + assert_eq!( + to_snapshot.schema(table.metadata()).unwrap().schema_id(), + 0, + "to-snapshot should reference the older single-column schema" + ); + } + + #[tokio::test] + async fn test_incremental_scan_deep_history_inclusive_with_overwrite() { + // Inclusive scan from S3 to S5: + // S3 (append) -> S4 (overwrite) -> S5 (append) + // Should return s3.parquet and s5.parquet, skipping s4.parquet + let mut fixture = TableTestFixture::new_with_deep_history(); + fixture.setup_manifest_files_deep_history().await; + + let s3_id = 3056729675574597004_i64; + let s5_id = 3059729675574597004_i64; + + let table_scan = fixture + .table + .incremental_append_scan_inclusive(s3_id, Some(s5_id)) + .build() + .unwrap(); + + let mut tasks: Vec<_> = table_scan + .plan_files() + .await + .unwrap() + .try_collect() + .await + .unwrap(); + + tasks.sort_by(|a, b| a.data_file_path.cmp(&b.data_file_path)); + + assert_eq!( + tasks.len(), + 2, + "Should return 2 files (s3, s5), skipping s4 (overwrite)" + ); + + let file_names: Vec<&str> = tasks + .iter() + .map(|t| { + t.data_file_path + .rsplit('/') + .next() + .unwrap_or(&t.data_file_path) + }) + .collect(); + + assert_eq!( + file_names, + vec!["s3.parquet", "s5.parquet"], + "Only files from APPEND snapshots should be returned" + ); + } +} diff --git a/crates/iceberg/src/scan/mod.rs b/crates/iceberg/src/scan/mod.rs index b7c7254ae2..144136a418 100644 --- a/crates/iceberg/src/scan/mod.rs +++ b/crates/iceberg/src/scan/mod.rs @@ -21,6 +21,7 @@ mod cache; use cache::*; mod context; use context::*; +mod incremental; mod task; use std::sync::Arc; @@ -29,6 +30,7 @@ use arrow_array::RecordBatch; use futures::channel::mpsc::{Sender, channel}; use futures::stream::BoxStream; use futures::{SinkExt, StreamExt, TryStreamExt}; +pub use incremental::IncrementalAppendScanBuilder; pub use task::*; use crate::arrow::ArrowReaderBuilder; @@ -42,7 +44,9 @@ use crate::metadata_columns::{ }; use crate::partitioning::compute_unified_partition_type; use crate::runtime::Runtime; -use crate::spec::{DEFAULT_SCHEMA_NAME_MAPPING, DataContentType, NameMapping, SnapshotRef}; +use crate::spec::{ + DEFAULT_SCHEMA_NAME_MAPPING, DataContentType, NameMapping, SchemaRef, SnapshotRef, +}; use crate::table::Table; use crate::util::available_parallelism; use crate::{Error, ErrorKind, Result}; @@ -50,6 +54,162 @@ use crate::{Error, ErrorKind, Result}; /// A stream of arrow [`RecordBatch`]es. pub type ArrowRecordBatchStream = BoxStream<'static, Result>; +/// Shared configuration extracted from scan builders, used by both +/// [`TableScanBuilder`] and [`IncrementalAppendScanBuilder`]. +pub(crate) struct ScanConfig<'a> { + table: &'a Table, + column_names: Option>, + batch_size: Option, + case_sensitive: bool, + filter: Option, + concurrency_limit_data_files: usize, + concurrency_limit_manifest_entries: usize, + concurrency_limit_manifest_files: usize, + row_group_filtering_enabled: bool, + row_selection_enabled: bool, + /// Schema to project the scan onto. A standard scan passes the snapshot's + /// own schema (the correct behavior for time-travel scans). An incremental + /// scan passes the table's current schema so that rows written under an + /// older schema in the range are projected onto it (newer columns become + /// `NULL`), matching the Java and PyIceberg implementations. + schema: SchemaRef, +} + +/// Shared build logic: validates columns, resolves field IDs, binds predicates, +/// and constructs [`PlanContext`] + [`TableScan`]. +pub(crate) fn build_table_scan( + config: ScanConfig<'_>, + snapshot: SnapshotRef, + manifest_file_filter: Option, + manifest_entry_filter: Option, +) -> Result { + let schema = config.schema.clone(); + + // Check that all column names exist in the schema (skip reserved columns). + if let Some(column_names) = config.column_names.as_ref() { + for column_name in column_names { + if is_metadata_column_name(column_name) { + continue; + } + if schema.field_by_name(column_name).is_none() { + return Err(Error::new( + ErrorKind::DataInvalid, + format!("Column {column_name} not found in table. Schema: {schema}"), + )); + } + } + } + + let mut field_ids = vec![]; + let column_names = config.column_names.clone().unwrap_or_else(|| { + schema + .as_struct() + .fields() + .iter() + .map(|f| f.name.clone()) + .collect() + }); + + for column_name in column_names.iter() { + if is_metadata_column_name(column_name) { + field_ids.push(get_metadata_field_id(column_name)?); + continue; + } + + let field_id = schema.field_id_by_name(column_name).ok_or_else(|| { + Error::new( + ErrorKind::DataInvalid, + format!("Column {column_name} not found in table. Schema: {schema}"), + ) + })?; + + schema + .as_struct() + .field_by_id(field_id) + .ok_or_else(|| { + Error::new( + ErrorKind::FeatureUnsupported, + format!( + "Column {column_name} is not a direct child of schema but a nested field, which is not supported now. Schema: {schema}" + ), + ) + })?; + + field_ids.push(field_id); + } + + let snapshot_bound_predicate = if let Some(ref predicates) = config.filter { + Some(predicates.bind(schema.clone(), true)?) + } else { + None + }; + + let name_mapping = config + .table + .metadata() + .properties() + .get(DEFAULT_SCHEMA_NAME_MAPPING) + .map(|raw| { + serde_json::from_str::(raw).map_err(|e| { + Error::new( + ErrorKind::DataInvalid, + format!( + "Failed to parse table property {DEFAULT_SCHEMA_NAME_MAPPING} as a NameMapping" + ), + ) + .with_source(e) + }) + }) + .transpose()? + .map(Arc::new); + + // Compute unified partition type if _partition is projected + let unified_partition_type = if field_ids.contains(&RESERVED_FIELD_ID_PARTITION) { + let partition_type = compute_unified_partition_type( + config + .table + .metadata() + .partition_specs_iter() + .map(|s| s.as_ref()), + &schema, + )?; + Some(Arc::new(partition_type)) + } else { + None + }; + + let plan_context = PlanContext { + snapshot, + table_metadata: config.table.metadata_ref(), + snapshot_schema: schema, + case_sensitive: config.case_sensitive, + predicate: config.filter.map(Arc::new), + snapshot_bound_predicate: snapshot_bound_predicate.map(Arc::new), + object_cache: config.table.object_cache(), + field_ids: Arc::new(field_ids), + name_mapping, + partition_filter_cache: Arc::new(PartitionFilterCache::new()), + manifest_evaluator_cache: Arc::new(ManifestEvaluatorCache::new()), + expression_evaluator_cache: Arc::new(ExpressionEvaluatorCache::new()), + manifest_file_filter, + manifest_entry_filter, + unified_partition_type, + }; + + Ok(TableScan { + batch_size: config.batch_size, + column_names: config.column_names, + file_io: config.table.file_io().clone(), + plan_context: Some(plan_context), + concurrency_limit_data_files: config.concurrency_limit_data_files, + concurrency_limit_manifest_entries: config.concurrency_limit_manifest_entries, + concurrency_limit_manifest_files: config.concurrency_limit_manifest_files, + row_group_filtering_enabled: config.row_group_filtering_enabled, + row_selection_enabled: config.row_selection_enabled, + runtime: config.table.runtime().clone(), + }) +} + /// Builder to create table scan. pub struct TableScanBuilder<'a> { table: &'a Table, @@ -203,7 +363,7 @@ impl<'a> TableScanBuilder<'a> { })? .clone(), None => { - let Some(current_snapshot_id) = self.table.metadata().current_snapshot() else { + let Some(current_snapshot) = self.table.metadata().current_snapshot() else { return Ok(TableScan { batch_size: self.batch_size, column_names: self.column_names, @@ -217,134 +377,32 @@ impl<'a> TableScanBuilder<'a> { runtime: self.table.runtime().clone(), }); }; - current_snapshot_id.clone() + current_snapshot.clone() } }; + // A standard scan projects onto the snapshot's own schema, so that + // time-travel reads see the table exactly as it was at that snapshot. let schema = snapshot.schema(self.table.metadata())?; - // Check that all column names exist in the schema (skip reserved columns). - if let Some(column_names) = self.column_names.as_ref() { - for column_name in column_names { - // Skip reserved columns that don't exist in the schema - if is_metadata_column_name(column_name) { - continue; - } - if schema.field_by_name(column_name).is_none() { - return Err(Error::new( - ErrorKind::DataInvalid, - format!("Column {column_name} not found in table. Schema: {schema}"), - )); - } - } - } - - let mut field_ids = vec![]; - let column_names = self.column_names.clone().unwrap_or_else(|| { - schema - .as_struct() - .fields() - .iter() - .map(|f| f.name.clone()) - .collect() - }); - - for column_name in column_names.iter() { - // Handle metadata columns (like "_file") - if is_metadata_column_name(column_name) { - field_ids.push(get_metadata_field_id(column_name)?); - continue; - } - - let field_id = schema.field_id_by_name(column_name).ok_or_else(|| { - Error::new( - ErrorKind::DataInvalid, - format!("Column {column_name} not found in table. Schema: {schema}"), - ) - })?; - - schema - .as_struct() - .field_by_id(field_id) - .ok_or_else(|| { - Error::new( - ErrorKind::FeatureUnsupported, - format!( - "Column {column_name} is not a direct child of schema but a nested field, which is not supported now. Schema: {schema}" - ), - ) - })?; - - field_ids.push(field_id); - } - - let snapshot_bound_predicate = if let Some(ref predicates) = self.filter { - Some(predicates.bind(schema.clone(), true)?) - } else { - None - }; - - let name_mapping = self - .table - .metadata() - .properties() - .get(DEFAULT_SCHEMA_NAME_MAPPING) - .map(|raw| { - serde_json::from_str::(raw).map_err(|e| { - Error::new( - ErrorKind::DataInvalid, - format!( - "Failed to parse table property {DEFAULT_SCHEMA_NAME_MAPPING} as a NameMapping" - ), - ) - .with_source(e) - }) - }) - .transpose()? - .map(Arc::new); - - // Compute unified partition type if _partition is projected - let unified_partition_type = if field_ids.contains(&RESERVED_FIELD_ID_PARTITION) { - let partition_type = compute_unified_partition_type( - self.table - .metadata() - .partition_specs_iter() - .map(|s| s.as_ref()), - &schema, - )?; - Some(Arc::new(partition_type)) - } else { - None - }; - - let plan_context = PlanContext { + build_table_scan( + ScanConfig { + table: self.table, + column_names: self.column_names, + batch_size: self.batch_size, + case_sensitive: self.case_sensitive, + filter: self.filter, + concurrency_limit_data_files: self.concurrency_limit_data_files, + concurrency_limit_manifest_entries: self.concurrency_limit_manifest_entries, + concurrency_limit_manifest_files: self.concurrency_limit_manifest_files, + row_group_filtering_enabled: self.row_group_filtering_enabled, + row_selection_enabled: self.row_selection_enabled, + schema, + }, snapshot, - table_metadata: self.table.metadata_ref(), - snapshot_schema: schema, - case_sensitive: self.case_sensitive, - predicate: self.filter.map(Arc::new), - snapshot_bound_predicate: snapshot_bound_predicate.map(Arc::new), - object_cache: self.table.object_cache(), - field_ids: Arc::new(field_ids), - name_mapping, - partition_filter_cache: Arc::new(PartitionFilterCache::new()), - manifest_evaluator_cache: Arc::new(ManifestEvaluatorCache::new()), - expression_evaluator_cache: Arc::new(ExpressionEvaluatorCache::new()), - unified_partition_type, - }; - - Ok(TableScan { - batch_size: self.batch_size, - column_names: self.column_names, - file_io: self.table.file_io().clone(), - plan_context: Some(plan_context), - concurrency_limit_data_files: self.concurrency_limit_data_files, - concurrency_limit_manifest_entries: self.concurrency_limit_manifest_entries, - concurrency_limit_manifest_files: self.concurrency_limit_manifest_files, - row_group_filtering_enabled: self.row_group_filtering_enabled, - row_selection_enabled: self.row_selection_enabled, - runtime: self.table.runtime().clone(), - }) + None, + None, + ) } } @@ -669,10 +727,10 @@ pub mod tests { use crate::scan::FileScanTask; use crate::spec::{ DEFAULT_SCHEMA_NAME_MAPPING, DataContentType, DataFileBuilder, DataFileFormat, Datum, - FormatVersion, Literal, MAIN_BRANCH, ManifestEntry, ManifestListWriter, ManifestStatus, - ManifestWriterBuilder, NestedField, Operation, PartitionSpec, PrimitiveType, Schema, - Snapshot, Struct, StructType, Summary, TableMetadata, TableMetadataBuilder, Type, - UnboundPartitionSpec, + FormatVersion, Literal, MAIN_BRANCH, ManifestEntry, ManifestFile, ManifestListWriter, + ManifestStatus, ManifestWriterBuilder, NestedField, Operation, PartitionSpec, + PrimitiveType, Schema, Snapshot, Struct, StructType, Summary, TableMetadata, + TableMetadataBuilder, Type, UnboundPartitionSpec, }; use crate::table::Table; use crate::test_utils::test_runtime; @@ -786,22 +844,72 @@ pub mod tests { } /// Creates a fixture with 5 snapshots chained as: - /// S1 (root) -> S2 -> S3 -> S4 -> S5 (current) - /// Useful for testing snapshot history traversal. + /// S1 (append) -> S2 (append) -> S3 (append) -> S4 (overwrite) -> S5 (append, current) + /// Useful for testing snapshot history traversal and incremental scans + /// with non-append operations in the chain. pub fn new_with_deep_history() -> Self { + Self::new_from_deep_history_metadata("example_table_metadata_v2_deep_history.json") + } + + /// Like [`Self::new_with_deep_history`] but every snapshot references + /// the older single-column schema (`schema-id` 0) while the table's + /// `current-schema-id` stays at the three-column schema (`schema-id` + /// 1). This models a table whose schema evolved *after* the snapshots + /// in an incremental range were written, so we can assert that an + /// incremental scan projects onto the current schema. + pub fn new_with_deep_history_stale_schema() -> Self { + let fixture = Self::new_from_deep_history_metadata( + "example_table_metadata_v2_deep_history_stale_schema.json", + ); + + // Sanity check: current schema (3 cols) differs from the schema the + // snapshots reference (1 col), otherwise the test would be vacuous. + assert_eq!(fixture.table.metadata().current_schema_id(), 1); + fixture + } + + /// Like [`Self::new_with_deep_history`] but the S4 snapshot is a + /// `replace` (the operation a compaction / `rewrite_data_files` + /// commits) rather than an `overwrite`. Used to prove that an + /// incremental append scan skips compaction output and never + /// double-counts the appended rows against their rewritten copies. + pub fn new_with_deep_history_compaction() -> Self { + Self::new_from_deep_history_metadata( + "example_table_metadata_v2_deep_history_compaction.json", + ) + } + + /// Builds a deep-history fixture from the named templated metadata file + /// in `testdata`. The five snapshot manifest-list paths are rendered to + /// point at this fixture's temp directory. + fn new_from_deep_history_metadata(metadata_file: &str) -> Self { let tmp_dir = TempDir::new().unwrap(); let table_location = tmp_dir.path().join("table1"); let table_metadata1_location = table_location.join("metadata/v1.json"); + let manifest_list_s1 = table_location.join("metadata/snap-3051729675574597004.avro"); + let manifest_list_s2 = table_location.join("metadata/snap-3055729675574597004.avro"); + let manifest_list_s3 = table_location.join("metadata/snap-3056729675574597004.avro"); + let manifest_list_s4 = table_location.join("metadata/snap-3057729675574597004.avro"); + let manifest_list_s5 = table_location.join("metadata/snap-3059729675574597004.avro"); + let file_io = FileIO::new_with_fs(); let table_metadata = { - let json_str = fs::read_to_string(format!( - "{}/testdata/example_table_metadata_v2_deep_history.json", + let template_json_str = fs::read_to_string(format!( + "{}/testdata/{metadata_file}", env!("CARGO_MANIFEST_DIR") )) .unwrap(); - serde_json::from_str::(&json_str).unwrap() + let metadata_json = render_template(&template_json_str, context! { + table_location => &table_location, + manifest_list_s1_location => &manifest_list_s1, + manifest_list_s2_location => &manifest_list_s2, + manifest_list_s3_location => &manifest_list_s3, + manifest_list_s4_location => &manifest_list_s4, + manifest_list_s5_location => &manifest_list_s5, + }); + serde_json::from_str::(&metadata_json).unwrap() }; let table = Table::builder() @@ -1015,6 +1123,151 @@ pub mod tests { manifest_list_write.close().await.unwrap(); } + /// Sets up manifest files for the deep history fixture. + /// + /// Creates one data file per snapshot (s1.parquet through s5.parquet), + /// each with a manifest and manifest list. Manifest lists are cumulative + /// (each snapshot's list includes all prior manifests), matching real + /// Iceberg behavior. The incremental scan should skip s4.parquet + /// (added in the overwrite snapshot S4). + pub async fn setup_manifest_files_deep_history(&mut self) { + let parquet_file_size = self.write_parquet_data_files_deep_history(); + let partition_spec = self.table.metadata().default_partition_spec(); + + // Snapshot chain: S1 -> S2 -> S3 -> S4 (overwrite) -> S5 + let snapshot_ids: Vec = vec![ + 3051729675574597004, + 3055729675574597004, + 3056729675574597004, + 3057729675574597004, + 3059729675574597004, + ]; + + // Accumulate manifests across snapshots (each manifest list is cumulative) + let mut all_manifests: Vec = Vec::new(); + + for (i, &snap_id) in snapshot_ids.iter().enumerate() { + let snapshot = self + .table + .metadata() + .snapshot_by_id(snap_id) + .unwrap() + .clone(); + let schema = snapshot.schema(self.table.metadata()).unwrap(); + + let file_name = format!("s{}.parquet", i + 1); + let partition_value = (i + 1) as i64 * 100; + + let mut writer = ManifestWriterBuilder::new( + self.next_manifest_file(), + Some(snap_id), + schema, + partition_spec.as_ref().clone(), + ) + .build_v2_data(); + + writer + .add_entry( + ManifestEntry::builder() + .status(ManifestStatus::Added) + .data_file( + DataFileBuilder::default() + .partition_spec_id(0) + .content(DataContentType::Data) + .file_path(format!("{}/{}", &self.table_location, file_name)) + .file_format(DataFileFormat::Parquet) + .file_size_in_bytes(parquet_file_size) + .record_count(1) + .partition(Struct::from_iter([Some(Literal::long( + partition_value, + ))])) + .key_metadata(None) + .build() + .unwrap(), + ) + .build(), + ) + .unwrap(); + + let mut data_file_manifest = writer.write_manifest_file().await.unwrap(); + // Assign sequence numbers so the manifest can be included in + // later snapshots' cumulative manifest lists without triggering + // the "unassigned sequence number" validation. + data_file_manifest.sequence_number = snapshot.sequence_number(); + data_file_manifest.min_sequence_number = snapshot.sequence_number(); + all_manifests.push(data_file_manifest); + + // Write cumulative manifest list for this snapshot + let manifest_list_writer = self + .table + .file_io() + .new_output(snapshot.manifest_list()) + .unwrap() + .writer() + .await + .unwrap(); + let mut manifest_list_write = ManifestListWriter::v2( + manifest_list_writer, + snap_id, + snapshot.parent_snapshot_id(), + snapshot.sequence_number(), + ); + manifest_list_write + .add_manifests(all_manifests.clone().into_iter()) + .unwrap(); + manifest_list_write.close().await.unwrap(); + } + } + + /// Writes parquet data files for the deep history fixture (3-column schema: x, y, z). + fn write_parquet_data_files_deep_history(&self) -> u64 { + fs::create_dir_all(&self.table_location).unwrap(); + + let schema = { + let fields = vec![ + arrow_schema::Field::new("x", arrow_schema::DataType::Int64, false) + .with_metadata(HashMap::from([( + PARQUET_FIELD_ID_META_KEY.to_string(), + "1".to_string(), + )])), + arrow_schema::Field::new("y", arrow_schema::DataType::Int64, false) + .with_metadata(HashMap::from([( + PARQUET_FIELD_ID_META_KEY.to_string(), + "2".to_string(), + )])), + arrow_schema::Field::new("z", arrow_schema::DataType::Int64, false) + .with_metadata(HashMap::from([( + PARQUET_FIELD_ID_META_KEY.to_string(), + "3".to_string(), + )])), + ]; + Arc::new(arrow_schema::Schema::new(fields)) + }; + + let col1 = Arc::new(Int64Array::from_iter_values(vec![1; 10])) as ArrayRef; + let col2 = Arc::new(Int64Array::from_iter_values(vec![2; 10])) as ArrayRef; + let col3 = Arc::new(Int64Array::from_iter_values(vec![3; 10])) as ArrayRef; + + let batch = RecordBatch::try_new(schema.clone(), vec![col1, col2, col3]).unwrap(); + + let props = WriterProperties::builder() + .set_compression(Compression::SNAPPY) + .build(); + + for i in 1..=5 { + let file = + File::create(format!("{}/s{}.parquet", &self.table_location, i)).unwrap(); + let mut writer = + ArrowWriter::try_new(file, batch.schema(), Some(props.clone())).unwrap(); + writer.write(&batch).expect("Writing batch"); + writer.close().unwrap(); + } + + fs::metadata(format!("{}/s1.parquet", &self.table_location)) + .unwrap() + .len() + } + /// Writes a v3 data manifest with a manifest-level `first_row_id` of 42, /// so live entries inherit a per-file `first_row_id` on read. Upgrades the /// table to v3 first, so the manifest list is read as v3. diff --git a/crates/iceberg/src/table.rs b/crates/iceberg/src/table.rs index 31feade038..734046b4f7 100644 --- a/crates/iceberg/src/table.rs +++ b/crates/iceberg/src/table.rs @@ -26,7 +26,7 @@ use crate::inspect::MetadataTable; use crate::io::FileIO; use crate::io::object_cache::ObjectCache; use crate::runtime::Runtime; -use crate::scan::TableScanBuilder; +use crate::scan::{IncrementalAppendScanBuilder, TableScanBuilder}; use crate::spec::{ManifestListReader, SchemaRef, SnapshotRef, TableMetadata, TableMetadataRef}; use crate::{Error, ErrorKind, Result, TableIdent}; @@ -280,6 +280,30 @@ impl Table { TableScanBuilder::new(self) } + /// Creates an incremental append scan starting from the given snapshot (exclusive). + /// + /// Returns only data files added in APPEND snapshots after `from_snapshot_id`, + /// up to `to_snapshot_id` or the current snapshot if `None`. + pub fn incremental_append_scan( + &self, + from_snapshot_id: i64, + to_snapshot_id: Option, + ) -> IncrementalAppendScanBuilder<'_> { + IncrementalAppendScanBuilder::new(self, from_snapshot_id, to_snapshot_id, false) + } + + /// Creates an incremental append scan starting from the given snapshot (inclusive). + /// + /// Returns only data files added in APPEND snapshots from `from_snapshot_id` (inclusive), + /// up to `to_snapshot_id` or the current snapshot if `None`. + pub fn incremental_append_scan_inclusive( + &self, + from_snapshot_id: i64, + to_snapshot_id: Option, + ) -> IncrementalAppendScanBuilder<'_> { + IncrementalAppendScanBuilder::new(self, from_snapshot_id, to_snapshot_id, true) + } + /// Creates a metadata table which provides table-like APIs for inspecting metadata. /// See [`MetadataTable`] for more details. pub fn inspect(&self) -> MetadataTable<'_> { @@ -385,6 +409,26 @@ impl StaticTable { self.0.scan() } + /// Creates an incremental append scan starting from the given snapshot (exclusive). + pub fn incremental_append_scan( + &self, + from_snapshot_id: i64, + to_snapshot_id: Option, + ) -> IncrementalAppendScanBuilder<'_> { + self.0 + .incremental_append_scan(from_snapshot_id, to_snapshot_id) + } + + /// Creates an incremental append scan starting from the given snapshot (inclusive). + pub fn incremental_append_scan_inclusive( + &self, + from_snapshot_id: i64, + to_snapshot_id: Option, + ) -> IncrementalAppendScanBuilder<'_> { + self.0 + .incremental_append_scan_inclusive(from_snapshot_id, to_snapshot_id) + } + /// Get TableMetadataRef for the static table pub fn metadata(&self) -> TableMetadataRef { self.0.metadata_ref() diff --git a/crates/iceberg/testdata/example_table_metadata_v2_deep_history.json b/crates/iceberg/testdata/example_table_metadata_v2_deep_history.json index a354958697..bd192ca6e2 100644 --- a/crates/iceberg/testdata/example_table_metadata_v2_deep_history.json +++ b/crates/iceberg/testdata/example_table_metadata_v2_deep_history.json @@ -1,7 +1,7 @@ { "format-version": 2, "table-uuid": "9c12d441-03fe-4693-9a96-a0705ddf69c1", - "location": "s3://bucket/test/location", + "location": "{{ table_location }}", "last-sequence-number": 34, "last-updated-ms": 1602638573590, "last-column-id": 3, @@ -53,7 +53,7 @@ "timestamp-ms": 1515100955770, "sequence-number": 0, "summary": {"operation": "append"}, - "manifest-list": "s3://bucket/metadata/snap-3051729675574597004.avro" + "manifest-list": "{{ manifest_list_s1_location }}" }, { "snapshot-id": 3055729675574597004, @@ -61,7 +61,7 @@ "timestamp-ms": 1555100955770, "sequence-number": 1, "summary": {"operation": "append"}, - "manifest-list": "s3://bucket/metadata/snap-3055729675574597004.avro", + "manifest-list": "{{ manifest_list_s2_location }}", "schema-id": 1 }, { @@ -70,7 +70,7 @@ "timestamp-ms": 1575100955770, "sequence-number": 2, "summary": {"operation": "append"}, - "manifest-list": "s3://bucket/metadata/snap-3056729675574597004.avro", + "manifest-list": "{{ manifest_list_s3_location }}", "schema-id": 1 }, { @@ -79,7 +79,7 @@ "timestamp-ms": 1595100955770, "sequence-number": 3, "summary": {"operation": "overwrite"}, - "manifest-list": "s3://bucket/metadata/snap-3057729675574597004.avro", + "manifest-list": "{{ manifest_list_s4_location }}", "schema-id": 1 }, { @@ -88,7 +88,7 @@ "timestamp-ms": 1602638573590, "sequence-number": 4, "summary": {"operation": "append"}, - "manifest-list": "s3://bucket/metadata/snap-3059729675574597004.avro", + "manifest-list": "{{ manifest_list_s5_location }}", "schema-id": 1 } ], diff --git a/crates/iceberg/testdata/example_table_metadata_v2_deep_history_compaction.json b/crates/iceberg/testdata/example_table_metadata_v2_deep_history_compaction.json new file mode 100644 index 0000000000..35f667caf3 --- /dev/null +++ b/crates/iceberg/testdata/example_table_metadata_v2_deep_history_compaction.json @@ -0,0 +1,104 @@ +{ + "format-version": 2, + "table-uuid": "9c12d441-03fe-4693-9a96-a0705ddf69c1", + "location": "{{ table_location }}", + "last-sequence-number": 34, + "last-updated-ms": 1602638573590, + "last-column-id": 3, + "current-schema-id": 1, + "schemas": [ + { + "type": "struct", + "schema-id": 0, + "fields": [ + {"id": 1, "name": "x", "required": true, "type": "long"} + ] + }, + { + "type": "struct", + "schema-id": 1, + "identifier-field-ids": [1, 2], + "fields": [ + {"id": 1, "name": "x", "required": true, "type": "long"}, + {"id": 2, "name": "y", "required": true, "type": "long", "doc": "comment"}, + {"id": 3, "name": "z", "required": true, "type": "long"} + ] + } + ], + "default-spec-id": 0, + "partition-specs": [ + { + "spec-id": 0, + "fields": [ + {"name": "x", "transform": "identity", "source-id": 1, "field-id": 1000} + ] + } + ], + "last-partition-id": 1000, + "default-sort-order-id": 3, + "sort-orders": [ + { + "order-id": 3, + "fields": [ + {"transform": "identity", "source-id": 2, "direction": "asc", "null-order": "nulls-first"}, + {"transform": "bucket[4]", "source-id": 3, "direction": "desc", "null-order": "nulls-last"} + ] + } + ], + "properties": {}, + "current-snapshot-id": 3059729675574597004, + "snapshots": [ + { + "snapshot-id": 3051729675574597004, + "timestamp-ms": 1515100955770, + "sequence-number": 0, + "summary": {"operation": "append"}, + "manifest-list": "{{ manifest_list_s1_location }}" + }, + { + "snapshot-id": 3055729675574597004, + "parent-snapshot-id": 3051729675574597004, + "timestamp-ms": 1555100955770, + "sequence-number": 1, + "summary": {"operation": "append"}, + "manifest-list": "{{ manifest_list_s2_location }}", + "schema-id": 1 + }, + { + "snapshot-id": 3056729675574597004, + "parent-snapshot-id": 3055729675574597004, + "timestamp-ms": 1575100955770, + "sequence-number": 2, + "summary": {"operation": "append"}, + "manifest-list": "{{ manifest_list_s3_location }}", + "schema-id": 1 + }, + { + "snapshot-id": 3057729675574597004, + "parent-snapshot-id": 3056729675574597004, + "timestamp-ms": 1595100955770, + "sequence-number": 3, + "summary": {"operation": "replace"}, + "manifest-list": "{{ manifest_list_s4_location }}", + "schema-id": 1 + }, + { + "snapshot-id": 3059729675574597004, + "parent-snapshot-id": 3057729675574597004, + "timestamp-ms": 1602638573590, + "sequence-number": 4, + "summary": {"operation": "append"}, + "manifest-list": "{{ manifest_list_s5_location }}", + "schema-id": 1 + } + ], + "snapshot-log": [ + {"snapshot-id": 3051729675574597004, "timestamp-ms": 1515100955770}, + {"snapshot-id": 3055729675574597004, "timestamp-ms": 1555100955770}, + {"snapshot-id": 3056729675574597004, "timestamp-ms": 1575100955770}, + {"snapshot-id": 3057729675574597004, "timestamp-ms": 1595100955770}, + {"snapshot-id": 3059729675574597004, "timestamp-ms": 1602638573590} + ], + "metadata-log": [], + "refs": {"main": {"snapshot-id": 3059729675574597004, "type": "branch"}} +} diff --git a/crates/iceberg/testdata/example_table_metadata_v2_deep_history_stale_schema.json b/crates/iceberg/testdata/example_table_metadata_v2_deep_history_stale_schema.json new file mode 100644 index 0000000000..f5a08e1b9b --- /dev/null +++ b/crates/iceberg/testdata/example_table_metadata_v2_deep_history_stale_schema.json @@ -0,0 +1,105 @@ +{ + "format-version": 2, + "table-uuid": "9c12d441-03fe-4693-9a96-a0705ddf69c1", + "location": "{{ table_location }}", + "last-sequence-number": 34, + "last-updated-ms": 1602638573590, + "last-column-id": 3, + "current-schema-id": 1, + "schemas": [ + { + "type": "struct", + "schema-id": 0, + "fields": [ + {"id": 1, "name": "x", "required": true, "type": "long"} + ] + }, + { + "type": "struct", + "schema-id": 1, + "identifier-field-ids": [1, 2], + "fields": [ + {"id": 1, "name": "x", "required": true, "type": "long"}, + {"id": 2, "name": "y", "required": true, "type": "long", "doc": "comment"}, + {"id": 3, "name": "z", "required": true, "type": "long"} + ] + } + ], + "default-spec-id": 0, + "partition-specs": [ + { + "spec-id": 0, + "fields": [ + {"name": "x", "transform": "identity", "source-id": 1, "field-id": 1000} + ] + } + ], + "last-partition-id": 1000, + "default-sort-order-id": 3, + "sort-orders": [ + { + "order-id": 3, + "fields": [ + {"transform": "identity", "source-id": 2, "direction": "asc", "null-order": "nulls-first"}, + {"transform": "bucket[4]", "source-id": 3, "direction": "desc", "null-order": "nulls-last"} + ] + } + ], + "properties": {}, + "current-snapshot-id": 3059729675574597004, + "snapshots": [ + { + "snapshot-id": 3051729675574597004, + "timestamp-ms": 1515100955770, + "sequence-number": 0, + "summary": {"operation": "append"}, + "manifest-list": "{{ manifest_list_s1_location }}", + "schema-id": 0 + }, + { + "snapshot-id": 3055729675574597004, + "parent-snapshot-id": 3051729675574597004, + "timestamp-ms": 1555100955770, + "sequence-number": 1, + "summary": {"operation": "append"}, + "manifest-list": "{{ manifest_list_s2_location }}", + "schema-id": 0 + }, + { + "snapshot-id": 3056729675574597004, + "parent-snapshot-id": 3055729675574597004, + "timestamp-ms": 1575100955770, + "sequence-number": 2, + "summary": {"operation": "append"}, + "manifest-list": "{{ manifest_list_s3_location }}", + "schema-id": 0 + }, + { + "snapshot-id": 3057729675574597004, + "parent-snapshot-id": 3056729675574597004, + "timestamp-ms": 1595100955770, + "sequence-number": 3, + "summary": {"operation": "overwrite"}, + "manifest-list": "{{ manifest_list_s4_location }}", + "schema-id": 0 + }, + { + "snapshot-id": 3059729675574597004, + "parent-snapshot-id": 3057729675574597004, + "timestamp-ms": 1602638573590, + "sequence-number": 4, + "summary": {"operation": "append"}, + "manifest-list": "{{ manifest_list_s5_location }}", + "schema-id": 0 + } + ], + "snapshot-log": [ + {"snapshot-id": 3051729675574597004, "timestamp-ms": 1515100955770}, + {"snapshot-id": 3055729675574597004, "timestamp-ms": 1555100955770}, + {"snapshot-id": 3056729675574597004, "timestamp-ms": 1575100955770}, + {"snapshot-id": 3057729675574597004, "timestamp-ms": 1595100955770}, + {"snapshot-id": 3059729675574597004, "timestamp-ms": 1602638573590} + ], + "metadata-log": [], + "refs": {"main": {"snapshot-id": 3059729675574597004, "type": "branch"}} +} From 2b4b71b1766ff0d695ba2b73e68158cb25417b70 Mon Sep 17 00:00:00 2001 From: Xander Date: Mon, 24 Aug 2026 09:26:25 +0100 Subject: [PATCH 2/9] read across multiple manifest lists --- crates/iceberg/public-api.txt | 1 - crates/iceberg/src/scan/context.rs | 62 +++- crates/iceberg/src/scan/incremental.rs | 284 ++++++++++++------ crates/iceberg/src/scan/mod.rs | 196 ++++++++++-- ...xample_table_metadata_v2_deep_history.json | 3 +- ...e_metadata_v2_deep_history_compaction.json | 3 +- 6 files changed, 427 insertions(+), 122 deletions(-) diff --git a/crates/iceberg/public-api.txt b/crates/iceberg/public-api.txt index 12e7293185..b0f960730a 100644 --- a/crates/iceberg/public-api.txt +++ b/crates/iceberg/public-api.txt @@ -1358,7 +1358,6 @@ pub struct iceberg::scan::TableScan impl iceberg::scan::TableScan pub fn iceberg::scan::TableScan::column_names(&self) -> core::option::Option<&[alloc::string::String]> pub async fn iceberg::scan::TableScan::plan_files(&self) -> iceberg::Result -pub fn iceberg::scan::TableScan::snapshot(&self) -> core::option::Option<&iceberg::spec::SnapshotRef> pub async fn iceberg::scan::TableScan::to_arrow(&self) -> iceberg::Result impl core::fmt::Debug for iceberg::scan::TableScan pub fn iceberg::scan::TableScan::fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result diff --git a/crates/iceberg/src/scan/context.rs b/crates/iceberg/src/scan/context.rs index b1d62c599b..e465431eac 100644 --- a/crates/iceberg/src/scan/context.rs +++ b/crates/iceberg/src/scan/context.rs @@ -15,10 +15,11 @@ // specific language governing permissions and limitations // under the License. +use std::collections::HashSet; use std::sync::Arc; use futures::channel::mpsc::Sender; -use futures::{SinkExt, TryFutureExt}; +use futures::{SinkExt, StreamExt, TryFutureExt, TryStreamExt}; use crate::delete_file_index::DeleteFileIndex; use crate::expr::{Bind, BoundPredicate, Predicate}; @@ -170,10 +171,15 @@ impl ManifestEntryContext { } } -/// PlanContext wraps a [`SnapshotRef`] alongside all the other -/// objects that are required to perform a scan file plan. +/// PlanContext holds everything required to perform a scan file plan. pub(crate) struct PlanContext { - pub snapshot: SnapshotRef, + /// Snapshots whose manifest lists are read to source the scan's manifests. + /// + /// A standard scan reads only its own snapshot's list. An incremental scan + /// reads every append snapshot in the range, because operations that rewrite + /// manifests re-emit earlier `ADDED` entries as `EXISTING`, leaving those + /// rows reachable only via the list of the snapshot that added them. + pub manifest_list_snapshots: Vec, pub table_metadata: TableMetadataRef, pub snapshot_schema: SchemaRef, @@ -196,20 +202,13 @@ pub(crate) struct PlanContext { impl std::fmt::Debug for PlanContext { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { f.debug_struct("PlanContext") - .field("snapshot", &self.snapshot) + .field("manifest_list_snapshots", &self.manifest_list_snapshots) .field("case_sensitive", &self.case_sensitive) .finish_non_exhaustive() } } impl PlanContext { - pub(crate) async fn get_manifest_list(&self) -> Result> { - self.object_cache - .as_ref() - .get_manifest_list(&self.snapshot, &self.table_metadata) - .await - } - /// Returns the partition filter for a manifest. See [`PartitionFilterCache::get`] for the /// always-true fallback when the manifest's spec cannot be resolved against the scan schema. fn get_partition_filter(&self, manifest_file: &ManifestFile) -> Result> { @@ -235,12 +234,24 @@ impl PlanContext { pub(crate) fn build_manifest_file_contexts( &self, - manifest_list: Arc, + manifest_lists: Vec>, tx_data: Sender, delete_file_idx: DeleteFileIndex, delete_file_tx: Sender, ) -> Result> + 'static>> { - let mut manifest_files = manifest_list.entries().iter().collect::>(); + // Manifest lists are cumulative, so a manifest carried forward appears in + // several lists. Deduplicate by path to read each one exactly once, as Java's + // BaseIncrementalAppendScan#appendFilesFromSnapshots does. + let mut seen = HashSet::new(); + let mut manifest_files = Vec::new(); + for manifest_list in &manifest_lists { + for manifest_file in manifest_list.entries() { + if seen.insert(manifest_file.manifest_path.as_str()) { + manifest_files.push(manifest_file); + } + } + } + // Sort manifest files to process delete manifests first. // This avoids a deadlock where the producer blocks on sending data manifest entries // (because the data channel is full) while the delete manifest consumer is waiting @@ -339,4 +350,27 @@ impl PlanContext { unified_partition_type: self.unified_partition_type.clone(), } } + + /// Reads the manifest list of every snapshot in [`Self::manifest_list_snapshots`], + /// in that order. Ordering only makes planning reproducible. + pub(crate) async fn collect_manifest_lists( + &self, + concurrency_limit: usize, + ) -> Result>> { + let object_cache = self.object_cache.clone(); + let table_metadata = self.table_metadata.clone(); + futures::stream::iter(self.manifest_list_snapshots.clone()) + .map(move |snapshot| { + let object_cache = object_cache.clone(); + let table_metadata = table_metadata.clone(); + async move { + object_cache + .get_manifest_list(&snapshot, &table_metadata) + .await + } + }) + .buffered(concurrency_limit.max(1)) + .try_collect() + .await + } } diff --git a/crates/iceberg/src/scan/incremental.rs b/crates/iceberg/src/scan/incremental.rs index d6d77bd184..c5a2a6f5ce 100644 --- a/crates/iceberg/src/scan/incremental.rs +++ b/crates/iceberg/src/scan/incremental.rs @@ -23,34 +23,23 @@ use std::sync::Arc; use crate::expr::Predicate; use crate::scan::context::{ManifestEntryFilter, ManifestFileFilter}; use crate::scan::{ScanConfig, TableScan, build_table_scan}; -use crate::spec::{ManifestContentType, ManifestStatus, Operation, TableMetadataRef}; +use crate::spec::{ManifestContentType, ManifestStatus, Operation, SnapshotRef, TableMetadataRef}; use crate::table::Table; use crate::util::available_parallelism; use crate::util::snapshot::ancestors_between; use crate::{Error, ErrorKind, Result}; -/// Represents a validated range of snapshots for incremental scanning. +/// A validated range of snapshots for incremental scanning. /// -/// This struct is used to track which snapshot IDs are included in an incremental -/// scan range, allowing efficient filtering of manifest entries. +/// Holds the APPEND snapshots of the range: their manifest lists are the scan's +/// manifest source, and their IDs select which manifests and entries it keeps. #[derive(Debug, Clone)] -pub(crate) struct AppendSnapshotSet { - /// Snapshot IDs in the range - snapshot_ids: HashSet, +pub(crate) struct AppendRange { + /// Newest first. + snapshots: Vec, } -impl AppendSnapshotSet { - /// Build a snapshot range by walking the snapshot ancestry chain. - /// - /// Validates that `from_snapshot_id` is an ancestor of `to_snapshot_id` and - /// collects all snapshot IDs in between. Also validates that all snapshots - /// in the range have APPEND operations. - /// - /// # Arguments - /// * `table_metadata` - The table metadata containing snapshot information - /// * `from_snapshot_id` - The starting snapshot ID - /// * `to_snapshot_id` - The ending snapshot ID - /// * `from_inclusive` - Whether to include the from_snapshot in the range +impl AppendRange { pub(crate) fn build( table_metadata: &TableMetadataRef, from_snapshot_id: i64, @@ -89,7 +78,7 @@ impl AppendSnapshotSet { // In inclusive mode, we should have exactly one snapshot. if !from_inclusive { return Ok(Self { - snapshot_ids: HashSet::new(), + snapshots: Vec::new(), }); } } else if snapshots.is_empty() { @@ -120,42 +109,50 @@ impl AppendSnapshotSet { } } - // Collect only APPEND snapshot IDs, silently skipping non-APPEND - // snapshots (e.g. replace/compaction, overwrite, delete). This matches - // the Java BaseIncrementalAppendScan behavior — only append operations - // contribute new data files to an incremental append scan. - let mut snapshot_ids = HashSet::with_capacity(snapshots.len()); - for snapshot in &snapshots { - if snapshot.summary().operation == Operation::Append { - snapshot_ids.insert(snapshot.snapshot_id()); - } - } + // Keep only APPEND snapshots, silently skipping the rest (replace, overwrite, + // delete) + let snapshots = snapshots + .into_iter() + .filter(|snapshot| snapshot.summary().operation == Operation::Append) + .collect(); + + Ok(Self { snapshots }) + } - Ok(Self { snapshot_ids }) + /// The APPEND snapshots in the range, newest first. Every one's manifest list + /// must be read; see [`PlanContext::manifest_list_snapshots`]. + /// + /// [`PlanContext::manifest_list_snapshots`]: crate::scan::context::PlanContext::manifest_list_snapshots + pub(crate) fn snapshots(&self) -> &[SnapshotRef] { + &self.snapshots } - /// Check if a snapshot_id is within this set - pub(crate) fn contains(&self, snapshot_id: i64) -> bool { - self.snapshot_ids.contains(&snapshot_id) + fn snapshot_ids(&self) -> HashSet { + self.snapshots + .iter() + .map(|snapshot| snapshot.snapshot_id()) + .collect() } /// Create a manifest file filter that skips delete manifests and data - /// manifests whose `added_snapshot_id` is outside this set. - pub(crate) fn manifest_file_filter(self: &Arc) -> ManifestFileFilter { - let set = self.clone(); + /// manifests whose `added_snapshot_id` is outside this range. + pub(crate) fn manifest_file_filter(&self) -> ManifestFileFilter { + let snapshot_ids = self.snapshot_ids(); Arc::new(move |manifest_file| { manifest_file.content != ManifestContentType::Deletes - && set.contains(manifest_file.added_snapshot_id) + && snapshot_ids.contains(&manifest_file.added_snapshot_id) }) } /// Create a manifest entry filter that includes only entries with - /// status ADDED and a snapshot_id within this set. - pub(crate) fn manifest_entry_filter(self: &Arc) -> ManifestEntryFilter { - let set = self.clone(); + /// status ADDED and a snapshot_id within this range. + pub(crate) fn manifest_entry_filter(&self) -> ManifestEntryFilter { + let snapshot_ids = self.snapshot_ids(); Arc::new(move |entry| { entry.status() == ManifestStatus::Added - && entry.snapshot_id().is_some_and(|id| set.contains(id)) + && entry + .snapshot_id() + .is_some_and(|id| snapshot_ids.contains(&id)) }) } } @@ -319,12 +316,12 @@ impl<'a> IncrementalAppendScanBuilder<'a> { } }; - let append_set = Arc::new(AppendSnapshotSet::build( + let append_range = AppendRange::build( &self.table.metadata_ref(), self.from_snapshot_id, to_snapshot.snapshot_id(), self.from_inclusive, - )?); + )?; build_table_scan( ScanConfig { @@ -344,9 +341,9 @@ impl<'a> IncrementalAppendScanBuilder<'a> { // the current schema, so newer columns become `NULL`. schema: self.table.metadata().current_schema().clone(), }, - to_snapshot, - Some(append_set.manifest_file_filter()), - Some(append_set.manifest_entry_filter()), + append_range.snapshots().to_vec(), + Some(append_range.manifest_file_filter()), + Some(append_range.manifest_entry_filter()), ) } } @@ -355,9 +352,35 @@ impl<'a> IncrementalAppendScanBuilder<'a> { mod tests { use futures::TryStreamExt; - use super::AppendSnapshotSet; + use super::AppendRange; + use crate::scan::TableScan; use crate::scan::tests::TableTestFixture; + /// Sorted base names of the data files `scan` yields. Duplicates are kept so + /// double-counting is visible. + async fn planned_file_names(scan: &TableScan) -> Vec { + let tasks: Vec<_> = scan + .plan_files() + .await + .unwrap() + .try_collect() + .await + .unwrap(); + + let mut names: Vec = tasks + .iter() + .map(|task| { + task.data_file_path + .rsplit('/') + .next() + .unwrap_or(&task.data_file_path) + .to_string() + }) + .collect(); + names.sort(); + names + } + #[test] fn test_incremental_scan_invalid_from_snapshot_exclusive() { let table = TableTestFixture::new().table; @@ -493,8 +516,8 @@ mod tests { "Inclusive scan of a single append snapshot should succeed" ); - // Verify AppendSnapshotSet directly - let set = AppendSnapshotSet::build( + // Verify AppendRange directly + let set = AppendRange::build( &table.metadata_ref(), current_snapshot_id, current_snapshot_id, @@ -502,7 +525,7 @@ mod tests { ) .unwrap(); assert!( - set.contains(current_snapshot_id), + set.snapshot_ids().contains(¤t_snapshot_id), "Inclusive set should contain the from_snapshot" ); } @@ -522,8 +545,8 @@ mod tests { "Exclusive scan from=to should succeed with empty range" ); - // Verify AppendSnapshotSet directly - let set = AppendSnapshotSet::build( + // Verify AppendRange directly + let set = AppendRange::build( &table.metadata_ref(), current_snapshot_id, current_snapshot_id, @@ -531,7 +554,7 @@ mod tests { ) .unwrap(); assert!( - !set.contains(current_snapshot_id), + !set.snapshot_ids().contains(¤t_snapshot_id), "Exclusive set should not contain the from_snapshot" ); } @@ -553,7 +576,7 @@ mod tests { "Should succeed, skipping non-APPEND snapshots" ); - let set = AppendSnapshotSet::build( + let set = AppendRange::build( &table.metadata_ref(), 3051729675574597004, 3059729675574597004, @@ -561,23 +584,23 @@ mod tests { ) .unwrap(); assert!( - !set.contains(3051729675574597004), + !set.snapshot_ids().contains(&3051729675574597004), "S1 (from) should be excluded" ); assert!( - set.contains(3055729675574597004), + set.snapshot_ids().contains(&3055729675574597004), "S2 (append) should be in set" ); assert!( - set.contains(3056729675574597004), + set.snapshot_ids().contains(&3056729675574597004), "S3 (append) should be in set" ); assert!( - !set.contains(3057729675574597004), + !set.snapshot_ids().contains(&3057729675574597004), "S4 (overwrite) should be skipped" ); assert!( - set.contains(3059729675574597004), + set.snapshot_ids().contains(&3059729675574597004), "S5 (append) should be in set" ); } @@ -589,7 +612,7 @@ mod tests { let table = TableTestFixture::new_with_deep_history().table; // Scanning from S1 to S3 (all appends) - let set = AppendSnapshotSet::build( + let set = AppendRange::build( &table.metadata_ref(), 3051729675574597004, 3056729675574597004, @@ -597,11 +620,17 @@ mod tests { ) .unwrap(); assert!( - !set.contains(3051729675574597004), + !set.snapshot_ids().contains(&3051729675574597004), "from_snapshot should be excluded" ); - assert!(set.contains(3055729675574597004), "S2 should be in range"); - assert!(set.contains(3056729675574597004), "S3 should be in range"); + assert!( + set.snapshot_ids().contains(&3055729675574597004), + "S2 should be in range" + ); + assert!( + set.snapshot_ids().contains(&3056729675574597004), + "S3 should be in range" + ); } #[tokio::test] @@ -690,42 +719,26 @@ mod tests { // Scanning from S1 (exclusive) to S5 should return only files from // APPEND snapshots: s2.parquet, s3.parquet, s5.parquet. The compacted // s4.parquet must be skipped. + // + // Uses the rewritten manifest layout: compaction also replaces the manifests + // it compacts, so S4 re-emits s1/s2/s3 as EXISTING under a manifest it owns. let mut fixture = TableTestFixture::new_with_deep_history_compaction(); - fixture.setup_manifest_files_deep_history().await; + fixture.setup_manifest_files_deep_history_rewritten().await; let s1_id = 3051729675574597004_i64; let s5_id = 3059729675574597004_i64; - let table_scan = fixture + let scan = fixture .table .incremental_append_scan(s1_id, Some(s5_id)) .build() .unwrap(); - let mut tasks: Vec<_> = table_scan - .plan_files() - .await - .unwrap() - .try_collect() - .await - .unwrap(); - - tasks.sort_by(|a, b| a.data_file_path.cmp(&b.data_file_path)); - - let file_names: Vec<&str> = tasks - .iter() - .map(|t| { - t.data_file_path - .rsplit('/') - .next() - .unwrap_or(&t.data_file_path) - }) - .collect(); - assert_eq!( - file_names, + planned_file_names(&scan).await, vec!["s2.parquet", "s3.parquet", "s5.parquet"], - "Compacted file (s4, from the replace snapshot) must be skipped" + "Compacted file (s4, from the replace snapshot) must be skipped, and the \ + appends it rewrote must each be returned exactly once" ); } @@ -860,6 +873,101 @@ mod tests { ); } + #[tokio::test] + async fn test_incremental_scan_merge_append_does_not_drop_earlier_appends() { + // S2 merge-appends S1's manifest away: s1 survives only as an EXISTING entry + // in a manifest S2 owns. Sourcing manifests from the to-snapshot's list alone + // would drop it, losing a row appended inside the range. + let mut fixture = TableTestFixture::new_with_deep_history(); + fixture.setup_manifest_files_deep_history_rewritten().await; + + let s1_id = 3051729675574597004_i64; + let s2_id = 3055729675574597004_i64; + + let scan = fixture + .table + .incremental_append_scan_inclusive(s1_id, Some(s2_id)) + .build() + .unwrap(); + + assert_eq!( + planned_file_names(&scan).await, + vec!["s1.parquet", "s2.parquet"], + "both appends in the range must be returned even though S2 merged S1's manifest away" + ); + } + + #[tokio::test] + async fn test_incremental_scan_rewritten_manifests_do_not_drop_appends() { + // S4 (overwrite) rewrites every live manifest into one it owns, re-emitting + // s1/s2/s3 as EXISTING. Its added_snapshot_id is not an append, so the whole + // manifest is filtered out and S2/S3's appends are reachable only through + // their own manifest lists. + let mut fixture = TableTestFixture::new_with_deep_history(); + fixture.setup_manifest_files_deep_history_rewritten().await; + + let s1_id = 3051729675574597004_i64; + let s4_id = 3057729675574597004_i64; + + let scan = fixture + .table + .incremental_append_scan(s1_id, Some(s4_id)) + .build() + .unwrap(); + + assert_eq!( + planned_file_names(&scan).await, + vec!["s2.parquet", "s3.parquet"], + "appends rewritten into an overwrite's manifest must still be returned" + ); + } + + #[tokio::test] + async fn test_incremental_scan_rewritten_manifests_with_later_append() { + // Same rewritten history, but scanning past the overwrite to S5. s4 came + // from the overwrite and must stay excluded; s2, s3 and s5 are appends. + let mut fixture = TableTestFixture::new_with_deep_history(); + fixture.setup_manifest_files_deep_history_rewritten().await; + + let s1_id = 3051729675574597004_i64; + let s5_id = 3059729675574597004_i64; + + let scan = fixture + .table + .incremental_append_scan(s1_id, Some(s5_id)) + .build() + .unwrap(); + + assert_eq!( + planned_file_names(&scan).await, + vec!["s2.parquet", "s3.parquet", "s5.parquet"], + "appends across a manifest-rewriting overwrite must be returned exactly once" + ); + } + + #[tokio::test] + async fn test_incremental_scan_does_not_duplicate_carried_forward_manifests() { + // S1's manifest appears in S1's, S2's and S3's lists. Reading every append + // snapshot's list must not emit the same manifest — and rows — twice. + let mut fixture = TableTestFixture::new_with_deep_history(); + fixture.setup_manifest_files_deep_history().await; + + let s1_id = 3051729675574597004_i64; + let s3_id = 3056729675574597004_i64; + + let scan = fixture + .table + .incremental_append_scan_inclusive(s1_id, Some(s3_id)) + .build() + .unwrap(); + + assert_eq!( + planned_file_names(&scan).await, + vec!["s1.parquet", "s2.parquet", "s3.parquet"], + "each file must appear exactly once despite cumulative manifest lists" + ); + } + #[tokio::test] async fn test_incremental_scan_deep_history_inclusive_with_overwrite() { // Inclusive scan from S3 to S5: diff --git a/crates/iceberg/src/scan/mod.rs b/crates/iceberg/src/scan/mod.rs index 144136a418..8ec50ef5d1 100644 --- a/crates/iceberg/src/scan/mod.rs +++ b/crates/iceberg/src/scan/mod.rs @@ -79,7 +79,7 @@ pub(crate) struct ScanConfig<'a> { /// and constructs [`PlanContext`] + [`TableScan`]. pub(crate) fn build_table_scan( config: ScanConfig<'_>, - snapshot: SnapshotRef, + manifest_list_snapshots: Vec, manifest_file_filter: Option, manifest_entry_filter: Option, ) -> Result { @@ -179,7 +179,7 @@ pub(crate) fn build_table_scan( }; let plan_context = PlanContext { - snapshot, + manifest_list_snapshots, table_metadata: config.table.metadata_ref(), snapshot_schema: schema, case_sensitive: config.case_sensitive, @@ -399,7 +399,7 @@ impl<'a> TableScanBuilder<'a> { row_selection_enabled: self.row_selection_enabled, schema, }, - snapshot, + vec![snapshot], None, None, ) @@ -455,13 +455,15 @@ impl TableScan { let (delete_file_idx, delete_file_tx) = DeleteFileIndex::new(self.runtime.clone()); - let manifest_list = plan_context.get_manifest_list().await?; + let manifest_lists = plan_context + .collect_manifest_lists(concurrency_limit_manifest_files) + .await?; - // get the [`ManifestFile`]s from the [`ManifestList`], filtering out any + // get the [`ManifestFile`]s from the [`ManifestList`]s, filtering out any // whose partitions cannot match this // scan's filter let manifest_file_contexts = plan_context.build_manifest_file_contexts( - manifest_list, + manifest_lists, manifest_entry_data_ctx_tx, delete_file_idx.clone(), manifest_entry_delete_ctx_tx, @@ -579,11 +581,6 @@ impl TableScan { self.column_names.as_deref() } - /// Returns a reference to the snapshot of the table scan. - pub fn snapshot(&self) -> Option<&SnapshotRef> { - self.plan_context.as_ref().map(|x| &x.snapshot) - } - async fn process_data_manifest_entry( manifest_entry_context: ManifestEntryContext, mut file_scan_task_tx: Sender>, @@ -730,7 +727,7 @@ pub mod tests { FormatVersion, Literal, MAIN_BRANCH, ManifestEntry, ManifestFile, ManifestListWriter, ManifestStatus, ManifestWriterBuilder, NestedField, Operation, PartitionSpec, PrimitiveType, Schema, Snapshot, Struct, StructType, Summary, TableMetadata, - TableMetadataBuilder, Type, UnboundPartitionSpec, + TableMetadataBuilder, Type, UNASSIGNED_SEQUENCE_NUMBER, UnboundPartitionSpec, }; use crate::table::Table; use crate::test_utils::test_runtime; @@ -1219,6 +1216,158 @@ pub mod tests { } } + /// Like [`Self::setup_manifest_files_deep_history`], but the manifest lists + /// model writers that *rewrite* manifests rather than carrying every one + /// forward verbatim: + /// + /// ```text + /// S1 append -> [A1] A1 = {s1 ADDED@S1} + /// S2 append -> [M2] M2 = {s1 EXISTING@S1, s2 ADDED@S2} (merge-append: A1 is gone) + /// S3 append -> [M2, A3] A3 = {s3 ADDED@S3} + /// S4 overwrite -> [C4] C4 = {s1,s2,s3 EXISTING, s4 ADDED@S4} (rewrite: M2, A3 are gone) + /// S5 append -> [C4, A5] A5 = {s5 ADDED@S5} + /// ``` + /// + /// The surviving copies of the earlier entries are `EXISTING`, not `ADDED`, so + /// a scan reading only the to-snapshot's list silently drops them. + pub async fn setup_manifest_files_deep_history_rewritten(&mut self) { + let file_size = self.write_parquet_data_files_deep_history(); + + let (s1, s2, s3, s4, s5) = ( + 3051729675574597004_i64, + 3055729675574597004_i64, + 3056729675574597004_i64, + 3057729675574597004_i64, + 3059729675574597004_i64, + ); + + // (file index, originating snapshot, that snapshot's sequence number) + let (f1, f2, f3, f4, f5) = ((1, s1, 0), (2, s2, 1), (3, s3, 2), (4, s4, 3), (5, s5, 4)); + + let a1 = self + .write_rewritten_manifest(s1, &[f1], &[], file_size) + .await; + let m2 = self + .write_rewritten_manifest(s2, &[f2], &[f1], file_size) + .await; + let a3 = self + .write_rewritten_manifest(s3, &[f3], &[], file_size) + .await; + let c4 = self + .write_rewritten_manifest(s4, &[f4], &[f1, f2, f3], file_size) + .await; + let a5 = self + .write_rewritten_manifest(s5, &[f5], &[], file_size) + .await; + + self.write_deep_history_manifest_list(s1, vec![a1]).await; + self.write_deep_history_manifest_list(s2, vec![m2.clone()]) + .await; + self.write_deep_history_manifest_list(s3, vec![m2, a3]) + .await; + self.write_deep_history_manifest_list(s4, vec![c4.clone()]) + .await; + self.write_deep_history_manifest_list(s5, vec![c4, a5]) + .await; + } + + /// Writes one data manifest owned by `owner_snapshot_id`. + /// + /// `added` and `existing` are `(file index, originating snapshot id, sequence + /// number)` triples naming `s{index}.parquet`. Added entries take the owning + /// snapshot's ID (the writer enforces this); existing entries keep the ID and + /// sequence number of the snapshot that first added them. + async fn write_rewritten_manifest( + &self, + owner_snapshot_id: i64, + added: &[(usize, i64, i64)], + existing: &[(usize, i64, i64)], + file_size: u64, + ) -> ManifestFile { + let snapshot = self + .table + .metadata() + .snapshot_by_id(owner_snapshot_id) + .unwrap() + .clone(); + let schema = snapshot.schema(self.table.metadata()).unwrap(); + let partition_spec = self.table.metadata().default_partition_spec(); + + let mut writer = ManifestWriterBuilder::new( + self.next_manifest_file(), + Some(owner_snapshot_id), + schema, + partition_spec.as_ref().clone(), + ) + .build_v2_data(); + + let data_file = |index: usize| { + DataFileBuilder::default() + .partition_spec_id(0) + .content(DataContentType::Data) + .file_path(format!("{}/s{}.parquet", &self.table_location, index)) + .file_format(DataFileFormat::Parquet) + .file_size_in_bytes(file_size) + .record_count(1) + .partition(Struct::from_iter([Some(Literal::long(index as i64 * 100))])) + .key_metadata(None) + .build() + .unwrap() + }; + + for &(index, _, sequence_number) in added { + writer.add_file(data_file(index), sequence_number).unwrap(); + } + for &(index, snapshot_id, sequence_number) in existing { + writer + .add_existing_file( + data_file(index), + snapshot_id, + sequence_number, + Some(sequence_number), + ) + .unwrap(); + } + + let mut manifest = writer.write_manifest_file().await.unwrap(); + manifest.sequence_number = snapshot.sequence_number(); + if manifest.min_sequence_number == UNASSIGNED_SEQUENCE_NUMBER { + manifest.min_sequence_number = snapshot.sequence_number(); + } + manifest + } + + /// Writes `manifests` as the manifest list of the named snapshot. + async fn write_deep_history_manifest_list( + &self, + snapshot_id: i64, + manifests: Vec, + ) { + let snapshot = self + .table + .metadata() + .snapshot_by_id(snapshot_id) + .unwrap() + .clone(); + + let output = self + .table + .file_io() + .new_output(snapshot.manifest_list()) + .unwrap() + .writer() + .await + .unwrap(); + let mut writer = ManifestListWriter::v2( + output, + snapshot_id, + snapshot.parent_snapshot_id(), + snapshot.sequence_number(), + ); + writer.add_manifests(manifests.into_iter()).unwrap(); + writer.close().await.unwrap(); + } + /// Writes parquet data files for the deep history fixture (3-column schema: x, y, z). fn write_parquet_data_files_deep_history(&self) -> u64 { fs::create_dir_all(&self.table_location).unwrap(); @@ -2022,6 +2171,22 @@ pub mod tests { assert!(table_scan.is_err()); } + /// The snapshot a standard scan resolved to, which is the single snapshot it + /// sources manifests from. + fn resolved_snapshot_id(scan: &super::TableScan) -> i64 { + let snapshots = &scan + .plan_context + .as_ref() + .expect("scan should have a plan context") + .manifest_list_snapshots; + assert_eq!( + snapshots.len(), + 1, + "a standard scan reads exactly one manifest list" + ); + snapshots[0].snapshot_id() + } + #[tokio::test] async fn test_table_scan_default_snapshot_id() { let table = TableTestFixture::new().table; @@ -2029,7 +2194,7 @@ pub mod tests { let table_scan = table.scan().build().unwrap(); assert_eq!( table.metadata().current_snapshot().unwrap().snapshot_id(), - table_scan.snapshot().unwrap().snapshot_id() + resolved_snapshot_id(&table_scan) ); } @@ -2051,10 +2216,7 @@ pub mod tests { .with_row_selection_enabled(true) .build() .unwrap(); - assert_eq!( - table_scan.snapshot().unwrap().snapshot_id(), - 3051729675574597004 - ); + assert_eq!(resolved_snapshot_id(&table_scan), 3051729675574597004); } fn table_with_property(key: &str, value: &str) -> Table { diff --git a/crates/iceberg/testdata/example_table_metadata_v2_deep_history.json b/crates/iceberg/testdata/example_table_metadata_v2_deep_history.json index bd192ca6e2..c46e9acf81 100644 --- a/crates/iceberg/testdata/example_table_metadata_v2_deep_history.json +++ b/crates/iceberg/testdata/example_table_metadata_v2_deep_history.json @@ -53,7 +53,8 @@ "timestamp-ms": 1515100955770, "sequence-number": 0, "summary": {"operation": "append"}, - "manifest-list": "{{ manifest_list_s1_location }}" + "manifest-list": "{{ manifest_list_s1_location }}", + "schema-id": 1 }, { "snapshot-id": 3055729675574597004, diff --git a/crates/iceberg/testdata/example_table_metadata_v2_deep_history_compaction.json b/crates/iceberg/testdata/example_table_metadata_v2_deep_history_compaction.json index 35f667caf3..19e435fc90 100644 --- a/crates/iceberg/testdata/example_table_metadata_v2_deep_history_compaction.json +++ b/crates/iceberg/testdata/example_table_metadata_v2_deep_history_compaction.json @@ -53,7 +53,8 @@ "timestamp-ms": 1515100955770, "sequence-number": 0, "summary": {"operation": "append"}, - "manifest-list": "{{ manifest_list_s1_location }}" + "manifest-list": "{{ manifest_list_s1_location }}", + "schema-id": 1 }, { "snapshot-id": 3055729675574597004, From 79b27b6838934535c0f28bdec1eb79d43549b59f Mon Sep 17 00:00:00 2001 From: Xander Date: Mon, 24 Aug 2026 12:39:23 +0100 Subject: [PATCH 3/9] Align optional from snapshot --- crates/iceberg/public-api.txt | 8 +- crates/iceberg/src/scan/incremental.rs | 214 +++++++++++++++++-------- crates/iceberg/src/table.rs | 15 +- 3 files changed, 161 insertions(+), 76 deletions(-) diff --git a/crates/iceberg/public-api.txt b/crates/iceberg/public-api.txt index b0f960730a..5ab7c99db0 100644 --- a/crates/iceberg/public-api.txt +++ b/crates/iceberg/public-api.txt @@ -3184,8 +3184,8 @@ pub struct iceberg::table::StaticTable(_) impl iceberg::table::StaticTable pub async fn iceberg::table::StaticTable::from_metadata(metadata: iceberg::spec::TableMetadata, table_ident: iceberg::TableIdent, file_io: iceberg::io::FileIO) -> iceberg::Result pub async fn iceberg::table::StaticTable::from_metadata_file(metadata_location: &str, table_ident: iceberg::TableIdent, file_io: iceberg::io::FileIO) -> iceberg::Result -pub fn iceberg::table::StaticTable::incremental_append_scan(&self, from_snapshot_id: i64, to_snapshot_id: core::option::Option) -> iceberg::scan::IncrementalAppendScanBuilder<'_> -pub fn iceberg::table::StaticTable::incremental_append_scan_inclusive(&self, from_snapshot_id: i64, to_snapshot_id: core::option::Option) -> iceberg::scan::IncrementalAppendScanBuilder<'_> +pub fn iceberg::table::StaticTable::incremental_append_scan(&self, from_snapshot_id: core::option::Option, to_snapshot_id: core::option::Option) -> iceberg::scan::IncrementalAppendScanBuilder<'_> +pub fn iceberg::table::StaticTable::incremental_append_scan_inclusive(&self, from_snapshot_id: core::option::Option, to_snapshot_id: core::option::Option) -> iceberg::scan::IncrementalAppendScanBuilder<'_> pub fn iceberg::table::StaticTable::into_table(self) -> iceberg::table::Table pub fn iceberg::table::StaticTable::metadata(&self) -> iceberg::spec::TableMetadataRef pub fn iceberg::table::StaticTable::reader_builder(&self) -> iceberg::arrow::ArrowReaderBuilder @@ -3201,8 +3201,8 @@ pub fn iceberg::table::Table::current_schema_ref(&self) -> iceberg::spec::Schema pub fn iceberg::table::Table::encryption_manager(&self) -> core::option::Option<&alloc::sync::Arc> pub fn iceberg::table::Table::file_io(&self) -> &iceberg::io::FileIO pub fn iceberg::table::Table::identifier(&self) -> &iceberg::TableIdent -pub fn iceberg::table::Table::incremental_append_scan(&self, from_snapshot_id: i64, to_snapshot_id: core::option::Option) -> iceberg::scan::IncrementalAppendScanBuilder<'_> -pub fn iceberg::table::Table::incremental_append_scan_inclusive(&self, from_snapshot_id: i64, to_snapshot_id: core::option::Option) -> iceberg::scan::IncrementalAppendScanBuilder<'_> +pub fn iceberg::table::Table::incremental_append_scan(&self, from_snapshot_id: core::option::Option, to_snapshot_id: core::option::Option) -> iceberg::scan::IncrementalAppendScanBuilder<'_> +pub fn iceberg::table::Table::incremental_append_scan_inclusive(&self, from_snapshot_id: core::option::Option, to_snapshot_id: core::option::Option) -> iceberg::scan::IncrementalAppendScanBuilder<'_> pub fn iceberg::table::Table::inspect(&self) -> iceberg::inspect::MetadataTable<'_> pub fn iceberg::table::Table::manifest_list_reader(&self, snapshot: &iceberg::spec::SnapshotRef) -> iceberg::spec::ManifestListReader pub fn iceberg::table::Table::manifest_reader(&self) -> iceberg::spec::ManifestReader diff --git a/crates/iceberg/src/scan/incremental.rs b/crates/iceberg/src/scan/incremental.rs index c5a2a6f5ce..0f5133d012 100644 --- a/crates/iceberg/src/scan/incremental.rs +++ b/crates/iceberg/src/scan/incremental.rs @@ -42,28 +42,32 @@ pub(crate) struct AppendRange { impl AppendRange { pub(crate) fn build( table_metadata: &TableMetadataRef, - from_snapshot_id: i64, + from_snapshot_id: Option, to_snapshot_id: i64, from_inclusive: bool, ) -> Result { // Determine the exclusive stop point for the ancestry walk. + // Without a from-snapshot the walk runs to the root, so the range starts at + // the oldest ancestor of the to-snapshot, inclusive. // For inclusive mode the from-snapshot must exist so we can look up // its parent. For exclusive mode the snapshot may have been expired // (the parent pointer on its child still references it), so we only // need the ID — matching Java's BaseIncrementalScan semantics. - let oldest_exclusive = if from_inclusive { - let from_snapshot = - table_metadata - .snapshot_by_id(from_snapshot_id) - .ok_or_else(|| { - Error::new( - ErrorKind::DataInvalid, - format!("Snapshot {from_snapshot_id} not found"), - ) - })?; - from_snapshot.parent_snapshot_id() - } else { - Some(from_snapshot_id) + let oldest_exclusive = match from_snapshot_id { + None => None, + Some(from_snapshot_id) if from_inclusive => { + let from_snapshot = + table_metadata + .snapshot_by_id(from_snapshot_id) + .ok_or_else(|| { + Error::new( + ErrorKind::DataInvalid, + format!("Snapshot {from_snapshot_id} not found"), + ) + })?; + from_snapshot.parent_snapshot_id() + } + Some(from_snapshot_id) => Some(from_snapshot_id), }; let snapshots: Vec<_> = @@ -72,40 +76,43 @@ impl AppendRange { // ancestors_between silently returns the full chain to root if // oldest_exclusive isn't in the ancestry chain. Detect this: // if we got snapshots but from_snapshot_id wasn't encountered as - // the stop point, the chain doesn't connect. - if from_snapshot_id == to_snapshot_id { - // Edge case: from == to. In exclusive mode, range is empty. - // In inclusive mode, we should have exactly one snapshot. - if !from_inclusive { - return Ok(Self { - snapshots: Vec::new(), - }); - } - } else if snapshots.is_empty() { - // to_snapshot_id doesn't exist - return Err(Error::new( - ErrorKind::DataInvalid, - format!( - "from_snapshot {from_snapshot_id} is not an ancestor of to_snapshot {to_snapshot_id}", - ), - )); - } else { - // Verify the oldest snapshot in our walk is actually connected - // to from_snapshot_id. The last snapshot's parent (for exclusive) - // or the last snapshot itself (for inclusive) should be from_snapshot_id. - let oldest_collected = snapshots.last().unwrap(); - let connects = if from_inclusive { - oldest_collected.snapshot_id() == from_snapshot_id - } else { - oldest_collected.parent_snapshot_id() == Some(from_snapshot_id) - }; - if !connects { + // the stop point, the chain doesn't connect. Without a from-snapshot + // there is no ancestry to verify — walking to the root is the intent. + if let Some(from_snapshot_id) = from_snapshot_id { + if from_snapshot_id == to_snapshot_id { + // Edge case: from == to. In exclusive mode, range is empty. + // In inclusive mode, we should have exactly one snapshot. + if !from_inclusive { + return Ok(Self { + snapshots: Vec::new(), + }); + } + } else if snapshots.is_empty() { + // to_snapshot_id doesn't exist return Err(Error::new( ErrorKind::DataInvalid, format!( "from_snapshot {from_snapshot_id} is not an ancestor of to_snapshot {to_snapshot_id}", ), )); + } else { + // Verify the oldest snapshot in our walk is actually connected + // to from_snapshot_id. The last snapshot's parent (for exclusive) + // or the last snapshot itself (for inclusive) should be from_snapshot_id. + let oldest_collected = snapshots.last().unwrap(); + let connects = if from_inclusive { + oldest_collected.snapshot_id() == from_snapshot_id + } else { + oldest_collected.parent_snapshot_id() == Some(from_snapshot_id) + }; + if !connects { + return Err(Error::new( + ErrorKind::DataInvalid, + format!( + "from_snapshot {from_snapshot_id} is not an ancestor of to_snapshot {to_snapshot_id}", + ), + )); + } } } @@ -176,7 +183,7 @@ impl AppendRange { /// [`Table::incremental_append_scan_inclusive`] to create an instance. pub struct IncrementalAppendScanBuilder<'a> { table: &'a Table, - from_snapshot_id: i64, + from_snapshot_id: Option, from_inclusive: bool, to_snapshot_id: Option, column_names: Option>, @@ -193,7 +200,7 @@ pub struct IncrementalAppendScanBuilder<'a> { impl<'a> IncrementalAppendScanBuilder<'a> { pub(crate) fn new( table: &'a Table, - from_snapshot_id: i64, + from_snapshot_id: Option, to_snapshot_id: Option, from_inclusive: bool, ) -> Self { @@ -388,7 +395,7 @@ mod tests { // Exclusive mode doesn't require from-snapshot to exist, but it must // be an ancestor of the to-snapshot. 999999999 is not in the ancestry // chain so this should fail. - let result = table.incremental_append_scan(999999999, None).build(); + let result = table.incremental_append_scan(Some(999999999), None).build(); assert!(result.is_err()); let err = result.unwrap_err(); @@ -404,7 +411,7 @@ mod tests { // Inclusive mode requires from-snapshot to exist (we need its parent ID). let result = table - .incremental_append_scan_inclusive(999999999, None) + .incremental_append_scan_inclusive(Some(999999999), None) .build(); assert!(result.is_err()); @@ -437,7 +444,9 @@ mod tests { Some(s1_id) ); - let result = table.incremental_append_scan(s1_id, Some(s2_id)).build(); + let result = table + .incremental_append_scan(Some(s1_id), Some(s2_id)) + .build(); assert!( result.is_ok(), @@ -450,7 +459,7 @@ mod tests { let table = TableTestFixture::new().table; let result = table - .incremental_append_scan(3051729675574597004, Some(999999999)) + .incremental_append_scan(Some(3051729675574597004), Some(999999999)) .build(); assert!(result.is_err()); @@ -464,7 +473,7 @@ mod tests { let table = TableTestFixture::new().table; let result = table - .incremental_append_scan(3051729675574597004, None) + .incremental_append_scan(Some(3051729675574597004), None) .build(); assert!( result.is_ok(), @@ -492,7 +501,7 @@ mod tests { .expect("Current snapshot should have a parent"); let result = table - .incremental_append_scan(parent_id, Some(current_snapshot_id)) + .incremental_append_scan(Some(parent_id), Some(current_snapshot_id)) .build(); assert!( @@ -509,7 +518,7 @@ mod tests { // Verify the scan builds successfully let result = table - .incremental_append_scan_inclusive(current_snapshot_id, Some(current_snapshot_id)) + .incremental_append_scan_inclusive(Some(current_snapshot_id), Some(current_snapshot_id)) .build(); assert!( result.is_ok(), @@ -519,7 +528,7 @@ mod tests { // Verify AppendRange directly let set = AppendRange::build( &table.metadata_ref(), - current_snapshot_id, + Some(current_snapshot_id), current_snapshot_id, true, ) @@ -538,7 +547,7 @@ mod tests { // Verify the scan builds successfully let result = table - .incremental_append_scan(current_snapshot_id, Some(current_snapshot_id)) + .incremental_append_scan(Some(current_snapshot_id), Some(current_snapshot_id)) .build(); assert!( result.is_ok(), @@ -548,7 +557,7 @@ mod tests { // Verify AppendRange directly let set = AppendRange::build( &table.metadata_ref(), - current_snapshot_id, + Some(current_snapshot_id), current_snapshot_id, false, ) @@ -568,7 +577,7 @@ mod tests { // Scanning from S1 to S5 crosses S4 (overwrite) — should succeed // but only include APPEND snapshots (S2, S3, S5), skipping S4 let result = table - .incremental_append_scan(3051729675574597004, Some(3059729675574597004)) + .incremental_append_scan(Some(3051729675574597004), Some(3059729675574597004)) .build(); assert!( @@ -578,7 +587,7 @@ mod tests { let set = AppendRange::build( &table.metadata_ref(), - 3051729675574597004, + Some(3051729675574597004), 3059729675574597004, false, ) @@ -614,7 +623,7 @@ mod tests { // Scanning from S1 to S3 (all appends) let set = AppendRange::build( &table.metadata_ref(), - 3051729675574597004, + Some(3051729675574597004), 3056729675574597004, false, ) @@ -649,7 +658,10 @@ mod tests { // Incremental scan from S1 (exclusive) to S2 should return only 1.parquet let table_scan = fixture .table - .incremental_append_scan(parent_snapshot_id, Some(current_snapshot.snapshot_id())) + .incremental_append_scan( + Some(parent_snapshot_id), + Some(current_snapshot.snapshot_id()), + ) .build() .unwrap(); @@ -689,7 +701,7 @@ mod tests { // Incremental scan from S2 to S2 (exclusive) should return nothing let table_scan = fixture .table - .incremental_append_scan(current_snapshot_id, Some(current_snapshot_id)) + .incremental_append_scan(Some(current_snapshot_id), Some(current_snapshot_id)) .build() .unwrap(); @@ -730,7 +742,7 @@ mod tests { let scan = fixture .table - .incremental_append_scan(s1_id, Some(s5_id)) + .incremental_append_scan(Some(s1_id), Some(s5_id)) .build() .unwrap(); @@ -742,6 +754,72 @@ mod tests { ); } + #[tokio::test] + async fn test_incremental_scan_without_from_snapshot_starts_at_oldest_ancestor() { + // Deep history: S1 (append) -> S2 -> S3 -> S4 (overwrite) -> S5 (append) + // Omitting from_snapshot_id must scan from the oldest ancestor inclusive, + // so S1's file is included and only the overwrite's file is skipped. + let mut fixture = TableTestFixture::new_with_deep_history(); + fixture.setup_manifest_files_deep_history().await; + + let s5_id = 3059729675574597004_i64; + + let scan = fixture + .table + .incremental_append_scan(None, Some(s5_id)) + .build() + .unwrap(); + + assert_eq!( + planned_file_names(&scan).await, + vec!["s1.parquet", "s2.parquet", "s3.parquet", "s5.parquet"], + "a missing from_snapshot_id scans the whole ancestry, skipping non-appends" + ); + } + + #[tokio::test] + async fn test_incremental_scan_without_from_snapshot_ignores_inclusivity() { + // With no from-snapshot there is nothing to include or exclude, so the + // inclusive and exclusive entry points must agree. + let mut fixture = TableTestFixture::new_with_deep_history(); + fixture.setup_manifest_files_deep_history().await; + + let s5_id = 3059729675574597004_i64; + + let exclusive = fixture + .table + .incremental_append_scan(None, Some(s5_id)) + .build() + .unwrap(); + let inclusive = fixture + .table + .incremental_append_scan_inclusive(None, Some(s5_id)) + .build() + .unwrap(); + + assert_eq!( + planned_file_names(&exclusive).await, + planned_file_names(&inclusive).await + ); + } + + #[test] + fn test_incremental_scan_without_from_snapshot_defaults_to_current_snapshot() { + // Neither end of the range set: the whole history up to the current snapshot. + let table = TableTestFixture::new_with_deep_history().table; + + let range = AppendRange::build(&table.metadata_ref(), None, 3059729675574597004, false) + .unwrap() + .snapshot_ids(); + + // Every append in the chain, but not the overwrite S4. + assert!(range.contains(&3051729675574597004)); + assert!(range.contains(&3055729675574597004)); + assert!(range.contains(&3056729675574597004)); + assert!(!range.contains(&3057729675574597004)); + assert!(range.contains(&3059729675574597004)); + } + #[tokio::test] async fn test_incremental_scan_deep_history_skips_overwrite_files() { // Deep history fixture: @@ -759,7 +837,7 @@ mod tests { let table_scan = fixture .table - .incremental_append_scan(s1_id, Some(s5_id)) + .incremental_append_scan(Some(s1_id), Some(s5_id)) .build() .unwrap(); @@ -808,7 +886,7 @@ mod tests { let table_scan = fixture .table - .incremental_append_scan(s2_id, Some(s3_id)) + .incremental_append_scan(Some(s2_id), Some(s3_id)) .build() .unwrap(); @@ -841,7 +919,7 @@ mod tests { let s5_id = 3059729675574597004_i64; let scan = table - .incremental_append_scan(s1_id, Some(s5_id)) + .incremental_append_scan(Some(s1_id), Some(s5_id)) .build() .unwrap(); @@ -886,7 +964,7 @@ mod tests { let scan = fixture .table - .incremental_append_scan_inclusive(s1_id, Some(s2_id)) + .incremental_append_scan_inclusive(Some(s1_id), Some(s2_id)) .build() .unwrap(); @@ -911,7 +989,7 @@ mod tests { let scan = fixture .table - .incremental_append_scan(s1_id, Some(s4_id)) + .incremental_append_scan(Some(s1_id), Some(s4_id)) .build() .unwrap(); @@ -934,7 +1012,7 @@ mod tests { let scan = fixture .table - .incremental_append_scan(s1_id, Some(s5_id)) + .incremental_append_scan(Some(s1_id), Some(s5_id)) .build() .unwrap(); @@ -957,7 +1035,7 @@ mod tests { let scan = fixture .table - .incremental_append_scan_inclusive(s1_id, Some(s3_id)) + .incremental_append_scan_inclusive(Some(s1_id), Some(s3_id)) .build() .unwrap(); @@ -981,7 +1059,7 @@ mod tests { let table_scan = fixture .table - .incremental_append_scan_inclusive(s3_id, Some(s5_id)) + .incremental_append_scan_inclusive(Some(s3_id), Some(s5_id)) .build() .unwrap(); diff --git a/crates/iceberg/src/table.rs b/crates/iceberg/src/table.rs index baf72c21b2..3387d23e6c 100644 --- a/crates/iceberg/src/table.rs +++ b/crates/iceberg/src/table.rs @@ -286,9 +286,12 @@ impl Table { /// /// Returns only data files added in APPEND snapshots after `from_snapshot_id`, /// up to `to_snapshot_id` or the current snapshot if `None`. + /// + /// If `from_snapshot_id` is `None` the scan starts from the oldest ancestor of + /// the end snapshot, inclusive. pub fn incremental_append_scan( &self, - from_snapshot_id: i64, + from_snapshot_id: Option, to_snapshot_id: Option, ) -> IncrementalAppendScanBuilder<'_> { IncrementalAppendScanBuilder::new(self, from_snapshot_id, to_snapshot_id, false) @@ -298,9 +301,13 @@ impl Table { /// /// Returns only data files added in APPEND snapshots from `from_snapshot_id` (inclusive), /// up to `to_snapshot_id` or the current snapshot if `None`. + /// + /// If `from_snapshot_id` is `None` the scan starts from the oldest ancestor of + /// the end snapshot, inclusive, so this behaves identically to + /// [`Self::incremental_append_scan`]. pub fn incremental_append_scan_inclusive( &self, - from_snapshot_id: i64, + from_snapshot_id: Option, to_snapshot_id: Option, ) -> IncrementalAppendScanBuilder<'_> { IncrementalAppendScanBuilder::new(self, from_snapshot_id, to_snapshot_id, true) @@ -419,7 +426,7 @@ impl StaticTable { /// Creates an incremental append scan starting from the given snapshot (exclusive). pub fn incremental_append_scan( &self, - from_snapshot_id: i64, + from_snapshot_id: Option, to_snapshot_id: Option, ) -> IncrementalAppendScanBuilder<'_> { self.0 @@ -429,7 +436,7 @@ impl StaticTable { /// Creates an incremental append scan starting from the given snapshot (inclusive). pub fn incremental_append_scan_inclusive( &self, - from_snapshot_id: i64, + from_snapshot_id: Option, to_snapshot_id: Option, ) -> IncrementalAppendScanBuilder<'_> { self.0 From 8df36e0431711125bdd7315fb41358107670ce08 Mon Sep 17 00:00:00 2001 From: Xander Date: Mon, 24 Aug 2026 13:08:09 +0100 Subject: [PATCH 4/9] form==to errors --- crates/iceberg/src/scan/incremental.rs | 152 +++++++++++++++---------- crates/iceberg/src/table.rs | 5 + 2 files changed, 97 insertions(+), 60 deletions(-) diff --git a/crates/iceberg/src/scan/incremental.rs b/crates/iceberg/src/scan/incremental.rs index 0f5133d012..397e08ae0b 100644 --- a/crates/iceberg/src/scan/incremental.rs +++ b/crates/iceberg/src/scan/incremental.rs @@ -73,46 +73,38 @@ impl AppendRange { let snapshots: Vec<_> = ancestors_between(table_metadata, to_snapshot_id, oldest_exclusive).collect(); - // ancestors_between silently returns the full chain to root if - // oldest_exclusive isn't in the ancestry chain. Detect this: - // if we got snapshots but from_snapshot_id wasn't encountered as - // the stop point, the chain doesn't connect. Without a from-snapshot - // there is no ancestry to verify — walking to the root is the intent. + // Verify the walk actually reached from_snapshot_id: ancestors_between + // silently returns the whole chain to the root when oldest_exclusive is + // not in the ancestry. Without a from-snapshot there is nothing to + // verify — walking to the root is the intent. + // + // This mirrors the preconditions in Java's BaseIncrementalScan. Inclusive + // mode requires from to be an ancestor of to, and a snapshot is its own + // ancestor, so `from == to` is a valid single-snapshot range. Exclusive + // mode requires an ancestor of to whose parent is from, which `from == to` + // can never satisfy — the walk stops immediately and yields nothing. if let Some(from_snapshot_id) = from_snapshot_id { - if from_snapshot_id == to_snapshot_id { - // Edge case: from == to. In exclusive mode, range is empty. - // In inclusive mode, we should have exactly one snapshot. - if !from_inclusive { - return Ok(Self { - snapshots: Vec::new(), - }); + let connects = snapshots.last().is_some_and(|oldest| { + if from_inclusive { + oldest.snapshot_id() == from_snapshot_id + } else { + oldest.parent_snapshot_id() == Some(from_snapshot_id) } - } else if snapshots.is_empty() { - // to_snapshot_id doesn't exist + }); + + if !connects { return Err(Error::new( ErrorKind::DataInvalid, - format!( - "from_snapshot {from_snapshot_id} is not an ancestor of to_snapshot {to_snapshot_id}", - ), - )); - } else { - // Verify the oldest snapshot in our walk is actually connected - // to from_snapshot_id. The last snapshot's parent (for exclusive) - // or the last snapshot itself (for inclusive) should be from_snapshot_id. - let oldest_collected = snapshots.last().unwrap(); - let connects = if from_inclusive { - oldest_collected.snapshot_id() == from_snapshot_id - } else { - oldest_collected.parent_snapshot_id() == Some(from_snapshot_id) - }; - if !connects { - return Err(Error::new( - ErrorKind::DataInvalid, + if from_inclusive { format!( - "from_snapshot {from_snapshot_id} is not an ancestor of to_snapshot {to_snapshot_id}", - ), - )); - } + "Starting snapshot (inclusive) {from_snapshot_id} is not an ancestor of end snapshot {to_snapshot_id}" + ) + } else { + format!( + "Starting snapshot (exclusive) {from_snapshot_id} is not a parent ancestor of end snapshot {to_snapshot_id}" + ) + }, + )); } } @@ -400,7 +392,7 @@ mod tests { assert!(result.is_err()); let err = result.unwrap_err(); assert!( - err.to_string().contains("not an ancestor"), + err.to_string().contains("is not a parent ancestor"), "Expected ancestry error, got: {err}" ); } @@ -543,29 +535,72 @@ mod tests { fn test_incremental_scan_from_snapshot_exclusive() { // Fixture has S1 (append) -> S2 (append, current) let table = TableTestFixture::new().table; + let parent_id = table + .metadata() + .current_snapshot() + .unwrap() + .parent_snapshot_id() + .unwrap(); let current_snapshot_id = table.metadata().current_snapshot().unwrap().snapshot_id(); - // Verify the scan builds successfully - let result = table + // The from-snapshot itself is excluded from the range. + let range = AppendRange::build( + &table.metadata_ref(), + Some(parent_id), + current_snapshot_id, + false, + ) + .unwrap(); + assert!( + !range.snapshot_ids().contains(&parent_id), + "Exclusive range should not contain the from_snapshot" + ); + assert!( + range.snapshot_ids().contains(¤t_snapshot_id), + "Exclusive range should contain the to_snapshot" + ); + } + + #[test] + fn test_incremental_scan_exclusive_same_snapshot_is_rejected() { + // Java requires an ancestor of the to-snapshot whose parent is the + // from-snapshot, which from == to can never satisfy, so it rejects this + // rather than returning an empty range. Match that. + let table = TableTestFixture::new().table; + let current_snapshot_id = table.metadata().current_snapshot().unwrap().snapshot_id(); + + let err = table .incremental_append_scan(Some(current_snapshot_id), Some(current_snapshot_id)) - .build(); + .build() + .expect_err("exclusive scan from == to should be rejected"); + assert!( - result.is_ok(), - "Exclusive scan from=to should succeed with empty range" + err.to_string().contains("is not a parent ancestor"), + "Expected parent-ancestor error, got: {err}" ); + } - // Verify AppendRange directly - let set = AppendRange::build( + #[test] + fn test_incremental_scan_inclusive_same_snapshot_is_allowed() { + // A snapshot is its own ancestor, so inclusive from == to is a valid + // single-snapshot range in Java. Match that too. + let table = TableTestFixture::new().table; + let current_snapshot_id = table.metadata().current_snapshot().unwrap().snapshot_id(); + + let range = AppendRange::build( &table.metadata_ref(), Some(current_snapshot_id), current_snapshot_id, - false, + true, ) .unwrap(); - assert!( - !set.snapshot_ids().contains(¤t_snapshot_id), - "Exclusive set should not contain the from_snapshot" + + assert_eq!( + range.snapshots().len(), + 1, + "inclusive from == to should yield exactly the one snapshot" ); + assert!(range.snapshot_ids().contains(¤t_snapshot_id)); } #[test] @@ -686,22 +721,19 @@ mod tests { } #[tokio::test] - async fn test_incremental_scan_exclusive_same_snapshot_returns_empty() { - // Fixture has S1 (append) -> S2 (append, current) - let mut fixture = TableTestFixture::new(); - fixture.setup_manifest_files().await; + async fn test_incremental_scan_range_without_appends_returns_empty() { + // Deep history: S3 (append) -> S4 (overwrite). Scanning from S3 exclusive + // to S4 is a valid range, but it contains no append snapshots, so there is + // no manifest list to read and the plan must come back empty. + let mut fixture = TableTestFixture::new_with_deep_history(); + fixture.setup_manifest_files_deep_history().await; - let current_snapshot_id = fixture - .table - .metadata() - .current_snapshot() - .unwrap() - .snapshot_id(); + let s3_id = 3056729675574597004_i64; + let s4_id = 3057729675574597004_i64; - // Incremental scan from S2 to S2 (exclusive) should return nothing let table_scan = fixture .table - .incremental_append_scan(Some(current_snapshot_id), Some(current_snapshot_id)) + .incremental_append_scan(Some(s3_id), Some(s4_id)) .build() .unwrap(); @@ -715,7 +747,7 @@ mod tests { assert!( tasks.is_empty(), - "Exclusive scan from=to should return no files" + "a range containing only non-append snapshots should return no files" ); } diff --git a/crates/iceberg/src/table.rs b/crates/iceberg/src/table.rs index 3387d23e6c..c595027e89 100644 --- a/crates/iceberg/src/table.rs +++ b/crates/iceberg/src/table.rs @@ -289,6 +289,11 @@ impl Table { /// /// If `from_snapshot_id` is `None` the scan starts from the oldest ancestor of /// the end snapshot, inclusive. + /// + /// Building fails if `from_snapshot_id` equals the end snapshot, matching Java: + /// an exclusive range needs an ancestor of the end snapshot whose parent is the + /// from-snapshot. Use [`Self::incremental_append_scan_inclusive`] to scan a + /// single snapshot. pub fn incremental_append_scan( &self, from_snapshot_id: Option, From 2b423da4853301eedd667f114a76db89e6c8eb25 Mon Sep 17 00:00:00 2001 From: Xander Date: Fri, 28 Aug 2026 12:22:35 +0100 Subject: [PATCH 5/9] try new scan struct --- crates/iceberg/public-api.txt | 10 +- crates/iceberg/src/scan/context.rs | 86 +--- crates/iceberg/src/scan/incremental.rs | 191 ++++++-- crates/iceberg/src/scan/mod.rs | 591 ++++++++++++------------- 4 files changed, 445 insertions(+), 433 deletions(-) diff --git a/crates/iceberg/public-api.txt b/crates/iceberg/public-api.txt index 5ab7c99db0..dc075607be 100644 --- a/crates/iceberg/public-api.txt +++ b/crates/iceberg/public-api.txt @@ -1329,9 +1329,16 @@ impl serde_core::ser::Serialize for iceberg::scan::FileScanTaskDeleteFile pub fn iceberg::scan::FileScanTaskDeleteFile::serialize<__S>(&self, __serializer: __S) -> core::result::Result<<__S as serde_core::ser::Serializer>::Ok, <__S as serde_core::ser::Serializer>::Error> where __S: serde_core::ser::Serializer impl<'de> serde_core::de::Deserialize<'de> for iceberg::scan::FileScanTaskDeleteFile pub fn iceberg::scan::FileScanTaskDeleteFile::deserialize<__D>(__deserializer: __D) -> core::result::Result::Error> where __D: serde_core::de::Deserializer<'de> +pub struct iceberg::scan::IncrementalAppendScan +impl iceberg::scan::IncrementalAppendScan +pub fn iceberg::scan::IncrementalAppendScan::column_names(&self) -> core::option::Option<&[alloc::string::String]> +pub async fn iceberg::scan::IncrementalAppendScan::plan_files(&self) -> iceberg::Result +pub async fn iceberg::scan::IncrementalAppendScan::to_arrow(&self) -> iceberg::Result +impl core::fmt::Debug for iceberg::scan::IncrementalAppendScan +pub fn iceberg::scan::IncrementalAppendScan::fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result pub struct iceberg::scan::IncrementalAppendScanBuilder<'a> impl<'a> iceberg::scan::IncrementalAppendScanBuilder<'a> -pub fn iceberg::scan::IncrementalAppendScanBuilder<'a>::build(self) -> iceberg::Result +pub fn iceberg::scan::IncrementalAppendScanBuilder<'a>::build(self) -> iceberg::Result pub fn iceberg::scan::IncrementalAppendScanBuilder<'a>::select(self, column_names: impl core::iter::traits::collect::IntoIterator) -> Self pub fn iceberg::scan::IncrementalAppendScanBuilder<'a>::select_all(self) -> Self pub fn iceberg::scan::IncrementalAppendScanBuilder<'a>::select_empty(self) -> Self @@ -1358,6 +1365,7 @@ pub struct iceberg::scan::TableScan impl iceberg::scan::TableScan pub fn iceberg::scan::TableScan::column_names(&self) -> core::option::Option<&[alloc::string::String]> pub async fn iceberg::scan::TableScan::plan_files(&self) -> iceberg::Result +pub fn iceberg::scan::TableScan::snapshot(&self) -> core::option::Option<&iceberg::spec::SnapshotRef> pub async fn iceberg::scan::TableScan::to_arrow(&self) -> iceberg::Result impl core::fmt::Debug for iceberg::scan::TableScan pub fn iceberg::scan::TableScan::fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result diff --git a/crates/iceberg/src/scan/context.rs b/crates/iceberg/src/scan/context.rs index e465431eac..e66d1873d2 100644 --- a/crates/iceberg/src/scan/context.rs +++ b/crates/iceberg/src/scan/context.rs @@ -15,11 +15,10 @@ // specific language governing permissions and limitations // under the License. -use std::collections::HashSet; use std::sync::Arc; use futures::channel::mpsc::Sender; -use futures::{SinkExt, StreamExt, TryFutureExt, TryStreamExt}; +use futures::{SinkExt, TryFutureExt}; use crate::delete_file_index::DeleteFileIndex; use crate::expr::{Bind, BoundPredicate, Predicate}; @@ -34,10 +33,6 @@ use crate::spec::{ }; use crate::{Error, ErrorKind, Result}; -/// Filter applied to each [`ManifestFile`] before fetching it. -/// Returns `true` to include the manifest, `false` to skip it. -pub(crate) type ManifestFileFilter = Arc bool + Send + Sync>; - /// Filter applied to each manifest entry after loading a manifest. /// Returns `true` to include the entry, `false` to skip it. pub(crate) type ManifestEntryFilter = Arc bool + Send + Sync>; @@ -171,15 +166,11 @@ impl ManifestEntryContext { } } -/// PlanContext holds everything required to perform a scan file plan. +/// PlanContext wraps a [`SnapshotRef`] alongside all the other +/// objects that are required to perform a scan file plan. +#[derive(Debug)] pub(crate) struct PlanContext { - /// Snapshots whose manifest lists are read to source the scan's manifests. - /// - /// A standard scan reads only its own snapshot's list. An incremental scan - /// reads every append snapshot in the range, because operations that rewrite - /// manifests re-emit earlier `ADDED` entries as `EXISTING`, leaving those - /// rows reachable only via the list of the snapshot that added them. - pub manifest_list_snapshots: Vec, + pub snapshot: SnapshotRef, pub table_metadata: TableMetadataRef, pub snapshot_schema: SchemaRef, @@ -193,22 +184,18 @@ pub(crate) struct PlanContext { pub partition_filter_cache: Arc, pub manifest_evaluator_cache: Arc, pub expression_evaluator_cache: Arc, - pub manifest_file_filter: Option, - pub manifest_entry_filter: Option, pub unified_partition_type: Option>, } -impl std::fmt::Debug for PlanContext { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - f.debug_struct("PlanContext") - .field("manifest_list_snapshots", &self.manifest_list_snapshots) - .field("case_sensitive", &self.case_sensitive) - .finish_non_exhaustive() +impl PlanContext { + pub(crate) async fn get_manifest_list(&self) -> Result> { + self.object_cache + .as_ref() + .get_manifest_list(&self.snapshot, &self.table_metadata) + .await } -} -impl PlanContext { /// Returns the partition filter for a manifest. See [`PartitionFilterCache::get`] for the /// always-true fallback when the manifest's spec cannot be resolved against the scan schema. fn get_partition_filter(&self, manifest_file: &ManifestFile) -> Result> { @@ -234,24 +221,12 @@ impl PlanContext { pub(crate) fn build_manifest_file_contexts( &self, - manifest_lists: Vec>, + mut manifest_files: Vec, + manifest_entry_filter: Option, tx_data: Sender, delete_file_idx: DeleteFileIndex, delete_file_tx: Sender, ) -> Result> + 'static>> { - // Manifest lists are cumulative, so a manifest carried forward appears in - // several lists. Deduplicate by path to read each one exactly once, as Java's - // BaseIncrementalAppendScan#appendFilesFromSnapshots does. - let mut seen = HashSet::new(); - let mut manifest_files = Vec::new(); - for manifest_list in &manifest_lists { - for manifest_file in manifest_list.entries() { - if seen.insert(manifest_file.manifest_path.as_str()) { - manifest_files.push(manifest_file); - } - } - } - // Sort manifest files to process delete manifests first. // This avoids a deadlock where the producer blocks on sending data manifest entries // (because the data channel is full) while the delete manifest consumer is waiting @@ -265,13 +240,7 @@ impl PlanContext { // TODO: Ideally we could ditch this intermediate Vec as we return an iterator. let mut filtered_mfcs = vec![]; - for manifest_file in manifest_files { - if let Some(ref filter) = self.manifest_file_filter - && !filter(manifest_file) - { - continue; - } - + for manifest_file in &manifest_files { let tx = if manifest_file.content == ManifestContentType::Deletes { delete_file_tx.clone() } else { @@ -304,6 +273,7 @@ impl PlanContext { partition_bound_predicate, tx, delete_file_idx.clone(), + manifest_entry_filter.clone(), ); filtered_mfcs.push(Ok(mfc)); @@ -318,6 +288,7 @@ impl PlanContext { partition_filter: Option>, sender: Sender, delete_file_index: DeleteFileIndex, + entry_filter: Option, ) -> ManifestFileContext { let bound_predicates = if let (Some(ref partition_bound_predicate), Some(snapshot_bound_predicate)) = @@ -342,7 +313,7 @@ impl PlanContext { delete_file_index, name_mapping: self.name_mapping.clone(), case_sensitive: self.case_sensitive, - entry_filter: self.manifest_entry_filter.clone(), + entry_filter, partition_spec: self .table_metadata .partition_spec_by_id(manifest_file.partition_spec_id) @@ -350,27 +321,4 @@ impl PlanContext { unified_partition_type: self.unified_partition_type.clone(), } } - - /// Reads the manifest list of every snapshot in [`Self::manifest_list_snapshots`], - /// in that order. Ordering only makes planning reproducible. - pub(crate) async fn collect_manifest_lists( - &self, - concurrency_limit: usize, - ) -> Result>> { - let object_cache = self.object_cache.clone(); - let table_metadata = self.table_metadata.clone(); - futures::stream::iter(self.manifest_list_snapshots.clone()) - .map(move |snapshot| { - let object_cache = object_cache.clone(); - let table_metadata = table_metadata.clone(); - async move { - object_cache - .get_manifest_list(&snapshot, &table_metadata) - .await - } - }) - .buffered(concurrency_limit.max(1)) - .try_collect() - .await - } } diff --git a/crates/iceberg/src/scan/incremental.rs b/crates/iceberg/src/scan/incremental.rs index 397e08ae0b..07503a1e8a 100644 --- a/crates/iceberg/src/scan/incremental.rs +++ b/crates/iceberg/src/scan/incremental.rs @@ -20,10 +20,22 @@ use std::collections::HashSet; use std::sync::Arc; +use futures::{StreamExt, TryStreamExt}; + +use crate::arrow::ArrowReaderBuilder; use crate::expr::Predicate; -use crate::scan::context::{ManifestEntryFilter, ManifestFileFilter}; -use crate::scan::{ScanConfig, TableScan, build_table_scan}; -use crate::spec::{ManifestContentType, ManifestStatus, Operation, SnapshotRef, TableMetadataRef}; +use crate::io::FileIO; +use crate::runtime::Runtime; +use crate::scan::context::ManifestEntryFilter; +use crate::scan::{ + ArrowRecordBatchStream, ExpressionEvaluatorCache, FileScanTaskStream, ManifestEvaluatorCache, + PartitionFilterCache, PlanContext, bind_scan_predicate, plan_scan_files, projected_field_ids, + projected_partition_type, table_name_mapping, +}; +use crate::spec::{ + ManifestContentType, ManifestFile, ManifestList, ManifestStatus, Operation, SnapshotRef, + TableMetadataRef, +}; use crate::table::Table; use crate::util::available_parallelism; use crate::util::snapshot::ancestors_between; @@ -118,10 +130,7 @@ impl AppendRange { Ok(Self { snapshots }) } - /// The APPEND snapshots in the range, newest first. Every one's manifest list - /// must be read; see [`PlanContext::manifest_list_snapshots`]. - /// - /// [`PlanContext::manifest_list_snapshots`]: crate::scan::context::PlanContext::manifest_list_snapshots + /// The APPEND snapshots in the range, newest first. pub(crate) fn snapshots(&self) -> &[SnapshotRef] { &self.snapshots } @@ -133,14 +142,20 @@ impl AppendRange { .collect() } - /// Create a manifest file filter that skips delete manifests and data - /// manifests whose `added_snapshot_id` is outside this range. - pub(crate) fn manifest_file_filter(&self) -> ManifestFileFilter { + fn manifest_files(&self, manifest_lists: &[Arc]) -> Vec { let snapshot_ids = self.snapshot_ids(); - Arc::new(move |manifest_file| { - manifest_file.content != ManifestContentType::Deletes - && snapshot_ids.contains(&manifest_file.added_snapshot_id) - }) + let mut seen = HashSet::new(); + + manifest_lists + .iter() + .flat_map(|manifest_list| manifest_list.entries()) + .filter(|manifest_file| { + manifest_file.content != ManifestContentType::Deletes + && snapshot_ids.contains(&manifest_file.added_snapshot_id) + }) + .filter(|manifest_file| seen.insert(manifest_file.manifest_path.clone())) + .cloned() + .collect() } /// Create a manifest entry filter that includes only entries with @@ -156,6 +171,78 @@ impl AppendRange { } } +/// An incremental scan of data appended between two snapshots. +#[derive(Debug)] +pub struct IncrementalAppendScan { + plan_context: PlanContext, + append_range: AppendRange, + batch_size: Option, + file_io: FileIO, + column_names: Option>, + concurrency_limit_manifest_files: usize, + concurrency_limit_manifest_entries: usize, + concurrency_limit_data_files: usize, + row_group_filtering_enabled: bool, + row_selection_enabled: bool, + runtime: Runtime, +} + +impl IncrementalAppendScan { + /// Returns a stream of files appended in the scan's snapshot range. + pub async fn plan_files(&self) -> Result { + let object_cache = self.plan_context.object_cache.clone(); + let table_metadata = self.plan_context.table_metadata.clone(); + let manifest_lists: Vec> = + futures::stream::iter(self.append_range.snapshots().iter().cloned()) + .map(move |snapshot| { + let object_cache = object_cache.clone(); + let table_metadata = table_metadata.clone(); + async move { + object_cache + .get_manifest_list(&snapshot, &table_metadata) + .await + } + }) + .buffered(self.concurrency_limit_manifest_files.max(1)) + .try_collect() + .await?; + let manifest_files = self.append_range.manifest_files(&manifest_lists); + + plan_scan_files( + &self.plan_context, + manifest_files, + Some(self.append_range.manifest_entry_filter()), + &self.runtime, + self.concurrency_limit_manifest_files, + self.concurrency_limit_manifest_entries, + ) + .await + } + + /// Returns an [`ArrowRecordBatchStream`]. + pub async fn to_arrow(&self) -> Result { + let mut arrow_reader_builder = + ArrowReaderBuilder::new(self.file_io.clone(), self.runtime.clone()) + .with_data_file_concurrency_limit(self.concurrency_limit_data_files) + .with_row_group_filtering_enabled(self.row_group_filtering_enabled) + .with_row_selection_enabled(self.row_selection_enabled); + + if let Some(batch_size) = self.batch_size { + arrow_reader_builder = arrow_reader_builder.with_batch_size(batch_size); + } + + arrow_reader_builder + .build() + .read(self.plan_files().await?) + .map(|result| result.stream()) + } + + /// Returns the selected column names. + pub fn column_names(&self) -> Option<&[String]> { + self.column_names.as_deref() + } +} + /// Builder to create an incremental append scan between two snapshots. /// /// An incremental append scan returns only data files that were added in @@ -291,7 +378,7 @@ impl<'a> IncrementalAppendScanBuilder<'a> { } /// Build the incremental append scan. - pub fn build(self) -> Result { + pub fn build(self) -> Result { let to_snapshot = match self.to_snapshot_id { Some(snapshot_id) => self .table @@ -322,28 +409,43 @@ impl<'a> IncrementalAppendScanBuilder<'a> { self.from_inclusive, )?; - build_table_scan( - ScanConfig { - table: self.table, - column_names: self.column_names, - batch_size: self.batch_size, - case_sensitive: self.case_sensitive, - filter: self.filter, - concurrency_limit_data_files: self.concurrency_limit_data_files, - concurrency_limit_manifest_entries: self.concurrency_limit_manifest_entries, - concurrency_limit_manifest_files: self.concurrency_limit_manifest_files, - row_group_filtering_enabled: self.row_group_filtering_enabled, - row_selection_enabled: self.row_selection_enabled, - // Project onto the table's current schema (not the to-snapshot's - // schema), matching the Java and PyIceberg implementations. Rows - // written under an older schema within the range are read against - // the current schema, so newer columns become `NULL`. - schema: self.table.metadata().current_schema().clone(), - }, - append_range.snapshots().to_vec(), - Some(append_range.manifest_file_filter()), - Some(append_range.manifest_entry_filter()), - ) + let schema = self.table.metadata().current_schema().clone(); + let field_ids = + projected_field_ids(&schema, self.column_names.as_deref(), self.case_sensitive)?; + let snapshot_bound_predicate = + bind_scan_predicate(&schema, self.filter.as_ref(), self.case_sensitive)?; + let name_mapping = table_name_mapping(self.table)?; + let unified_partition_type = projected_partition_type(self.table, &schema, &field_ids)?; + + let plan_context = PlanContext { + snapshot: to_snapshot, + table_metadata: self.table.metadata_ref(), + snapshot_schema: schema, + case_sensitive: self.case_sensitive, + predicate: self.filter.map(Arc::new), + snapshot_bound_predicate, + object_cache: self.table.object_cache(), + field_ids: Arc::new(field_ids), + name_mapping, + partition_filter_cache: Arc::new(PartitionFilterCache::new()), + manifest_evaluator_cache: Arc::new(ManifestEvaluatorCache::new()), + expression_evaluator_cache: Arc::new(ExpressionEvaluatorCache::new()), + unified_partition_type, + }; + + Ok(IncrementalAppendScan { + plan_context, + append_range, + batch_size: self.batch_size, + file_io: self.table.file_io().clone(), + column_names: self.column_names, + concurrency_limit_manifest_files: self.concurrency_limit_manifest_files, + concurrency_limit_manifest_entries: self.concurrency_limit_manifest_entries, + concurrency_limit_data_files: self.concurrency_limit_data_files, + row_group_filtering_enabled: self.row_group_filtering_enabled, + row_selection_enabled: self.row_selection_enabled, + runtime: self.table.runtime().clone(), + }) } } @@ -351,13 +453,12 @@ impl<'a> IncrementalAppendScanBuilder<'a> { mod tests { use futures::TryStreamExt; - use super::AppendRange; - use crate::scan::TableScan; + use super::{AppendRange, IncrementalAppendScan}; use crate::scan::tests::TableTestFixture; /// Sorted base names of the data files `scan` yields. Duplicates are kept so /// double-counting is visible. - async fn planned_file_names(scan: &TableScan) -> Vec { + async fn planned_file_names(scan: &IncrementalAppendScan) -> Vec { let tasks: Vec<_> = scan .plan_files() .await @@ -473,10 +574,7 @@ mod tests { ); let scan = result.unwrap(); - assert!( - scan.plan_context.is_some(), - "Incremental scan should have a plan context" - ); + assert_eq!(scan.append_range.snapshots().len(), 1); } #[test] @@ -955,10 +1053,7 @@ mod tests { .build() .unwrap(); - let plan_context = scan - .plan_context - .as_ref() - .expect("incremental scan should have a plan context"); + let plan_context = &scan.plan_context; // The scan must use the current schema (3 columns), not the // to-snapshot's schema (1 column). diff --git a/crates/iceberg/src/scan/mod.rs b/crates/iceberg/src/scan/mod.rs index 680e13b5c1..6be17d50b6 100644 --- a/crates/iceberg/src/scan/mod.rs +++ b/crates/iceberg/src/scan/mod.rs @@ -30,7 +30,7 @@ use arrow_array::RecordBatch; use futures::channel::mpsc::{Sender, channel}; use futures::stream::BoxStream; use futures::{SinkExt, StreamExt, TryStreamExt}; -pub use incremental::IncrementalAppendScanBuilder; +pub use incremental::{IncrementalAppendScan, IncrementalAppendScanBuilder}; pub use task::*; use crate::arrow::ArrowReaderBuilder; @@ -45,7 +45,8 @@ use crate::metadata_columns::{ use crate::partitioning::compute_unified_partition_type; use crate::runtime::Runtime; use crate::spec::{ - DEFAULT_SCHEMA_NAME_MAPPING, DataContentType, NameMapping, Schema, SchemaRef, SnapshotRef, + DEFAULT_SCHEMA_NAME_MAPPING, DataContentType, ManifestFile, NameMapping, Schema, SchemaRef, + SnapshotRef, StructType, }; use crate::table::Table; use crate::util::available_parallelism; @@ -65,60 +66,33 @@ fn resolve_field_id(schema: &Schema, column_name: &str, case_sensitive: bool) -> } } -/// Shared configuration extracted from scan builders, used by both -/// [`TableScanBuilder`] and [`IncrementalAppendScanBuilder`]. -pub(crate) struct ScanConfig<'a> { - table: &'a Table, - column_names: Option>, - batch_size: Option, +fn projected_field_ids( + schema: &Schema, + column_names: Option<&[String]>, case_sensitive: bool, - filter: Option, - concurrency_limit_data_files: usize, - concurrency_limit_manifest_entries: usize, - concurrency_limit_manifest_files: usize, - row_group_filtering_enabled: bool, - row_selection_enabled: bool, - /// Schema to project the scan onto. A standard scan passes the snapshot's - /// own schema (the correct behavior for time-travel scans). An incremental - /// scan passes the table's current schema so that rows written under an - /// older schema in the range are projected onto it (newer columns become - /// `NULL`), matching the Java and PyIceberg implementations. - schema: SchemaRef, -} - -/// Shared build logic: resolves field IDs, binds predicates, and constructs -/// [`PlanContext`] + [`TableScan`]. -pub(crate) fn build_table_scan( - config: ScanConfig<'_>, - manifest_list_snapshots: Vec, - manifest_file_filter: Option, - manifest_entry_filter: Option, -) -> Result { - let schema = config.schema.clone(); - +) -> Result> { let mut field_ids = vec![]; - let column_names = config.column_names.clone().unwrap_or_else(|| { + let column_names = column_names.map(<[String]>::to_vec).unwrap_or_else(|| { schema .as_struct() .fields() .iter() - .map(|f| f.name.clone()) + .map(|field| field.name.clone()) .collect() }); - for column_name in column_names.iter() { + for column_name in &column_names { if is_metadata_column_name(column_name) { field_ids.push(get_metadata_field_id(column_name)?); continue; } - let field_id = - resolve_field_id(&schema, column_name, config.case_sensitive).ok_or_else(|| { - Error::new( - ErrorKind::DataInvalid, - format!("Column {column_name} not found in table. Schema: {schema}"), - ) - })?; + let field_id = resolve_field_id(schema, column_name, case_sensitive).ok_or_else(|| { + Error::new( + ErrorKind::DataInvalid, + format!("Column {column_name} not found in table. Schema: {schema}"), + ) + })?; schema .as_struct() @@ -135,76 +109,58 @@ pub(crate) fn build_table_scan( field_ids.push(field_id); } - let snapshot_bound_predicate = if let Some(ref predicates) = config.filter { - Some(predicates.bind(schema.clone(), config.case_sensitive)?) - } else { - None - }; + Ok(field_ids) +} + +fn bind_scan_predicate( + schema: &SchemaRef, + predicate: Option<&Predicate>, + case_sensitive: bool, +) -> Result>> { + predicate + .map(|predicate| predicate.bind(schema.clone(), case_sensitive)) + .transpose() + .map(|predicate| predicate.map(Arc::new)) +} - let name_mapping = config - .table +fn table_name_mapping(table: &Table) -> Result>> { + Ok(table .metadata() .properties() .get(DEFAULT_SCHEMA_NAME_MAPPING) .map(|raw| { - serde_json::from_str::(raw).map_err(|e| { + serde_json::from_str::(raw).map_err(|error| { Error::new( ErrorKind::DataInvalid, format!( "Failed to parse table property {DEFAULT_SCHEMA_NAME_MAPPING} as a NameMapping" ), ) - .with_source(e) + .with_source(error) }) }) .transpose()? - .map(Arc::new); - - // Compute unified partition type if _partition is projected - let unified_partition_type = if field_ids.contains(&RESERVED_FIELD_ID_PARTITION) { - let partition_type = compute_unified_partition_type( - config - .table - .metadata() - .partition_specs_iter() - .map(|s| s.as_ref()), - &schema, - )?; - Some(Arc::new(partition_type)) - } else { - None - }; + .map(Arc::new)) +} - let plan_context = PlanContext { - manifest_list_snapshots, - table_metadata: config.table.metadata_ref(), - snapshot_schema: schema, - case_sensitive: config.case_sensitive, - predicate: config.filter.map(Arc::new), - snapshot_bound_predicate: snapshot_bound_predicate.map(Arc::new), - object_cache: config.table.object_cache(), - field_ids: Arc::new(field_ids), - name_mapping, - partition_filter_cache: Arc::new(PartitionFilterCache::new()), - manifest_evaluator_cache: Arc::new(ManifestEvaluatorCache::new()), - expression_evaluator_cache: Arc::new(ExpressionEvaluatorCache::new()), - manifest_file_filter, - manifest_entry_filter, - unified_partition_type, - }; +fn projected_partition_type( + table: &Table, + schema: &Schema, + field_ids: &[i32], +) -> Result>> { + if !field_ids.contains(&RESERVED_FIELD_ID_PARTITION) { + return Ok(None); + } - Ok(TableScan { - batch_size: config.batch_size, - column_names: config.column_names, - file_io: config.table.file_io().clone(), - plan_context: Some(plan_context), - concurrency_limit_data_files: config.concurrency_limit_data_files, - concurrency_limit_manifest_entries: config.concurrency_limit_manifest_entries, - concurrency_limit_manifest_files: config.concurrency_limit_manifest_files, - row_group_filtering_enabled: config.row_group_filtering_enabled, - row_selection_enabled: config.row_selection_enabled, - runtime: config.table.runtime().clone(), - }) + compute_unified_partition_type( + table + .metadata() + .partition_specs_iter() + .map(|spec| spec.as_ref()), + schema, + ) + .map(Arc::new) + .map(Some) } /// Builder to create table scan. @@ -378,28 +334,42 @@ impl<'a> TableScanBuilder<'a> { } }; - // A standard scan projects onto the snapshot's own schema, so that - // time-travel reads see the table exactly as it was at that snapshot. let schema = snapshot.schema(self.table.metadata())?; + let field_ids = + projected_field_ids(&schema, self.column_names.as_deref(), self.case_sensitive)?; + let snapshot_bound_predicate = + bind_scan_predicate(&schema, self.filter.as_ref(), self.case_sensitive)?; + let name_mapping = table_name_mapping(self.table)?; + let unified_partition_type = projected_partition_type(self.table, &schema, &field_ids)?; + + let plan_context = PlanContext { + snapshot, + table_metadata: self.table.metadata_ref(), + snapshot_schema: schema, + case_sensitive: self.case_sensitive, + predicate: self.filter.map(Arc::new), + snapshot_bound_predicate, + object_cache: self.table.object_cache(), + field_ids: Arc::new(field_ids), + name_mapping, + partition_filter_cache: Arc::new(PartitionFilterCache::new()), + manifest_evaluator_cache: Arc::new(ManifestEvaluatorCache::new()), + expression_evaluator_cache: Arc::new(ExpressionEvaluatorCache::new()), + unified_partition_type, + }; - build_table_scan( - ScanConfig { - table: self.table, - column_names: self.column_names, - batch_size: self.batch_size, - case_sensitive: self.case_sensitive, - filter: self.filter, - concurrency_limit_data_files: self.concurrency_limit_data_files, - concurrency_limit_manifest_entries: self.concurrency_limit_manifest_entries, - concurrency_limit_manifest_files: self.concurrency_limit_manifest_files, - row_group_filtering_enabled: self.row_group_filtering_enabled, - row_selection_enabled: self.row_selection_enabled, - schema, - }, - vec![snapshot], - None, - None, - ) + Ok(TableScan { + batch_size: self.batch_size, + column_names: self.column_names, + file_io: self.table.file_io().clone(), + plan_context: Some(plan_context), + concurrency_limit_data_files: self.concurrency_limit_data_files, + concurrency_limit_manifest_entries: self.concurrency_limit_manifest_entries, + concurrency_limit_manifest_files: self.concurrency_limit_manifest_files, + row_group_filtering_enabled: self.row_group_filtering_enabled, + row_selection_enabled: self.row_selection_enabled, + runtime: self.table.runtime().clone(), + }) } } @@ -431,128 +401,126 @@ pub struct TableScan { runtime: Runtime, } -impl TableScan { - /// Returns a stream of [`FileScanTask`]s. - pub async fn plan_files(&self) -> Result { - let Some(plan_context) = self.plan_context.as_ref() else { - return Ok(Box::pin(futures::stream::empty())); - }; +pub(crate) async fn plan_scan_files( + plan_context: &PlanContext, + manifest_files: Vec, + manifest_entry_filter: Option, + runtime: &Runtime, + concurrency_limit_manifest_files: usize, + concurrency_limit_manifest_entries: usize, +) -> Result { + let (manifest_entry_data_ctx_tx, manifest_entry_data_ctx_rx) = + channel(concurrency_limit_manifest_files); + let (manifest_entry_delete_ctx_tx, manifest_entry_delete_ctx_rx) = + channel(concurrency_limit_manifest_files); + let (file_scan_task_tx, file_scan_task_rx) = channel(concurrency_limit_manifest_entries); - let concurrency_limit_manifest_files = self.concurrency_limit_manifest_files; - let concurrency_limit_manifest_entries = self.concurrency_limit_manifest_entries; + let (delete_file_idx, delete_file_tx) = DeleteFileIndex::new(runtime.clone()); - // used to stream ManifestEntryContexts between stages of the file plan operation - let (manifest_entry_data_ctx_tx, manifest_entry_data_ctx_rx) = - channel(concurrency_limit_manifest_files); - let (manifest_entry_delete_ctx_tx, manifest_entry_delete_ctx_rx) = - channel(concurrency_limit_manifest_files); + let manifest_file_contexts = plan_context.build_manifest_file_contexts( + manifest_files, + manifest_entry_filter, + manifest_entry_data_ctx_tx, + delete_file_idx.clone(), + manifest_entry_delete_ctx_tx, + )?; - // used to stream the results back to the caller - let (file_scan_task_tx, file_scan_task_rx) = channel(concurrency_limit_manifest_entries); + let mut channel_for_manifest_error = file_scan_task_tx.clone(); + let mut channel_for_data_manifest_entry_error = file_scan_task_tx.clone(); + let mut channel_for_delete_manifest_entry_error = file_scan_task_tx.clone(); - let (delete_file_idx, delete_file_tx) = DeleteFileIndex::new(self.runtime.clone()); + let rt = runtime.clone(); - let manifest_lists = plan_context - .collect_manifest_lists(concurrency_limit_manifest_files) - .await?; + rt.io().spawn(async move { + let result = futures::stream::iter(manifest_file_contexts) + .try_for_each_concurrent(concurrency_limit_manifest_files, |ctx| async move { + ctx.fetch_manifest_and_stream_manifest_entries().await + }) + .await; - // get the [`ManifestFile`]s from the [`ManifestList`]s, filtering out any - // whose partitions cannot match this - // scan's filter - let manifest_file_contexts = plan_context.build_manifest_file_contexts( - manifest_lists, - manifest_entry_data_ctx_tx, - delete_file_idx.clone(), - manifest_entry_delete_ctx_tx, - )?; + if let Err(error) = result { + let _ = channel_for_manifest_error.send(Err(error)).await; + } + }); - let mut channel_for_manifest_error = file_scan_task_tx.clone(); - let mut channel_for_data_manifest_entry_error = file_scan_task_tx.clone(); - let mut channel_for_delete_manifest_entry_error = file_scan_task_tx.clone(); + { + let rt = rt.clone(); + let rt_inner = rt.clone(); + rt.cpu().spawn(async move { + let result = manifest_entry_delete_ctx_rx + .map(|me_ctx| Ok((me_ctx, delete_file_tx.clone()))) + .try_for_each_concurrent( + concurrency_limit_manifest_entries, + |(manifest_entry_context, tx)| { + let rt_inner = rt_inner.clone(); + async move { + rt_inner + .cpu() + .spawn(async move { + process_delete_manifest_entry(manifest_entry_context, tx).await + }) + .await? + } + }, + ) + .await; - let rt = self.runtime.clone(); + if let Err(error) = result { + let _ = channel_for_delete_manifest_entry_error + .send(Err(error)) + .await; + } + }); + } - // Concurrently load all [`Manifest`]s and stream their [`ManifestEntry`]s - rt.io().spawn(async move { - let result = futures::stream::iter(manifest_file_contexts) - .try_for_each_concurrent(concurrency_limit_manifest_files, |ctx| async move { - ctx.fetch_manifest_and_stream_manifest_entries().await - }) + { + let rt_inner = rt.clone(); + rt.cpu().spawn(async move { + let result = manifest_entry_data_ctx_rx + .map(|me_ctx| Ok((me_ctx, file_scan_task_tx.clone()))) + .try_for_each_concurrent( + concurrency_limit_manifest_entries, + |(manifest_entry_context, tx)| { + let rt_inner = rt_inner.clone(); + async move { + rt_inner + .cpu() + .spawn(async move { + process_data_manifest_entry(manifest_entry_context, tx).await + }) + .await? + } + }, + ) .await; if let Err(error) = result { - let _ = channel_for_manifest_error.send(Err(error)).await; + let _ = channel_for_data_manifest_entry_error.send(Err(error)).await; } }); + } - // Process the delete file [`ManifestEntry`] stream in parallel - { - let rt = rt.clone(); - let rt_inner = rt.clone(); - rt.cpu().spawn(async move { - let result = manifest_entry_delete_ctx_rx - .map(|me_ctx| Ok((me_ctx, delete_file_tx.clone()))) - .try_for_each_concurrent( - concurrency_limit_manifest_entries, - |(manifest_entry_context, tx)| { - let rt_inner = rt_inner.clone(); - async move { - rt_inner - .cpu() - .spawn(async move { - Self::process_delete_manifest_entry( - manifest_entry_context, - tx, - ) - .await - }) - .await? - } - }, - ) - .await; - - if let Err(error) = result { - let _ = channel_for_delete_manifest_entry_error - .send(Err(error)) - .await; - } - }); - } + Ok(file_scan_task_rx.boxed()) +} - // Process the data file [`ManifestEntry`] stream in parallel - { - let rt_inner = rt.clone(); - rt.cpu().spawn(async move { - let result = manifest_entry_data_ctx_rx - .map(|me_ctx| Ok((me_ctx, file_scan_task_tx.clone()))) - .try_for_each_concurrent( - concurrency_limit_manifest_entries, - |(manifest_entry_context, tx)| { - let rt_inner = rt_inner.clone(); - async move { - rt_inner - .cpu() - .spawn(async move { - Self::process_data_manifest_entry( - manifest_entry_context, - tx, - ) - .await - }) - .await? - } - }, - ) - .await; +impl TableScan { + /// Returns a stream of [`FileScanTask`]s. + pub async fn plan_files(&self) -> Result { + let Some(plan_context) = self.plan_context.as_ref() else { + return Ok(Box::pin(futures::stream::empty())); + }; - if let Err(error) = result { - let _ = channel_for_data_manifest_entry_error.send(Err(error)).await; - } - }); - } + let manifest_list = plan_context.get_manifest_list().await?; - Ok(file_scan_task_rx.boxed()) + plan_scan_files( + plan_context, + manifest_list.entries().to_vec(), + None, + &self.runtime, + self.concurrency_limit_manifest_files, + self.concurrency_limit_manifest_entries, + ) + .await } /// Returns an [`ArrowRecordBatchStream`]. @@ -578,105 +546,108 @@ impl TableScan { self.column_names.as_deref() } - async fn process_data_manifest_entry( - manifest_entry_context: ManifestEntryContext, - mut file_scan_task_tx: Sender>, - ) -> Result<()> { - // skip processing this manifest entry if it has been marked as deleted - if !manifest_entry_context.manifest_entry.is_alive() { - return Ok(()); - } + /// Returns a reference to the snapshot of the table scan. + pub fn snapshot(&self) -> Option<&SnapshotRef> { + self.plan_context.as_ref().map(|x| &x.snapshot) + } +} - // abort the plan if we encounter a manifest entry for a delete file - if manifest_entry_context.manifest_entry.content_type() != DataContentType::Data { - return Err(Error::new( - ErrorKind::FeatureUnsupported, - "Encountered an entry for a delete file in a data file manifest", - )); - } +async fn process_data_manifest_entry( + manifest_entry_context: ManifestEntryContext, + mut file_scan_task_tx: Sender>, +) -> Result<()> { + // skip processing this manifest entry if it has been marked as deleted + if !manifest_entry_context.manifest_entry.is_alive() { + return Ok(()); + } - if let Some(ref bound_predicates) = manifest_entry_context.bound_predicates { - let BoundPredicates { - snapshot_bound_predicate, - partition_bound_predicate, - } = bound_predicates.as_ref(); + // abort the plan if we encounter a manifest entry for a delete file + if manifest_entry_context.manifest_entry.content_type() != DataContentType::Data { + return Err(Error::new( + ErrorKind::FeatureUnsupported, + "Encountered an entry for a delete file in a data file manifest", + )); + } - let expression_evaluator_cache = - manifest_entry_context.expression_evaluator_cache.as_ref(); + if let Some(ref bound_predicates) = manifest_entry_context.bound_predicates { + let BoundPredicates { + snapshot_bound_predicate, + partition_bound_predicate, + } = bound_predicates.as_ref(); - let expression_evaluator = expression_evaluator_cache.get( - manifest_entry_context.partition_spec_id, - partition_bound_predicate, - )?; + let expression_evaluator_cache = manifest_entry_context.expression_evaluator_cache.as_ref(); - // skip any data file whose partition data indicates that it can't contain - // any data that matches this scan's filter - if !expression_evaluator.eval(manifest_entry_context.manifest_entry.data_file())? { - return Ok(()); - } + let expression_evaluator = expression_evaluator_cache.get( + manifest_entry_context.partition_spec_id, + partition_bound_predicate, + )?; - // skip any data file whose metrics don't match this scan's filter - if !InclusiveMetricsEvaluator::eval( - snapshot_bound_predicate, - manifest_entry_context.manifest_entry.data_file(), - false, - )? { - return Ok(()); - } + // skip any data file whose partition data indicates that it can't contain + // any data that matches this scan's filter + if !expression_evaluator.eval(manifest_entry_context.manifest_entry.data_file())? { + return Ok(()); } - // congratulations! the manifest entry has made its way through the - // entire plan without getting filtered out. Create a corresponding - // FileScanTask and push it to the result stream - file_scan_task_tx - .send(Ok(manifest_entry_context.into_file_scan_task().await?)) - .await?; - - Ok(()) - } - - async fn process_delete_manifest_entry( - manifest_entry_context: ManifestEntryContext, - mut delete_file_ctx_tx: Sender, - ) -> Result<()> { - // skip processing this manifest entry if it has been marked as deleted - if !manifest_entry_context.manifest_entry.is_alive() { + // skip any data file whose metrics don't match this scan's filter + if !InclusiveMetricsEvaluator::eval( + snapshot_bound_predicate, + manifest_entry_context.manifest_entry.data_file(), + false, + )? { return Ok(()); } + } - // abort the plan if we encounter a manifest entry that is not for a delete file - if manifest_entry_context.manifest_entry.content_type() == DataContentType::Data { - return Err(Error::new( - ErrorKind::FeatureUnsupported, - "Encountered an entry for a data file in a delete manifest", - )); - } + // congratulations! the manifest entry has made its way through the + // entire plan without getting filtered out. Create a corresponding + // FileScanTask and push it to the result stream + file_scan_task_tx + .send(Ok(manifest_entry_context.into_file_scan_task().await?)) + .await?; - if let Some(ref bound_predicates) = manifest_entry_context.bound_predicates { - let expression_evaluator_cache = - manifest_entry_context.expression_evaluator_cache.as_ref(); + Ok(()) +} - let expression_evaluator = expression_evaluator_cache.get( - manifest_entry_context.partition_spec_id, - &bound_predicates.partition_bound_predicate, - )?; +async fn process_delete_manifest_entry( + manifest_entry_context: ManifestEntryContext, + mut delete_file_ctx_tx: Sender, +) -> Result<()> { + // skip processing this manifest entry if it has been marked as deleted + if !manifest_entry_context.manifest_entry.is_alive() { + return Ok(()); + } - // skip any data file whose partition data indicates that it can't contain - // any data that matches this scan's filter - if !expression_evaluator.eval(manifest_entry_context.manifest_entry.data_file())? { - return Ok(()); - } - } + // abort the plan if we encounter a manifest entry that is not for a delete file + if manifest_entry_context.manifest_entry.content_type() == DataContentType::Data { + return Err(Error::new( + ErrorKind::FeatureUnsupported, + "Encountered an entry for a data file in a delete manifest", + )); + } - delete_file_ctx_tx - .send(DeleteFileContext { - manifest_entry: manifest_entry_context.manifest_entry.clone(), - partition_spec_id: manifest_entry_context.partition_spec_id, - }) - .await?; + if let Some(ref bound_predicates) = manifest_entry_context.bound_predicates { + let expression_evaluator_cache = manifest_entry_context.expression_evaluator_cache.as_ref(); + + let expression_evaluator = expression_evaluator_cache.get( + manifest_entry_context.partition_spec_id, + &bound_predicates.partition_bound_predicate, + )?; - Ok(()) + // skip any data file whose partition data indicates that it can't contain + // any data that matches this scan's filter + if !expression_evaluator.eval(manifest_entry_context.manifest_entry.data_file())? { + return Ok(()); + } } + + delete_file_ctx_tx + .send(DeleteFileContext { + manifest_entry: manifest_entry_context.manifest_entry.clone(), + partition_spec_id: manifest_entry_context.partition_spec_id, + }) + .await?; + + Ok(()) } pub(crate) struct BoundPredicates { @@ -2168,20 +2139,10 @@ pub mod tests { assert!(table_scan.is_err()); } - /// The snapshot a standard scan resolved to, which is the single snapshot it - /// sources manifests from. fn resolved_snapshot_id(scan: &super::TableScan) -> i64 { - let snapshots = &scan - .plan_context - .as_ref() - .expect("scan should have a plan context") - .manifest_list_snapshots; - assert_eq!( - snapshots.len(), - 1, - "a standard scan reads exactly one manifest list" - ); - snapshots[0].snapshot_id() + scan.snapshot() + .expect("scan should have a snapshot") + .snapshot_id() } #[test] From 8620ebf45ca29542a19503da571662445686ebfe Mon Sep 17 00:00:00 2001 From: Xander Date: Fri, 28 Aug 2026 12:42:15 +0100 Subject: [PATCH 6/9] move test utils --- crates/iceberg/src/inspect/history.rs | 2 +- crates/iceberg/src/inspect/manifests.rs | 2 +- crates/iceberg/src/inspect/snapshots.rs | 2 +- crates/iceberg/src/scan/incremental.rs | 2 +- crates/iceberg/src/scan/mod.rs | 1413 +---------------------- crates/iceberg/src/scan/test_utils.rs | 1409 ++++++++++++++++++++++ crates/iceberg/src/util/snapshot.rs | 2 +- 7 files changed, 1423 insertions(+), 1409 deletions(-) create mode 100644 crates/iceberg/src/scan/test_utils.rs diff --git a/crates/iceberg/src/inspect/history.rs b/crates/iceberg/src/inspect/history.rs index fef9b5d6a3..67591c948c 100644 --- a/crates/iceberg/src/inspect/history.rs +++ b/crates/iceberg/src/inspect/history.rs @@ -126,7 +126,7 @@ mod tests { use crate::TableIdent; use crate::io::FileIO; - use crate::scan::tests::TableTestFixture; + use crate::scan::test_utils::TableTestFixture; use crate::spec::TableMetadata; use crate::table::Table; use crate::test_utils::{check_record_batches, test_runtime}; diff --git a/crates/iceberg/src/inspect/manifests.rs b/crates/iceberg/src/inspect/manifests.rs index 38351a8c54..59549c739d 100644 --- a/crates/iceberg/src/inspect/manifests.rs +++ b/crates/iceberg/src/inspect/manifests.rs @@ -289,7 +289,7 @@ mod tests { use expect_test::expect; use futures::TryStreamExt; - use crate::scan::tests::TableTestFixture; + use crate::scan::test_utils::TableTestFixture; use crate::spec::TableMetadata; use crate::test_utils::check_record_batches; diff --git a/crates/iceberg/src/inspect/snapshots.rs b/crates/iceberg/src/inspect/snapshots.rs index fbed7ec11e..32441249ae 100644 --- a/crates/iceberg/src/inspect/snapshots.rs +++ b/crates/iceberg/src/inspect/snapshots.rs @@ -139,7 +139,7 @@ mod tests { use expect_test::expect; use futures::TryStreamExt; - use crate::scan::tests::TableTestFixture; + use crate::scan::test_utils::TableTestFixture; use crate::test_utils::check_record_batches; #[tokio::test] diff --git a/crates/iceberg/src/scan/incremental.rs b/crates/iceberg/src/scan/incremental.rs index 07503a1e8a..bf38c4306d 100644 --- a/crates/iceberg/src/scan/incremental.rs +++ b/crates/iceberg/src/scan/incremental.rs @@ -454,7 +454,7 @@ mod tests { use futures::TryStreamExt; use super::{AppendRange, IncrementalAppendScan}; - use crate::scan::tests::TableTestFixture; + use crate::scan::test_utils::TableTestFixture; /// Sorted base names of the data files `scan` yields. Duplicates are kept so /// double-counting is visible. diff --git a/crates/iceberg/src/scan/mod.rs b/crates/iceberg/src/scan/mod.rs index 6be17d50b6..d0bcda8699 100644 --- a/crates/iceberg/src/scan/mod.rs +++ b/crates/iceberg/src/scan/mod.rs @@ -23,6 +23,8 @@ mod context; use context::*; mod incremental; mod task; +#[cfg(test)] +pub(crate) mod test_utils; use std::sync::Arc; @@ -656,13 +658,10 @@ pub(crate) struct BoundPredicates { } #[cfg(test)] -pub mod tests { - //! shared tests for the table scan API +mod tests { #![allow(missing_docs)] use std::collections::HashMap; - use std::fs; - use std::fs::File; use std::sync::Arc; use arrow_array::cast::AsArray; @@ -672,41 +671,25 @@ pub mod tests { StringArray, }; use futures::{TryStreamExt, stream}; - use minijinja::value::Value; - use minijinja::{AutoEscape, Environment, context}; - use parquet::arrow::{ArrowWriter, PARQUET_FIELD_ID_META_KEY}; - use parquet::basic::Compression; - use parquet::file::properties::WriterProperties; - use tempfile::TempDir; - use uuid::Uuid; use crate::arrow::ArrowReaderBuilder; use crate::expr::{BoundPredicate, Reference}; - use crate::io::{FileIO, OutputFile}; + use crate::io::FileIO; use crate::metadata_columns::{ - RESERVED_COL_NAME_DELETE_FILE_PATH, RESERVED_COL_NAME_DELETE_FILE_POS, RESERVED_COL_NAME_FILE, RESERVED_COL_NAME_LAST_UPDATED_SEQUENCE_NUMBER, - RESERVED_COL_NAME_POS, RESERVED_COL_NAME_SPEC_ID, RESERVED_FIELD_ID_DELETE_FILE_PATH, - RESERVED_FIELD_ID_DELETE_FILE_POS, RESERVED_FIELD_ID_POS, + RESERVED_COL_NAME_POS, RESERVED_COL_NAME_SPEC_ID, RESERVED_FIELD_ID_POS, }; use crate::scan::FileScanTask; + use crate::scan::test_utils::TableTestFixture; use crate::spec::{ - DEFAULT_SCHEMA_NAME_MAPPING, DataContentType, DataFileBuilder, DataFileFormat, Datum, - FormatVersion, Literal, MAIN_BRANCH, ManifestEntry, ManifestFile, ManifestListWriter, - ManifestStatus, ManifestWriterBuilder, NestedField, Operation, PartitionSpec, - PrimitiveType, Schema, Snapshot, Struct, StructType, Summary, TableMetadata, - TableMetadataBuilder, Type, UNASSIGNED_SEQUENCE_NUMBER, UnboundPartitionSpec, + DEFAULT_SCHEMA_NAME_MAPPING, DataContentType, DataFileFormat, Datum, MAIN_BRANCH, + NestedField, Operation, PrimitiveType, Schema, Snapshot, Summary, TableMetadataBuilder, + Type, UnboundPartitionSpec, }; use crate::table::Table; use crate::test_utils::test_runtime; use crate::{ErrorKind, TableIdent}; - fn render_template(template: &str, ctx: Value) -> String { - let mut env = Environment::new(); - env.set_auto_escape_callback(|_| AutoEscape::None); - env.render_str(template, ctx).unwrap() - } - /// Asserts every row of the `_last_updated_sequence_number` column across all /// batches equals `expected` (or is null when `expected` is `None`), decoding /// the logical value independent of the physical (run-end) encoding. @@ -726,1384 +709,6 @@ pub mod tests { } } - pub struct TableTestFixture { - pub table_location: String, - pub table: Table, - } - - impl TableTestFixture { - #[allow(clippy::new_without_default)] - pub fn new() -> Self { - let tmp_dir = TempDir::new().unwrap(); - let table_location = tmp_dir.path().join("table1"); - let manifest_list1_location = table_location.join("metadata/manifests_list_1.avro"); - let manifest_list2_location = table_location.join("metadata/manifests_list_2.avro"); - let table_metadata1_location = table_location.join("metadata/v1.json"); - - let file_io = FileIO::new_with_fs(); - - let table_metadata = { - let template_json_str = fs::read_to_string(format!( - "{}/testdata/example_table_metadata_v2.json", - env!("CARGO_MANIFEST_DIR") - )) - .unwrap(); - let metadata_json = render_template(&template_json_str, context! { - table_location => &table_location, - manifest_list_1_location => &manifest_list1_location, - manifest_list_2_location => &manifest_list2_location, - table_metadata_1_location => &table_metadata1_location, - }); - serde_json::from_str::(&metadata_json).unwrap() - }; - - let table = Table::builder() - .metadata(table_metadata) - .identifier(TableIdent::from_strs(["db", "table1"]).unwrap()) - .file_io(file_io.clone()) - .metadata_location(table_metadata1_location.as_os_str().to_str().unwrap()) - .runtime(test_runtime()) - .build() - .unwrap(); - - Self { - table_location: table_location.to_str().unwrap().to_string(), - table, - } - } - - #[allow(clippy::new_without_default)] - pub fn new_empty() -> Self { - let tmp_dir = TempDir::new().unwrap(); - let table_location = tmp_dir.path().join("table1"); - let table_metadata1_location = table_location.join("metadata/v1.json"); - - let file_io = FileIO::new_with_fs(); - - let table_metadata = { - let template_json_str = fs::read_to_string(format!( - "{}/testdata/example_empty_table_metadata_v2.json", - env!("CARGO_MANIFEST_DIR") - )) - .unwrap(); - let metadata_json = render_template(&template_json_str, context! { - table_location => &table_location, - table_metadata_1_location => &table_metadata1_location, - }); - serde_json::from_str::(&metadata_json).unwrap() - }; - - let table = Table::builder() - .metadata(table_metadata) - .identifier(TableIdent::from_strs(["db", "table1"]).unwrap()) - .file_io(file_io.clone()) - .metadata_location(table_metadata1_location.as_os_str().to_str().unwrap()) - .runtime(test_runtime()) - .build() - .unwrap(); - - Self { - table_location: table_location.to_str().unwrap().to_string(), - table, - } - } - - /// Creates a fixture with 5 snapshots chained as: - /// S1 (append) -> S2 (append) -> S3 (append) -> S4 (overwrite) -> S5 (append, current) - /// Useful for testing snapshot history traversal and incremental scans - /// with non-append operations in the chain. - pub fn new_with_deep_history() -> Self { - Self::new_from_deep_history_metadata("example_table_metadata_v2_deep_history.json") - } - - /// Like [`Self::new_with_deep_history`] but every snapshot references - /// the older single-column schema (`schema-id` 0) while the table's - /// `current-schema-id` stays at the three-column schema (`schema-id` - /// 1). This models a table whose schema evolved *after* the snapshots - /// in an incremental range were written, so we can assert that an - /// incremental scan projects onto the current schema. - pub fn new_with_deep_history_stale_schema() -> Self { - let fixture = Self::new_from_deep_history_metadata( - "example_table_metadata_v2_deep_history_stale_schema.json", - ); - - // Sanity check: current schema (3 cols) differs from the schema the - // snapshots reference (1 col), otherwise the test would be vacuous. - assert_eq!(fixture.table.metadata().current_schema_id(), 1); - fixture - } - - /// Like [`Self::new_with_deep_history`] but the S4 snapshot is a - /// `replace` (the operation a compaction / `rewrite_data_files` - /// commits) rather than an `overwrite`. Used to prove that an - /// incremental append scan skips compaction output and never - /// double-counts the appended rows against their rewritten copies. - pub fn new_with_deep_history_compaction() -> Self { - Self::new_from_deep_history_metadata( - "example_table_metadata_v2_deep_history_compaction.json", - ) - } - - /// Builds a deep-history fixture from the named templated metadata file - /// in `testdata`. The five snapshot manifest-list paths are rendered to - /// point at this fixture's temp directory. - fn new_from_deep_history_metadata(metadata_file: &str) -> Self { - let tmp_dir = TempDir::new().unwrap(); - let table_location = tmp_dir.path().join("table1"); - let table_metadata1_location = table_location.join("metadata/v1.json"); - - let manifest_list_s1 = table_location.join("metadata/snap-3051729675574597004.avro"); - let manifest_list_s2 = table_location.join("metadata/snap-3055729675574597004.avro"); - let manifest_list_s3 = table_location.join("metadata/snap-3056729675574597004.avro"); - let manifest_list_s4 = table_location.join("metadata/snap-3057729675574597004.avro"); - let manifest_list_s5 = table_location.join("metadata/snap-3059729675574597004.avro"); - - let file_io = FileIO::new_with_fs(); - - let table_metadata = { - let template_json_str = fs::read_to_string(format!( - "{}/testdata/{metadata_file}", - env!("CARGO_MANIFEST_DIR") - )) - .unwrap(); - let metadata_json = render_template(&template_json_str, context! { - table_location => &table_location, - manifest_list_s1_location => &manifest_list_s1, - manifest_list_s2_location => &manifest_list_s2, - manifest_list_s3_location => &manifest_list_s3, - manifest_list_s4_location => &manifest_list_s4, - manifest_list_s5_location => &manifest_list_s5, - }); - serde_json::from_str::(&metadata_json).unwrap() - }; - - let table = Table::builder() - .metadata(table_metadata) - .identifier(TableIdent::from_strs(["db", "table1"]).unwrap()) - .file_io(file_io.clone()) - .metadata_location(table_metadata1_location.as_os_str().to_str().unwrap()) - .runtime(test_runtime()) - .build() - .unwrap(); - - Self { - table_location: table_location.to_str().unwrap().to_string(), - table, - } - } - - pub fn new_unpartitioned() -> Self { - let tmp_dir = TempDir::new().unwrap(); - let table_location = tmp_dir.path().join("table1"); - let manifest_list1_location = table_location.join("metadata/manifests_list_1.avro"); - let manifest_list2_location = table_location.join("metadata/manifests_list_2.avro"); - let table_metadata1_location = table_location.join("metadata/v1.json"); - - let file_io = FileIO::new_with_fs(); - - let mut table_metadata = { - let template_json_str = fs::read_to_string(format!( - "{}/testdata/example_table_metadata_v2.json", - env!("CARGO_MANIFEST_DIR") - )) - .unwrap(); - let metadata_json = render_template(&template_json_str, context! { - table_location => &table_location, - manifest_list_1_location => &manifest_list1_location, - manifest_list_2_location => &manifest_list2_location, - table_metadata_1_location => &table_metadata1_location, - }); - serde_json::from_str::(&metadata_json).unwrap() - }; - - table_metadata.default_spec = Arc::new(PartitionSpec::unpartition_spec()); - table_metadata.partition_specs.clear(); - table_metadata.default_partition_type = StructType::new(vec![]); - table_metadata - .partition_specs - .insert(0, table_metadata.default_spec.clone()); - - let table = Table::builder() - .metadata(table_metadata) - .identifier(TableIdent::from_strs(["db", "table1"]).unwrap()) - .file_io(file_io.clone()) - .metadata_location(table_metadata1_location.to_str().unwrap()) - .runtime(test_runtime()) - .build() - .unwrap(); - - Self { - table_location: table_location.to_str().unwrap().to_string(), - table, - } - } - - pub fn new_with_partition_evolution() -> Self { - let table = Self::new().table; - let table_location = table.metadata().location.clone(); - - let manifest_list1_location = - format!("{}/metadata/manifests_list_1.avro", table_location); - let manifest_list2_location = - format!("{}/metadata/manifests_list_2.avro", table_location); - let manifest_list3_location = - format!("{}/metadata/manifests_list_3.avro", table_location); - let table_metadata1_location = format!("{}/metadata/v1.json", table_location); - - let new_table_metadata = { - let template_json_str = fs::read_to_string(format!( - "{}/testdata/example_table_metadata_v2_partition_evolution.json", - env!("CARGO_MANIFEST_DIR") - )) - .unwrap(); - let metadata_json = render_template(&template_json_str, context! { - table_location => &table_location, - manifest_list_1_location => &manifest_list1_location, - manifest_list_2_location => &manifest_list2_location, - manifest_list_3_location => &manifest_list3_location, - table_metadata_1_location => &table_metadata1_location, - }); - Arc::new(serde_json::from_str::(&metadata_json).unwrap()) - }; - - Self { - table_location, - table: table.with_metadata(new_table_metadata), - } - } - - fn next_manifest_file(&self) -> OutputFile { - self.table - .file_io() - .new_output(format!( - "{}/metadata/manifest_{}.avro", - self.table_location, - Uuid::new_v4() - )) - .unwrap() - } - - pub async fn setup_manifest_files(&mut self) { - let current_snapshot = self.table.metadata().current_snapshot().unwrap(); - let parent_snapshot = current_snapshot - .parent_snapshot(self.table.metadata()) - .unwrap(); - let current_schema = current_snapshot.schema(self.table.metadata()).unwrap(); - let current_partition_spec = self.table.metadata().default_partition_spec(); - - // Write the data files first, then use the file size in the manifest entries - let parquet_file_size = self.write_parquet_data_files(); - - let mut writer = ManifestWriterBuilder::new( - self.next_manifest_file(), - Some(current_snapshot.snapshot_id()), - current_schema.clone(), - current_partition_spec.as_ref().clone(), - ) - .build_v2_data(); - writer - .add_entry( - ManifestEntry::builder() - .status(ManifestStatus::Added) - .data_file( - DataFileBuilder::default() - .partition_spec_id(0) - .content(DataContentType::Data) - .file_path(format!("{}/1.parquet", &self.table_location)) - .file_format(DataFileFormat::Parquet) - .file_size_in_bytes(parquet_file_size) - .record_count(1) - .partition(Struct::from_iter([Some(Literal::long(100))])) - .key_metadata(None) - .build() - .unwrap(), - ) - .build(), - ) - .unwrap(); - writer - .add_delete_entry( - ManifestEntry::builder() - .status(ManifestStatus::Deleted) - .snapshot_id(parent_snapshot.snapshot_id()) - .sequence_number(parent_snapshot.sequence_number()) - .file_sequence_number(parent_snapshot.sequence_number()) - .data_file( - DataFileBuilder::default() - .partition_spec_id(0) - .content(DataContentType::Data) - .file_path(format!("{}/2.parquet", &self.table_location)) - .file_format(DataFileFormat::Parquet) - .file_size_in_bytes(parquet_file_size) - .record_count(1) - .partition(Struct::from_iter([Some(Literal::long(200))])) - .build() - .unwrap(), - ) - .build(), - ) - .unwrap(); - writer - .add_existing_entry( - ManifestEntry::builder() - .status(ManifestStatus::Existing) - .snapshot_id(parent_snapshot.snapshot_id()) - .sequence_number(parent_snapshot.sequence_number()) - .file_sequence_number(parent_snapshot.sequence_number()) - .data_file( - DataFileBuilder::default() - .partition_spec_id(0) - .content(DataContentType::Data) - .file_path(format!("{}/3.parquet", &self.table_location)) - .file_format(DataFileFormat::Parquet) - .file_size_in_bytes(parquet_file_size) - .record_count(1) - .partition(Struct::from_iter([Some(Literal::long(300))])) - .build() - .unwrap(), - ) - .build(), - ) - .unwrap(); - let data_file_manifest = writer.write_manifest_file().await.unwrap(); - - // Write to manifest list - let manifest_list_writer = self - .table - .file_io() - .new_output(current_snapshot.manifest_list()) - .unwrap() - .writer() - .await - .unwrap(); - let mut manifest_list_write = ManifestListWriter::v2( - manifest_list_writer, - current_snapshot.snapshot_id(), - current_snapshot.parent_snapshot_id(), - current_snapshot.sequence_number(), - ); - manifest_list_write - .add_manifests(vec![data_file_manifest].into_iter()) - .unwrap(); - manifest_list_write.close().await.unwrap(); - } - - /// Sets up manifest files for the deep history fixture. - /// - /// Creates one data file per snapshot (s1.parquet through s5.parquet), - /// each with a manifest and manifest list. Manifest lists are cumulative - /// (each snapshot's list includes all prior manifests), matching real - /// Iceberg behavior. The incremental scan should skip s4.parquet - /// (added in the overwrite snapshot S4). - pub async fn setup_manifest_files_deep_history(&mut self) { - let parquet_file_size = self.write_parquet_data_files_deep_history(); - let partition_spec = self.table.metadata().default_partition_spec(); - - // Snapshot chain: S1 -> S2 -> S3 -> S4 (overwrite) -> S5 - let snapshot_ids: Vec = vec![ - 3051729675574597004, - 3055729675574597004, - 3056729675574597004, - 3057729675574597004, - 3059729675574597004, - ]; - - // Accumulate manifests across snapshots (each manifest list is cumulative) - let mut all_manifests: Vec = Vec::new(); - - for (i, &snap_id) in snapshot_ids.iter().enumerate() { - let snapshot = self - .table - .metadata() - .snapshot_by_id(snap_id) - .unwrap() - .clone(); - let schema = snapshot.schema(self.table.metadata()).unwrap(); - - let file_name = format!("s{}.parquet", i + 1); - let partition_value = (i + 1) as i64 * 100; - - let mut writer = ManifestWriterBuilder::new( - self.next_manifest_file(), - Some(snap_id), - schema, - partition_spec.as_ref().clone(), - ) - .build_v2_data(); - - writer - .add_entry( - ManifestEntry::builder() - .status(ManifestStatus::Added) - .data_file( - DataFileBuilder::default() - .partition_spec_id(0) - .content(DataContentType::Data) - .file_path(format!("{}/{}", &self.table_location, file_name)) - .file_format(DataFileFormat::Parquet) - .file_size_in_bytes(parquet_file_size) - .record_count(1) - .partition(Struct::from_iter([Some(Literal::long( - partition_value, - ))])) - .key_metadata(None) - .build() - .unwrap(), - ) - .build(), - ) - .unwrap(); - - let mut data_file_manifest = writer.write_manifest_file().await.unwrap(); - // Assign sequence numbers so the manifest can be included in - // later snapshots' cumulative manifest lists without triggering - // the "unassigned sequence number" validation. - data_file_manifest.sequence_number = snapshot.sequence_number(); - data_file_manifest.min_sequence_number = snapshot.sequence_number(); - all_manifests.push(data_file_manifest); - - // Write cumulative manifest list for this snapshot - let manifest_list_writer = self - .table - .file_io() - .new_output(snapshot.manifest_list()) - .unwrap() - .writer() - .await - .unwrap(); - let mut manifest_list_write = ManifestListWriter::v2( - manifest_list_writer, - snap_id, - snapshot.parent_snapshot_id(), - snapshot.sequence_number(), - ); - manifest_list_write - .add_manifests(all_manifests.clone().into_iter()) - .unwrap(); - manifest_list_write.close().await.unwrap(); - } - } - - /// Like [`Self::setup_manifest_files_deep_history`], but the manifest lists - /// model writers that *rewrite* manifests rather than carrying every one - /// forward verbatim: - /// - /// ```text - /// S1 append -> [A1] A1 = {s1 ADDED@S1} - /// S2 append -> [M2] M2 = {s1 EXISTING@S1, s2 ADDED@S2} (merge-append: A1 is gone) - /// S3 append -> [M2, A3] A3 = {s3 ADDED@S3} - /// S4 overwrite -> [C4] C4 = {s1,s2,s3 EXISTING, s4 ADDED@S4} (rewrite: M2, A3 are gone) - /// S5 append -> [C4, A5] A5 = {s5 ADDED@S5} - /// ``` - /// - /// The surviving copies of the earlier entries are `EXISTING`, not `ADDED`, so - /// a scan reading only the to-snapshot's list silently drops them. - pub async fn setup_manifest_files_deep_history_rewritten(&mut self) { - let file_size = self.write_parquet_data_files_deep_history(); - - let (s1, s2, s3, s4, s5) = ( - 3051729675574597004_i64, - 3055729675574597004_i64, - 3056729675574597004_i64, - 3057729675574597004_i64, - 3059729675574597004_i64, - ); - - // (file index, originating snapshot, that snapshot's sequence number) - let (f1, f2, f3, f4, f5) = ((1, s1, 0), (2, s2, 1), (3, s3, 2), (4, s4, 3), (5, s5, 4)); - - let a1 = self - .write_rewritten_manifest(s1, &[f1], &[], file_size) - .await; - let m2 = self - .write_rewritten_manifest(s2, &[f2], &[f1], file_size) - .await; - let a3 = self - .write_rewritten_manifest(s3, &[f3], &[], file_size) - .await; - let c4 = self - .write_rewritten_manifest(s4, &[f4], &[f1, f2, f3], file_size) - .await; - let a5 = self - .write_rewritten_manifest(s5, &[f5], &[], file_size) - .await; - - self.write_deep_history_manifest_list(s1, vec![a1]).await; - self.write_deep_history_manifest_list(s2, vec![m2.clone()]) - .await; - self.write_deep_history_manifest_list(s3, vec![m2, a3]) - .await; - self.write_deep_history_manifest_list(s4, vec![c4.clone()]) - .await; - self.write_deep_history_manifest_list(s5, vec![c4, a5]) - .await; - } - - /// Writes one data manifest owned by `owner_snapshot_id`. - /// - /// `added` and `existing` are `(file index, originating snapshot id, sequence - /// number)` triples naming `s{index}.parquet`. Added entries take the owning - /// snapshot's ID (the writer enforces this); existing entries keep the ID and - /// sequence number of the snapshot that first added them. - async fn write_rewritten_manifest( - &self, - owner_snapshot_id: i64, - added: &[(usize, i64, i64)], - existing: &[(usize, i64, i64)], - file_size: u64, - ) -> ManifestFile { - let snapshot = self - .table - .metadata() - .snapshot_by_id(owner_snapshot_id) - .unwrap() - .clone(); - let schema = snapshot.schema(self.table.metadata()).unwrap(); - let partition_spec = self.table.metadata().default_partition_spec(); - - let mut writer = ManifestWriterBuilder::new( - self.next_manifest_file(), - Some(owner_snapshot_id), - schema, - partition_spec.as_ref().clone(), - ) - .build_v2_data(); - - let data_file = |index: usize| { - DataFileBuilder::default() - .partition_spec_id(0) - .content(DataContentType::Data) - .file_path(format!("{}/s{}.parquet", &self.table_location, index)) - .file_format(DataFileFormat::Parquet) - .file_size_in_bytes(file_size) - .record_count(1) - .partition(Struct::from_iter([Some(Literal::long(index as i64 * 100))])) - .key_metadata(None) - .build() - .unwrap() - }; - - for &(index, _, sequence_number) in added { - writer.add_file(data_file(index), sequence_number).unwrap(); - } - for &(index, snapshot_id, sequence_number) in existing { - writer - .add_existing_file( - data_file(index), - snapshot_id, - sequence_number, - Some(sequence_number), - ) - .unwrap(); - } - - let mut manifest = writer.write_manifest_file().await.unwrap(); - manifest.sequence_number = snapshot.sequence_number(); - if manifest.min_sequence_number == UNASSIGNED_SEQUENCE_NUMBER { - manifest.min_sequence_number = snapshot.sequence_number(); - } - manifest - } - - /// Writes `manifests` as the manifest list of the named snapshot. - async fn write_deep_history_manifest_list( - &self, - snapshot_id: i64, - manifests: Vec, - ) { - let snapshot = self - .table - .metadata() - .snapshot_by_id(snapshot_id) - .unwrap() - .clone(); - - let output = self - .table - .file_io() - .new_output(snapshot.manifest_list()) - .unwrap() - .writer() - .await - .unwrap(); - let mut writer = ManifestListWriter::v2( - output, - snapshot_id, - snapshot.parent_snapshot_id(), - snapshot.sequence_number(), - ); - writer.add_manifests(manifests.into_iter()).unwrap(); - writer.close().await.unwrap(); - } - - /// Writes parquet data files for the deep history fixture (3-column schema: x, y, z). - fn write_parquet_data_files_deep_history(&self) -> u64 { - fs::create_dir_all(&self.table_location).unwrap(); - - let schema = { - let fields = vec![ - arrow_schema::Field::new("x", arrow_schema::DataType::Int64, false) - .with_metadata(HashMap::from([( - PARQUET_FIELD_ID_META_KEY.to_string(), - "1".to_string(), - )])), - arrow_schema::Field::new("y", arrow_schema::DataType::Int64, false) - .with_metadata(HashMap::from([( - PARQUET_FIELD_ID_META_KEY.to_string(), - "2".to_string(), - )])), - arrow_schema::Field::new("z", arrow_schema::DataType::Int64, false) - .with_metadata(HashMap::from([( - PARQUET_FIELD_ID_META_KEY.to_string(), - "3".to_string(), - )])), - ]; - Arc::new(arrow_schema::Schema::new(fields)) - }; - - let col1 = Arc::new(Int64Array::from_iter_values(vec![1; 10])) as ArrayRef; - let col2 = Arc::new(Int64Array::from_iter_values(vec![2; 10])) as ArrayRef; - let col3 = Arc::new(Int64Array::from_iter_values(vec![3; 10])) as ArrayRef; - - let batch = RecordBatch::try_new(schema.clone(), vec![col1, col2, col3]).unwrap(); - - let props = WriterProperties::builder() - .set_compression(Compression::SNAPPY) - .build(); - - for i in 1..=5 { - let file = - File::create(format!("{}/s{}.parquet", &self.table_location, i)).unwrap(); - let mut writer = - ArrowWriter::try_new(file, batch.schema(), Some(props.clone())).unwrap(); - writer.write(&batch).expect("Writing batch"); - writer.close().unwrap(); - } - - fs::metadata(format!("{}/s1.parquet", &self.table_location)) - .unwrap() - .len() - } - - /// Writes a v3 data manifest with a manifest-level `first_row_id` of 42, - /// so live entries inherit a per-file `first_row_id` on read. Upgrades the - /// table to v3 first, so the manifest list is read as v3. - pub async fn setup_v3_manifest_files(&mut self) { - let metadata = TableMetadataBuilder::new_from_metadata( - self.table.metadata().clone(), - self.table.metadata_location().map(str::to_string), - ) - .upgrade_format_version(FormatVersion::V3) - .unwrap() - .build() - .unwrap() - .metadata; - self.table = Table::builder() - .metadata(metadata) - .identifier(self.table.identifier().clone()) - .file_io(self.table.file_io().clone()) - .metadata_location(self.table.metadata_location().unwrap().to_string()) - .runtime(test_runtime()) - .build() - .unwrap(); - - let current_snapshot = self.table.metadata().current_snapshot().unwrap(); - let current_schema = current_snapshot.schema(self.table.metadata()).unwrap(); - let current_partition_spec = self.table.metadata().default_partition_spec(); - - let parquet_file_size = self.write_parquet_data_files(); - - let mut writer = ManifestWriterBuilder::new( - self.next_manifest_file(), - Some(current_snapshot.snapshot_id()), - current_schema.clone(), - current_partition_spec.as_ref().clone(), - ) - .build_v3_data(); - writer - .add_entry( - ManifestEntry::builder() - .status(ManifestStatus::Added) - .data_file( - DataFileBuilder::default() - .partition_spec_id(0) - .content(DataContentType::Data) - .file_path(format!("{}/1.parquet", &self.table_location)) - .file_format(DataFileFormat::Parquet) - .file_size_in_bytes(parquet_file_size) - .record_count(1) - .partition(Struct::from_iter([Some(Literal::long(100))])) - .key_metadata(None) - .build() - .unwrap(), - ) - .build(), - ) - .unwrap(); - let data_file_manifest = writer.write_manifest_file().await.unwrap(); - - let manifest_list_writer = self - .table - .file_io() - .new_output(current_snapshot.manifest_list()) - .unwrap() - .writer() - .await - .unwrap(); - let mut manifest_list_write = ManifestListWriter::v3( - manifest_list_writer, - current_snapshot.snapshot_id(), - current_snapshot.parent_snapshot_id(), - current_snapshot.sequence_number(), - Some(42), - ); - manifest_list_write - .add_manifests(vec![data_file_manifest].into_iter()) - .unwrap(); - manifest_list_write.close().await.unwrap(); - } - - pub async fn setup_manifest_files_with_partition_evolution(&mut self) { - let current_snapshot = self.table.metadata().current_snapshot().unwrap(); - let parent_snapshot = current_snapshot - .parent_snapshot(self.table.metadata()) - .unwrap(); - let current_schema = current_snapshot.schema(self.table.metadata()).unwrap(); - let current_partition_spec = self.table.metadata().default_partition_spec(); - - // Write the data files first, then use the file size in the manifest entries - let parquet_file_size = self.write_parquet_data_files(); - - let mut writer = ManifestWriterBuilder::new( - self.next_manifest_file(), - Some(current_snapshot.snapshot_id()), - current_schema.clone(), - current_partition_spec.as_ref().clone(), - ) - .build_v2_data(); - writer - .add_entry( - ManifestEntry::builder() - .status(ManifestStatus::Added) - .data_file( - DataFileBuilder::default() - .partition_spec_id(1) - .content(DataContentType::Data) - .file_path(format!("{}/1.parquet", &self.table_location)) - .file_format(DataFileFormat::Parquet) - .file_size_in_bytes(parquet_file_size) - .record_count(1) - .partition(Struct::from_iter([ - Some(Literal::long(100)), - Some(Literal::string("apa")), - Some(Literal::int(27)), - ])) - .key_metadata(None) - .build() - .unwrap(), - ) - .build(), - ) - .unwrap(); - writer - .add_delete_entry( - ManifestEntry::builder() - .status(ManifestStatus::Deleted) - .snapshot_id(parent_snapshot.snapshot_id()) - .sequence_number(parent_snapshot.sequence_number()) - .file_sequence_number(parent_snapshot.sequence_number()) - .data_file( - DataFileBuilder::default() - .partition_spec_id(1) - .content(DataContentType::Data) - .file_path(format!("{}/2.parquet", &self.table_location)) - .file_format(DataFileFormat::Parquet) - .file_size_in_bytes(parquet_file_size) - .record_count(1) - .partition(Struct::from_iter([ - Some(Literal::long(200)), - Some(Literal::string("ice")), - Some(Literal::int(5)), - ])) - .build() - .unwrap(), - ) - .build(), - ) - .unwrap(); - writer - .add_existing_entry( - ManifestEntry::builder() - .status(ManifestStatus::Existing) - .snapshot_id(parent_snapshot.snapshot_id()) - .sequence_number(parent_snapshot.sequence_number()) - .file_sequence_number(parent_snapshot.sequence_number()) - .data_file( - DataFileBuilder::default() - .partition_spec_id(1) - .content(DataContentType::Data) - .file_path(format!("{}/3.parquet", &self.table_location)) - .file_format(DataFileFormat::Parquet) - .file_size_in_bytes(parquet_file_size) - .record_count(1) - .partition(Struct::from_iter([ - Some(Literal::long(300)), - Some(Literal::string("apa")), - Some(Literal::int(19)), - ])) - .build() - .unwrap(), - ) - .build(), - ) - .unwrap(); - let data_file_manifest = writer.write_manifest_file().await.unwrap(); - - // Write to manifest list - let manifest_list_writer = self - .table - .file_io() - .new_output(current_snapshot.manifest_list()) - .unwrap() - .writer() - .await - .unwrap(); - let mut manifest_list_write = ManifestListWriter::v2( - manifest_list_writer, - current_snapshot.snapshot_id(), - current_snapshot.parent_snapshot_id(), - current_snapshot.sequence_number(), - ); - manifest_list_write - .add_manifests(vec![data_file_manifest].into_iter()) - .unwrap(); - manifest_list_write.close().await.unwrap(); - } - - /// Writes identical Parquet data files (1.parquet, 2.parquet, 3.parquet) - /// and returns the file size in bytes. - fn write_parquet_data_files(&self) -> u64 { - fs::create_dir_all(&self.table_location).unwrap(); - - let schema = { - let fields = vec![ - arrow_schema::Field::new("x", arrow_schema::DataType::Int64, false) - .with_metadata(HashMap::from([( - PARQUET_FIELD_ID_META_KEY.to_string(), - "1".to_string(), - )])), - arrow_schema::Field::new("y", arrow_schema::DataType::Int64, false) - .with_metadata(HashMap::from([( - PARQUET_FIELD_ID_META_KEY.to_string(), - "2".to_string(), - )])), - arrow_schema::Field::new("z", arrow_schema::DataType::Int64, false) - .with_metadata(HashMap::from([( - PARQUET_FIELD_ID_META_KEY.to_string(), - "3".to_string(), - )])), - arrow_schema::Field::new("a", arrow_schema::DataType::Utf8, false) - .with_metadata(HashMap::from([( - PARQUET_FIELD_ID_META_KEY.to_string(), - "4".to_string(), - )])), - arrow_schema::Field::new("dbl", arrow_schema::DataType::Float64, false) - .with_metadata(HashMap::from([( - PARQUET_FIELD_ID_META_KEY.to_string(), - "5".to_string(), - )])), - arrow_schema::Field::new("i32", arrow_schema::DataType::Int32, false) - .with_metadata(HashMap::from([( - PARQUET_FIELD_ID_META_KEY.to_string(), - "6".to_string(), - )])), - arrow_schema::Field::new("i64", arrow_schema::DataType::Int64, false) - .with_metadata(HashMap::from([( - PARQUET_FIELD_ID_META_KEY.to_string(), - "7".to_string(), - )])), - arrow_schema::Field::new("bool", arrow_schema::DataType::Boolean, false) - .with_metadata(HashMap::from([( - PARQUET_FIELD_ID_META_KEY.to_string(), - "8".to_string(), - )])), - ]; - Arc::new(arrow_schema::Schema::new(fields)) - }; - // x: [1, 1, 1, 1, ...] - let col1 = Arc::new(Int64Array::from_iter_values(vec![1; 1024])) as ArrayRef; - - let mut values = vec![2; 512]; - values.append(vec![3; 200].as_mut()); - values.append(vec![4; 300].as_mut()); - values.append(vec![5; 12].as_mut()); - - // y: [2, 2, 2, 2, ..., 3, 3, 3, 3, ..., 4, 4, 4, 4, ..., 5, 5, 5, 5] - let col2 = Arc::new(Int64Array::from_iter_values(values)) as ArrayRef; - - let mut values = vec![3; 512]; - values.append(vec![4; 512].as_mut()); - - // z: [3, 3, 3, 3, ..., 4, 4, 4, 4] - let col3 = Arc::new(Int64Array::from_iter_values(values)) as ArrayRef; - - // a: ["Apache", "Apache", "Apache", ..., "Iceberg", "Iceberg", "Iceberg"] - let mut values = vec!["Apache"; 512]; - values.append(vec!["Iceberg"; 512].as_mut()); - let col4 = Arc::new(StringArray::from_iter_values(values)) as ArrayRef; - - // dbl: - let mut values = vec![100.0f64; 512]; - values.append(vec![150.0f64; 12].as_mut()); - values.append(vec![200.0f64; 500].as_mut()); - let col5 = Arc::new(Float64Array::from_iter_values(values)) as ArrayRef; - - // i32: - let mut values = vec![100i32; 512]; - values.append(vec![150i32; 12].as_mut()); - values.append(vec![200i32; 500].as_mut()); - let col6 = Arc::new(Int32Array::from_iter_values(values)) as ArrayRef; - - // i64: - let mut values = vec![100i64; 512]; - values.append(vec![150i64; 12].as_mut()); - values.append(vec![200i64; 500].as_mut()); - let col7 = Arc::new(Int64Array::from_iter_values(values)) as ArrayRef; - - // bool: - let mut values = vec![false; 512]; - values.append(vec![true; 512].as_mut()); - let values: BooleanArray = values.into(); - let col8 = Arc::new(values) as ArrayRef; - - let to_write = RecordBatch::try_new(schema.clone(), vec![ - col1, col2, col3, col4, col5, col6, col7, col8, - ]) - .unwrap(); - - // Write the Parquet files - let props = WriterProperties::builder() - .set_compression(Compression::SNAPPY) - .build(); - - for n in 1..=3 { - let file = File::create(format!("{}/{}.parquet", &self.table_location, n)).unwrap(); - let mut writer = - ArrowWriter::try_new(file, to_write.schema(), Some(props.clone())).unwrap(); - - writer.write(&to_write).expect("Writing batch"); - - // writer must be closed to write footer - writer.close().unwrap(); - } - - fs::metadata(format!("{}/1.parquet", &self.table_location)) - .unwrap() - .len() - } - - pub async fn setup_unpartitioned_manifest_files(&mut self) { - let current_snapshot = self.table.metadata().current_snapshot().unwrap(); - let parent_snapshot = current_snapshot - .parent_snapshot(self.table.metadata()) - .unwrap(); - let current_schema = current_snapshot.schema(self.table.metadata()).unwrap(); - let current_partition_spec = Arc::new(PartitionSpec::unpartition_spec()); - - // Write the data files first, then use the file size in the manifest entries - let parquet_file_size = self.write_parquet_data_files(); - - // Write data files using an empty partition for unpartitioned tables. - let mut writer = ManifestWriterBuilder::new( - self.next_manifest_file(), - Some(current_snapshot.snapshot_id()), - current_schema.clone(), - current_partition_spec.as_ref().clone(), - ) - .build_v2_data(); - - // Create an empty partition value. - let empty_partition = Struct::empty(); - - writer - .add_entry( - ManifestEntry::builder() - .status(ManifestStatus::Added) - .data_file( - DataFileBuilder::default() - .partition_spec_id(0) - .content(DataContentType::Data) - .file_path(format!("{}/1.parquet", &self.table_location)) - .file_format(DataFileFormat::Parquet) - .file_size_in_bytes(parquet_file_size) - .record_count(1) - .partition(empty_partition.clone()) - .key_metadata(None) - .build() - .unwrap(), - ) - .build(), - ) - .unwrap(); - - writer - .add_delete_entry( - ManifestEntry::builder() - .status(ManifestStatus::Deleted) - .snapshot_id(parent_snapshot.snapshot_id()) - .sequence_number(parent_snapshot.sequence_number()) - .file_sequence_number(parent_snapshot.sequence_number()) - .data_file( - DataFileBuilder::default() - .partition_spec_id(0) - .content(DataContentType::Data) - .file_path(format!("{}/2.parquet", &self.table_location)) - .file_format(DataFileFormat::Parquet) - .file_size_in_bytes(parquet_file_size) - .record_count(1) - .partition(empty_partition.clone()) - .build() - .unwrap(), - ) - .build(), - ) - .unwrap(); - - writer - .add_existing_entry( - ManifestEntry::builder() - .status(ManifestStatus::Existing) - .snapshot_id(parent_snapshot.snapshot_id()) - .sequence_number(parent_snapshot.sequence_number()) - .file_sequence_number(parent_snapshot.sequence_number()) - .data_file( - DataFileBuilder::default() - .partition_spec_id(0) - .content(DataContentType::Data) - .file_path(format!("{}/3.parquet", &self.table_location)) - .file_format(DataFileFormat::Parquet) - .file_size_in_bytes(parquet_file_size) - .record_count(1) - .partition(empty_partition.clone()) - .build() - .unwrap(), - ) - .build(), - ) - .unwrap(); - - let data_file_manifest = writer.write_manifest_file().await.unwrap(); - - // Write to manifest list - let manifest_list_writer = self - .table - .file_io() - .new_output(current_snapshot.manifest_list()) - .unwrap() - .writer() - .await - .unwrap(); - let mut manifest_list_write = ManifestListWriter::v2( - manifest_list_writer, - current_snapshot.snapshot_id(), - current_snapshot.parent_snapshot_id(), - current_snapshot.sequence_number(), - ); - manifest_list_write - .add_manifests(vec![data_file_manifest].into_iter()) - .unwrap(); - manifest_list_write.close().await.unwrap(); - } - - pub async fn setup_deadlock_manifests(&mut self) { - let current_snapshot = self.table.metadata().current_snapshot().unwrap(); - let _parent_snapshot = current_snapshot - .parent_snapshot(self.table.metadata()) - .unwrap(); - let current_schema = current_snapshot.schema(self.table.metadata()).unwrap(); - let current_partition_spec = self.table.metadata().default_partition_spec(); - - // 1. Write DATA manifest with MULTIPLE entries to fill buffer - let mut writer = ManifestWriterBuilder::new( - self.next_manifest_file(), - Some(current_snapshot.snapshot_id()), - current_schema.clone(), - current_partition_spec.as_ref().clone(), - ) - .build_v2_data(); - - // Add 10 data entries - for i in 0..10 { - writer - .add_entry( - ManifestEntry::builder() - .status(ManifestStatus::Added) - .data_file( - DataFileBuilder::default() - .partition_spec_id(0) - .content(DataContentType::Data) - .file_path(format!("{}/{}.parquet", &self.table_location, i)) - .file_format(DataFileFormat::Parquet) - .file_size_in_bytes(100) - .record_count(1) - .partition(Struct::from_iter([Some(Literal::long(100))])) - .key_metadata(None) - .build() - .unwrap(), - ) - .build(), - ) - .unwrap(); - } - let data_manifest = writer.write_manifest_file().await.unwrap(); - - // 2. Write DELETE manifest - let mut writer = ManifestWriterBuilder::new( - self.next_manifest_file(), - Some(current_snapshot.snapshot_id()), - current_schema.clone(), - current_partition_spec.as_ref().clone(), - ) - .build_v2_deletes(); - - writer - .add_entry( - ManifestEntry::builder() - .status(ManifestStatus::Added) - .data_file( - DataFileBuilder::default() - .partition_spec_id(0) - .content(DataContentType::PositionDeletes) - .file_path(format!("{}/del.parquet", &self.table_location)) - .file_format(DataFileFormat::Parquet) - .file_size_in_bytes(100) - .record_count(1) - .partition(Struct::from_iter([Some(Literal::long(100))])) - .build() - .unwrap(), - ) - .build(), - ) - .unwrap(); - let delete_manifest = writer.write_manifest_file().await.unwrap(); - - // Write to manifest list - DATA FIRST then DELETE - // This order is crucial for reproduction - let manifest_list_writer = self - .table - .file_io() - .new_output(current_snapshot.manifest_list()) - .unwrap() - .writer() - .await - .unwrap(); - let mut manifest_list_write = ManifestListWriter::v2( - manifest_list_writer, - current_snapshot.snapshot_id(), - current_snapshot.parent_snapshot_id(), - current_snapshot.sequence_number(), - ); - manifest_list_write - .add_manifests(vec![data_manifest, delete_manifest].into_iter()) - .unwrap(); - manifest_list_write.close().await.unwrap(); - } - - /// Sets up a single data file `mrg.parquet` with three 100-row row groups - /// (column `x` = 1000..1300, so row position `p` carries `x = 1000 + p`) and - /// registers it in the current snapshot. When `delete_positions` is non-empty, - /// also writes a positional delete file targeting those file-absolute positions - /// and registers it in a delete manifest. - /// - /// Used to exercise the `_pos` metadata column through the real `TableScan` - /// planning path across row-group boundaries and (optionally) positional deletes. - pub async fn setup_multi_row_group_manifest(&mut self, delete_positions: &[i64]) { - let current_snapshot = self.table.metadata().current_snapshot().unwrap(); - let current_schema = current_snapshot.schema(self.table.metadata()).unwrap(); - let current_partition_spec = self.table.metadata().default_partition_spec(); - - // The table's spec 0 is identity on `x`, so give the data and delete files a - // fixed partition value. Filter tests deliberately filter on `y` (a - // non-partition column) so pruning is driven by Parquet row-group statistics - // rather than partition values. - let partition = Struct::from_iter([Some(Literal::long(1000))]); - - let (data_file_path, data_file_size) = self.write_multi_row_group_data_file(); - - let mut data_writer = ManifestWriterBuilder::new( - self.next_manifest_file(), - Some(current_snapshot.snapshot_id()), - current_schema.clone(), - current_partition_spec.as_ref().clone(), - ) - .build_v2_data(); - data_writer - .add_entry( - ManifestEntry::builder() - .status(ManifestStatus::Added) - .data_file( - DataFileBuilder::default() - .partition_spec_id(0) - .content(DataContentType::Data) - .file_path(data_file_path.clone()) - .file_format(DataFileFormat::Parquet) - .file_size_in_bytes(data_file_size) - .record_count(300) - .partition(partition.clone()) - .key_metadata(None) - .build() - .unwrap(), - ) - .build(), - ) - .unwrap(); - let data_manifest = data_writer.write_manifest_file().await.unwrap(); - - let mut manifests = vec![data_manifest]; - - if !delete_positions.is_empty() { - let (del_path, del_size) = - self.write_positional_delete_file(&data_file_path, delete_positions); - - let mut delete_writer = ManifestWriterBuilder::new( - self.next_manifest_file(), - Some(current_snapshot.snapshot_id()), - current_schema.clone(), - current_partition_spec.as_ref().clone(), - ) - .build_v2_deletes(); - delete_writer - .add_entry( - ManifestEntry::builder() - .status(ManifestStatus::Added) - .data_file( - DataFileBuilder::default() - .partition_spec_id(0) - .content(DataContentType::PositionDeletes) - .file_path(del_path) - .file_format(DataFileFormat::Parquet) - .file_size_in_bytes(del_size) - .record_count(delete_positions.len() as u64) - .partition(partition.clone()) - .build() - .unwrap(), - ) - .build(), - ) - .unwrap(); - manifests.push(delete_writer.write_manifest_file().await.unwrap()); - } - - let manifest_list_writer = self - .table - .file_io() - .new_output(current_snapshot.manifest_list()) - .unwrap() - .writer() - .await - .unwrap(); - let mut manifest_list_write = ManifestListWriter::v2( - manifest_list_writer, - current_snapshot.snapshot_id(), - current_snapshot.parent_snapshot_id(), - current_snapshot.sequence_number(), - ); - manifest_list_write - .add_manifests(manifests.into_iter()) - .unwrap(); - manifest_list_write.close().await.unwrap(); - } - - /// Writes `mrg.parquet` with three 100-row row groups. Columns `x` (field - /// id `1`) and `y` (field id `2`) both run 1000..1300, so row position `p` - /// carries `x = y = 1000 + p`. Returns `(path, file_size_in_bytes)`. - fn write_multi_row_group_data_file(&self) -> (String, u64) { - fs::create_dir_all(&self.table_location).unwrap(); - - let arrow_schema = Arc::new(arrow_schema::Schema::new(vec![ - arrow_schema::Field::new("x", arrow_schema::DataType::Int64, false).with_metadata( - HashMap::from([(PARQUET_FIELD_ID_META_KEY.to_string(), "1".to_string())]), - ), - arrow_schema::Field::new("y", arrow_schema::DataType::Int64, false).with_metadata( - HashMap::from([(PARQUET_FIELD_ID_META_KEY.to_string(), "2".to_string())]), - ), - ])); - - let path = format!("{}/mrg.parquet", &self.table_location); - let max_row_group_row_count = 100; - let props = WriterProperties::builder() - .set_compression(Compression::SNAPPY) - .set_max_row_group_row_count(Some(max_row_group_row_count)) - .build(); - - let file = File::create(&path).unwrap(); - let mut writer = ArrowWriter::try_new(file, arrow_schema.clone(), Some(props)).unwrap(); - for group in 0..3i64 { - let base = 1000 + group * max_row_group_row_count as i64; - let col = Arc::new(Int64Array::from_iter_values( - base..base + max_row_group_row_count as i64, - )) as ArrayRef; - let batch = - RecordBatch::try_new(arrow_schema.clone(), vec![col.clone(), col]).unwrap(); - writer.write(&batch).unwrap(); - } - writer.close().unwrap(); - - let size = fs::metadata(&path).unwrap().len(); - (path, size) - } - - /// Writes a positional delete file targeting `positions` in `data_path`. - /// Returns `(path, file_size_in_bytes)`. - fn write_positional_delete_file( - &self, - data_path: &str, - positions: &[i64], - ) -> (String, u64) { - let del_schema = Arc::new(arrow_schema::Schema::new(vec![ - arrow_schema::Field::new( - RESERVED_COL_NAME_DELETE_FILE_PATH, - arrow_schema::DataType::Utf8, - false, - ) - .with_metadata(HashMap::from([( - PARQUET_FIELD_ID_META_KEY.to_string(), - RESERVED_FIELD_ID_DELETE_FILE_PATH.to_string(), // 2147483546 - )])), - arrow_schema::Field::new( - RESERVED_COL_NAME_DELETE_FILE_POS, - arrow_schema::DataType::Int64, - false, - ) - .with_metadata(HashMap::from([( - PARQUET_FIELD_ID_META_KEY.to_string(), - RESERVED_FIELD_ID_DELETE_FILE_POS.to_string(), // 2147483545 - )])), - ])); - - let batch = RecordBatch::try_new(del_schema.clone(), vec![ - Arc::new(StringArray::from_iter_values(std::iter::repeat_n( - data_path.to_string(), - positions.len(), - ))) as ArrayRef, - Arc::new(Int64Array::from_iter_values(positions.iter().copied())) as ArrayRef, - ]) - .unwrap(); - - let path = format!("{}/pos-del.parquet", &self.table_location); - let props = WriterProperties::builder() - .set_compression(Compression::SNAPPY) - .build(); - let file = File::create(&path).unwrap(); - let mut writer = ArrowWriter::try_new(file, del_schema, Some(props)).unwrap(); - writer.write(&batch).unwrap(); - writer.close().unwrap(); - - let size = fs::metadata(&path).unwrap().len(); - (path, size) - } - } - #[tokio::test] async fn test_table_scan_columns() { let table = TableTestFixture::new().table; diff --git a/crates/iceberg/src/scan/test_utils.rs b/crates/iceberg/src/scan/test_utils.rs new file mode 100644 index 0000000000..64fb93e7b5 --- /dev/null +++ b/crates/iceberg/src/scan/test_utils.rs @@ -0,0 +1,1409 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +#![allow(missing_docs)] + +use std::collections::HashMap; +use std::fs; +use std::fs::File; +use std::sync::Arc; + +use arrow_array::{ + ArrayRef, BooleanArray, Float64Array, Int32Array, Int64Array, RecordBatch, StringArray, +}; +use minijinja::value::Value; +use minijinja::{AutoEscape, Environment, context}; +use parquet::arrow::{ArrowWriter, PARQUET_FIELD_ID_META_KEY}; +use parquet::basic::Compression; +use parquet::file::properties::WriterProperties; +use tempfile::TempDir; +use uuid::Uuid; + +use crate::TableIdent; +use crate::io::{FileIO, OutputFile}; +use crate::metadata_columns::{ + RESERVED_COL_NAME_DELETE_FILE_PATH, RESERVED_COL_NAME_DELETE_FILE_POS, + RESERVED_FIELD_ID_DELETE_FILE_PATH, RESERVED_FIELD_ID_DELETE_FILE_POS, +}; +use crate::spec::{ + DataContentType, DataFileBuilder, DataFileFormat, FormatVersion, Literal, ManifestEntry, + ManifestFile, ManifestListWriter, ManifestStatus, ManifestWriterBuilder, PartitionSpec, Struct, + StructType, TableMetadata, TableMetadataBuilder, UNASSIGNED_SEQUENCE_NUMBER, +}; +use crate::table::Table; +use crate::test_utils::test_runtime; + +fn render_template(template: &str, ctx: Value) -> String { + let mut env = Environment::new(); + env.set_auto_escape_callback(|_| AutoEscape::None); + env.render_str(template, ctx).unwrap() +} + +pub struct TableTestFixture { + pub table_location: String, + pub table: Table, +} + +impl TableTestFixture { + #[allow(clippy::new_without_default)] + pub fn new() -> Self { + let tmp_dir = TempDir::new().unwrap(); + let table_location = tmp_dir.path().join("table1"); + let manifest_list1_location = table_location.join("metadata/manifests_list_1.avro"); + let manifest_list2_location = table_location.join("metadata/manifests_list_2.avro"); + let table_metadata1_location = table_location.join("metadata/v1.json"); + + let file_io = FileIO::new_with_fs(); + + let table_metadata = { + let template_json_str = fs::read_to_string(format!( + "{}/testdata/example_table_metadata_v2.json", + env!("CARGO_MANIFEST_DIR") + )) + .unwrap(); + let metadata_json = render_template(&template_json_str, context! { + table_location => &table_location, + manifest_list_1_location => &manifest_list1_location, + manifest_list_2_location => &manifest_list2_location, + table_metadata_1_location => &table_metadata1_location, + }); + serde_json::from_str::(&metadata_json).unwrap() + }; + + let table = Table::builder() + .metadata(table_metadata) + .identifier(TableIdent::from_strs(["db", "table1"]).unwrap()) + .file_io(file_io.clone()) + .metadata_location(table_metadata1_location.as_os_str().to_str().unwrap()) + .runtime(test_runtime()) + .build() + .unwrap(); + + Self { + table_location: table_location.to_str().unwrap().to_string(), + table, + } + } + + #[allow(clippy::new_without_default)] + pub fn new_empty() -> Self { + let tmp_dir = TempDir::new().unwrap(); + let table_location = tmp_dir.path().join("table1"); + let table_metadata1_location = table_location.join("metadata/v1.json"); + + let file_io = FileIO::new_with_fs(); + + let table_metadata = { + let template_json_str = fs::read_to_string(format!( + "{}/testdata/example_empty_table_metadata_v2.json", + env!("CARGO_MANIFEST_DIR") + )) + .unwrap(); + let metadata_json = render_template(&template_json_str, context! { + table_location => &table_location, + table_metadata_1_location => &table_metadata1_location, + }); + serde_json::from_str::(&metadata_json).unwrap() + }; + + let table = Table::builder() + .metadata(table_metadata) + .identifier(TableIdent::from_strs(["db", "table1"]).unwrap()) + .file_io(file_io.clone()) + .metadata_location(table_metadata1_location.as_os_str().to_str().unwrap()) + .runtime(test_runtime()) + .build() + .unwrap(); + + Self { + table_location: table_location.to_str().unwrap().to_string(), + table, + } + } + + /// Creates a fixture with 5 snapshots chained as: + /// S1 (append) -> S2 (append) -> S3 (append) -> S4 (overwrite) -> S5 (append, current) + /// Useful for testing snapshot history traversal and incremental scans + /// with non-append operations in the chain. + pub fn new_with_deep_history() -> Self { + Self::new_from_deep_history_metadata("example_table_metadata_v2_deep_history.json") + } + + /// Like [`Self::new_with_deep_history`] but every snapshot references + /// the older single-column schema (`schema-id` 0) while the table's + /// `current-schema-id` stays at the three-column schema (`schema-id` + /// 1). This models a table whose schema evolved *after* the snapshots + /// in an incremental range were written, so we can assert that an + /// incremental scan projects onto the current schema. + pub fn new_with_deep_history_stale_schema() -> Self { + let fixture = Self::new_from_deep_history_metadata( + "example_table_metadata_v2_deep_history_stale_schema.json", + ); + + // Sanity check: current schema (3 cols) differs from the schema the + // snapshots reference (1 col), otherwise the test would be vacuous. + assert_eq!(fixture.table.metadata().current_schema_id(), 1); + fixture + } + + /// Like [`Self::new_with_deep_history`] but the S4 snapshot is a + /// `replace` (the operation a compaction / `rewrite_data_files` + /// commits) rather than an `overwrite`. Used to prove that an + /// incremental append scan skips compaction output and never + /// double-counts the appended rows against their rewritten copies. + pub fn new_with_deep_history_compaction() -> Self { + Self::new_from_deep_history_metadata( + "example_table_metadata_v2_deep_history_compaction.json", + ) + } + + /// Builds a deep-history fixture from the named templated metadata file + /// in `testdata`. The five snapshot manifest-list paths are rendered to + /// point at this fixture's temp directory. + fn new_from_deep_history_metadata(metadata_file: &str) -> Self { + let tmp_dir = TempDir::new().unwrap(); + let table_location = tmp_dir.path().join("table1"); + let table_metadata1_location = table_location.join("metadata/v1.json"); + + let manifest_list_s1 = table_location.join("metadata/snap-3051729675574597004.avro"); + let manifest_list_s2 = table_location.join("metadata/snap-3055729675574597004.avro"); + let manifest_list_s3 = table_location.join("metadata/snap-3056729675574597004.avro"); + let manifest_list_s4 = table_location.join("metadata/snap-3057729675574597004.avro"); + let manifest_list_s5 = table_location.join("metadata/snap-3059729675574597004.avro"); + + let file_io = FileIO::new_with_fs(); + + let table_metadata = { + let template_json_str = fs::read_to_string(format!( + "{}/testdata/{metadata_file}", + env!("CARGO_MANIFEST_DIR") + )) + .unwrap(); + let metadata_json = render_template(&template_json_str, context! { + table_location => &table_location, + manifest_list_s1_location => &manifest_list_s1, + manifest_list_s2_location => &manifest_list_s2, + manifest_list_s3_location => &manifest_list_s3, + manifest_list_s4_location => &manifest_list_s4, + manifest_list_s5_location => &manifest_list_s5, + }); + serde_json::from_str::(&metadata_json).unwrap() + }; + + let table = Table::builder() + .metadata(table_metadata) + .identifier(TableIdent::from_strs(["db", "table1"]).unwrap()) + .file_io(file_io.clone()) + .metadata_location(table_metadata1_location.as_os_str().to_str().unwrap()) + .runtime(test_runtime()) + .build() + .unwrap(); + + Self { + table_location: table_location.to_str().unwrap().to_string(), + table, + } + } + + pub fn new_unpartitioned() -> Self { + let tmp_dir = TempDir::new().unwrap(); + let table_location = tmp_dir.path().join("table1"); + let manifest_list1_location = table_location.join("metadata/manifests_list_1.avro"); + let manifest_list2_location = table_location.join("metadata/manifests_list_2.avro"); + let table_metadata1_location = table_location.join("metadata/v1.json"); + + let file_io = FileIO::new_with_fs(); + + let mut table_metadata = { + let template_json_str = fs::read_to_string(format!( + "{}/testdata/example_table_metadata_v2.json", + env!("CARGO_MANIFEST_DIR") + )) + .unwrap(); + let metadata_json = render_template(&template_json_str, context! { + table_location => &table_location, + manifest_list_1_location => &manifest_list1_location, + manifest_list_2_location => &manifest_list2_location, + table_metadata_1_location => &table_metadata1_location, + }); + serde_json::from_str::(&metadata_json).unwrap() + }; + + table_metadata.default_spec = Arc::new(PartitionSpec::unpartition_spec()); + table_metadata.partition_specs.clear(); + table_metadata.default_partition_type = StructType::new(vec![]); + table_metadata + .partition_specs + .insert(0, table_metadata.default_spec.clone()); + + let table = Table::builder() + .metadata(table_metadata) + .identifier(TableIdent::from_strs(["db", "table1"]).unwrap()) + .file_io(file_io.clone()) + .metadata_location(table_metadata1_location.to_str().unwrap()) + .runtime(test_runtime()) + .build() + .unwrap(); + + Self { + table_location: table_location.to_str().unwrap().to_string(), + table, + } + } + + pub fn new_with_partition_evolution() -> Self { + let table = Self::new().table; + let table_location = table.metadata().location.clone(); + + let manifest_list1_location = format!("{}/metadata/manifests_list_1.avro", table_location); + let manifest_list2_location = format!("{}/metadata/manifests_list_2.avro", table_location); + let manifest_list3_location = format!("{}/metadata/manifests_list_3.avro", table_location); + let table_metadata1_location = format!("{}/metadata/v1.json", table_location); + + let new_table_metadata = { + let template_json_str = fs::read_to_string(format!( + "{}/testdata/example_table_metadata_v2_partition_evolution.json", + env!("CARGO_MANIFEST_DIR") + )) + .unwrap(); + let metadata_json = render_template(&template_json_str, context! { + table_location => &table_location, + manifest_list_1_location => &manifest_list1_location, + manifest_list_2_location => &manifest_list2_location, + manifest_list_3_location => &manifest_list3_location, + table_metadata_1_location => &table_metadata1_location, + }); + Arc::new(serde_json::from_str::(&metadata_json).unwrap()) + }; + + Self { + table_location, + table: table.with_metadata(new_table_metadata), + } + } + + fn next_manifest_file(&self) -> OutputFile { + self.table + .file_io() + .new_output(format!( + "{}/metadata/manifest_{}.avro", + self.table_location, + Uuid::new_v4() + )) + .unwrap() + } + + pub async fn setup_manifest_files(&mut self) { + let current_snapshot = self.table.metadata().current_snapshot().unwrap(); + let parent_snapshot = current_snapshot + .parent_snapshot(self.table.metadata()) + .unwrap(); + let current_schema = current_snapshot.schema(self.table.metadata()).unwrap(); + let current_partition_spec = self.table.metadata().default_partition_spec(); + + // Write the data files first, then use the file size in the manifest entries + let parquet_file_size = self.write_parquet_data_files(); + + let mut writer = ManifestWriterBuilder::new( + self.next_manifest_file(), + Some(current_snapshot.snapshot_id()), + current_schema.clone(), + current_partition_spec.as_ref().clone(), + ) + .build_v2_data(); + writer + .add_entry( + ManifestEntry::builder() + .status(ManifestStatus::Added) + .data_file( + DataFileBuilder::default() + .partition_spec_id(0) + .content(DataContentType::Data) + .file_path(format!("{}/1.parquet", &self.table_location)) + .file_format(DataFileFormat::Parquet) + .file_size_in_bytes(parquet_file_size) + .record_count(1) + .partition(Struct::from_iter([Some(Literal::long(100))])) + .key_metadata(None) + .build() + .unwrap(), + ) + .build(), + ) + .unwrap(); + writer + .add_delete_entry( + ManifestEntry::builder() + .status(ManifestStatus::Deleted) + .snapshot_id(parent_snapshot.snapshot_id()) + .sequence_number(parent_snapshot.sequence_number()) + .file_sequence_number(parent_snapshot.sequence_number()) + .data_file( + DataFileBuilder::default() + .partition_spec_id(0) + .content(DataContentType::Data) + .file_path(format!("{}/2.parquet", &self.table_location)) + .file_format(DataFileFormat::Parquet) + .file_size_in_bytes(parquet_file_size) + .record_count(1) + .partition(Struct::from_iter([Some(Literal::long(200))])) + .build() + .unwrap(), + ) + .build(), + ) + .unwrap(); + writer + .add_existing_entry( + ManifestEntry::builder() + .status(ManifestStatus::Existing) + .snapshot_id(parent_snapshot.snapshot_id()) + .sequence_number(parent_snapshot.sequence_number()) + .file_sequence_number(parent_snapshot.sequence_number()) + .data_file( + DataFileBuilder::default() + .partition_spec_id(0) + .content(DataContentType::Data) + .file_path(format!("{}/3.parquet", &self.table_location)) + .file_format(DataFileFormat::Parquet) + .file_size_in_bytes(parquet_file_size) + .record_count(1) + .partition(Struct::from_iter([Some(Literal::long(300))])) + .build() + .unwrap(), + ) + .build(), + ) + .unwrap(); + let data_file_manifest = writer.write_manifest_file().await.unwrap(); + + // Write to manifest list + let manifest_list_writer = self + .table + .file_io() + .new_output(current_snapshot.manifest_list()) + .unwrap() + .writer() + .await + .unwrap(); + let mut manifest_list_write = ManifestListWriter::v2( + manifest_list_writer, + current_snapshot.snapshot_id(), + current_snapshot.parent_snapshot_id(), + current_snapshot.sequence_number(), + ); + manifest_list_write + .add_manifests(vec![data_file_manifest].into_iter()) + .unwrap(); + manifest_list_write.close().await.unwrap(); + } + + /// Sets up manifest files for the deep history fixture. + /// + /// Creates one data file per snapshot (s1.parquet through s5.parquet), + /// each with a manifest and manifest list. Manifest lists are cumulative + /// (each snapshot's list includes all prior manifests), matching real + /// Iceberg behavior. The incremental scan should skip s4.parquet + /// (added in the overwrite snapshot S4). + pub async fn setup_manifest_files_deep_history(&mut self) { + let parquet_file_size = self.write_parquet_data_files_deep_history(); + let partition_spec = self.table.metadata().default_partition_spec(); + + // Snapshot chain: S1 -> S2 -> S3 -> S4 (overwrite) -> S5 + let snapshot_ids: Vec = vec![ + 3051729675574597004, + 3055729675574597004, + 3056729675574597004, + 3057729675574597004, + 3059729675574597004, + ]; + + // Accumulate manifests across snapshots (each manifest list is cumulative) + let mut all_manifests: Vec = Vec::new(); + + for (i, &snap_id) in snapshot_ids.iter().enumerate() { + let snapshot = self + .table + .metadata() + .snapshot_by_id(snap_id) + .unwrap() + .clone(); + let schema = snapshot.schema(self.table.metadata()).unwrap(); + + let file_name = format!("s{}.parquet", i + 1); + let partition_value = (i + 1) as i64 * 100; + + let mut writer = ManifestWriterBuilder::new( + self.next_manifest_file(), + Some(snap_id), + schema, + partition_spec.as_ref().clone(), + ) + .build_v2_data(); + + writer + .add_entry( + ManifestEntry::builder() + .status(ManifestStatus::Added) + .data_file( + DataFileBuilder::default() + .partition_spec_id(0) + .content(DataContentType::Data) + .file_path(format!("{}/{}", &self.table_location, file_name)) + .file_format(DataFileFormat::Parquet) + .file_size_in_bytes(parquet_file_size) + .record_count(1) + .partition(Struct::from_iter([Some(Literal::long( + partition_value, + ))])) + .key_metadata(None) + .build() + .unwrap(), + ) + .build(), + ) + .unwrap(); + + let mut data_file_manifest = writer.write_manifest_file().await.unwrap(); + // Assign sequence numbers so the manifest can be included in + // later snapshots' cumulative manifest lists without triggering + // the "unassigned sequence number" validation. + data_file_manifest.sequence_number = snapshot.sequence_number(); + data_file_manifest.min_sequence_number = snapshot.sequence_number(); + all_manifests.push(data_file_manifest); + + // Write cumulative manifest list for this snapshot + let manifest_list_writer = self + .table + .file_io() + .new_output(snapshot.manifest_list()) + .unwrap() + .writer() + .await + .unwrap(); + let mut manifest_list_write = ManifestListWriter::v2( + manifest_list_writer, + snap_id, + snapshot.parent_snapshot_id(), + snapshot.sequence_number(), + ); + manifest_list_write + .add_manifests(all_manifests.clone().into_iter()) + .unwrap(); + manifest_list_write.close().await.unwrap(); + } + } + + /// Like [`Self::setup_manifest_files_deep_history`], but the manifest lists + /// model writers that *rewrite* manifests rather than carrying every one + /// forward verbatim: + /// + /// ```text + /// S1 append -> [A1] A1 = {s1 ADDED@S1} + /// S2 append -> [M2] M2 = {s1 EXISTING@S1, s2 ADDED@S2} (merge-append: A1 is gone) + /// S3 append -> [M2, A3] A3 = {s3 ADDED@S3} + /// S4 overwrite -> [C4] C4 = {s1,s2,s3 EXISTING, s4 ADDED@S4} (rewrite: M2, A3 are gone) + /// S5 append -> [C4, A5] A5 = {s5 ADDED@S5} + /// ``` + /// + /// The surviving copies of the earlier entries are `EXISTING`, not `ADDED`, so + /// a scan reading only the to-snapshot's list silently drops them. + pub async fn setup_manifest_files_deep_history_rewritten(&mut self) { + let file_size = self.write_parquet_data_files_deep_history(); + + let (s1, s2, s3, s4, s5) = ( + 3051729675574597004_i64, + 3055729675574597004_i64, + 3056729675574597004_i64, + 3057729675574597004_i64, + 3059729675574597004_i64, + ); + + // (file index, originating snapshot, that snapshot's sequence number) + let (f1, f2, f3, f4, f5) = ((1, s1, 0), (2, s2, 1), (3, s3, 2), (4, s4, 3), (5, s5, 4)); + + let a1 = self + .write_rewritten_manifest(s1, &[f1], &[], file_size) + .await; + let m2 = self + .write_rewritten_manifest(s2, &[f2], &[f1], file_size) + .await; + let a3 = self + .write_rewritten_manifest(s3, &[f3], &[], file_size) + .await; + let c4 = self + .write_rewritten_manifest(s4, &[f4], &[f1, f2, f3], file_size) + .await; + let a5 = self + .write_rewritten_manifest(s5, &[f5], &[], file_size) + .await; + + self.write_deep_history_manifest_list(s1, vec![a1]).await; + self.write_deep_history_manifest_list(s2, vec![m2.clone()]) + .await; + self.write_deep_history_manifest_list(s3, vec![m2, a3]) + .await; + self.write_deep_history_manifest_list(s4, vec![c4.clone()]) + .await; + self.write_deep_history_manifest_list(s5, vec![c4, a5]) + .await; + } + + /// Writes one data manifest owned by `owner_snapshot_id`. + /// + /// `added` and `existing` are `(file index, originating snapshot id, sequence + /// number)` triples naming `s{index}.parquet`. Added entries take the owning + /// snapshot's ID (the writer enforces this); existing entries keep the ID and + /// sequence number of the snapshot that first added them. + async fn write_rewritten_manifest( + &self, + owner_snapshot_id: i64, + added: &[(usize, i64, i64)], + existing: &[(usize, i64, i64)], + file_size: u64, + ) -> ManifestFile { + let snapshot = self + .table + .metadata() + .snapshot_by_id(owner_snapshot_id) + .unwrap() + .clone(); + let schema = snapshot.schema(self.table.metadata()).unwrap(); + let partition_spec = self.table.metadata().default_partition_spec(); + + let mut writer = ManifestWriterBuilder::new( + self.next_manifest_file(), + Some(owner_snapshot_id), + schema, + partition_spec.as_ref().clone(), + ) + .build_v2_data(); + + let data_file = |index: usize| { + DataFileBuilder::default() + .partition_spec_id(0) + .content(DataContentType::Data) + .file_path(format!("{}/s{}.parquet", &self.table_location, index)) + .file_format(DataFileFormat::Parquet) + .file_size_in_bytes(file_size) + .record_count(1) + .partition(Struct::from_iter([Some(Literal::long(index as i64 * 100))])) + .key_metadata(None) + .build() + .unwrap() + }; + + for &(index, _, sequence_number) in added { + writer.add_file(data_file(index), sequence_number).unwrap(); + } + for &(index, snapshot_id, sequence_number) in existing { + writer + .add_existing_file( + data_file(index), + snapshot_id, + sequence_number, + Some(sequence_number), + ) + .unwrap(); + } + + let mut manifest = writer.write_manifest_file().await.unwrap(); + manifest.sequence_number = snapshot.sequence_number(); + if manifest.min_sequence_number == UNASSIGNED_SEQUENCE_NUMBER { + manifest.min_sequence_number = snapshot.sequence_number(); + } + manifest + } + + /// Writes `manifests` as the manifest list of the named snapshot. + async fn write_deep_history_manifest_list( + &self, + snapshot_id: i64, + manifests: Vec, + ) { + let snapshot = self + .table + .metadata() + .snapshot_by_id(snapshot_id) + .unwrap() + .clone(); + + let output = self + .table + .file_io() + .new_output(snapshot.manifest_list()) + .unwrap() + .writer() + .await + .unwrap(); + let mut writer = ManifestListWriter::v2( + output, + snapshot_id, + snapshot.parent_snapshot_id(), + snapshot.sequence_number(), + ); + writer.add_manifests(manifests.into_iter()).unwrap(); + writer.close().await.unwrap(); + } + + /// Writes parquet data files for the deep history fixture (3-column schema: x, y, z). + fn write_parquet_data_files_deep_history(&self) -> u64 { + fs::create_dir_all(&self.table_location).unwrap(); + + let schema = { + let fields = vec![ + arrow_schema::Field::new("x", arrow_schema::DataType::Int64, false).with_metadata( + HashMap::from([(PARQUET_FIELD_ID_META_KEY.to_string(), "1".to_string())]), + ), + arrow_schema::Field::new("y", arrow_schema::DataType::Int64, false).with_metadata( + HashMap::from([(PARQUET_FIELD_ID_META_KEY.to_string(), "2".to_string())]), + ), + arrow_schema::Field::new("z", arrow_schema::DataType::Int64, false).with_metadata( + HashMap::from([(PARQUET_FIELD_ID_META_KEY.to_string(), "3".to_string())]), + ), + ]; + Arc::new(arrow_schema::Schema::new(fields)) + }; + + let col1 = Arc::new(Int64Array::from_iter_values(vec![1; 10])) as ArrayRef; + let col2 = Arc::new(Int64Array::from_iter_values(vec![2; 10])) as ArrayRef; + let col3 = Arc::new(Int64Array::from_iter_values(vec![3; 10])) as ArrayRef; + + let batch = RecordBatch::try_new(schema.clone(), vec![col1, col2, col3]).unwrap(); + + let props = WriterProperties::builder() + .set_compression(Compression::SNAPPY) + .build(); + + for i in 1..=5 { + let file = File::create(format!("{}/s{}.parquet", &self.table_location, i)).unwrap(); + let mut writer = + ArrowWriter::try_new(file, batch.schema(), Some(props.clone())).unwrap(); + writer.write(&batch).expect("Writing batch"); + writer.close().unwrap(); + } + + fs::metadata(format!("{}/s1.parquet", &self.table_location)) + .unwrap() + .len() + } + + /// Writes a v3 data manifest with a manifest-level `first_row_id` of 42, + /// so live entries inherit a per-file `first_row_id` on read. Upgrades the + /// table to v3 first, so the manifest list is read as v3. + pub async fn setup_v3_manifest_files(&mut self) { + let metadata = TableMetadataBuilder::new_from_metadata( + self.table.metadata().clone(), + self.table.metadata_location().map(str::to_string), + ) + .upgrade_format_version(FormatVersion::V3) + .unwrap() + .build() + .unwrap() + .metadata; + self.table = Table::builder() + .metadata(metadata) + .identifier(self.table.identifier().clone()) + .file_io(self.table.file_io().clone()) + .metadata_location(self.table.metadata_location().unwrap().to_string()) + .runtime(test_runtime()) + .build() + .unwrap(); + + let current_snapshot = self.table.metadata().current_snapshot().unwrap(); + let current_schema = current_snapshot.schema(self.table.metadata()).unwrap(); + let current_partition_spec = self.table.metadata().default_partition_spec(); + + let parquet_file_size = self.write_parquet_data_files(); + + let mut writer = ManifestWriterBuilder::new( + self.next_manifest_file(), + Some(current_snapshot.snapshot_id()), + current_schema.clone(), + current_partition_spec.as_ref().clone(), + ) + .build_v3_data(); + writer + .add_entry( + ManifestEntry::builder() + .status(ManifestStatus::Added) + .data_file( + DataFileBuilder::default() + .partition_spec_id(0) + .content(DataContentType::Data) + .file_path(format!("{}/1.parquet", &self.table_location)) + .file_format(DataFileFormat::Parquet) + .file_size_in_bytes(parquet_file_size) + .record_count(1) + .partition(Struct::from_iter([Some(Literal::long(100))])) + .key_metadata(None) + .build() + .unwrap(), + ) + .build(), + ) + .unwrap(); + let data_file_manifest = writer.write_manifest_file().await.unwrap(); + + let manifest_list_writer = self + .table + .file_io() + .new_output(current_snapshot.manifest_list()) + .unwrap() + .writer() + .await + .unwrap(); + let mut manifest_list_write = ManifestListWriter::v3( + manifest_list_writer, + current_snapshot.snapshot_id(), + current_snapshot.parent_snapshot_id(), + current_snapshot.sequence_number(), + Some(42), + ); + manifest_list_write + .add_manifests(vec![data_file_manifest].into_iter()) + .unwrap(); + manifest_list_write.close().await.unwrap(); + } + + pub async fn setup_manifest_files_with_partition_evolution(&mut self) { + let current_snapshot = self.table.metadata().current_snapshot().unwrap(); + let parent_snapshot = current_snapshot + .parent_snapshot(self.table.metadata()) + .unwrap(); + let current_schema = current_snapshot.schema(self.table.metadata()).unwrap(); + let current_partition_spec = self.table.metadata().default_partition_spec(); + + // Write the data files first, then use the file size in the manifest entries + let parquet_file_size = self.write_parquet_data_files(); + + let mut writer = ManifestWriterBuilder::new( + self.next_manifest_file(), + Some(current_snapshot.snapshot_id()), + current_schema.clone(), + current_partition_spec.as_ref().clone(), + ) + .build_v2_data(); + writer + .add_entry( + ManifestEntry::builder() + .status(ManifestStatus::Added) + .data_file( + DataFileBuilder::default() + .partition_spec_id(1) + .content(DataContentType::Data) + .file_path(format!("{}/1.parquet", &self.table_location)) + .file_format(DataFileFormat::Parquet) + .file_size_in_bytes(parquet_file_size) + .record_count(1) + .partition(Struct::from_iter([ + Some(Literal::long(100)), + Some(Literal::string("apa")), + Some(Literal::int(27)), + ])) + .key_metadata(None) + .build() + .unwrap(), + ) + .build(), + ) + .unwrap(); + writer + .add_delete_entry( + ManifestEntry::builder() + .status(ManifestStatus::Deleted) + .snapshot_id(parent_snapshot.snapshot_id()) + .sequence_number(parent_snapshot.sequence_number()) + .file_sequence_number(parent_snapshot.sequence_number()) + .data_file( + DataFileBuilder::default() + .partition_spec_id(1) + .content(DataContentType::Data) + .file_path(format!("{}/2.parquet", &self.table_location)) + .file_format(DataFileFormat::Parquet) + .file_size_in_bytes(parquet_file_size) + .record_count(1) + .partition(Struct::from_iter([ + Some(Literal::long(200)), + Some(Literal::string("ice")), + Some(Literal::int(5)), + ])) + .build() + .unwrap(), + ) + .build(), + ) + .unwrap(); + writer + .add_existing_entry( + ManifestEntry::builder() + .status(ManifestStatus::Existing) + .snapshot_id(parent_snapshot.snapshot_id()) + .sequence_number(parent_snapshot.sequence_number()) + .file_sequence_number(parent_snapshot.sequence_number()) + .data_file( + DataFileBuilder::default() + .partition_spec_id(1) + .content(DataContentType::Data) + .file_path(format!("{}/3.parquet", &self.table_location)) + .file_format(DataFileFormat::Parquet) + .file_size_in_bytes(parquet_file_size) + .record_count(1) + .partition(Struct::from_iter([ + Some(Literal::long(300)), + Some(Literal::string("apa")), + Some(Literal::int(19)), + ])) + .build() + .unwrap(), + ) + .build(), + ) + .unwrap(); + let data_file_manifest = writer.write_manifest_file().await.unwrap(); + + // Write to manifest list + let manifest_list_writer = self + .table + .file_io() + .new_output(current_snapshot.manifest_list()) + .unwrap() + .writer() + .await + .unwrap(); + let mut manifest_list_write = ManifestListWriter::v2( + manifest_list_writer, + current_snapshot.snapshot_id(), + current_snapshot.parent_snapshot_id(), + current_snapshot.sequence_number(), + ); + manifest_list_write + .add_manifests(vec![data_file_manifest].into_iter()) + .unwrap(); + manifest_list_write.close().await.unwrap(); + } + + /// Writes identical Parquet data files (1.parquet, 2.parquet, 3.parquet) + /// and returns the file size in bytes. + fn write_parquet_data_files(&self) -> u64 { + fs::create_dir_all(&self.table_location).unwrap(); + + let schema = { + let fields = vec![ + arrow_schema::Field::new("x", arrow_schema::DataType::Int64, false).with_metadata( + HashMap::from([(PARQUET_FIELD_ID_META_KEY.to_string(), "1".to_string())]), + ), + arrow_schema::Field::new("y", arrow_schema::DataType::Int64, false).with_metadata( + HashMap::from([(PARQUET_FIELD_ID_META_KEY.to_string(), "2".to_string())]), + ), + arrow_schema::Field::new("z", arrow_schema::DataType::Int64, false).with_metadata( + HashMap::from([(PARQUET_FIELD_ID_META_KEY.to_string(), "3".to_string())]), + ), + arrow_schema::Field::new("a", arrow_schema::DataType::Utf8, false).with_metadata( + HashMap::from([(PARQUET_FIELD_ID_META_KEY.to_string(), "4".to_string())]), + ), + arrow_schema::Field::new("dbl", arrow_schema::DataType::Float64, false) + .with_metadata(HashMap::from([( + PARQUET_FIELD_ID_META_KEY.to_string(), + "5".to_string(), + )])), + arrow_schema::Field::new("i32", arrow_schema::DataType::Int32, false) + .with_metadata(HashMap::from([( + PARQUET_FIELD_ID_META_KEY.to_string(), + "6".to_string(), + )])), + arrow_schema::Field::new("i64", arrow_schema::DataType::Int64, false) + .with_metadata(HashMap::from([( + PARQUET_FIELD_ID_META_KEY.to_string(), + "7".to_string(), + )])), + arrow_schema::Field::new("bool", arrow_schema::DataType::Boolean, false) + .with_metadata(HashMap::from([( + PARQUET_FIELD_ID_META_KEY.to_string(), + "8".to_string(), + )])), + ]; + Arc::new(arrow_schema::Schema::new(fields)) + }; + // x: [1, 1, 1, 1, ...] + let col1 = Arc::new(Int64Array::from_iter_values(vec![1; 1024])) as ArrayRef; + + let mut values = vec![2; 512]; + values.append(vec![3; 200].as_mut()); + values.append(vec![4; 300].as_mut()); + values.append(vec![5; 12].as_mut()); + + // y: [2, 2, 2, 2, ..., 3, 3, 3, 3, ..., 4, 4, 4, 4, ..., 5, 5, 5, 5] + let col2 = Arc::new(Int64Array::from_iter_values(values)) as ArrayRef; + + let mut values = vec![3; 512]; + values.append(vec![4; 512].as_mut()); + + // z: [3, 3, 3, 3, ..., 4, 4, 4, 4] + let col3 = Arc::new(Int64Array::from_iter_values(values)) as ArrayRef; + + // a: ["Apache", "Apache", "Apache", ..., "Iceberg", "Iceberg", "Iceberg"] + let mut values = vec!["Apache"; 512]; + values.append(vec!["Iceberg"; 512].as_mut()); + let col4 = Arc::new(StringArray::from_iter_values(values)) as ArrayRef; + + // dbl: + let mut values = vec![100.0f64; 512]; + values.append(vec![150.0f64; 12].as_mut()); + values.append(vec![200.0f64; 500].as_mut()); + let col5 = Arc::new(Float64Array::from_iter_values(values)) as ArrayRef; + + // i32: + let mut values = vec![100i32; 512]; + values.append(vec![150i32; 12].as_mut()); + values.append(vec![200i32; 500].as_mut()); + let col6 = Arc::new(Int32Array::from_iter_values(values)) as ArrayRef; + + // i64: + let mut values = vec![100i64; 512]; + values.append(vec![150i64; 12].as_mut()); + values.append(vec![200i64; 500].as_mut()); + let col7 = Arc::new(Int64Array::from_iter_values(values)) as ArrayRef; + + // bool: + let mut values = vec![false; 512]; + values.append(vec![true; 512].as_mut()); + let values: BooleanArray = values.into(); + let col8 = Arc::new(values) as ArrayRef; + + let to_write = RecordBatch::try_new(schema.clone(), vec![ + col1, col2, col3, col4, col5, col6, col7, col8, + ]) + .unwrap(); + + // Write the Parquet files + let props = WriterProperties::builder() + .set_compression(Compression::SNAPPY) + .build(); + + for n in 1..=3 { + let file = File::create(format!("{}/{}.parquet", &self.table_location, n)).unwrap(); + let mut writer = + ArrowWriter::try_new(file, to_write.schema(), Some(props.clone())).unwrap(); + + writer.write(&to_write).expect("Writing batch"); + + // writer must be closed to write footer + writer.close().unwrap(); + } + + fs::metadata(format!("{}/1.parquet", &self.table_location)) + .unwrap() + .len() + } + + pub async fn setup_unpartitioned_manifest_files(&mut self) { + let current_snapshot = self.table.metadata().current_snapshot().unwrap(); + let parent_snapshot = current_snapshot + .parent_snapshot(self.table.metadata()) + .unwrap(); + let current_schema = current_snapshot.schema(self.table.metadata()).unwrap(); + let current_partition_spec = Arc::new(PartitionSpec::unpartition_spec()); + + // Write the data files first, then use the file size in the manifest entries + let parquet_file_size = self.write_parquet_data_files(); + + // Write data files using an empty partition for unpartitioned tables. + let mut writer = ManifestWriterBuilder::new( + self.next_manifest_file(), + Some(current_snapshot.snapshot_id()), + current_schema.clone(), + current_partition_spec.as_ref().clone(), + ) + .build_v2_data(); + + // Create an empty partition value. + let empty_partition = Struct::empty(); + + writer + .add_entry( + ManifestEntry::builder() + .status(ManifestStatus::Added) + .data_file( + DataFileBuilder::default() + .partition_spec_id(0) + .content(DataContentType::Data) + .file_path(format!("{}/1.parquet", &self.table_location)) + .file_format(DataFileFormat::Parquet) + .file_size_in_bytes(parquet_file_size) + .record_count(1) + .partition(empty_partition.clone()) + .key_metadata(None) + .build() + .unwrap(), + ) + .build(), + ) + .unwrap(); + + writer + .add_delete_entry( + ManifestEntry::builder() + .status(ManifestStatus::Deleted) + .snapshot_id(parent_snapshot.snapshot_id()) + .sequence_number(parent_snapshot.sequence_number()) + .file_sequence_number(parent_snapshot.sequence_number()) + .data_file( + DataFileBuilder::default() + .partition_spec_id(0) + .content(DataContentType::Data) + .file_path(format!("{}/2.parquet", &self.table_location)) + .file_format(DataFileFormat::Parquet) + .file_size_in_bytes(parquet_file_size) + .record_count(1) + .partition(empty_partition.clone()) + .build() + .unwrap(), + ) + .build(), + ) + .unwrap(); + + writer + .add_existing_entry( + ManifestEntry::builder() + .status(ManifestStatus::Existing) + .snapshot_id(parent_snapshot.snapshot_id()) + .sequence_number(parent_snapshot.sequence_number()) + .file_sequence_number(parent_snapshot.sequence_number()) + .data_file( + DataFileBuilder::default() + .partition_spec_id(0) + .content(DataContentType::Data) + .file_path(format!("{}/3.parquet", &self.table_location)) + .file_format(DataFileFormat::Parquet) + .file_size_in_bytes(parquet_file_size) + .record_count(1) + .partition(empty_partition.clone()) + .build() + .unwrap(), + ) + .build(), + ) + .unwrap(); + + let data_file_manifest = writer.write_manifest_file().await.unwrap(); + + // Write to manifest list + let manifest_list_writer = self + .table + .file_io() + .new_output(current_snapshot.manifest_list()) + .unwrap() + .writer() + .await + .unwrap(); + let mut manifest_list_write = ManifestListWriter::v2( + manifest_list_writer, + current_snapshot.snapshot_id(), + current_snapshot.parent_snapshot_id(), + current_snapshot.sequence_number(), + ); + manifest_list_write + .add_manifests(vec![data_file_manifest].into_iter()) + .unwrap(); + manifest_list_write.close().await.unwrap(); + } + + pub async fn setup_deadlock_manifests(&mut self) { + let current_snapshot = self.table.metadata().current_snapshot().unwrap(); + let _parent_snapshot = current_snapshot + .parent_snapshot(self.table.metadata()) + .unwrap(); + let current_schema = current_snapshot.schema(self.table.metadata()).unwrap(); + let current_partition_spec = self.table.metadata().default_partition_spec(); + + // 1. Write DATA manifest with MULTIPLE entries to fill buffer + let mut writer = ManifestWriterBuilder::new( + self.next_manifest_file(), + Some(current_snapshot.snapshot_id()), + current_schema.clone(), + current_partition_spec.as_ref().clone(), + ) + .build_v2_data(); + + // Add 10 data entries + for i in 0..10 { + writer + .add_entry( + ManifestEntry::builder() + .status(ManifestStatus::Added) + .data_file( + DataFileBuilder::default() + .partition_spec_id(0) + .content(DataContentType::Data) + .file_path(format!("{}/{}.parquet", &self.table_location, i)) + .file_format(DataFileFormat::Parquet) + .file_size_in_bytes(100) + .record_count(1) + .partition(Struct::from_iter([Some(Literal::long(100))])) + .key_metadata(None) + .build() + .unwrap(), + ) + .build(), + ) + .unwrap(); + } + let data_manifest = writer.write_manifest_file().await.unwrap(); + + // 2. Write DELETE manifest + let mut writer = ManifestWriterBuilder::new( + self.next_manifest_file(), + Some(current_snapshot.snapshot_id()), + current_schema.clone(), + current_partition_spec.as_ref().clone(), + ) + .build_v2_deletes(); + + writer + .add_entry( + ManifestEntry::builder() + .status(ManifestStatus::Added) + .data_file( + DataFileBuilder::default() + .partition_spec_id(0) + .content(DataContentType::PositionDeletes) + .file_path(format!("{}/del.parquet", &self.table_location)) + .file_format(DataFileFormat::Parquet) + .file_size_in_bytes(100) + .record_count(1) + .partition(Struct::from_iter([Some(Literal::long(100))])) + .build() + .unwrap(), + ) + .build(), + ) + .unwrap(); + let delete_manifest = writer.write_manifest_file().await.unwrap(); + + // Write to manifest list - DATA FIRST then DELETE + // This order is crucial for reproduction + let manifest_list_writer = self + .table + .file_io() + .new_output(current_snapshot.manifest_list()) + .unwrap() + .writer() + .await + .unwrap(); + let mut manifest_list_write = ManifestListWriter::v2( + manifest_list_writer, + current_snapshot.snapshot_id(), + current_snapshot.parent_snapshot_id(), + current_snapshot.sequence_number(), + ); + manifest_list_write + .add_manifests(vec![data_manifest, delete_manifest].into_iter()) + .unwrap(); + manifest_list_write.close().await.unwrap(); + } + + /// Sets up a single data file `mrg.parquet` with three 100-row row groups + /// (column `x` = 1000..1300, so row position `p` carries `x = 1000 + p`) and + /// registers it in the current snapshot. When `delete_positions` is non-empty, + /// also writes a positional delete file targeting those file-absolute positions + /// and registers it in a delete manifest. + /// + /// Used to exercise the `_pos` metadata column through the real `TableScan` + /// planning path across row-group boundaries and (optionally) positional deletes. + pub async fn setup_multi_row_group_manifest(&mut self, delete_positions: &[i64]) { + let current_snapshot = self.table.metadata().current_snapshot().unwrap(); + let current_schema = current_snapshot.schema(self.table.metadata()).unwrap(); + let current_partition_spec = self.table.metadata().default_partition_spec(); + + // The table's spec 0 is identity on `x`, so give the data and delete files a + // fixed partition value. Filter tests deliberately filter on `y` (a + // non-partition column) so pruning is driven by Parquet row-group statistics + // rather than partition values. + let partition = Struct::from_iter([Some(Literal::long(1000))]); + + let (data_file_path, data_file_size) = self.write_multi_row_group_data_file(); + + let mut data_writer = ManifestWriterBuilder::new( + self.next_manifest_file(), + Some(current_snapshot.snapshot_id()), + current_schema.clone(), + current_partition_spec.as_ref().clone(), + ) + .build_v2_data(); + data_writer + .add_entry( + ManifestEntry::builder() + .status(ManifestStatus::Added) + .data_file( + DataFileBuilder::default() + .partition_spec_id(0) + .content(DataContentType::Data) + .file_path(data_file_path.clone()) + .file_format(DataFileFormat::Parquet) + .file_size_in_bytes(data_file_size) + .record_count(300) + .partition(partition.clone()) + .key_metadata(None) + .build() + .unwrap(), + ) + .build(), + ) + .unwrap(); + let data_manifest = data_writer.write_manifest_file().await.unwrap(); + + let mut manifests = vec![data_manifest]; + + if !delete_positions.is_empty() { + let (del_path, del_size) = + self.write_positional_delete_file(&data_file_path, delete_positions); + + let mut delete_writer = ManifestWriterBuilder::new( + self.next_manifest_file(), + Some(current_snapshot.snapshot_id()), + current_schema.clone(), + current_partition_spec.as_ref().clone(), + ) + .build_v2_deletes(); + delete_writer + .add_entry( + ManifestEntry::builder() + .status(ManifestStatus::Added) + .data_file( + DataFileBuilder::default() + .partition_spec_id(0) + .content(DataContentType::PositionDeletes) + .file_path(del_path) + .file_format(DataFileFormat::Parquet) + .file_size_in_bytes(del_size) + .record_count(delete_positions.len() as u64) + .partition(partition.clone()) + .build() + .unwrap(), + ) + .build(), + ) + .unwrap(); + manifests.push(delete_writer.write_manifest_file().await.unwrap()); + } + + let manifest_list_writer = self + .table + .file_io() + .new_output(current_snapshot.manifest_list()) + .unwrap() + .writer() + .await + .unwrap(); + let mut manifest_list_write = ManifestListWriter::v2( + manifest_list_writer, + current_snapshot.snapshot_id(), + current_snapshot.parent_snapshot_id(), + current_snapshot.sequence_number(), + ); + manifest_list_write + .add_manifests(manifests.into_iter()) + .unwrap(); + manifest_list_write.close().await.unwrap(); + } + + /// Writes `mrg.parquet` with three 100-row row groups. Columns `x` (field + /// id `1`) and `y` (field id `2`) both run 1000..1300, so row position `p` + /// carries `x = y = 1000 + p`. Returns `(path, file_size_in_bytes)`. + fn write_multi_row_group_data_file(&self) -> (String, u64) { + fs::create_dir_all(&self.table_location).unwrap(); + + let arrow_schema = Arc::new(arrow_schema::Schema::new(vec![ + arrow_schema::Field::new("x", arrow_schema::DataType::Int64, false).with_metadata( + HashMap::from([(PARQUET_FIELD_ID_META_KEY.to_string(), "1".to_string())]), + ), + arrow_schema::Field::new("y", arrow_schema::DataType::Int64, false).with_metadata( + HashMap::from([(PARQUET_FIELD_ID_META_KEY.to_string(), "2".to_string())]), + ), + ])); + + let path = format!("{}/mrg.parquet", &self.table_location); + let max_row_group_row_count = 100; + let props = WriterProperties::builder() + .set_compression(Compression::SNAPPY) + .set_max_row_group_row_count(Some(max_row_group_row_count)) + .build(); + + let file = File::create(&path).unwrap(); + let mut writer = ArrowWriter::try_new(file, arrow_schema.clone(), Some(props)).unwrap(); + for group in 0..3i64 { + let base = 1000 + group * max_row_group_row_count as i64; + let col = Arc::new(Int64Array::from_iter_values( + base..base + max_row_group_row_count as i64, + )) as ArrayRef; + let batch = RecordBatch::try_new(arrow_schema.clone(), vec![col.clone(), col]).unwrap(); + writer.write(&batch).unwrap(); + } + writer.close().unwrap(); + + let size = fs::metadata(&path).unwrap().len(); + (path, size) + } + + /// Writes a positional delete file targeting `positions` in `data_path`. + /// Returns `(path, file_size_in_bytes)`. + fn write_positional_delete_file(&self, data_path: &str, positions: &[i64]) -> (String, u64) { + let del_schema = Arc::new(arrow_schema::Schema::new(vec![ + arrow_schema::Field::new( + RESERVED_COL_NAME_DELETE_FILE_PATH, + arrow_schema::DataType::Utf8, + false, + ) + .with_metadata(HashMap::from([( + PARQUET_FIELD_ID_META_KEY.to_string(), + RESERVED_FIELD_ID_DELETE_FILE_PATH.to_string(), // 2147483546 + )])), + arrow_schema::Field::new( + RESERVED_COL_NAME_DELETE_FILE_POS, + arrow_schema::DataType::Int64, + false, + ) + .with_metadata(HashMap::from([( + PARQUET_FIELD_ID_META_KEY.to_string(), + RESERVED_FIELD_ID_DELETE_FILE_POS.to_string(), // 2147483545 + )])), + ])); + + let batch = RecordBatch::try_new(del_schema.clone(), vec![ + Arc::new(StringArray::from_iter_values(std::iter::repeat_n( + data_path.to_string(), + positions.len(), + ))) as ArrayRef, + Arc::new(Int64Array::from_iter_values(positions.iter().copied())) as ArrayRef, + ]) + .unwrap(); + + let path = format!("{}/pos-del.parquet", &self.table_location); + let props = WriterProperties::builder() + .set_compression(Compression::SNAPPY) + .build(); + let file = File::create(&path).unwrap(); + let mut writer = ArrowWriter::try_new(file, del_schema, Some(props)).unwrap(); + writer.write(&batch).unwrap(); + writer.close().unwrap(); + + let size = fs::metadata(&path).unwrap().len(); + (path, size) + } +} diff --git a/crates/iceberg/src/util/snapshot.rs b/crates/iceberg/src/util/snapshot.rs index d5b87a340d..c45dc6f1e1 100644 --- a/crates/iceberg/src/util/snapshot.rs +++ b/crates/iceberg/src/util/snapshot.rs @@ -79,7 +79,7 @@ pub fn ancestors_between( #[cfg(test)] mod tests { use super::*; - use crate::scan::tests::TableTestFixture; + use crate::scan::test_utils::TableTestFixture; // Five snapshots chained as: S1 (root) -> S2 -> S3 -> S4 -> S5 (current) const S1: i64 = 3051729675574597004; From d5f492ac3deb10e7e1f6a5778b100bf3ef5a2d63 Mon Sep 17 00:00:00 2001 From: Xander Date: Fri, 28 Aug 2026 13:04:43 +0100 Subject: [PATCH 7/9] try new context --- crates/iceberg/src/scan/cache.rs | 2 +- crates/iceberg/src/scan/context.rs | 57 +++++++++------- crates/iceberg/src/scan/incremental.rs | 95 ++++++++++++++++---------- crates/iceberg/src/scan/mod.rs | 46 +++++++------ 4 files changed, 117 insertions(+), 83 deletions(-) diff --git a/crates/iceberg/src/scan/cache.rs b/crates/iceberg/src/scan/cache.rs index b5eb3c4ce0..e0f0c35218 100644 --- a/crates/iceberg/src/scan/cache.rs +++ b/crates/iceberg/src/scan/cache.rs @@ -74,7 +74,7 @@ impl PartitionFilterCache { // 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` + // other filter; this is safe only because the cache lives per-scan in `ScanPlanningContext` // 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 diff --git a/crates/iceberg/src/scan/context.rs b/crates/iceberg/src/scan/context.rs index e66d1873d2..24a638e815 100644 --- a/crates/iceberg/src/scan/context.rs +++ b/crates/iceberg/src/scan/context.rs @@ -28,8 +28,8 @@ use crate::scan::{ PartitionFilterCache, }; use crate::spec::{ - ManifestContentType, ManifestEntryRef, ManifestFile, ManifestList, NameMapping, - PartitionSpecRef, SchemaRef, SnapshotRef, StructType, TableMetadataRef, + ManifestContentType, ManifestEntryRef, ManifestFile, NameMapping, PartitionSpecRef, SchemaRef, + SnapshotRef, StructType, TableMetadataRef, }; use crate::{Error, ErrorKind, Result}; @@ -47,7 +47,7 @@ pub(crate) struct ManifestFileContext { field_ids: Arc>, bound_predicates: Option>, object_cache: Arc, - snapshot_schema: SchemaRef, + scan_schema: SchemaRef, expression_evaluator_cache: Arc, delete_file_index: DeleteFileIndex, name_mapping: Option>, @@ -66,7 +66,7 @@ pub(crate) struct ManifestEntryContext { pub field_ids: Arc>, pub bound_predicates: Option>, pub partition_spec_id: i32, - pub snapshot_schema: SchemaRef, + pub scan_schema: SchemaRef, pub delete_file_index: DeleteFileIndex, pub name_mapping: Option>, pub case_sensitive: bool, @@ -82,7 +82,7 @@ impl ManifestFileContext { object_cache, manifest_file, bound_predicates, - snapshot_schema, + scan_schema, field_ids, mut sender, expression_evaluator_cache, @@ -110,7 +110,7 @@ impl ManifestFileContext { field_ids: field_ids.clone(), partition_spec_id: manifest_file.partition_spec_id, bound_predicates: bound_predicates.clone(), - snapshot_schema: snapshot_schema.clone(), + scan_schema: scan_schema.clone(), delete_file_index: delete_file_index.clone(), name_mapping: name_mapping.clone(), case_sensitive, @@ -149,11 +149,11 @@ impl ManifestEntryContext { .with_data_sequence_number(self.manifest_entry.sequence_number()) .with_data_file_path(self.manifest_entry.file_path().to_string()) .with_data_file_format(self.manifest_entry.file_format()) - .with_schema(self.snapshot_schema) + .with_schema(self.scan_schema) .with_project_field_ids(self.field_ids.to_vec()) .with_predicate( self.bound_predicates - .map(|x| x.as_ref().snapshot_bound_predicate.clone()), + .map(|x| x.as_ref().scan_bound_predicate.clone()), ) .with_deletes(deletes) .with_partition(Some(self.manifest_entry.data_file.partition.clone())) @@ -166,17 +166,29 @@ impl ManifestEntryContext { } } -/// PlanContext wraps a [`SnapshotRef`] alongside all the other -/// objects that are required to perform a scan file plan. #[derive(Debug)] pub(crate) struct PlanContext { pub snapshot: SnapshotRef, + pub scan_context: ScanPlanningContext, +} + +impl PlanContext { + pub(crate) async fn manifest_files(&self) -> Result> { + self.scan_context + .object_cache + .get_manifest_list(&self.snapshot, &self.scan_context.table_metadata) + .await + .map(|manifest_list| manifest_list.entries().to_vec()) + } +} +#[derive(Debug)] +pub(crate) struct ScanPlanningContext { pub table_metadata: TableMetadataRef, - pub snapshot_schema: SchemaRef, + pub scan_schema: SchemaRef, pub case_sensitive: bool, pub predicate: Option>, - pub snapshot_bound_predicate: Option>, + pub scan_bound_predicate: Option>, pub object_cache: Arc, pub field_ids: Arc>, pub name_mapping: Option>, @@ -188,14 +200,7 @@ pub(crate) struct PlanContext { pub unified_partition_type: Option>, } -impl PlanContext { - pub(crate) async fn get_manifest_list(&self) -> Result> { - self.object_cache - .as_ref() - .get_manifest_list(&self.snapshot, &self.table_metadata) - .await - } - +impl ScanPlanningContext { /// Returns the partition filter for a manifest. See [`PartitionFilterCache::get`] for the /// always-true fallback when the manifest's spec cannot be resolved against the scan schema. fn get_partition_filter(&self, manifest_file: &ManifestFile) -> Result> { @@ -204,7 +209,7 @@ impl PlanContext { let partition_filter = self.partition_filter_cache.get( partition_spec_id, &self.table_metadata, - &self.snapshot_schema, + &self.scan_schema, self.case_sensitive, self.predicate .as_ref() @@ -213,7 +218,7 @@ impl PlanContext { "Expected a predicate but none present", ))? .as_ref() - .bind(self.snapshot_schema.clone(), self.case_sensitive)?, + .bind(self.scan_schema.clone(), self.case_sensitive)?, )?; Ok(partition_filter) @@ -291,12 +296,12 @@ impl PlanContext { entry_filter: Option, ) -> ManifestFileContext { let bound_predicates = - if let (Some(ref partition_bound_predicate), Some(snapshot_bound_predicate)) = - (partition_filter, &self.snapshot_bound_predicate) + if let (Some(ref partition_bound_predicate), Some(scan_bound_predicate)) = + (partition_filter, &self.scan_bound_predicate) { Some(Arc::new(BoundPredicates { partition_bound_predicate: partition_bound_predicate.as_ref().clone(), - snapshot_bound_predicate: snapshot_bound_predicate.as_ref().clone(), + scan_bound_predicate: scan_bound_predicate.as_ref().clone(), })) } else { None @@ -307,7 +312,7 @@ impl PlanContext { bound_predicates, sender, object_cache: self.object_cache.clone(), - snapshot_schema: self.snapshot_schema.clone(), + scan_schema: self.scan_schema.clone(), field_ids: self.field_ids.clone(), expression_evaluator_cache: self.expression_evaluator_cache.clone(), delete_file_index, diff --git a/crates/iceberg/src/scan/incremental.rs b/crates/iceberg/src/scan/incremental.rs index bf38c4306d..527a1d5cc9 100644 --- a/crates/iceberg/src/scan/incremental.rs +++ b/crates/iceberg/src/scan/incremental.rs @@ -29,8 +29,8 @@ use crate::runtime::Runtime; use crate::scan::context::ManifestEntryFilter; use crate::scan::{ ArrowRecordBatchStream, ExpressionEvaluatorCache, FileScanTaskStream, ManifestEvaluatorCache, - PartitionFilterCache, PlanContext, bind_scan_predicate, plan_scan_files, projected_field_ids, - projected_partition_type, table_name_mapping, + PartitionFilterCache, ScanPlanningContext, bind_scan_predicate, plan_scan_files, + projected_field_ids, projected_partition_type, table_name_mapping, }; use crate::spec::{ ManifestContentType, ManifestFile, ManifestList, ManifestStatus, Operation, SnapshotRef, @@ -171,27 +171,16 @@ impl AppendRange { } } -/// An incremental scan of data appended between two snapshots. #[derive(Debug)] -pub struct IncrementalAppendScan { - plan_context: PlanContext, +struct IncrementalAppendPlanContext { append_range: AppendRange, - batch_size: Option, - file_io: FileIO, - column_names: Option>, - concurrency_limit_manifest_files: usize, - concurrency_limit_manifest_entries: usize, - concurrency_limit_data_files: usize, - row_group_filtering_enabled: bool, - row_selection_enabled: bool, - runtime: Runtime, + scan_context: ScanPlanningContext, } -impl IncrementalAppendScan { - /// Returns a stream of files appended in the scan's snapshot range. - pub async fn plan_files(&self) -> Result { - let object_cache = self.plan_context.object_cache.clone(); - let table_metadata = self.plan_context.table_metadata.clone(); +impl IncrementalAppendPlanContext { + async fn manifest_files(&self, concurrency_limit: usize) -> Result> { + let object_cache = self.scan_context.object_cache.clone(); + let table_metadata = self.scan_context.table_metadata.clone(); let manifest_lists: Vec> = futures::stream::iter(self.append_range.snapshots().iter().cloned()) .map(move |snapshot| { @@ -203,15 +192,45 @@ impl IncrementalAppendScan { .await } }) - .buffered(self.concurrency_limit_manifest_files.max(1)) + .buffered(concurrency_limit.max(1)) .try_collect() .await?; - let manifest_files = self.append_range.manifest_files(&manifest_lists); + + Ok(self.append_range.manifest_files(&manifest_lists)) + } + + fn manifest_entry_filter(&self) -> ManifestEntryFilter { + self.append_range.manifest_entry_filter() + } +} + +/// An incremental scan of data appended between two snapshots. +#[derive(Debug)] +pub struct IncrementalAppendScan { + plan_context: IncrementalAppendPlanContext, + batch_size: Option, + file_io: FileIO, + column_names: Option>, + concurrency_limit_manifest_files: usize, + concurrency_limit_manifest_entries: usize, + concurrency_limit_data_files: usize, + row_group_filtering_enabled: bool, + row_selection_enabled: bool, + runtime: Runtime, +} + +impl IncrementalAppendScan { + /// Returns a stream of files appended in the scan's snapshot range. + pub async fn plan_files(&self) -> Result { + let manifest_files = self + .plan_context + .manifest_files(self.concurrency_limit_manifest_files) + .await?; plan_scan_files( - &self.plan_context, + &self.plan_context.scan_context, manifest_files, - Some(self.append_range.manifest_entry_filter()), + Some(self.plan_context.manifest_entry_filter()), &self.runtime, self.concurrency_limit_manifest_files, self.concurrency_limit_manifest_entries, @@ -379,7 +398,7 @@ impl<'a> IncrementalAppendScanBuilder<'a> { /// Build the incremental append scan. pub fn build(self) -> Result { - let to_snapshot = match self.to_snapshot_id { + let to_snapshot_id = match self.to_snapshot_id { Some(snapshot_id) => self .table .metadata() @@ -390,7 +409,7 @@ impl<'a> IncrementalAppendScanBuilder<'a> { format!("to_snapshot with id {snapshot_id} not found"), ) })? - .clone(), + .snapshot_id(), None => { let Some(current_snapshot) = self.table.metadata().current_snapshot() else { return Err(Error::new( @@ -398,32 +417,31 @@ impl<'a> IncrementalAppendScanBuilder<'a> { "Cannot perform incremental scan: table has no snapshots", )); }; - current_snapshot.clone() + current_snapshot.snapshot_id() } }; let append_range = AppendRange::build( &self.table.metadata_ref(), self.from_snapshot_id, - to_snapshot.snapshot_id(), + to_snapshot_id, self.from_inclusive, )?; let schema = self.table.metadata().current_schema().clone(); let field_ids = projected_field_ids(&schema, self.column_names.as_deref(), self.case_sensitive)?; - let snapshot_bound_predicate = + let scan_bound_predicate = bind_scan_predicate(&schema, self.filter.as_ref(), self.case_sensitive)?; let name_mapping = table_name_mapping(self.table)?; let unified_partition_type = projected_partition_type(self.table, &schema, &field_ids)?; - let plan_context = PlanContext { - snapshot: to_snapshot, + let scan_context = ScanPlanningContext { table_metadata: self.table.metadata_ref(), - snapshot_schema: schema, + scan_schema: schema, case_sensitive: self.case_sensitive, predicate: self.filter.map(Arc::new), - snapshot_bound_predicate, + scan_bound_predicate, object_cache: self.table.object_cache(), field_ids: Arc::new(field_ids), name_mapping, @@ -432,10 +450,13 @@ impl<'a> IncrementalAppendScanBuilder<'a> { expression_evaluator_cache: Arc::new(ExpressionEvaluatorCache::new()), unified_partition_type, }; + let plan_context = IncrementalAppendPlanContext { + append_range, + scan_context, + }; Ok(IncrementalAppendScan { plan_context, - append_range, batch_size: self.batch_size, file_io: self.table.file_io().clone(), column_names: self.column_names, @@ -574,7 +595,7 @@ mod tests { ); let scan = result.unwrap(); - assert_eq!(scan.append_range.snapshots().len(), 1); + assert_eq!(scan.plan_context.append_range.snapshots().len(), 1); } #[test] @@ -1053,18 +1074,18 @@ mod tests { .build() .unwrap(); - let plan_context = &scan.plan_context; + let scan_context = &scan.plan_context.scan_context; // The scan must use the current schema (3 columns), not the // to-snapshot's schema (1 column). let current_schema = table.metadata().current_schema(); assert_eq!( - plan_context.snapshot_schema.schema_id(), + scan_context.scan_schema.schema_id(), current_schema.schema_id(), "incremental scan should project onto the current schema" ); assert_eq!( - plan_context.snapshot_schema.as_struct().fields().len(), + scan_context.scan_schema.as_struct().fields().len(), 3, "current schema has three columns (x, y, z)" ); diff --git a/crates/iceberg/src/scan/mod.rs b/crates/iceberg/src/scan/mod.rs index d0bcda8699..9e7907fbfe 100644 --- a/crates/iceberg/src/scan/mod.rs +++ b/crates/iceberg/src/scan/mod.rs @@ -339,18 +339,17 @@ impl<'a> TableScanBuilder<'a> { let schema = snapshot.schema(self.table.metadata())?; let field_ids = projected_field_ids(&schema, self.column_names.as_deref(), self.case_sensitive)?; - let snapshot_bound_predicate = + let scan_bound_predicate = bind_scan_predicate(&schema, self.filter.as_ref(), self.case_sensitive)?; let name_mapping = table_name_mapping(self.table)?; let unified_partition_type = projected_partition_type(self.table, &schema, &field_ids)?; - let plan_context = PlanContext { - snapshot, + let scan_context = ScanPlanningContext { table_metadata: self.table.metadata_ref(), - snapshot_schema: schema, + scan_schema: schema, case_sensitive: self.case_sensitive, predicate: self.filter.map(Arc::new), - snapshot_bound_predicate, + scan_bound_predicate, object_cache: self.table.object_cache(), field_ids: Arc::new(field_ids), name_mapping, @@ -359,6 +358,10 @@ impl<'a> TableScanBuilder<'a> { expression_evaluator_cache: Arc::new(ExpressionEvaluatorCache::new()), unified_partition_type, }; + let plan_context = PlanContext { + snapshot, + scan_context, + }; Ok(TableScan { batch_size: self.batch_size, @@ -378,9 +381,6 @@ impl<'a> TableScanBuilder<'a> { /// Table scan. #[derive(Debug)] pub struct TableScan { - /// A [PlanContext], if this table has at least one snapshot, otherwise None. - /// - /// If this is None, then the scan contains no rows. plan_context: Option, batch_size: Option, file_io: FileIO, @@ -404,7 +404,7 @@ pub struct TableScan { } pub(crate) async fn plan_scan_files( - plan_context: &PlanContext, + scan_context: &ScanPlanningContext, manifest_files: Vec, manifest_entry_filter: Option, runtime: &Runtime, @@ -419,7 +419,7 @@ pub(crate) async fn plan_scan_files( let (delete_file_idx, delete_file_tx) = DeleteFileIndex::new(runtime.clone()); - let manifest_file_contexts = plan_context.build_manifest_file_contexts( + let manifest_file_contexts = scan_context.build_manifest_file_contexts( manifest_files, manifest_entry_filter, manifest_entry_data_ctx_tx, @@ -511,12 +511,11 @@ impl TableScan { let Some(plan_context) = self.plan_context.as_ref() else { return Ok(Box::pin(futures::stream::empty())); }; - - let manifest_list = plan_context.get_manifest_list().await?; + let manifest_files = plan_context.manifest_files().await?; plan_scan_files( - plan_context, - manifest_list.entries().to_vec(), + &plan_context.scan_context, + manifest_files, None, &self.runtime, self.concurrency_limit_manifest_files, @@ -550,7 +549,7 @@ impl TableScan { /// Returns a reference to the snapshot of the table scan. pub fn snapshot(&self) -> Option<&SnapshotRef> { - self.plan_context.as_ref().map(|x| &x.snapshot) + self.plan_context.as_ref().map(|context| &context.snapshot) } } @@ -573,7 +572,7 @@ async fn process_data_manifest_entry( if let Some(ref bound_predicates) = manifest_entry_context.bound_predicates { let BoundPredicates { - snapshot_bound_predicate, + scan_bound_predicate, partition_bound_predicate, } = bound_predicates.as_ref(); @@ -592,7 +591,7 @@ async fn process_data_manifest_entry( // skip any data file whose metrics don't match this scan's filter if !InclusiveMetricsEvaluator::eval( - snapshot_bound_predicate, + scan_bound_predicate, manifest_entry_context.manifest_entry.data_file(), false, )? { @@ -654,7 +653,7 @@ async fn process_delete_manifest_entry( pub(crate) struct BoundPredicates { partition_bound_predicate: BoundPredicate, - snapshot_bound_predicate: BoundPredicate, + scan_bound_predicate: BoundPredicate, } #[cfg(test)] @@ -870,6 +869,7 @@ mod tests { .plan_context .as_ref() .unwrap() + .scan_context .name_mapping .is_none() ); @@ -885,6 +885,7 @@ mod tests { .plan_context .as_ref() .unwrap() + .scan_context .name_mapping .as_ref() .expect("name_mapping should be parsed from the table property"); @@ -2080,7 +2081,13 @@ mod tests { .unwrap_or_else(|e| panic!("scan of data column `{column_name}` failed: {e}")); assert_eq!( - table_scan.plan_context.as_ref().unwrap().field_ids.as_ref(), + table_scan + .plan_context + .as_ref() + .unwrap() + .scan_context + .field_ids + .as_ref(), &[2] ); @@ -2092,6 +2099,7 @@ mod tests { .plan_context .as_ref() .unwrap() + .scan_context .field_ids .as_ref(), &[1, 2] From 3be1c618c2ab5fa26af39146d83665cb8a46c5e3 Mon Sep 17 00:00:00 2001 From: Xander Date: Fri, 28 Aug 2026 14:37:59 +0100 Subject: [PATCH 8/9] don't leak --- crates/iceberg/src/scan/context.rs | 96 ++++++++++++++----- crates/iceberg/src/scan/incremental.rs | 124 +++++++------------------ crates/iceberg/src/scan/mod.rs | 58 +++++------- 3 files changed, 129 insertions(+), 149 deletions(-) diff --git a/crates/iceberg/src/scan/context.rs b/crates/iceberg/src/scan/context.rs index 24a638e815..8164d50dd9 100644 --- a/crates/iceberg/src/scan/context.rs +++ b/crates/iceberg/src/scan/context.rs @@ -15,10 +15,11 @@ // specific language governing permissions and limitations // under the License. +use std::collections::HashSet; use std::sync::Arc; use futures::channel::mpsc::Sender; -use futures::{SinkExt, TryFutureExt}; +use futures::{SinkExt, StreamExt, TryFutureExt, TryStreamExt}; use crate::delete_file_index::DeleteFileIndex; use crate::expr::{Bind, BoundPredicate, Predicate}; @@ -28,15 +29,39 @@ use crate::scan::{ PartitionFilterCache, }; use crate::spec::{ - ManifestContentType, ManifestEntryRef, ManifestFile, NameMapping, PartitionSpecRef, SchemaRef, - SnapshotRef, StructType, TableMetadataRef, + ManifestContentType, ManifestEntryRef, ManifestFile, ManifestList, NameMapping, + PartitionSpecRef, SchemaRef, SnapshotRef, StructType, TableMetadataRef, }; use crate::{Error, ErrorKind, Result}; +/// Filter applied to each manifest file before it is loaded. +/// Returns `true` to include the manifest, `false` to skip it. +pub(crate) type ManifestFileFilter = Arc bool + Send + Sync>; + /// Filter applied to each manifest entry after loading a manifest. /// Returns `true` to include the entry, `false` to skip it. pub(crate) type ManifestEntryFilter = Arc bool + Send + Sync>; +/// Declares the metadata a scan reads: the snapshots whose manifest lists the +/// manifests are drawn from, plus the filters that narrow those manifests and +/// their entries. +pub(crate) struct ManifestSelection { + pub snapshots: Vec, + pub manifest_filter: Option, + pub entry_filter: Option, +} + +impl ManifestSelection { + /// Every manifest listed by a single snapshot. + pub(crate) fn from_snapshot(snapshot: SnapshotRef) -> Self { + Self { + snapshots: vec![snapshot], + manifest_filter: None, + entry_filter: None, + } + } +} + /// Wraps a [`ManifestFile`] alongside the objects that are needed /// to process it in a thread-safe manner pub(crate) struct ManifestFileContext { @@ -166,24 +191,11 @@ impl ManifestEntryContext { } } +/// PlanContext holds everything needed to plan a scan's files: how to project, +/// filter and evaluate them. Which manifests to read is a [`ManifestSelection`], +/// so the same context serves a single-snapshot scan and a snapshot range alike. #[derive(Debug)] pub(crate) struct PlanContext { - pub snapshot: SnapshotRef, - pub scan_context: ScanPlanningContext, -} - -impl PlanContext { - pub(crate) async fn manifest_files(&self) -> Result> { - self.scan_context - .object_cache - .get_manifest_list(&self.snapshot, &self.scan_context.table_metadata) - .await - .map(|manifest_list| manifest_list.entries().to_vec()) - } -} - -#[derive(Debug)] -pub(crate) struct ScanPlanningContext { pub table_metadata: TableMetadataRef, pub scan_schema: SchemaRef, pub case_sensitive: bool, @@ -200,7 +212,39 @@ pub(crate) struct ScanPlanningContext { pub unified_partition_type: Option>, } -impl ScanPlanningContext { +impl PlanContext { + /// Reads the selected snapshots' manifest lists and returns the manifests to + /// scan, deduplicated by path: snapshots in a range often list the same manifest. + async fn manifest_files( + &self, + selection: &ManifestSelection, + concurrency_limit: usize, + ) -> Result> { + let manifest_lists: Vec> = futures::stream::iter(&selection.snapshots) + .map(|snapshot| { + self.object_cache + .get_manifest_list(snapshot, &self.table_metadata) + }) + .buffered(concurrency_limit.max(1)) + .try_collect() + .await?; + + let mut seen = HashSet::new(); + + Ok(manifest_lists + .iter() + .flat_map(|manifest_list| manifest_list.entries()) + .filter(|manifest_file| { + selection + .manifest_filter + .as_ref() + .is_none_or(|filter| filter(manifest_file)) + }) + .filter(|manifest_file| seen.insert(manifest_file.manifest_path.clone())) + .cloned() + .collect()) + } + /// Returns the partition filter for a manifest. See [`PartitionFilterCache::get`] for the /// always-true fallback when the manifest's spec cannot be resolved against the scan schema. fn get_partition_filter(&self, manifest_file: &ManifestFile) -> Result> { @@ -224,14 +268,18 @@ impl ScanPlanningContext { Ok(partition_filter) } - pub(crate) fn build_manifest_file_contexts( + pub(crate) async fn build_manifest_file_contexts( &self, - mut manifest_files: Vec, - manifest_entry_filter: Option, + selection: ManifestSelection, + concurrency_limit_manifest_files: usize, tx_data: Sender, delete_file_idx: DeleteFileIndex, delete_file_tx: Sender, ) -> Result> + 'static>> { + let mut manifest_files = self + .manifest_files(&selection, concurrency_limit_manifest_files) + .await?; + // Sort manifest files to process delete manifests first. // This avoids a deadlock where the producer blocks on sending data manifest entries // (because the data channel is full) while the delete manifest consumer is waiting @@ -278,7 +326,7 @@ impl ScanPlanningContext { partition_bound_predicate, tx, delete_file_idx.clone(), - manifest_entry_filter.clone(), + selection.entry_filter.clone(), ); filtered_mfcs.push(Ok(mfc)); diff --git a/crates/iceberg/src/scan/incremental.rs b/crates/iceberg/src/scan/incremental.rs index 527a1d5cc9..f7f7a6ac20 100644 --- a/crates/iceberg/src/scan/incremental.rs +++ b/crates/iceberg/src/scan/incremental.rs @@ -20,22 +20,16 @@ use std::collections::HashSet; use std::sync::Arc; -use futures::{StreamExt, TryStreamExt}; - use crate::arrow::ArrowReaderBuilder; use crate::expr::Predicate; use crate::io::FileIO; use crate::runtime::Runtime; -use crate::scan::context::ManifestEntryFilter; use crate::scan::{ ArrowRecordBatchStream, ExpressionEvaluatorCache, FileScanTaskStream, ManifestEvaluatorCache, - PartitionFilterCache, ScanPlanningContext, bind_scan_predicate, plan_scan_files, + ManifestSelection, PartitionFilterCache, PlanContext, bind_scan_predicate, plan_scan_files, projected_field_ids, projected_partition_type, table_name_mapping, }; -use crate::spec::{ - ManifestContentType, ManifestFile, ManifestList, ManifestStatus, Operation, SnapshotRef, - TableMetadataRef, -}; +use crate::spec::{ManifestContentType, ManifestStatus, Operation, SnapshotRef, TableMetadataRef}; use crate::table::Table; use crate::util::available_parallelism; use crate::util::snapshot::ancestors_between; @@ -46,13 +40,13 @@ use crate::{Error, ErrorKind, Result}; /// Holds the APPEND snapshots of the range: their manifest lists are the scan's /// manifest source, and their IDs select which manifests and entries it keeps. #[derive(Debug, Clone)] -pub(crate) struct AppendRange { +struct AppendRange { /// Newest first. snapshots: Vec, } impl AppendRange { - pub(crate) fn build( + fn build( table_metadata: &TableMetadataRef, from_snapshot_id: Option, to_snapshot_id: i64, @@ -130,11 +124,6 @@ impl AppendRange { Ok(Self { snapshots }) } - /// The APPEND snapshots in the range, newest first. - pub(crate) fn snapshots(&self) -> &[SnapshotRef] { - &self.snapshots - } - fn snapshot_ids(&self) -> HashSet { self.snapshots .iter() @@ -142,72 +131,34 @@ impl AppendRange { .collect() } - fn manifest_files(&self, manifest_lists: &[Arc]) -> Vec { - let snapshot_ids = self.snapshot_ids(); - let mut seen = HashSet::new(); + /// The metadata this range reads: every snapshot's manifest list is a source, + /// narrowed to the data manifests those snapshots added and, within them, to + /// the entries added by a snapshot of the range. + fn manifest_selection(&self) -> ManifestSelection { + let manifest_snapshot_ids = self.snapshot_ids(); + let entry_snapshot_ids = self.snapshot_ids(); - manifest_lists - .iter() - .flat_map(|manifest_list| manifest_list.entries()) - .filter(|manifest_file| { + ManifestSelection { + snapshots: self.snapshots.clone(), + manifest_filter: Some(Arc::new(move |manifest_file| { manifest_file.content != ManifestContentType::Deletes - && snapshot_ids.contains(&manifest_file.added_snapshot_id) - }) - .filter(|manifest_file| seen.insert(manifest_file.manifest_path.clone())) - .cloned() - .collect() - } - - /// Create a manifest entry filter that includes only entries with - /// status ADDED and a snapshot_id within this range. - pub(crate) fn manifest_entry_filter(&self) -> ManifestEntryFilter { - let snapshot_ids = self.snapshot_ids(); - Arc::new(move |entry| { - entry.status() == ManifestStatus::Added - && entry - .snapshot_id() - .is_some_and(|id| snapshot_ids.contains(&id)) - }) - } -} - -#[derive(Debug)] -struct IncrementalAppendPlanContext { - append_range: AppendRange, - scan_context: ScanPlanningContext, -} - -impl IncrementalAppendPlanContext { - async fn manifest_files(&self, concurrency_limit: usize) -> Result> { - let object_cache = self.scan_context.object_cache.clone(); - let table_metadata = self.scan_context.table_metadata.clone(); - let manifest_lists: Vec> = - futures::stream::iter(self.append_range.snapshots().iter().cloned()) - .map(move |snapshot| { - let object_cache = object_cache.clone(); - let table_metadata = table_metadata.clone(); - async move { - object_cache - .get_manifest_list(&snapshot, &table_metadata) - .await - } - }) - .buffered(concurrency_limit.max(1)) - .try_collect() - .await?; - - Ok(self.append_range.manifest_files(&manifest_lists)) - } - - fn manifest_entry_filter(&self) -> ManifestEntryFilter { - self.append_range.manifest_entry_filter() + && manifest_snapshot_ids.contains(&manifest_file.added_snapshot_id) + })), + entry_filter: Some(Arc::new(move |entry| { + entry.status() == ManifestStatus::Added + && entry + .snapshot_id() + .is_some_and(|id| entry_snapshot_ids.contains(&id)) + })), + } } } /// An incremental scan of data appended between two snapshots. #[derive(Debug)] pub struct IncrementalAppendScan { - plan_context: IncrementalAppendPlanContext, + append_range: AppendRange, + plan_context: PlanContext, batch_size: Option, file_io: FileIO, column_names: Option>, @@ -222,15 +173,9 @@ pub struct IncrementalAppendScan { impl IncrementalAppendScan { /// Returns a stream of files appended in the scan's snapshot range. pub async fn plan_files(&self) -> Result { - let manifest_files = self - .plan_context - .manifest_files(self.concurrency_limit_manifest_files) - .await?; - plan_scan_files( - &self.plan_context.scan_context, - manifest_files, - Some(self.plan_context.manifest_entry_filter()), + &self.plan_context, + self.append_range.manifest_selection(), &self.runtime, self.concurrency_limit_manifest_files, self.concurrency_limit_manifest_entries, @@ -436,7 +381,7 @@ impl<'a> IncrementalAppendScanBuilder<'a> { let name_mapping = table_name_mapping(self.table)?; let unified_partition_type = projected_partition_type(self.table, &schema, &field_ids)?; - let scan_context = ScanPlanningContext { + let plan_context = PlanContext { table_metadata: self.table.metadata_ref(), scan_schema: schema, case_sensitive: self.case_sensitive, @@ -450,12 +395,9 @@ impl<'a> IncrementalAppendScanBuilder<'a> { expression_evaluator_cache: Arc::new(ExpressionEvaluatorCache::new()), unified_partition_type, }; - let plan_context = IncrementalAppendPlanContext { - append_range, - scan_context, - }; Ok(IncrementalAppendScan { + append_range, plan_context, batch_size: self.batch_size, file_io: self.table.file_io().clone(), @@ -595,7 +537,7 @@ mod tests { ); let scan = result.unwrap(); - assert_eq!(scan.plan_context.append_range.snapshots().len(), 1); + assert_eq!(scan.append_range.snapshots.len(), 1); } #[test] @@ -715,7 +657,7 @@ mod tests { .unwrap(); assert_eq!( - range.snapshots().len(), + range.snapshots.len(), 1, "inclusive from == to should yield exactly the one snapshot" ); @@ -1074,18 +1016,18 @@ mod tests { .build() .unwrap(); - let scan_context = &scan.plan_context.scan_context; + let plan_context = &scan.plan_context; // The scan must use the current schema (3 columns), not the // to-snapshot's schema (1 column). let current_schema = table.metadata().current_schema(); assert_eq!( - scan_context.scan_schema.schema_id(), + plan_context.scan_schema.schema_id(), current_schema.schema_id(), "incremental scan should project onto the current schema" ); assert_eq!( - scan_context.scan_schema.as_struct().fields().len(), + plan_context.scan_schema.as_struct().fields().len(), 3, "current schema has three columns (x, y, z)" ); diff --git a/crates/iceberg/src/scan/mod.rs b/crates/iceberg/src/scan/mod.rs index 9e7907fbfe..cd998a39c1 100644 --- a/crates/iceberg/src/scan/mod.rs +++ b/crates/iceberg/src/scan/mod.rs @@ -47,8 +47,8 @@ use crate::metadata_columns::{ use crate::partitioning::compute_unified_partition_type; use crate::runtime::Runtime; use crate::spec::{ - DEFAULT_SCHEMA_NAME_MAPPING, DataContentType, ManifestFile, NameMapping, Schema, SchemaRef, - SnapshotRef, StructType, + DEFAULT_SCHEMA_NAME_MAPPING, DataContentType, NameMapping, Schema, SchemaRef, SnapshotRef, + StructType, }; use crate::table::Table; use crate::util::available_parallelism; @@ -323,6 +323,7 @@ impl<'a> TableScanBuilder<'a> { batch_size: self.batch_size, column_names: self.column_names, file_io: self.table.file_io().clone(), + snapshot: None, plan_context: None, concurrency_limit_data_files: self.concurrency_limit_data_files, concurrency_limit_manifest_entries: self.concurrency_limit_manifest_entries, @@ -344,7 +345,7 @@ impl<'a> TableScanBuilder<'a> { let name_mapping = table_name_mapping(self.table)?; let unified_partition_type = projected_partition_type(self.table, &schema, &field_ids)?; - let scan_context = ScanPlanningContext { + let plan_context = PlanContext { table_metadata: self.table.metadata_ref(), scan_schema: schema, case_sensitive: self.case_sensitive, @@ -358,15 +359,12 @@ impl<'a> TableScanBuilder<'a> { expression_evaluator_cache: Arc::new(ExpressionEvaluatorCache::new()), unified_partition_type, }; - let plan_context = PlanContext { - snapshot, - scan_context, - }; Ok(TableScan { batch_size: self.batch_size, column_names: self.column_names, file_io: self.table.file_io().clone(), + snapshot: Some(snapshot), plan_context: Some(plan_context), concurrency_limit_data_files: self.concurrency_limit_data_files, concurrency_limit_manifest_entries: self.concurrency_limit_manifest_entries, @@ -381,6 +379,7 @@ impl<'a> TableScanBuilder<'a> { /// Table scan. #[derive(Debug)] pub struct TableScan { + snapshot: Option, plan_context: Option, batch_size: Option, file_io: FileIO, @@ -404,9 +403,8 @@ pub struct TableScan { } pub(crate) async fn plan_scan_files( - scan_context: &ScanPlanningContext, - manifest_files: Vec, - manifest_entry_filter: Option, + plan_context: &PlanContext, + selection: ManifestSelection, runtime: &Runtime, concurrency_limit_manifest_files: usize, concurrency_limit_manifest_entries: usize, @@ -419,13 +417,15 @@ pub(crate) async fn plan_scan_files( let (delete_file_idx, delete_file_tx) = DeleteFileIndex::new(runtime.clone()); - let manifest_file_contexts = scan_context.build_manifest_file_contexts( - manifest_files, - manifest_entry_filter, - manifest_entry_data_ctx_tx, - delete_file_idx.clone(), - manifest_entry_delete_ctx_tx, - )?; + let manifest_file_contexts = plan_context + .build_manifest_file_contexts( + selection, + concurrency_limit_manifest_files, + manifest_entry_data_ctx_tx, + delete_file_idx.clone(), + manifest_entry_delete_ctx_tx, + ) + .await?; let mut channel_for_manifest_error = file_scan_task_tx.clone(); let mut channel_for_data_manifest_entry_error = file_scan_task_tx.clone(); @@ -508,15 +508,14 @@ pub(crate) async fn plan_scan_files( impl TableScan { /// Returns a stream of [`FileScanTask`]s. pub async fn plan_files(&self) -> Result { - let Some(plan_context) = self.plan_context.as_ref() else { + let (Some(plan_context), Some(snapshot)) = + (self.plan_context.as_ref(), self.snapshot.as_ref()) + else { return Ok(Box::pin(futures::stream::empty())); }; - let manifest_files = plan_context.manifest_files().await?; - plan_scan_files( - &plan_context.scan_context, - manifest_files, - None, + plan_context, + ManifestSelection::from_snapshot(snapshot.clone()), &self.runtime, self.concurrency_limit_manifest_files, self.concurrency_limit_manifest_entries, @@ -549,7 +548,7 @@ impl TableScan { /// Returns a reference to the snapshot of the table scan. pub fn snapshot(&self) -> Option<&SnapshotRef> { - self.plan_context.as_ref().map(|context| &context.snapshot) + self.snapshot.as_ref() } } @@ -869,7 +868,6 @@ mod tests { .plan_context .as_ref() .unwrap() - .scan_context .name_mapping .is_none() ); @@ -885,7 +883,6 @@ mod tests { .plan_context .as_ref() .unwrap() - .scan_context .name_mapping .as_ref() .expect("name_mapping should be parsed from the table property"); @@ -2081,13 +2078,7 @@ mod tests { .unwrap_or_else(|e| panic!("scan of data column `{column_name}` failed: {e}")); assert_eq!( - table_scan - .plan_context - .as_ref() - .unwrap() - .scan_context - .field_ids - .as_ref(), + table_scan.plan_context.as_ref().unwrap().field_ids.as_ref(), &[2] ); @@ -2099,7 +2090,6 @@ mod tests { .plan_context .as_ref() .unwrap() - .scan_context .field_ids .as_ref(), &[1, 2] From 9f133ecdfe59ac9df7c4195959a323a150cf4a83 Mon Sep 17 00:00:00 2001 From: Xander Date: Fri, 28 Aug 2026 15:30:13 +0100 Subject: [PATCH 9/9] fix --- crates/iceberg/src/scan/cache.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/iceberg/src/scan/cache.rs b/crates/iceberg/src/scan/cache.rs index e0f0c35218..b5eb3c4ce0 100644 --- a/crates/iceberg/src/scan/cache.rs +++ b/crates/iceberg/src/scan/cache.rs @@ -74,7 +74,7 @@ impl PartitionFilterCache { // 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 `ScanPlanningContext` + // 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