Repository navigation
Conversation
f4e4c83 to
b0f596c
Compare
There was a problem hiding this comment.
Pull request overview
Adds new “system” metadata tables under iceberg/inspect/ and shared utilities to scan Iceberg metadata into bounded Arrow batches, aligning C++ inspect APIs with Iceberg’s reference behavior. This builds on the streaming metadata-table API work referenced in #801.
Changes:
- Adds new metadata table implementations: branches, tags, files, partitions, manifests (plus supporting shared stream/util code).
- Refactors metadata-table base APIs to support typed factories, bounded batch sizing, and time-travel scans via
SnapshotSelection. - Adds/updates unit tests and wires them into CMake/Meson builds.
Reviewed changes
Copilot reviewed 33 out of 33 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/iceberg/type_fwd.h | Forward declares the new inspect table classes. |
| src/iceberg/inspect/metadata_table.h | Expands/modernizes the metadata-table base API (kinds, typed factory, batch size, time travel interface). |
| src/iceberg/inspect/metadata_table.cc | Implements the refactored base behavior and TimeTravelMetadataTable scan dispatch. |
| src/iceberg/inspect/metadata_table_stream_internal.h | Adds a shared Arrow stream implementation for row-vector-backed metadata tables. |
| src/iceberg/inspect/metadata_table_util_internal.h | Declares shared internal helpers for snapshot resolution, schema building, partition projection, and live-file loading. |
| src/iceberg/inspect/metadata_table_util_internal.cc | Implements shared internal utilities used by files/partitions/manifests scans. |
| src/iceberg/inspect/branches_table.h | Declares the branches metadata table. |
| src/iceberg/inspect/branches_table.cc | Implements branch reference scanning into Arrow batches. |
| src/iceberg/inspect/tags_table.h | Declares the tags metadata table. |
| src/iceberg/inspect/tags_table.cc | Implements tag reference scanning into Arrow batches. |
| src/iceberg/inspect/files_table.h | Declares the snapshot-scoped files metadata table (time travel capable). |
| src/iceberg/inspect/files_table.cc | Implements live-file scanning for a selected snapshot into Arrow batches. |
| src/iceberg/inspect/partitions_table.h | Declares the snapshot-scoped partitions aggregates table (time travel capable). |
| src/iceberg/inspect/partitions_table.cc | Implements partition aggregation over live files for a selected snapshot. |
| src/iceberg/inspect/manifests_table.h | Declares the snapshot-scoped manifests table (time travel capable). |
| src/iceberg/inspect/manifests_table.cc | Implements manifest-list scanning for a selected snapshot, including partition summaries. |
| src/iceberg/inspect/snapshots_table.h | Extends snapshots table API with schema + streaming Scan(). |
| src/iceberg/inspect/snapshots_table.cc | Implements snapshots streaming scan with bounded batch size and resource cleanup. |
| src/iceberg/inspect/history_table.h | Updates history table interface to match new base API expectations. |
| src/iceberg/inspect/history_table.cc | Updates history table implementation; currently returns NotSupported for Scan(). |
| src/iceberg/inspect/meson.build | Installs the new public inspect headers. |
| src/iceberg/arrow_row_builder_internal.h | Adds num_rows() to support batch-size enforcement in streaming producers. |
| src/iceberg/arrow_row_builder.cc | Implements ArrowRowBuilder::num_rows(). |
| src/iceberg/test/arrow_row_builder_test.cc | Adds assertions covering the new num_rows() behavior. |
| src/iceberg/test/metadata_table_test_base.h | Introduces a shared fixture/helpers for metadata-table tests. |
| src/iceberg/test/metadata_table_test.cc | Updates tests to the typed metadata-table factory and validates time-travel support flags. |
| src/iceberg/test/history_table_test.cc | Adds schema-focused unit test coverage for HistoryTable. |
| src/iceberg/test/snapshots_table_test.cc | Adds snapshots streaming scan tests (schema/values/batching/null handling). |
| src/iceberg/test/system_metadata_tables_test.cc | Adds integration-style tests for the new system metadata tables (refs/files/partitions/manifests and snapshot selection). |
| src/iceberg/test/CMakeLists.txt | Wires the new/updated metadata-table tests into the CMake test target. |
| src/iceberg/test/meson.build | Wires (some) of the new tests into Meson’s table_test target. |
| src/iceberg/CMakeLists.txt | Adds new inspect sources to the main CMake build. |
| src/iceberg/meson.build | Adds new inspect sources to the main Meson build. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| Result<ArrowArrayStream> HistoryTable::Scan() { | ||
| return NotSupported("Scan is not supported for the history table"); | ||
| } |
b0f596c to
cf28ec0
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved critical and moderate findings affect decimal handling, schema correctness, manifest counts, and streaming memory bounds.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (4)
src/iceberg/inspect/files_table.cc:54
- These schema/type objects are fixed when
FilesTable::Makeruns, butScanSnapshotresolves snapshots and loads files from the source table's current metadata. Aftersource_table()->Refresh()with schema or partition evolution, the scan mixes new files/specs with the stale output schema, silently omitting new metrics or partition fields (or projecting them with the wrong type). Rebuild the scan schema from the same metadata snapshot used for the scan, or otherwise make the cached schema and source metadata consistent.
ICEBERG_ASSIGN_OR_RAISE(auto partition_type, internal::UnifiedPartitionType(*table));
ICEBERG_ASSIGN_OR_RAISE(auto schema,
internal::FilesTableSchema(*table_schema, partition_type));
return std::unique_ptr<FilesTable>(new FilesTable(std::move(table), std::move(schema),
std::move(table_schema),
std::move(partition_type)));
src/iceberg/inspect/manifests_table.cc:123
- When a manifest count is absent,
.value_or(0)converts Iceberg's unknown count into a definitive zero.ManifestFiledocuments a null count as “assumed to be non-zero”, so$manifestsreports incorrect counts for older manifests; preserve null for the applicable content counts and use zero only for the opposite content type.
AppendInt(builder.column(5), data ? manifest.added_files_count.value_or(0) : 0));
ICEBERG_RETURN_UNEXPECTED(
AppendInt(builder.column(6), data ? manifest.existing_files_count.value_or(0) : 0));
src/iceberg/inspect/metadata_table_util_internal.cc:409
- Because specs are processed newest-to-oldest,
existingis the newer void definition andpartition_fieldis the older non-void definition. Replacingdefinitionhere makes the unified schema use the old field name (for example,old_bucket) instead of the latest name (new_name), so the added legacy-evolution test will fail and consumers see a schema that no longer matches the current spec. Keep the latest definition/name and only adopt the older transform's type for decoding legacy values.
if (existing_transform == TransformType::kVoid &&
current_transform != TransformType::kVoid) {
iter->second.definition = &partition_field;
iter->second.type = spec_field.type();
}
src/iceberg/inspect/metadata_table_util_internal.cc:617
- This vector accumulates every live file returned by every manifest, while
LiveEntries()has already materialized each manifest, and the stream is created only after the full vector is built. For a large snapshot, the Arrow batch limit does not bound scan memory or delay the first batch until the entire snapshot has been read. Feed entries through a cursor/iterator directly into bounded batches (or otherwise avoid retaining the complete result).
std::vector<LiveFile> files;
- Files reviewed: 21/21 changed files
- Comments generated: 2
- Review effort level: Lite
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved critical refresh-staleness findings and additional schema, validation, and projection issues remain.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (3)
src/iceberg/inspect/metadata_table_util_internal.cc:383
- On an unpartitioned table this erases
partitionfrom the public files schema even thoughDataFile::Typestill defines it as an empty struct. Consumers therefore get a different column set for partitioned versus unpartitioned tables; the corresponding conditional inPartitionsTable::Makealso dropspartitionandspec_id. Keep these empty-struct/metadata columns and append empty partition values so the metadata-table schema remains stable.
if (partition_type->fields().empty()) {
std::erase_if(fields, [](const SchemaField& field) {
return field.field_id() == DataFile::kPartitionFieldId;
});
}
src/iceberg/inspect/metadata_table_util_internal.cc:75
- An unknown named reference is caller-provided selection input, but
ICEBERG_CHECKmaps this failure toErrorKind::kValidationFailed.Scan(SnapshotSelection{.ref_name = ...})therefore reports a state error instead of the invalid-argument error used for invalid selection values. UseICEBERG_PRECHECKfor this lookup, consistent with the null-reference check below.
ICEBERG_CHECK(ref != metadata->refs.end(), "Cannot find snapshot reference '{}'",
ref_name);
src/iceberg/inspect/partitions_table.cc:259
PartitionsTableonly consumes the partition, content, record count, and file size from each data file, but this callsLiveFileswithout a projection.ManifestReader::ReadEntriesconsequently materializes the complete data-file schema, including all stats maps, bounds, and key metadata, for every live entry before aggregation; wide metrics can make$partitionsscans unnecessarily memory-heavy. Add a projection path forLiveFilesand select only the fields needed by partition projection andUpdateCounts, as Java does for this table.
ICEBERG_ASSIGN_OR_RAISE(auto files, internal::LiveFiles(*source_table(), snapshot));
- Files reviewed: 25/25 changed files
- Comments generated: 2
- Review effort level: Lite
Co-authored-by: Codex <codex@openai.com>
8fe47a4 to
5c8c962
Compare
5c8c962 to
93234f2
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Critical issues remain in partition-evolution schema selection and partition aggregation grouping.
4 open findings
Preserve newest partition field definition during legacy void evolution · New Include partition spec identity in aggregate grouping key · New Although the returned Arrow batches are capped, this call eagerly reads every live manifest entry… HistoryTable is exposed as a concrete MetadataTable with a defined schema, but Scan()…
🧠 Review effort: Lite
| if (existing_transform == TransformType::kVoid && | ||
| current_transform != TransformType::kVoid) { | ||
| iter->second.definition = &partition_field; | ||
| iter->second.type = spec_field.type(); | ||
| } |
| struct PartitionKey { | ||
| PartitionValues values; | ||
| size_t projected_fields; | ||
|
|
||
| bool operator==(const PartitionKey& other) const { | ||
| if (projected_fields != other.projected_fields || | ||
| values.num_fields() != other.values.num_fields()) { |
Co-authored-by: Codex <codex@openai.com>
Co-authored-by: Codex <codex@openai.com>
93234f2 to
a0902e1
Compare


Depends on #998 for shared Arrow/decimal manifest read/write support and #999 for partition transform result types after source columns are dropped. Both fixes are excluded from this PR. Merge the prerequisites first, then rebase this draft onto main so it can build independently with the complete regression coverage.
Add
RefsTable,FilesTable,PartitionsTable, andManifestsTableto inspect Iceberg metadata through Arrow streams.RefsTableexposes branches and tags together using Java's six-column schema andBRANCH/TAGvalues. Snapshot-scoped tables support snapshot IDs, timestamps, or named references; non-main references cannot be combined with an ID or timestamp, matching Java's scan selection rules.File scans load manifests on demand and emit batches of at most 1,024 rows. Partition aggregation canonicalizes NaNs while keeping signed zeros distinct. Decimal partitions and readable bounds use the shared decimal support from #998. Fixed-result partition transforms retain their types after source columns are dropped, and absent manifest bounds render as
"null"to match Java. Sources, public headers, and tests are wired into CMake.Validation: pre-commit and
git diff --checkpassed for the reduced 17-file diff ata0902e1d. Its combined source tree with #998 atbb00e0aaand #999 at76bbabbdis identical to the previously validated tree from93234f2bplus #998:metadata_table_test(48 passed),schema_test(561 passed), andmanifest_test(210 passed, 18 skipped). #999 was also validated independently with all 561 schema tests passing and its expanded regression failing against the old implementation. The prerequisites are not included in this branch.