fix(scan): don't treat data columns named pos or file_path as metadata columns (#2865)

## Which issue does this PR close?

- Closes #2837.

## What changes are included in this PR?

A user data column named `pos` or `file_path` can't be scanned:
projecting it fails with `field not found`, and renaming the column
makes it work again. This bites VCF → Iceberg pipelines, where `pos` is
a common column name.

`pos` and `file_path` are the internal columns of a position-delete
file, but they were registered in the same reserved name→field-id map
(`get_metadata_field_id`) that a data-table scan consults through
`is_metadata_column_name`. Scan planning checks that map first and never
falls back to the data schema, so a real `pos` column gets mapped to a
reserved delete-file id and is never looked up in the actual schema.

The Iceberg spec reserves only `_`-prefixed names as metadata columns,
so they can't collide with user columns. This guards
`is_metadata_column_name` with a `starts_with('_')` check, and the two
bare names stop shadowing data columns. `get_metadata_field_id` still
maps them to their reserved ids, which the delete read path needs via
the id→field direction; its doc now says it isn't a membership test. No
public API signature changes.

## Are these changes tested?

Unit tests, no infrastructure needed:

- `metadata_columns`: `is_metadata_column_name` accepts the `_`-prefixed
names and rejects the bare `pos`/`file_path`, while
`get_metadata_field_id` still maps them to reserved ids.
- `scan`: projecting `pos`/`file_path`, both explicitly and via the
default projection, resolves to the real field id; an absent column
still fails with `DataInvalid` / "not found". Both fail before the fix
and pass after.

`cargo test -p iceberg --lib`, `cargo fmt --check`, and `cargo clippy -D
warnings` all pass.

## One open question for reviewers

This is the minimal fix, on the scan side. The bare names still live in
the shared name→id map, so `get_metadata_field_id("pos").is_ok()` stays
true and the two public predicates now disagree
(`is_metadata_column_name("pos")` is false). Removing the two names from
that map would be the deeper fix and looks safe in-repo (the delete read
path uses the untouched id→field direction), but it changes a second
public function's behavior, so I left it out. Happy to fold it in if
you'd rather go deeper.

Author: LuciferYang

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<i32> {
     }
 }
 
-/// 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"));
+    }
 }

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();