Copilot commented on code in PR #1070:
URL: https://github.com/apache/iceberg-cpp/pull/1070#discussion_r4444891758
##########
src/iceberg/json_serde.cc:
##########
Review Comment:
This still decodes `format-version` for the upgrade action as `int8_t`,
which narrows during JSON decoding. The PR description states upgrade inputs
should remain `int32_t` through strict JSON decoding and only be narrowed after
validation. Consider decoding as `int32_t`, validating
`[1..kSupportedTableFormatVersion]`, then casting/storing the validated value
(and updating `table::UpgradeFormatVersion` if it currently stores an
`int8_t`). This also keeps error classification consistent (unsupported version
vs JSON parse error).
##########
src/iceberg/table_metadata.cc:
##########
@@ -556,6 +573,8 @@ Result<std::string>
TableMetadataUtil::NewTableMetadataFilePath(const TableMetad
return std::format("{}/{}",
LocationUtil::StripTrailingSlash(metadata_location),
filename);
} else {
+ ICEBERG_PRECHECK(!meta.location.empty(),
+ "Cannot generate a metadata file location without a table
location");
Review Comment:
The new precondition error is actionable but doesn’t indicate how to resolve
it. Consider expanding the message to mention the supported alternative(s),
e.g. setting an explicit metadata location property (like
`write.metadata.location`, if applicable in this codebase) or providing a table
base location, so callers know what knob to turn when v4 metadata omits
`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]
