Conversation
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| // 7. Create ColumnInfos vector sized to num_columns, with placeholders for non-needed columns | ||
| let mut column_infos: Vec<Arc<ColumnInfo>> = Vec::with_capacity(num_columns); | ||
| for (col_idx, meta_opt) in column_metadatas_opt.iter().enumerate() { | ||
| if let Some(meta) = meta_opt { | ||
| column_infos.push(Self::col_meta_to_col_info( | ||
| col_idx as u32, | ||
| meta, | ||
| file_version, | ||
| )); | ||
| } else { | ||
| // Placeholder ColumnInfo for non-requested columns | ||
| column_infos.push(Arc::new(ColumnInfo { | ||
| index: col_idx as u32, | ||
| page_infos: Arc::from([]), | ||
| buffer_offsets_and_sizes: Arc::from([]), | ||
| encoding: ColumnEncodingMsg { | ||
| column_encoding: Some(ColumnEncodingKind::Values(())), | ||
| }, | ||
| })); | ||
| } | ||
| } |
There was a problem hiding this comment.
When try_open is given a base projection, read_metadata_for_projection now returns CachedFileMetadata with empty ColumnInfo/ColumnMetadata placeholders for every column that was not requested (column_infos.push(Arc::new(ColumnInfo { … page_infos: [], buffer_offsets_and_sizes: [] }))). However, the reader still exposes collect_columns_from_projection which blindly returns self.metadata.column_infos for any projection. Callers can therefore open a file with a small base projection and later pass a new projection that includes additional columns; those columns will be backed by the placeholder metadata and have zero pages/buffers. Downstream decoding will either produce empty arrays or fail because the requested column appears to contain no data, a regression from previous behaviour where the reader could load any projection. Either the API needs to reject projections for columns not loaded during try_open, or the metadata for missing columns must be fetched lazily when first requested.
Useful? React with 👍 / 👎.
|
I think this has been superseded by #7375 |
Currently, if we want to perform a projected read of a file, we load all of the column metadata, and then only read the column data that is requested. This is fine for most cases. However, if there are many columns then loading all of the column metadata may be expensive. We should support a mode where we only load the column metadata for the columns that are requested (the file format supports this).
There is a benchmark in the F3 paper's 7.2 section, it also describes this issue as below: