Copilot commented on code in PR #960:
URL: https://github.com/apache/iceberg-cpp/pull/960#discussion_r4249694128
##########
src/iceberg/catalog/rest/json_serde.cc:
##########
@@ -474,6 +479,59 @@ Result<RenameTableRequest>
RenameTableRequestFromJson(const nlohmann::json& json
return request;
}
+nlohmann::json ToJson(const FieldLabel& field_label) {
+ nlohmann::json json;
+ json[kFieldId] = field_label.field_id;
+ json[kLabels] = field_label.labels;
+ return json;
+}
+
+Result<FieldLabel> FieldLabelFromJson(const nlohmann::json& json) {
+ if (!json.is_object()) {
+ return JsonParseError("Cannot parse field label from non-object: {}",
+ SafeDumpJson(json));
+ }
+ FieldLabel field_label;
+ ICEBERG_ASSIGN_OR_RAISE(field_label.field_id, GetJsonInteger<int32_t>(json,
kFieldId));
+ ICEBERG_ASSIGN_OR_RAISE(field_label.labels,
+ GetJsonValue<decltype(field_label.labels)>(json,
kLabels));
+ return field_label;
+}
+
+nlohmann::json ToJson(const Labels& labels) {
+ nlohmann::json json = nlohmann::json::object();
+ if (!labels.object_labels.empty()) {
+ json[kObjectLabels] = labels.object_labels;
+ }
+ if (!labels.fields.empty()) {
+ nlohmann::json fields = nlohmann::json::array();
+ for (const auto& field_label : labels.fields) {
+ fields.push_back(ToJson(field_label));
+ }
+ json[kFields] = std::move(fields);
+ }
+ return json;
+}
+
+// A non-object value is treated as absent labels rather than failing the load.
+Result<Labels> LabelsFromJson(const nlohmann::json& json) {
+ Labels labels;
+ ICEBERG_ASSIGN_OR_RAISE(
+ labels.object_labels,
+ GetJsonValueOrDefault<decltype(labels.object_labels)>(json,
kObjectLabels));
+ if (auto it = json.find(kFields); it != json.end() && !it->is_null()) {
+ if (!it->is_array()) {
+ return JsonParseError("Cannot parse '{}' from non-array: {}", kFields,
+ SafeDumpJson(*it));
+ }
+ for (const auto& entry : *it) {
+ ICEBERG_ASSIGN_OR_RAISE(auto field_label, FieldLabelFromJson(entry));
+ labels.fields.push_back(std::move(field_label));
+ }
+ }
+ return labels;
+}
Review Comment:
`LabelsFromJson` claims (and tests expect) that non-object `"labels"` values
(e.g., string/array) are treated as “absent labels”, but the implementation
unconditionally calls `json.find(...)` / `GetJsonValueOrDefault(...)` on
`json`. With nlohmann::json, calling `find()` on a non-object can throw, and
even if it doesn’t, the helpers may return an error rather than defaulting. Fix
by explicitly handling `!json.is_object()` early in `LabelsFromJson` (return
empty `Labels`), or by guarding in `LoadTableResultFromJson` to only parse when
`labels` is an object (otherwise ignore), matching the documented behavior and
the `StringLabels`/`ArrayLabels` test expectations.
##########
src/iceberg/table.h:
##########
@@ -50,11 +51,12 @@ class ICEBERG_EXPORT Table : public
std::enable_shared_from_this<Table> {
/// string representation of identifier when empty.
/// \param[in] reporter Optional metrics reporter for this table. Defaults
to nullptr
/// (noop).
+ /// \param[in] labels Catalog-provided labels for this table. Defaults to
empty.
static Result<std::shared_ptr<Table>> Make(
TableIdentifier identifier, std::shared_ptr<TableMetadata> metadata,
std::string metadata_location, std::shared_ptr<FileIO> io,
std::shared_ptr<Catalog> catalog, std::string full_name = "",
- std::shared_ptr<MetricsReporter> reporter = nullptr);
+ std::shared_ptr<MetricsReporter> reporter = nullptr, Labels labels = {});
Review Comment:
`Table::Make` is exported (`ICEBERG_EXPORT`) and its mangled signature
changes by adding the `Labels` parameter, which is an ABI break for any
downstream binaries linked against the previous library symbol. If ABI
stability matters for this project/library, consider preserving the old
exported entry point by adding an overload with the prior signature (without
`Labels`) that forwards to the new one with `Labels{}`; this keeps existing
binaries working while still enabling labels for new callers.
--
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]
