Copilot commented on code in PR #1070:
URL: https://github.com/apache/iceberg-cpp/pull/1070#discussion_r4444502458
##########
src/iceberg/json_serde.cc:
##########
@@ -1130,7 +1136,16 @@ Result<std::unique_ptr<Snapshot>> SnapshotFromJson(const
nlohmann::json& json) {
ICEBERG_ASSIGN_OR_RAISE(auto unix_ms, GetJsonValue<int64_t>(json,
kTimestampMs));
auto timestamp_ms = TimePointMsFromUnixMs(unix_ms);
ICEBERG_ASSIGN_OR_RAISE(auto manifest_list,
- GetJsonValue<std::string>(json, kManifestList));
+ GetJsonValueOptional<std::string>(json,
kManifestList));
+ ICEBERG_ASSIGN_OR_RAISE(auto root_manifest,
+ GetJsonValueOptional<std::string>(json,
kRootManifest));
+ if (manifest_list.has_value() == root_manifest.has_value()) {
+ return JsonParseError(
+ "Snapshot requires exactly one of manifest-list or root-manifest");
+ }
+ if (json.contains("constraint-statuses") &&
!json["constraint-statuses"].empty()) {
+ return NotSupported("V4 snapshot constraint statuses are not supported
yet");
Review Comment:
`constraint-statuses` is not type-validated. For malformed JSON (e.g.,
boolean/number/object), this will return NotSupported (or behave
inconsistently) rather than a JSON parse/type error, and it doesn’t distinguish
an empty array from an invalid scalar. Prefer strict decoding: if present and
non-null, require an array type; return JsonParseError when the type is wrong;
only return NotSupported when it’s a non-empty array.
##########
src/iceberg/update/expire_snapshots.cc:
##########
@@ -1026,6 +1039,15 @@ Status ExpireSnapshots::Finalize(const TableMetadata&
metadata_after_expiration)
const TableMetadata& metadata_before_expiration =
*apply_result->metadata_before_expiration;
+ // Later transaction updates or the catalog's committed result may introduce
+ // root manifests after Apply(). Never treat untraversable roots as empty.
+ if (HasV4RootManifest(metadata_before_expiration) ||
+ HasV4RootManifest(metadata_after_expiration)) {
+ return NotSupported(
+ "Snapshot expiration file cleanup with v4 root manifests "
+ "is not supported yet");
Review Comment:
Returning `NotSupported` from `Finalize()` is risky because Finalize runs
after the metadata update path has already been selected (and may run
post-commit depending on the transaction/commit implementation). If the intent
is to *skip file deletion* when v4 roots make reachability unsafe, this should
be treated as a successful no-op cleanup (return OK and leave files in place),
not as an operation failure. Consider short-circuiting cleanup while preserving
a successful expiration commit result.
##########
src/iceberg/manifest/v4_types.cc:
##########
@@ -0,0 +1,82 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+#include "iceberg/manifest/v4_types.h"
+
+#include "iceberg/schema_field.h"
+#include "iceberg/type.h"
+
+namespace iceberg {
+
+Result<FileTrackingStatus> FileTrackingStatusFromInt(int32_t status) {
+ switch (status) {
+ case 0:
+ return FileTrackingStatus::kExisting;
+ case 1:
+ return FileTrackingStatus::kAdded;
+ case 2:
+ return FileTrackingStatus::kDeleted;
+ case 3:
+ return FileTrackingStatus::kReplaced;
+ case 4:
+ return FileTrackingStatus::kModified;
+ default:
+ return InvalidArgument("Invalid file tracking status: {}", status);
+ }
+}
+
+std::shared_ptr<StructType> FileTracking::Type() {
+ return std::make_shared<StructType>(std::vector<SchemaField>{
+ SchemaField::MakeRequired(0, "status", int32()),
+ SchemaField::MakeOptional(1, "snapshot_id", int64()),
+ SchemaField::MakeOptional(5, "modified_snapshot_id", int64()),
+ SchemaField::MakeOptional(3, "sequence_number", int64()),
+ SchemaField::MakeOptional(4, "file_sequence_number", int64()),
+ SchemaField::MakeOptional(142, "first_row_id", int64()),
+ SchemaField::MakeOptional(6, "deleted_positions", binary()),
+ SchemaField::MakeOptional(7, "replaced_positions", binary())});
+}
+
+std::shared_ptr<StructType> DeletionVectorMetadata::Type() {
+ return std::make_shared<StructType>(
+ std::vector<SchemaField>{SchemaField::MakeRequired(155, "location",
string()),
+ SchemaField::MakeRequired(144, "offset",
int64()),
+ SchemaField::MakeRequired(145, "size_in_bytes",
int64()),
+ SchemaField::MakeRequired(156, "cardinality",
int64()),
+ SchemaField::MakeOptional(149, "key_metadata",
binary())});
+}
+
+std::shared_ptr<StructType> ManifestInfo::Type() {
+ return std::make_shared<StructType>(std::vector<SchemaField>{
+ SchemaField::MakeRequired(521, "format_version", int32()),
+ SchemaField::MakeOptional(522, "dv", binary()),
+ SchemaField::MakeRequired(504, "added_files_count", int32()),
+ SchemaField::MakeRequired(505, "existing_files_count", int32()),
+ SchemaField::MakeRequired(525, "modified_files_count", int32()),
+ SchemaField::MakeRequired(506, "deleted_files_count", int32()),
+ SchemaField::MakeRequired(523, "replaced_files_count", int32()),
+ SchemaField::MakeRequired(512, "added_rows_count", int64()),
+ SchemaField::MakeRequired(513, "existing_rows_count", int64()),
+ SchemaField::MakeRequired(526, "modified_rows_count", int64()),
+ SchemaField::MakeRequired(514, "deleted_rows_count", int64()),
+ SchemaField::MakeRequired(524, "replaced_rows_count", int64()),
+ SchemaField::MakeRequired(516, "min_sequence_number", int64())});
Review Comment:
`Type()` allocates a new `StructType` (and vector) every call. These schema
objects are typically immutable and reused; consider caching them (e.g.,
function-local static `std::shared_ptr<StructType>`) to avoid repeated heap
allocations in hot paths like schema serialization/validation.
##########
src/iceberg/json_serde.cc:
##########
@@ -1345,11 +1370,30 @@ Result<EncryptedKey> EncryptedKeyFromJson(const
nlohmann::json& json) {
}
Result<nlohmann::json> ToJson(const TableMetadata& table_metadata) {
+ ICEBERG_PRECHECK(
+ table_metadata.format_version >= 1 &&
+ table_metadata.format_version <=
TableMetadata::kSupportedTableFormatVersion,
+ "Unsupported table format version: {}", table_metadata.format_version);
+ for (const auto& snapshot : table_metadata.snapshots) {
+ if (auto status =
snapshot->ValidateForFormatVersion(table_metadata.format_version);
+ !status) {
+ return InvalidArgument("Cannot write snapshot: {}",
status.error().message);
+ }
+ }
Review Comment:
This loop dereferences `snapshot` unconditionally. Elsewhere in this PR
(`HasV4RootManifest`) snapshots are treated as potentially null (`snapshot &&
...`), so `table_metadata.snapshots` can contain null pointers. Add an explicit
null check here (and either skip or return an InvalidArgument/JsonParseError)
to avoid a crash during metadata serialization.
--
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]
