From 4a21e4088f81ced6799d55c269e410fd78d493f2 Mon Sep 17 00:00:00 2001 From: yangjie01 Date: Tue, 21 Jul 2026 21:04:09 +0800 Subject: [PATCH 1/3] fix(scan): don't treat data columns named pos or file_path as metadata columns The bare names `pos` and `file_path` are the internal columns of a position-delete file, but they were registered in the same reserved name to field-id map that data-table scans consult. Because scan planning resolves metadata column names with metadata-first precedence and no fallback to the data schema, a real user data column named `pos` or `file_path` was shadowed: projecting it failed with "field not found". Per the Iceberg spec, reserved metadata column names are `_`-prefixed so they cannot collide with user data columns. Guard `is_metadata_column_name` with a `starts_with('_')` check so the two bare delete-file names are no longer treated as projectable data-table metadata columns. `get_metadata_field_id` still maps them to their reserved ids (the delete-file read path relies on the id->field direction); its doc now notes it is not a membership test. Adds unit tests for the name membership check and scan regression tests covering explicit and default projection of a data column named like a delete-file column. Closes #2837 --- crates/iceberg/src/metadata_columns.rs | 64 +++++++++++++- crates/iceberg/src/scan/mod.rs | 118 +++++++++++++++++++++++++ 2 files changed, 179 insertions(+), 3 deletions(-) diff --git a/crates/iceberg/src/metadata_columns.rs b/crates/iceberg/src/metadata_columns.rs index b622a76edc..b51b3f727e 100644 --- a/crates/iceberg/src/metadata_columns.rs +++ b/crates/iceberg/src/metadata_columns.rs @@ -425,6 +425,12 @@ pub fn get_metadata_field(field_id: i32) -> Result<&'static NestedFieldRef> { /// Returns the field ID for a metadata column name. /// +/// This maps every reserved name to its field id, including the position-delete-file +/// internal columns `pos` and `file_path`, which are not `_`-prefixed. It is therefore +/// not a membership test for data-table metadata columns: `get_metadata_field_id("pos")` +/// returns `Ok` even though `pos` is a valid user data column name. To decide whether a +/// name is a projectable data-table metadata column, use [`is_metadata_column_name`]. +/// /// # Arguments /// * `column_name` - The metadata column name /// @@ -478,15 +484,26 @@ pub fn is_metadata_field(field_id: i32) -> bool { ) } -/// Checks if a column name is a metadata column. +/// Checks if a column name is a metadata column of a data table. +/// +/// Only the `_`-prefixed reserved names (`_file`, `_pos`, `_partition`, `_spec_id`, +/// `_deleted`, and the changelog/row-lineage columns) are metadata columns that can be +/// projected in a data-table scan. Per the Iceberg spec, reserved metadata column names +/// are `_`-prefixed precisely so they cannot collide with user data columns. +/// +/// The bare names `pos` and `file_path` are the internal columns of a position-delete +/// file, not projectable metadata columns of a data table, so they are excluded here even +/// though [`get_metadata_field_id`] still maps them to their reserved field ids. Without +/// this exclusion a real data column named `pos` or `file_path` would be shadowed and fail +/// to scan. /// /// # Arguments /// * `column_name` - The column name to check /// /// # Returns -/// `true` if the column name is a metadata column, `false` otherwise +/// `true` if the column name is a projectable data-table metadata column, `false` otherwise pub fn is_metadata_column_name(column_name: &str) -> bool { - get_metadata_field_id(column_name).is_ok() + column_name.starts_with('_') && get_metadata_field_id(column_name).is_ok() } #[cfg(test)] @@ -569,4 +586,45 @@ mod tests { assert!(get_metadata_field(RESERVED_FIELD_ID_ROW_ID).is_ok()); assert!(get_metadata_field(RESERVED_FIELD_ID_LAST_UPDATED_SEQUENCE_NUMBER).is_ok()); } + + #[test] + fn test_underscore_prefixed_names_are_metadata_columns() { + assert!(is_metadata_column_name(RESERVED_COL_NAME_FILE)); + assert!(is_metadata_column_name(RESERVED_COL_NAME_POS)); + assert!(is_metadata_column_name(RESERVED_COL_NAME_DELETED)); + assert!(is_metadata_column_name(RESERVED_COL_NAME_SPEC_ID)); + assert!(is_metadata_column_name(RESERVED_COL_NAME_PARTITION)); + assert!(is_metadata_column_name(RESERVED_COL_NAME_CHANGE_TYPE)); + assert!(is_metadata_column_name(RESERVED_COL_NAME_ROW_ID)); + } + + #[test] + fn test_delete_file_column_names_are_not_data_table_metadata_columns() { + // `pos` and `file_path` are the internal columns of a position-delete file, not + // projectable metadata columns of a data table. They must not shadow real data + // columns of the same name during a scan (issue #2837). + assert!(!is_metadata_column_name(RESERVED_COL_NAME_DELETE_FILE_POS)); + assert!(!is_metadata_column_name(RESERVED_COL_NAME_DELETE_FILE_PATH)); + assert!(!is_metadata_column_name("pos")); + assert!(!is_metadata_column_name("file_path")); + + // `get_metadata_field_id` intentionally still maps these bare names to their + // reserved field ids; only the data-table membership check excludes them. + assert_eq!( + get_metadata_field_id(RESERVED_COL_NAME_DELETE_FILE_POS).unwrap(), + RESERVED_FIELD_ID_DELETE_FILE_POS + ); + assert_eq!( + get_metadata_field_id(RESERVED_COL_NAME_DELETE_FILE_PATH).unwrap(), + RESERVED_FIELD_ID_DELETE_FILE_PATH + ); + } + + #[test] + fn test_ordinary_data_column_names_are_not_metadata_columns() { + assert!(!is_metadata_column_name("id")); + assert!(!is_metadata_column_name("name")); + // A user column that merely starts with an underscore but is not reserved. + assert!(!is_metadata_column_name("_custom")); + } } diff --git a/crates/iceberg/src/scan/mod.rs b/crates/iceberg/src/scan/mod.rs index e5f2bf9979..a37747825c 100644 --- a/crates/iceberg/src/scan/mod.rs +++ b/crates/iceberg/src/scan/mod.rs @@ -2482,6 +2482,124 @@ pub mod tests { ); } + /// Builds a minimal single-snapshot table (no manifests on disk) whose schema + /// contains a column with the given name, so scan planning resolves column names + /// against a real schema. `TableScan::build()` only reads metadata, not manifest + /// files, so a snapshot pointing at a dummy manifest list is enough. Used to + /// reproduce issue #2837. + fn table_with_data_column(column_name: &str) -> Table { + use crate::spec::{ + FormatVersion, MAIN_BRANCH, Operation, Snapshot, SnapshotReference, SnapshotRetention, + SortOrder, Summary, TableMetadataBuilder, UnboundPartitionSpec, + }; + + let schema = Schema::builder() + .with_schema_id(0) + .with_fields(vec![ + NestedField::required(1, "id", Type::Primitive(PrimitiveType::Int)).into(), + NestedField::required(2, column_name, Type::Primitive(PrimitiveType::Int)).into(), + ]) + .build() + .unwrap(); + + let snapshot = Snapshot::builder() + .with_snapshot_id(1) + .with_timestamp_ms(1) + .with_sequence_number(0) + .with_schema_id(0) + .with_manifest_list("/snap-1.avro") + .with_summary(Summary { + operation: Operation::Append, + additional_properties: HashMap::new(), + }) + .build(); + + let metadata = TableMetadataBuilder::new( + schema, + UnboundPartitionSpec::builder().with_spec_id(0).build(), + SortOrder::unsorted_order(), + "s3://bucket/table".to_string(), + FormatVersion::V2, + HashMap::new(), + ) + .unwrap() + .add_snapshot(snapshot) + .unwrap() + .set_ref(MAIN_BRANCH, SnapshotReference { + snapshot_id: 1, + retention: SnapshotRetention::Branch { + min_snapshots_to_keep: None, + max_snapshot_age_ms: None, + max_ref_age_ms: None, + }, + }) + .unwrap() + .build() + .unwrap() + .metadata; + + Table::builder() + .metadata(metadata) + .identifier(TableIdent::from_strs(["db", "table1"]).unwrap()) + .file_io(FileIO::new_with_fs()) + .metadata_location("s3://bucket/table/metadata/v1.json") + .runtime(test_runtime()) + .build() + .unwrap() + } + + /// A user data column named `pos` (a delete-file internal column name that is not a + /// data-table metadata column) must be projectable rather than shadowed. Regression + /// test for issue #2837. + #[test] + fn test_scan_projects_data_column_named_like_delete_file_column() { + for column_name in ["pos", "file_path"] { + let table = table_with_data_column(column_name); + + // Projecting the data column must succeed and resolve to its real field id (2), + // not the reserved delete-file field id. + let table_scan = table + .scan() + .select([column_name]) + .build() + .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(), + &[2] + ); + + // The default projection (all columns) must resolve to the real field ids + // too, not shadow the data column with a reserved delete-file id. + let default_scan = table.scan().build().unwrap(); + assert_eq!( + default_scan + .plan_context + .as_ref() + .unwrap() + .field_ids + .as_ref(), + &[1, 2] + ); + } + } + + /// Projecting a genuinely absent column still fails with a clear "not found" error + /// rather than being silently accepted as a metadata column. + #[test] + fn test_scan_rejects_unknown_column_named_like_delete_file_column() { + // This table has no `pos` column (only `id` and `file_path`). + let table = table_with_data_column("file_path"); + + let err = table + .scan() + .select(["pos"]) + .build() + .expect_err("projecting an absent column should fail"); + assert_eq!(err.kind(), ErrorKind::DataInvalid); + assert!(err.to_string().contains("not found")); + } + #[tokio::test] async fn test_scan_deadlock() { let mut fixture = TableTestFixture::new(); From dd5285e1fc5bb1eac3029f8d4223db1039386a3c Mon Sep 17 00:00:00 2001 From: yangjie01 Date: Wed, 22 Jul 2026 12:56:29 +0800 Subject: [PATCH 2/3] test: drop redundant assertions and unused metadata_location in #2837 tests The `is_metadata_column_name` negative assertions repeated the same calls via the RESERVED_COL_NAME_DELETE_FILE_* constants and their bare-string values (the constants are defined as "pos" / "file_path"); keep the string form. The scan test helper set a `metadata_location` that scan planning never reads and TableBuilder does not require. --- crates/iceberg/src/metadata_columns.rs | 2 -- crates/iceberg/src/scan/mod.rs | 1 - 2 files changed, 3 deletions(-) diff --git a/crates/iceberg/src/metadata_columns.rs b/crates/iceberg/src/metadata_columns.rs index b51b3f727e..43031f9724 100644 --- a/crates/iceberg/src/metadata_columns.rs +++ b/crates/iceberg/src/metadata_columns.rs @@ -603,8 +603,6 @@ mod tests { // `pos` and `file_path` are the internal columns of a position-delete file, not // projectable metadata columns of a data table. They must not shadow real data // columns of the same name during a scan (issue #2837). - assert!(!is_metadata_column_name(RESERVED_COL_NAME_DELETE_FILE_POS)); - assert!(!is_metadata_column_name(RESERVED_COL_NAME_DELETE_FILE_PATH)); assert!(!is_metadata_column_name("pos")); assert!(!is_metadata_column_name("file_path")); diff --git a/crates/iceberg/src/scan/mod.rs b/crates/iceberg/src/scan/mod.rs index a37747825c..ee6b3241b4 100644 --- a/crates/iceberg/src/scan/mod.rs +++ b/crates/iceberg/src/scan/mod.rs @@ -2542,7 +2542,6 @@ pub mod tests { .metadata(metadata) .identifier(TableIdent::from_strs(["db", "table1"]).unwrap()) .file_io(FileIO::new_with_fs()) - .metadata_location("s3://bucket/table/metadata/v1.json") .runtime(test_runtime()) .build() .unwrap() From 5f7f8dce13e57a61f6bf3805d3b691458f0ecbfe Mon Sep 17 00:00:00 2001 From: yangjie01 Date: Thu, 23 Jul 2026 16:09:17 +0800 Subject: [PATCH 3/3] refactor(metadata): mirror Java allowlist in is_metadata_column_name / is_metadata_field Following review on #2865, replace the ad-hoc `starts_with('_')` guard with an explicit allowlist so both the name and field-id membership checks match Java's `MetadataColumns` exactly (as of iceberg 1.11.0): - `is_metadata_column_name` now matches the seven names Java accepts in `isMetadataColumn(String)` (the six `META_COLUMNS` keys plus the `_partition` special case), instead of "any `_`-prefixed name that resolves". This still excludes the position-delete-file columns `pos`/`file_path` (the #2837 fix) and additionally excludes the changelog columns `_change_type` / `_change_ordinal` / `_commit_snapshot_id`, which Java also keeps out of `META_COLUMNS`. - `is_metadata_field` drops the delete-file ids (i32::MAX - 101/-102) and the changelog ids (i32::MAX - 104/-105/-106) so it equals Java's `META_IDS`. Its only caller filters metadata field ids out of the Parquet projection; those ids can never reach `project_field_ids`, so this is not an observable change, it just keeps the id side honest. `get_metadata_field_id` and `get_metadata_field` are unchanged: they still map the bare names and resolve the reserved ids, so the delete read path is unaffected. Tests assert both sides (name and id) for the accepted columns and for the excluded delete-file/changelog/`row` names. --- crates/iceberg/src/metadata_columns.rs | 107 ++++++++++++++++++------- 1 file changed, 79 insertions(+), 28 deletions(-) diff --git a/crates/iceberg/src/metadata_columns.rs b/crates/iceberg/src/metadata_columns.rs index 43031f9724..d95b075be4 100644 --- a/crates/iceberg/src/metadata_columns.rs +++ b/crates/iceberg/src/metadata_columns.rs @@ -459,13 +459,20 @@ pub fn get_metadata_field_id(column_name: &str) -> Result { } } -/// Checks if a field ID is a metadata field. +/// Checks if a field ID is a data-table metadata column. +/// +/// Mirrors Java `MetadataColumns.isMetadataColumn(int)` (backed by `META_IDS`): only the +/// field ids of columns projectable in a data-table scan return `true`. The +/// position-delete-file internal columns (`file_path`/`pos`, ids `i32::MAX - 101/-102`) +/// and the changelog columns (`_change_type`/`_change_ordinal`/`_commit_snapshot_id`) are +/// deliberately excluded, so their ids return `false` here even though +/// [`get_metadata_field`] can still resolve them. /// /// # Arguments /// * `field_id` - The field ID to check /// /// # Returns -/// `true` if the field ID is a (currently supported) metadata field, `false` otherwise +/// `true` if the field ID is a data-table metadata column, `false` otherwise pub fn is_metadata_field(field_id: i32) -> bool { matches!( field_id, @@ -474,36 +481,43 @@ pub fn is_metadata_field(field_id: i32) -> bool { | RESERVED_FIELD_ID_DELETED | RESERVED_FIELD_ID_SPEC_ID | RESERVED_FIELD_ID_PARTITION - | RESERVED_FIELD_ID_DELETE_FILE_PATH - | RESERVED_FIELD_ID_DELETE_FILE_POS - | RESERVED_FIELD_ID_CHANGE_TYPE - | RESERVED_FIELD_ID_CHANGE_ORDINAL - | RESERVED_FIELD_ID_COMMIT_SNAPSHOT_ID | RESERVED_FIELD_ID_ROW_ID | RESERVED_FIELD_ID_LAST_UPDATED_SEQUENCE_NUMBER ) } -/// Checks if a column name is a metadata column of a data table. +/// Checks if a column name is a data-table metadata column. +/// +/// Mirrors Java `MetadataColumns.isMetadataColumn(String)` (backed by its `META_COLUMNS` +/// allowlist plus the `_partition` special case): a column name is a data-table metadata +/// column only if it is one of the reserved names that a scan can project. /// -/// Only the `_`-prefixed reserved names (`_file`, `_pos`, `_partition`, `_spec_id`, -/// `_deleted`, and the changelog/row-lineage columns) are metadata columns that can be -/// projected in a data-table scan. Per the Iceberg spec, reserved metadata column names -/// are `_`-prefixed precisely so they cannot collide with user data columns. +/// This deliberately excludes two groups of reserved names that Java also keeps out of +/// `META_COLUMNS`: +/// - the position-delete-file internal columns `pos`, `file_path`, and `row`, so a user +/// data column of the same name is not shadowed during a scan (issue #2837); +/// - the changelog columns `_change_type`, `_change_ordinal`, and `_commit_snapshot_id`. /// -/// The bare names `pos` and `file_path` are the internal columns of a position-delete -/// file, not projectable metadata columns of a data table, so they are excluded here even -/// though [`get_metadata_field_id`] still maps them to their reserved field ids. Without -/// this exclusion a real data column named `pos` or `file_path` would be shadowed and fail -/// to scan. +/// [`get_metadata_field_id`] still maps the reserved names it knows (including `pos` and +/// `file_path`) to their field ids; only this membership check excludes them. (`row` has +/// no reserved id in this crate, so `get_metadata_field_id("row")` is an error.) /// /// # Arguments /// * `column_name` - The column name to check /// /// # Returns -/// `true` if the column name is a projectable data-table metadata column, `false` otherwise +/// `true` if the column name is a data-table metadata column, `false` otherwise pub fn is_metadata_column_name(column_name: &str) -> bool { - column_name.starts_with('_') && get_metadata_field_id(column_name).is_ok() + matches!( + column_name, + RESERVED_COL_NAME_FILE + | RESERVED_COL_NAME_POS + | RESERVED_COL_NAME_DELETED + | RESERVED_COL_NAME_SPEC_ID + | RESERVED_COL_NAME_PARTITION + | RESERVED_COL_NAME_ROW_ID + | RESERVED_COL_NAME_LAST_UPDATED_SEQUENCE_NUMBER + ) } #[cfg(test)] @@ -588,26 +602,63 @@ mod tests { } #[test] - fn test_underscore_prefixed_names_are_metadata_columns() { + fn test_data_table_metadata_columns() { + // Both sides of the data-table metadata check (name and field id) must accept + // these seven columns. On the name side Java keeps six in `META_COLUMNS` and + // recognizes `_partition` via the `isMetadataColumn(String)` special case; on the + // id side all seven are in `META_IDS`. assert!(is_metadata_column_name(RESERVED_COL_NAME_FILE)); assert!(is_metadata_column_name(RESERVED_COL_NAME_POS)); assert!(is_metadata_column_name(RESERVED_COL_NAME_DELETED)); assert!(is_metadata_column_name(RESERVED_COL_NAME_SPEC_ID)); assert!(is_metadata_column_name(RESERVED_COL_NAME_PARTITION)); - assert!(is_metadata_column_name(RESERVED_COL_NAME_CHANGE_TYPE)); assert!(is_metadata_column_name(RESERVED_COL_NAME_ROW_ID)); + assert!(is_metadata_column_name( + RESERVED_COL_NAME_LAST_UPDATED_SEQUENCE_NUMBER + )); + + assert!(is_metadata_field(RESERVED_FIELD_ID_FILE)); + assert!(is_metadata_field(RESERVED_FIELD_ID_POS)); + assert!(is_metadata_field(RESERVED_FIELD_ID_DELETED)); + assert!(is_metadata_field(RESERVED_FIELD_ID_SPEC_ID)); + assert!(is_metadata_field(RESERVED_FIELD_ID_PARTITION)); + assert!(is_metadata_field(RESERVED_FIELD_ID_ROW_ID)); + assert!(is_metadata_field( + RESERVED_FIELD_ID_LAST_UPDATED_SEQUENCE_NUMBER + )); } #[test] - fn test_delete_file_column_names_are_not_data_table_metadata_columns() { - // `pos` and `file_path` are the internal columns of a position-delete file, not - // projectable metadata columns of a data table. They must not shadow real data - // columns of the same name during a scan (issue #2837). + fn test_changelog_columns_are_not_data_table_metadata_columns() { + // Changelog columns are not in Java's `META_COLUMNS`, so they are not projectable + // data-table metadata columns (`isMetadataColumn` is false for them). + assert!(!is_metadata_column_name(RESERVED_COL_NAME_CHANGE_TYPE)); + assert!(!is_metadata_column_name(RESERVED_COL_NAME_CHANGE_ORDINAL)); + assert!(!is_metadata_column_name( + RESERVED_COL_NAME_COMMIT_SNAPSHOT_ID + )); + assert!(!is_metadata_field(RESERVED_FIELD_ID_CHANGE_TYPE)); + assert!(!is_metadata_field(RESERVED_FIELD_ID_CHANGE_ORDINAL)); + assert!(!is_metadata_field(RESERVED_FIELD_ID_COMMIT_SNAPSHOT_ID)); + } + + #[test] + fn test_delete_file_columns_are_not_data_table_metadata_columns() { + // `pos`, `file_path`, and `row` are the internal columns of a position-delete file, + // not projectable metadata columns of a data table. They must not shadow real data + // columns of the same name during a scan (issue #2837). Java returns false for both + // their names and their field ids; mirror that on both sides here. assert!(!is_metadata_column_name("pos")); assert!(!is_metadata_column_name("file_path")); - - // `get_metadata_field_id` intentionally still maps these bare names to their - // reserved field ids; only the data-table membership check excludes them. + assert!(!is_metadata_column_name("row")); + assert!(!is_metadata_field(RESERVED_FIELD_ID_DELETE_FILE_POS)); + assert!(!is_metadata_field(RESERVED_FIELD_ID_DELETE_FILE_PATH)); + // `row` (Java's DELETE_FILE_ROW_FIELD_ID, i32::MAX - 103) has no reserved id in + // this crate, so it is not a metadata field either. + assert!(!is_metadata_field(i32::MAX - 103)); + + // `get_metadata_field_id` intentionally still maps the two bare names it knows to + // their reserved field ids; only the data-table membership check excludes them. assert_eq!( get_metadata_field_id(RESERVED_COL_NAME_DELETE_FILE_POS).unwrap(), RESERVED_FIELD_ID_DELETE_FILE_POS