Skip to content
41 changes: 39 additions & 2 deletions crates/iceberg/src/arrow/reader/pipeline.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,12 +20,14 @@
//! predicates, row-group / row selection, and delete handling into a stream
//! of transformed Arrow `RecordBatch`es.

use std::collections::HashMap;
use std::sync::Arc;
use std::sync::atomic::AtomicU64;

use arrow_schema::{DataType, Field};
use futures::{StreamExt, TryStreamExt};
use parquet::arrow::arrow_reader::{ArrowReaderMetadata, ArrowReaderOptions};
use parquet::arrow::{PARQUET_FIELD_ID_META_KEY, ParquetRecordBatchStreamBuilder};
use parquet::arrow::{PARQUET_FIELD_ID_META_KEY, ParquetRecordBatchStreamBuilder, RowNumber};
use parquet::encryption::decrypt::FileDecryptionProperties;

use super::{
Expand All @@ -40,7 +42,8 @@ use crate::encryption::StandardKeyMetadata;
use crate::error::Result;
use crate::io::{FileIO, FileMetadata, FileRead};
use crate::metadata_columns::{
RESERVED_FIELD_ID_FILE, RESERVED_FIELD_ID_SPEC_ID, is_metadata_field,
RESERVED_COL_NAME_POS, RESERVED_FIELD_ID_FILE, RESERVED_FIELD_ID_POS,
RESERVED_FIELD_ID_SPEC_ID, is_metadata_field,
};
use crate::scan::{ArrowRecordBatchStream, FileScanTask, FileScanTaskStream};
use crate::spec::Datum;
Expand Down Expand Up @@ -212,6 +215,35 @@ impl FileScanTaskReader {
arrow_metadata
};

let project_pos = task.project_field_ids().contains(&RESERVED_FIELD_ID_POS);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Reusing RowNumber instead of a manual counter is the right, idiomatic call. It inherits the parquet reader's correctness for selection and row-group boundaries and matches Java's SupportsRowPosition semantics. Nice.


let arrow_metadata = if project_pos {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

There are now up to three sequential ArrowReaderMetadata::try_new rebuilds per file (field-ID schema, INT96 coercion, virtual columns), each re-deriving from parquet metadata. Could the final ArrowReaderOptions (schema plus virtual columns) be assembled once and try_new called a single time? Minor and not blocking; it just compounds with the two existing passes.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

+1, the code is getting harder to follow. I have reviewed this part code several times in the past two weeks but still need to refresh my memory every time I see it :)

I think we can create a tracking issue and have AI check if there is any opportunity to refactor it

let row_number_field = Arc::new(
Field::new(RESERVED_COL_NAME_POS, DataType::Int64, false)
.with_metadata(HashMap::from([(
PARQUET_FIELD_ID_META_KEY.to_string(),
Comment thread
hsiang-c marked this conversation as resolved.
RESERVED_FIELD_ID_POS.to_string(),
)]))
.with_extension_type(RowNumber),
);

let options = ArrowReaderOptions::new()
.with_schema(Arc::clone(arrow_metadata.schema()))
.with_virtual_columns(vec![row_number_field])?;

ArrowReaderMetadata::try_new(Arc::clone(arrow_metadata.metadata()), options).map_err(
|e| {
Error::new(
ErrorKind::Unexpected,
"Failed to create ArrowReaderMetadata with the 'row_number' virtual_column",
)
.with_source(e)
},
)?
} else {
arrow_metadata
};

// Build the stream reader, reusing the already-opened file reader
let mut record_batch_stream_builder =
ParquetRecordBatchStreamBuilder::new_with_metadata(parquet_file_reader, arrow_metadata);
Expand Down Expand Up @@ -274,6 +306,11 @@ impl FileScanTaskReader {
record_batch_transformer_builder.with_partition(partition_spec, partition_data)?;
}

if project_pos {
record_batch_transformer_builder =
record_batch_transformer_builder.with_virtual_field(RESERVED_FIELD_ID_POS);
}

let mut record_batch_transformer = record_batch_transformer_builder.build();

if let Some(batch_size) = self.batch_size {
Expand Down
Loading
Loading