Copilot commented on code in PR #960:
URL: https://github.com/apache/iceberg-cpp/pull/960#discussion_r4269400181
##########
src/iceberg/catalog/rest/rest_table.h:
##########
@@ -41,7 +41,8 @@ class ICEBERG_REST_EXPORT RestTable final : public Table {
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, RestScanContext rest_context);
+ std::shared_ptr<MetricsReporter> reporter, RestScanContext rest_context,
+ Labels labels = {});
Review Comment:
Same ABI concern as `Table::Make`: this modifies an exported factory
function signature. If ABI stability is a goal, consider adding a new overload
that takes `Labels` while preserving the old signature, forwarding from the old
to the new with an empty `Labels{}`.
##########
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:
This changes the signature (and mangled symbol) of an `ICEBERG_EXPORT`
function. Even with a default parameter, it is an ABI-breaking change for
existing binaries. If ABI compatibility matters for this library, keep the
existing `Make(..., reporter)` overload and add a new overload `Make(...,
reporter, Labels)` that delegates internally, so older binaries continue to
link.
##########
src/iceberg/catalog/rest/json_serde.cc:
##########
@@ -488,6 +493,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:
The comment says non-object `labels` should be treated as absent, but
`LabelsFromJson` doesn’t explicitly guard `json.is_object()`. Adding an early
`if (!json.is_object()) return Labels{};` will make this behavior unambiguous
and resilient to any helper/JSON-library behavior changes (and it matches the
documented tolerance semantics).
--
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]
