diff --git a/crates/iceberg/src/metadata_columns.rs b/crates/iceberg/src/metadata_columns.rs index b622a76edc..d95b075be4 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 /// @@ -453,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, @@ -468,25 +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. +/// 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. +/// +/// 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`. +/// +/// [`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 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 { - 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)] @@ -569,4 +600,80 @@ 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_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_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_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")); + 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 + ); + 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 1bad4500aa..19f46c2cb1 100644 --- a/crates/iceberg/src/scan/mod.rs +++ b/crates/iceberg/src/scan/mod.rs @@ -2775,6 +2775,123 @@ 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()) + .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();