Copilot commented on code in PR #1070:
URL: https://github.com/apache/iceberg-cpp/pull/1070#discussion_r4443846869
##########
src/iceberg/snapshot.cc:
##########
@@ -165,6 +165,31 @@ Result<std::optional<int64_t>> Snapshot::FirstRowId()
const { return first_row_i
Result<std::optional<int64_t>> Snapshot::AddedRows() const { return
added_rows; }
+Status Snapshot::ValidateForFormatVersion(int8_t format_version,
+ bool is_new_snapshot) const {
+ if (!root_manifest.has_value()) {
+ ICEBERG_CHECK(!is_new_snapshot || format_version < 4,
+ "New v4 snapshots require root-manifest instead of
manifest-list");
Review Comment:
When `root_manifest` is absent, `ValidateForFormatVersion` returns success
without validating the legacy `manifest_list` at all. This allows invalid
states (e.g., both `root_manifest` unset and an empty `manifest_list`) to pass
validation and be serialized as an empty `manifest-list` string. Consider
enforcing that `manifest_list` must be non-empty whenever `root_manifest` is
not present (and, for v4, that legacy snapshots keep a non-empty
`manifest_list`). This keeps in-memory mutations from producing metadata that
can’t round-trip or violates the spec’s expectations.
##########
src/iceberg/inspect/snapshots_table.cc:
##########
@@ -63,7 +63,11 @@ Status AppendSnapshot(ArrowRowBuilder& builder, const
Snapshot& snapshot) {
ICEBERG_RETURN_UNEXPECTED(AppendNull(builder.column(3)));
}
- ICEBERG_RETURN_UNEXPECTED(AppendString(builder.column(4),
snapshot.manifest_list));
+ if (!snapshot.manifest_list.empty()) {
+ ICEBERG_RETURN_UNEXPECTED(AppendString(builder.column(4),
snapshot.manifest_list));
+ } else {
+ ICEBERG_RETURN_UNEXPECTED(AppendNull(builder.column(4)));
Review Comment:
This nulls `manifest_list` based on `snapshot.manifest_list.empty()` rather
than on whether the snapshot is a v4 root snapshot
(`root_manifest.has_value()`). If an invalid snapshot ever has neither
`root_manifest` nor a non-empty `manifest_list`, the inspection table will
silently show NULL instead of surfacing the empty/invalid value. Prefer
toggling `manifest_list` NULLness based on `root_manifest` presence (NULL only
for root-manifest snapshots), and otherwise append `manifest_list` as-is (or
rely on stricter snapshot validation to prevent empties).
##########
src/iceberg/json_serde.cc:
##########
@@ -1594,18 +1651,37 @@ Result<std::unique_ptr<TableMetadata>>
TableMetadataFromJson(const nlohmann::jso
auto table_metadata = std::make_unique<TableMetadata>();
- ICEBERG_ASSIGN_OR_RAISE(table_metadata->format_version,
- GetJsonValue<int8_t>(json, kFormatVersion));
- if (table_metadata->format_version < 1 ||
- table_metadata->format_version >
TableMetadata::kSupportedTableFormatVersion) {
- return JsonParseError("Cannot read unsupported version: {}",
- table_metadata->format_version);
+ ICEBERG_ASSIGN_OR_RAISE(auto format_version,
+ GetJsonInteger<int32_t>(json, kFormatVersion));
+ if (format_version < 1 ||
+ format_version > TableMetadata::kSupportedTableFormatVersion) {
+ return JsonParseError("Cannot read unsupported version: {}",
format_version);
}
+ table_metadata->format_version = static_cast<int8_t>(format_version);
ICEBERG_ASSIGN_OR_RAISE(table_metadata->table_uuid,
GetJsonValueOrDefault<std::string>(json,
kTableUuid));
- ICEBERG_ASSIGN_OR_RAISE(table_metadata->location,
- GetJsonValue<std::string>(json, kLocation));
+ if (table_metadata->format_version >= 4) {
+ if (json.contains("constraints") && !json["constraints"].empty()) {
+ return NotSupported("V4 table constraints are not supported yet");
+ }
+ if (json.contains("last-constraint-id") && json["last-constraint-id"] !=
0) {
+ return NotSupported("V4 table constraint IDs are not supported yet");
Review Comment:
These checks use direct JSON access and type-agnostic comparisons
(`json[\"last-constraint-id\"] != 0`). For malformed inputs (e.g., `null`,
string, float), this can yield confusing classification (NotSupported) instead
of a JSON parse/type error, and the `constraints` check treats `null` as empty.
Consider parsing these fields via existing JSON helpers (e.g., optional
integer/array getters) so incorrect types consistently return
`kJsonParseError`, while valid-but-nonempty values still return NotSupported.
##########
src/iceberg/location_provider.cc:
##########
@@ -191,11 +191,21 @@ Result<std::string>
ObjectStoreLocationProvider::NewDataLocation(
Result<std::unique_ptr<LocationProvider>> LocationProvider::Make(
std::string_view location, const TableProperties& properties) {
+ const bool object_store =
properties.Get(TableProperties::kObjectStoreEnabled);
+ // Validate once before either provider can derive a data path or table
context.
+ // An explicit absolute URI is sufficient only for the default provider;
object
+ // stores also need the table location to derive their context.
+ ICEBERG_PRECHECK(
+ !location.empty() ||
+ (!object_store &&
+
!LocationUtil::ParseScheme(properties.Get(TableProperties::kWriteDataLocation))
+ .empty()),
+ "Cannot create a location provider without a table location");
Review Comment:
The precondition message is overly generic given the branching logic. When
`location` is empty, the real requirement is either (a) default provider + an
absolute `write.data.location`, or (b) object-store provider always requires
`location`. Consider making the error message reflect that, e.g., mentioning
`write.data.location` must be an absolute URI when no table location is
provided, and that object-store mode requires a table location.
--
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]
