Repository navigation
Conversation
Add ColumnSelection and Scan overloads that return only the requested columns, matching Java's metadata table select(). Names resolve through Schema::Select, so nested names, case sensitivity, `*`, and unknown names behave as in TableScanBuilder::Select. Tables now implement ScanProjection(const Schema&) and build only the selected columns. SnapshotsTable uses a per-column append table from the new metadata_table_columns_internal.h helper. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Copilot review overview
2 open findings
What changed in this PR
Adds column projection support to C++ Iceberg metadata table scans (including time travel), aligning behavior with Java (newScan().select(...)) and enabling engines to request only needed columns.
Changes:
- Introduces
ColumnSelectionand overloads ofMetadataTable::Scan(...)/TimeTravelMetadataTable::Scan(...)to project columns viaSchema::Select. - Refactors scanning to be implemented via
ScanProjection(const Schema&)/ScanSnapshot(..., const Schema&)instead of overridingScan(). - Adds internal helpers to map projected schema columns to per-column append functions, and updates
SnapshotsTable+ tests accordingly.
| File | Description |
|---|---|
| src/iceberg/inspect/metadata_table.h | Adds ColumnSelection and new Scan overloads; refactors scan implementation to ScanProjection / projected-schema time travel. |
| src/iceberg/inspect/metadata_table.cc | Implements column selection via Schema::Select and routes scans through projected schemas. |
| src/iceberg/inspect/metadata_table_columns_internal.h | New internal helper to select per-column append functions based on projected schema. |
| src/iceberg/inspect/snapshots_table.h | Updates SnapshotsTable to implement ScanProjection(...) rather than Scan(). |
| src/iceberg/inspect/snapshots_table.cc | Implements projected-column building for snapshots using selected append functions. |
| src/iceberg/inspect/history_table.h / .cc | Updates HistoryTable to ScanProjection(...) while keeping scan unsupported. |
| src/iceberg/test/snapshots_table_test.cc | Adds tests for column selection semantics on SnapshotsTable. |
| src/iceberg/test/metadata_table_test.cc | Adds tests for time-travel scans with column projection and HistoryTable behavior. |
🧠 Review effort: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+67
to
+77
| ICEBERG_CHECK(field.has_value(), "Metadata table column '{}' is not in the schema", | ||
| projected_field.name()); | ||
| if (*field->get().type() != *projected_field.type()) { | ||
| return NotSupported("Cannot select nested fields of metadata table column '{}'", | ||
| projected_field.name()); | ||
| } | ||
| auto column = std::ranges::find(columns, projected_field.field_id(), | ||
| &MetadataTableColumn<Row>::field_id); | ||
| ICEBERG_CHECK(column != columns.end(), | ||
| "Metadata table column '{}' has no append function", | ||
| projected_field.name()); |
| EXPECT_EQ(batch->column_name(0), "summary"); | ||
| ASSERT_EQ(batch->column(0)->type_id(), ::arrow::Type::MAP); | ||
| auto summaries = std::static_pointer_cast<::arrow::MapArray>(batch->column(0)); | ||
| EXPECT_EQ(summaries->value_length(0), 10); |
Copilot stopped reviewing on behalf of
manuzhang due to an error
October 13, 2026 20:17
shangxinli
reviewed
Oct 14, 2026
| /// \brief Scan using the current snapshot on the main branch. | ||
| Result<ArrowArrayStream> Scan() final; | ||
| // Scans without a snapshot selection read the current snapshot on the main branch. | ||
| using MetadataTable::Scan; |
Contributor
There was a problem hiding this comment.
This pulls in Scan(const ColumnSelection&), which makes an existing call like Scan({}) ambiguous against Scan(const SnapshotSelection&) since both are aggregates. Is that source break intended, or would a distinct name for the column-selection form avoid it?
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Metadata table scans currently always return every column. Java's metadata tables support
newScan().select(...):StaticDataTaskprojects each row toscan.schema(), so an engine runningSELECT name FROM t.refsonly getsname. This adds the same column selection to C++ metadata tables.ColumnSelection(column names plus case sensitivity),MetadataTable::Scan(const ColumnSelection&), andTimeTravelMetadataTable::Scan(const SnapshotSelection&, const ColumnSelection&). Names resolve throughSchema::Select, as inTableScanBuilder::Select: nested names are dot-concatenated, columns come back in schema order,*selects all columns, and unknown names are ignored (as in Java'sSchema.select). An empty list returns rows without columns, which is enough to count rows.Scan()is no longer virtual. Tables implement the protectedScanProjection(const Schema& projected_schema)instead, andScanSnapshotalso receives the projected schema.inspect/metadata_table_columns_internal.h, which maps the projected columns to per-column append functions so a table builds only the selected columns. Selecting only some fields of a nested struct column returnsNotSupportedfor now; no current metadata table has a struct column.SnapshotsTablebuilds only the selected columns; for example, it no longer copies thesummarymap unless that column is selected.HistoryTable::Scanstill returnsNotSupported.Open metadata table PRs (#876, #920, #1007) will need to override
ScanProjection(const Schema&)instead ofScan().Validation: pre-commit passed. All 18
metadata_table_testcases pass locally, including 8 new ones covering selection order, unknown names, case sensitivity,*and no selection, an empty selection, map columns, time-travel scans (using a recording test table), andHistoryTable. The changed public headers also compile as a C++20 consumer.🤖 Generated with Claude Code