Skip to content

Commit 09ca619

Browse files
committed
fix(inspect): address remaining PR review comments
1 parent 9bb29f6 commit 09ca619

3 files changed

Lines changed: 19 additions & 13 deletions

File tree

‎src/iceberg/inspect/snapshots_table.cc‎

Lines changed: 11 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -23,15 +23,14 @@
2323
#include <cstddef>
2424
#include <memory>
2525
#include <optional>
26-
#include <tuple>
2726
#include <utility>
2827
#include <vector>
2928

3029
#include <nanoarrow/nanoarrow.h>
3130

32-
#include "iceberg/arrow/nanoarrow_status_internal.h"
3331
#include "iceberg/arrow_c_data_util_internal.h"
3432
#include "iceberg/arrow_row_builder_internal.h"
33+
#include "iceberg/nanoarrow_status_internal.h"
3534
#include "iceberg/schema.h"
3635
#include "iceberg/schema_field.h"
3736
#include "iceberg/schema_internal.h"
@@ -66,12 +65,16 @@ Status AppendSnapshot(ArrowRowBuilder& builder, const Snapshot& snapshot) {
6665

6766
ICEBERG_RETURN_UNEXPECTED(AppendString(builder.column(4), snapshot.manifest_list));
6867

69-
auto summary = snapshot.summary;
70-
summary.erase(SnapshotSummaryFields::kOperation);
71-
if (summary.empty()) {
72-
ICEBERG_RETURN_UNEXPECTED(AppendNull(builder.column(5)));
68+
if (!snapshot.summary.empty()) {
69+
if (snapshot.summary.contains(SnapshotSummaryFields::kOperation)) {
70+
auto summary = snapshot.summary;
71+
summary.erase(SnapshotSummaryFields::kOperation);
72+
ICEBERG_RETURN_UNEXPECTED(AppendStringMap(builder.column(5), summary));
73+
} else {
74+
ICEBERG_RETURN_UNEXPECTED(AppendStringMap(builder.column(5), snapshot.summary));
75+
}
7376
} else {
74-
ICEBERG_RETURN_UNEXPECTED(AppendStringMap(builder.column(5), summary));
77+
ICEBERG_RETURN_UNEXPECTED(AppendNull(builder.column(5)));
7578
}
7679

7780
return builder.FinishRow();
@@ -87,7 +90,7 @@ class SnapshotsTableStream {
8790
new SnapshotsTableStream(std::move(metadata), std::move(arrow_schema)));
8891
}
8992

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

9295
Status Close() {
9396
metadata_.reset();

‎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)