Skip to content

Conversation

@gbrgr
Copy link

@gbrgr gbrgr commented Nov 4, 2025

Which issue does this PR close?

What changes are included in this PR?

Integrates virtual field handling for the _file metadata column into RecordBatchTransformer using a pre-computed constants map, eliminating post-processing and duplicate lookups.

Key Changes

New metadata_columns.rs module: Centralized utilities for metadata columns

  • Constants: RESERVED_FIELD_ID_FILE, RESERVED_COL_NAME_FILE
  • Helper functions: get_metadata_column_name(), get_metadata_field_id(), is_metadata_field(), is_metadata_column_name()

Enhanced RecordBatchTransformer:

  • Added constant_fields: HashMap<i32, (DataType, PrimitiveLiteral)> - pre-computed during initialization
  • New with_constant() method - computes Arrow type once during setup
  • Updated to use pre-computed types and values (avoids duplicate lookups)
  • Handles DataType::RunEndEncoded for constant strings (memory efficient)

Simplified reader.rs:

  • Pass full project_field_ids (including virtual) to RecordBatchTransformer
  • Single with_constant() call to register _file column
  • Removed post-processing loop

Updated scan/mod.rs:

  • Use is_metadata_column_name() and get_metadata_field_id() instead of hardcoded checks

Are these changes tested?

Yes, comprehensive tests have been added to verify the functionality:

New Tests (7 tests added)

Table Scan API Tests (7 tests)

  1. test_select_with_file_column - Verifies basic functionality of selecting _file with regular columns
  2. test_select_file_column_position - Verifies column ordering is preserved
  3. test_select_file_column_only - Tests selecting only the _file column
  4. test_file_column_with_multiple_files - Tests multiple data files scenario
  5. test_file_column_at_start - Tests _file at position 0
  6. test_file_column_at_end - Tests _file at the last position
  7. test_select_with_repeated_column_names - Tests repeated column selection

/// // Select regular columns along with the file path
/// let scan = table
/// .scan()
/// .select(["id", "name", RESERVED_COL_NAME_FILE])
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How do we ask for _file column without having to explicitly list all the other columns? E.g. get me all columns + _file. There should be some shortcut for this.

@gbrgr gbrgr changed the title Add support for _file column feat(core): Add support for _file column Nov 4, 2025
@gbrgr gbrgr marked this pull request as ready for review November 4, 2025 14:32
Copy link
Contributor

@liurenjie1024 liurenjie1024 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @gbrgr for this pr. But I think we need to rethink how to compute the _file, _pos metadata column. While it's somehow trivial to compute _file, it's non trivial to compute _pos efficient, since when we read parquet files, we have filtered out some row groups. I think the best way is to push reading these two columns to arrow-rs.

pub(crate) const RESERVED_FIELD_ID_FILE: i32 = 2147483646;

/// Column name for the file path metadata column per Iceberg spec
pub(crate) const RESERVED_COL_NAME_FILE: &str = "_file";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vustef
Copy link

vustef commented Nov 6, 2025

Thanks @gbrgr for this pr. But I think we need to rethink how to compute the _file, _pos metadata column. While it's somehow trivial to compute _file, it's non trivial to compute _pos efficient, since when we read parquet files, we have filtered out some row groups. I think the best way is to push reading these two columns to arrow-rs.

@liurenjie1024 I agree for _pos, and we have a PR there: apache/arrow-rs#8715
But _file seems like something that we don't need the arrow-rs to know about. Similarly, in future, for _row_id from V3 spec, we cannot expect arrow-rs to be responsible for computing that one.

How do we go forward with rethinking this, what would be the action items for us?

Copy link
Contributor

@liurenjie1024 liurenjie1024 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @gbrgr for this pr, I left some comments to improve.

/// # Ok(())
/// # }
/// ```
pub const RESERVED_COL_NAME_FILE: &str = RESERVED_COL_NAME_FILE_INTERNAL;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We will have more metadata columns, so I would prefert to put these definition in sth like metadata_columns module.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a new module

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 column_name == RESERVED_COL_NAME_FILE_INTERNAL {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should have sth like is_metadata_column_name() in metadata_columns module, and useis_metadata_column_name so that we could avoid such changes when we add more metadata columns.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in the new module


/// Helper function to add a `_file` column to a RecordBatch at a specific position.
/// Takes the array, field to add, and position where to insert.
fn create_file_field_at_position(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this approach is not extensible. I prefer what's similar in this pr:

  1. Add constant_map for ArrowReader
  2. Add another variant of RecordBatchTransformer to handle constant field like _file

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I sketched an approach in the record batch transformer, I took the path of the transformer just having a constant_map stored which can be populated by the reader.

@liurenjie1024
Copy link
Contributor

Thanks @gbrgr for this pr. But I think we need to rethink how to compute the _file, _pos metadata column. While it's somehow trivial to compute _file, it's non trivial to compute _pos efficient, since when we read parquet files, we have filtered out some row groups. I think the best way is to push reading these two columns to arrow-rs.

@liurenjie1024 I agree for _pos, and we have a PR there: apache/arrow-rs#8715 But _file seems like something that we don't need the arrow-rs to know about. Similarly, in future, for _row_id from V3 spec, we cannot expect arrow-rs to be responsible for computing that one.

How do we go forward with rethinking this, what would be the action items for us?

Hi, @vustef I also agree that we should put _file in iceberg-rust, and I left some comments about how to proceed.

let vals: Vec<Option<f64>> = vec![None; num_rows];
Arc::new(Float64Array::from(vals))
}
(DataType::RunEndEncoded(_, _), Some(PrimitiveLiteral::String(value))) => {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we in general encode constant columns as REE? Or should we make this custom per field? For the file path it definitely makes sense to run-end encode.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems a bit random to me that only strings use REE in primitive_literal_to_arrow_type below. Yes, they might use the most memory otherwise, but other types also have similar kind of redundancy suitable for the REE.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes that is why I am pointing it out here. If the reviewers are fine with it, I'd go with REE everywhere.

Copy link

@vustef vustef left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the new API with the with_constant. Curious to see what Renjie thinks.

// This is a virtual field - add it with the constant value
return Ok(ColumnSource::Add {
value: Some(constant_value.clone()),
target_type: Self::primitive_literal_to_arrow_type(constant_value)?,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Feels a bit redundant to have to not only lookup constants_map twice, but call primitive_literal_to_arrow_type twice (once here and once in generate_batch_transform)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I store fields now instead of constants, the double lookup is not easily avoidable in the current grand scheme of things, and should not hurt.

let vals: Vec<Option<f64>> = vec![None; num_rows];
Arc::new(Float64Array::from(vals))
}
(DataType::RunEndEncoded(_, _), Some(PrimitiveLiteral::String(value))) => {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems a bit random to me that only strings use REE in primitive_literal_to_arrow_type below. Yes, they might use the most memory otherwise, but other types also have similar kind of redundancy suitable for the REE.

/// The field ID of the metadata column, or an error if the column name is not recognized
pub fn get_metadata_field_id(column_name: &str) -> Result<i32> {
match column_name {
RESERVED_COL_NAME_FILE => Ok(RESERVED_FIELD_ID_FILE),
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wish that we could somehow reuse the mapping from get_metadata_column_name, to form some bidirectional 1-1 mapping. Perhaps some macro magic or something else?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Possible, but I don't think it pays off for a hand full of fields.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Support for _file metadata column

3 participants