Copilot commented on code in PR #756:
URL: https://github.com/apache/iceberg-cpp/pull/756#discussion_r3437260281
##########
src/iceberg/manifest/manifest_reader.cc:
##########
@@ -477,7 +477,7 @@ Status ParseDataFile(const std::shared_ptr<StructType>&
data_file_schema,
auto part_view = field_view->children[part_idx];
for (int64_t row_idx = 0; row_idx < part_view->length; row_idx++) {
if (ArrowArrayViewIsNull(part_view, row_idx)) {
- break;
+ continue;
}
ICEBERG_RETURN_UNEXPECTED(
ParsePartitionValues(part_view, row_idx, manifest_entries));
Review Comment:
Skipping null partition values with `continue` drops the field entirely,
producing `PartitionValues` with fewer fields than the partition spec (and
shifting positions when there are multiple partition fields). Instead, nulls
should be preserved by appending a typed `Literal::Null(...)` for that
row/partition-field, so `partition.num_fields()` always matches the partition
struct arity.
##########
src/iceberg/test/manifest_reader_test.cc:
##########
@@ -288,6 +288,47 @@ TEST_P(TestManifestReader,
TestManifestReaderWithPartitionMetadata) {
EXPECT_EQ(read_entry.data_file->partition.values()[0], Literal::Int(0));
}
+TEST_P(TestManifestReader, NullPartitionValueDoesNotSkipSubsequentRows) {
+ auto version = GetParam();
+
+ // The first entry has a null partition value, followed by entries with
+ // non-null partition values. A null value must only skip its own row, not
+ // abort parsing of the remaining rows for that partition column (regression
+ // test: the inner loop previously used `break` instead of `continue`).
+ auto file_null = MakeDataFile("/path/to/data-null.parquet",
+ PartitionValues({Literal::Null(int32())}));
+ auto file_b =
+ MakeDataFile("/path/to/data-b.parquet",
PartitionValues({Literal::Int(5)}));
+ auto file_c =
+ MakeDataFile("/path/to/data-c.parquet",
PartitionValues({Literal::Int(7)}));
+
+ std::vector<ManifestEntry> entries;
+ entries.push_back(MakeEntry(ManifestStatus::kAdded, 1000L,
std::move(file_null)));
+ entries.push_back(MakeEntry(ManifestStatus::kAdded, 1000L,
std::move(file_b)));
+ entries.push_back(MakeEntry(ManifestStatus::kAdded, 1000L,
std::move(file_c)));
+
+ auto manifest = WriteManifest(version, /*snapshot_id=*/1000L,
std::move(entries));
+
+ ICEBERG_UNWRAP_OR_FAIL(auto reader,
+ ManifestReader::Make(manifest, file_io_, schema_,
spec_));
+ ICEBERG_UNWRAP_OR_FAIL(auto read_entries, reader->Entries());
+
+ ASSERT_EQ(read_entries.size(), 3U);
+
+ // The null-partition row contributes no partition value.
+ EXPECT_EQ(read_entries[0].data_file->file_path,
"/path/to/data-null.parquet");
+ EXPECT_EQ(read_entries[0].data_file->partition.num_fields(), 0);
+
Review Comment:
This test currently expects a null partition field to result in
`partition.num_fields() == 0`, but the manifest writer encodes nulls as null
elements in the partition struct (preserving arity). The reader should likewise
preserve the field position and return a single typed null literal, otherwise
downstream code that prechecks `spec.fields().size() == partition.num_fields()`
will fail.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
