Skip to content

Commit 6c112cc

Browse files
committed
fix(inspect): address metadata table review feedback
1 parent 3b11911 commit 6c112cc

5 files changed

Lines changed: 9 additions & 20 deletions

File tree

‎src/iceberg/inspect/metadata_table.cc‎

Lines changed: 2 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -35,20 +35,10 @@ MetadataTable::MetadataTable(std::shared_ptr<Table> source_table,
3535

3636
MetadataTable::~MetadataTable() = default;
3737

38-
bool MetadataTable::supports_time_travel() const noexcept {
39-
// Time travel is supported for tables that read from a single snapshot's
40-
// manifests. Tables that scan all snapshots or return in-memory history do
41-
// not.
42-
switch (kind()) {
43-
case Kind::kSnapshots:
44-
case Kind::kHistory:
45-
return false;
46-
}
47-
return false;
48-
}
38+
bool MetadataTable::supports_time_travel() const noexcept { return false; }
4939

5040
Result<ArrowArray> MetadataTable::Scan(
51-
std::optional<SnapshotSelection> /*snapshot_selection*/) {
41+
const std::optional<SnapshotSelection>& /*snapshot_selection*/) {
5242
return NotSupported("Scan is not supported for this metadata table type");
5343
}
5444

‎src/iceberg/inspect/metadata_table.h‎

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -62,10 +62,8 @@ class ICEBERG_EXPORT MetadataTable {
6262

6363
/// \brief Whether this metadata table supports time-travel queries.
6464
///
65-
/// Time travel is supported for tables that read from a single snapshot's
66-
/// manifests (e.g., Entries, Files, Manifests, Partitions). Tables that
67-
/// scan all snapshots (All*) or return in-memory history (Snapshots,
68-
/// History, Refs) do not support time travel.
65+
/// The currently supported snapshots and history metadata tables do not
66+
/// support time travel.
6967
bool supports_time_travel() const noexcept;
7068

7169
/// \brief Scan the metadata table using the current snapshot.
@@ -80,7 +78,8 @@ class ICEBERG_EXPORT MetadataTable {
8078
///
8179
/// The default implementation returns NotSupported. Subclasses override this
8280
/// to materialize their data.
83-
virtual Result<ArrowArray> Scan(std::optional<SnapshotSelection> snapshot_selection);
81+
virtual Result<ArrowArray> Scan(
82+
const std::optional<SnapshotSelection>& snapshot_selection);
8483

8584
const TableIdentifier& name() const { return identifier_; }
8685

‎src/iceberg/inspect/snapshots_table.cc‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -69,7 +69,7 @@ Result<std::unique_ptr<SnapshotsTable>> SnapshotsTable::Make(
6969
}
7070

7171
Result<ArrowArray> SnapshotsTable::Scan(
72-
std::optional<SnapshotSelection> /*snapshot_selection*/) {
72+
const std::optional<SnapshotSelection>& /*snapshot_selection*/) {
7373
ICEBERG_ASSIGN_OR_RAISE(auto builder, ArrowRowBuilder::Make(*schema()));
7474

7575
for (const auto& snapshot : source_table()->snapshots()) {

‎src/iceberg/inspect/snapshots_table.h‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -45,7 +45,7 @@ class ICEBERG_EXPORT SnapshotsTable : public MetadataTable {
4545
/// The snapshots table always returns every known snapshot, so the
4646
/// snapshot_selection parameter is ignored.
4747
Result<ArrowArray> Scan(
48-
std::optional<SnapshotSelection> /*snapshot_selection*/) override;
48+
const std::optional<SnapshotSelection>& /*snapshot_selection*/) override;
4949

5050
private:
5151
explicit SnapshotsTable(std::shared_ptr<Table> table);

‎src/iceberg/test/metadata_table_test_base.h‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -78,7 +78,7 @@ class MetadataTableTestBase : public ::testing::Test {
7878
/// \brief Import a Scan()-produced ArrowArray into an Arrow RecordBatch.
7979
static std::shared_ptr<::arrow::RecordBatch> FinishAndImport(ArrowArray array,
8080
const Schema& schema) {
81-
ArrowSchema c_schema;
81+
ArrowSchema c_schema{};
8282
EXPECT_THAT(ToArrowSchema(schema, &c_schema), IsOk());
8383
auto arrow_schema = ::arrow::ImportSchema(&c_schema).ValueOrDie();
8484

0 commit comments

Comments
 (0)