Skip to content

fix: project columns correctly when scanning iceberg table metadata - #47

Merged
gabotechs merged 3 commits into
apache:mainfrom
Perseus:fix/iceberg-metadata-table-projection
Oct 13, 2026
Merged

gabotechs merged 3 commits into
apache:mainfrom
Perseus:fix/iceberg-metadata-table-projection

Conversation

@Perseus

@Perseus Perseus commented Oct 6, 2026

Copy link
Copy Markdown
Contributor

Summary

Adds support for projecting columns when scanning Iceberg metadata tables.

Addresses #41

Changes

  • Propagate the given projection into the schema stored in the ExecutionPlan's properties
  • Apply the projection to the batches being produced by execute

check_record_batches(
multiple_projected_columns_result,
expect![[r#"
Field { "operation": nullable Utf8, metadata: {"PARQUET:field_id": "4"} },

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'm not entirely sure if we should be committing these - I see that other tests don't have them, but while running tests locally it asked me to run env UPDATE_EXPECT=1 cargo test so that these expect sections get populated correctly

Comment thread crates/datafusion/src/metadata/scan.rs
@Perseus
Perseus force-pushed the fix/iceberg-metadata-table-projection branch from 537a745 to a2c73d6 Compare October 6, 2026 20:27
Comment on lines +115 to +124
let stream =
futures::stream::once(fut)
.try_flatten()
.map(move |result| -> Result<_> {
let batch = result?;
match &projection {
Some(indices) => Ok(batch.project(indices)?),
None => Ok(batch),
}
});

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🤔 This seems off. This implies that we might be paying the price of reading columns that we are going to end up dropping.

Probably worth checking that we are not reading unnecessary columns on the first place.

@Perseus Perseus Oct 7, 2026 •

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I did try to see if I could push this projection down further into the scan so that we don't have to manually project them after reading all of the columns, but it looks like iceberg-rust doesn't support this - the scan of the tables happens here which calls this in iceberg-rust and that doesn't seem to take projections

Admittedly, I'm still trying to build a mental model of all of this and might be missing something, so I can spend some more time trying to make this work (or open PRs on iceberg-rust to add support for this)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🤔 Ok, this might be worth at least creating an issue in iceberg-rust upstream requesting for this functionality so that we can leave a TODO comment in the code here for future contributors, and leave it like this for now.

WDYT?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes, sounds good - I'll create an issue on that repo and add a comment above the changes in this PR

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

👋 I've updated the PR with the latest changes from main, opened an issue in iceberg-rust apache/iceberg-rust#3391 , and added a TODO in the code here

@Perseus
Perseus force-pushed the fix/iceberg-metadata-table-projection branch from a2c73d6 to 2c56abf Compare October 10, 2026 11:38
Comment thread crates/datafusion/tests/integration_datafusion_test.rs Outdated
@Perseus
Perseus force-pushed the fix/iceberg-metadata-table-projection branch from 21d41de to cc706f1 Compare October 12, 2026 15:21
@Perseus
Perseus force-pushed the fix/iceberg-metadata-table-projection branch from cc706f1 to f1383a9 Compare October 12, 2026 17:18

@gabotechs gabotechs left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

👍 Looks good! thanks @Perseus

@gabotechs
gabotechs merged commit b641e5e into apache:main Oct 13, 2026
5 checks passed
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.

2 participants