Skip to content

Commit 5a46a3e

Browse files
committed
fix(inspect): address remaining PR review comments
1 parent bffa46b commit 5a46a3e

4 files changed

Lines changed: 13 additions & 7 deletions

File tree

‎src/iceberg/inspect/snapshots_table.cc‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -66,9 +66,10 @@ Status AppendSnapshot(ArrowRowBuilder& builder, const Snapshot& snapshot) {
6666

6767
ICEBERG_RETURN_UNEXPECTED(AppendString(builder.column(4), snapshot.manifest_list));
6868

69+
const bool has_summary = !snapshot.summary.empty();
6970
auto summary = snapshot.summary;
7071
summary.erase(SnapshotSummaryFields::kOperation);
71-
if (summary.empty()) {
72+
if (!has_summary) {
7273
ICEBERG_RETURN_UNEXPECTED(AppendNull(builder.column(5)));
7374
} else {
7475
ICEBERG_RETURN_UNEXPECTED(AppendStringMap(builder.column(5), summary));
@@ -87,7 +88,7 @@ class SnapshotsTableStream {
8788
new SnapshotsTableStream(std::move(metadata), std::move(arrow_schema)));
8889
}
8990

90-
~SnapshotsTableStream() { std::ignore = Close(); }
91+
~SnapshotsTableStream() = default;
9192

9293
Status Close() {
9394
metadata_.reset();

‎src/iceberg/test/meson.build‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -47,13 +47,15 @@ iceberg_tests = {
4747
},
4848
'table_test': {
4949
'sources': files(
50+
'history_table_test.cc',
5051
'location_provider_test.cc',
5152
'metadata_table_test.cc',
5253
'metrics_config_test.cc',
5354
'metrics_reporter_test.cc',
5455
'metrics_test.cc',
5556
'snapshot_test.cc',
5657
'snapshot_util_test.cc',
58+
'snapshots_table_test.cc',
5759
'table_metadata_builder_test.cc',
5860
'table_requirement_test.cc',
5961
'table_requirements_test.cc',

‎src/iceberg/test/metadata_table_test_base.h‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -85,9 +85,11 @@ class MetadataTableTestBase : public ::testing::Test {
8585
ArrowArrayStream&& stream) {
8686
auto reader_result = ::arrow::ImportRecordBatchReader(&stream);
8787
if (!reader_result.ok()) {
88+
if (stream.release != nullptr) {
89+
stream.release(&stream);
90+
}
8891
return InvalidArrowData(reader_result.status().ToString());
8992
}
90-
9193
auto batches_result = reader_result.ValueUnsafe()->ToRecordBatches();
9294
if (!batches_result.ok()) {
9395
return InvalidArrowData(batches_result.status().ToString());

‎src/iceberg/test/snapshots_table_test.cc‎

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -88,8 +88,8 @@ TEST_F(SnapshotsTableTest, Scan) {
8888

8989
// Column 0: committed_at (timestamptz) — microseconds since epoch.
9090
auto committed_at = std::static_pointer_cast<::arrow::TimestampArray>(batch->column(0));
91-
EXPECT_EQ(committed_at->Value(0), 1234567890000 * 1000);
92-
EXPECT_EQ(committed_at->Value(1), 9876543210000 * 1000);
91+
EXPECT_EQ(committed_at->Value(0), 1234567890000LL * 1000);
92+
EXPECT_EQ(committed_at->Value(1), 9876543210000LL * 1000);
9393

9494
// Column 1: snapshot_id (long) — returned in storage order.
9595
auto snapshot_ids = std::static_pointer_cast<::arrow::Int64Array>(batch->column(1));
@@ -159,7 +159,7 @@ TEST_F(SnapshotsTableTest, ScanSkipsNullSnapshots) {
159159
EXPECT_EQ(batches.front()->num_rows(), 2);
160160
}
161161

162-
TEST_F(SnapshotsTableTest, ScanTreatsEmptySummaryAsNull) {
162+
TEST_F(SnapshotsTableTest, ScanEmptySummary) {
163163
auto [missing_summary, operation_only_summary] = MakeTestSnapshots();
164164
missing_summary->summary.clear();
165165
operation_only_summary->summary = {
@@ -176,7 +176,8 @@ TEST_F(SnapshotsTableTest, ScanTreatsEmptySummaryAsNull) {
176176
auto summaries =
177177
std::static_pointer_cast<::arrow::MapArray>(batches.front()->column(5));
178178
EXPECT_TRUE(summaries->IsNull(0));
179-
EXPECT_TRUE(summaries->IsNull(1));
179+
EXPECT_FALSE(summaries->IsNull(1));
180+
EXPECT_EQ(summaries->value_length(1), 0);
180181
}
181182

182183
TEST_F(SnapshotsTableTest, ScanReturnsMultipleBatches) {

0 commit comments

Comments
 (0)