From 97938794eb7d6b614a18a1c8bad926e6c0a20bd1 Mon Sep 17 00:00:00 2001 From: Nihal Jain Date: Thu, 24 Sep 2026 00:20:56 +0530 Subject: [PATCH 1/3] feat: support catalog labels on tables loaded from REST catalogs Parse the optional `labels` field of the REST load-table response and expose it through Table::labels(), mirroring apache/iceberg#18045 and apache/iceberg#18046. Labels are catalog-provided rather than table state, so they live on the Table instance instead of TableMetadata. Part of #938. --- src/iceberg/catalog/rest/json_serde.cc | 66 ++++++ .../catalog/rest/json_serde_internal.h | 2 + src/iceberg/catalog/rest/rest_catalog.cc | 9 +- src/iceberg/catalog/rest/rest_catalog.h | 2 +- src/iceberg/catalog/rest/rest_table.cc | 16 +- src/iceberg/catalog/rest/rest_table.h | 6 +- src/iceberg/catalog/rest/types.cc | 2 +- src/iceberg/catalog/rest/types.h | 3 + src/iceberg/cpp20_compatibility_internal.h | 1 + src/iceberg/labels.h | 56 +++++ src/iceberg/table.cc | 26 ++- src/iceberg/table.h | 14 +- src/iceberg/test/rest_json_serde_test.cc | 209 +++++++++++++++++- src/iceberg/test/table_test.cc | 50 +++++ src/iceberg/type_fwd.h | 2 + 15 files changed, 433 insertions(+), 31 deletions(-) create mode 100644 src/iceberg/labels.h diff --git a/src/iceberg/catalog/rest/json_serde.cc b/src/iceberg/catalog/rest/json_serde.cc index c7d4662dd9..a54042c209 100644 --- a/src/iceberg/catalog/rest/json_serde.cc +++ b/src/iceberg/catalog/rest/json_serde.cc @@ -33,6 +33,7 @@ #include "iceberg/expression/json_serde_internal.h" #include "iceberg/file_format.h" #include "iceberg/json_serde_internal.h" +#include "iceberg/labels.h" #include "iceberg/manifest/manifest_entry.h" #include "iceberg/partition_spec.h" #include "iceberg/schema.h" @@ -72,6 +73,10 @@ constexpr std::string_view kDestination = "destination"; constexpr std::string_view kMetadata = "metadata"; constexpr std::string_view kConfig = "config"; constexpr std::string_view kStorageCredentials = "storage-credentials"; +constexpr std::string_view kLabels = "labels"; +constexpr std::string_view kObjectLabels = "object-labels"; +constexpr std::string_view kFields = "fields"; +constexpr std::string_view kFieldId = "field-id"; constexpr std::string_view kPrefix = "prefix"; constexpr std::string_view kIdentifiers = "identifiers"; constexpr std::string_view kOverrides = "overrides"; @@ -488,6 +493,59 @@ Result 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 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(json, kFieldId)); + ICEBERG_ASSIGN_OR_RAISE(field_label.labels, + GetJsonValue(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 LabelsFromJson(const nlohmann::json& json) { + Labels labels; + ICEBERG_ASSIGN_OR_RAISE( + labels.object_labels, + GetJsonValueOrDefault(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; +} + // LoadTableResult (used by CreateTableResponse, LoadTableResponse) Result ToJson(const LoadTableResult& result) { nlohmann::json json; @@ -502,6 +560,9 @@ Result ToJson(const LoadTableResult& result) { } json[kStorageCredentials] = std::move(creds); } + if (!result.labels.empty()) { + json[kLabels] = ToJson(result.labels); + } return json; } @@ -515,6 +576,9 @@ Result LoadTableResultFromJson(const nlohmann::json& json) { ICEBERG_ASSIGN_OR_RAISE(result.config, GetJsonValueOrDefault(json, kConfig)); ICEBERG_ASSIGN_OR_RAISE(result.storage_credentials, StorageCredentialsFromJson(json)); + if (auto it = json.find(kLabels); it != json.end() && !it->is_null()) { + ICEBERG_ASSIGN_OR_RAISE(result.labels, LabelsFromJson(*it)); + } ICEBERG_RETURN_UNEXPECTED(result.Validate()); return result; } @@ -1003,5 +1067,7 @@ ICEBERG_DEFINE_FROM_JSON(CommitTableResponse) ICEBERG_DEFINE_FROM_JSON(OAuthTokenResponse) ICEBERG_DEFINE_FROM_JSON(PlanTableScanRequest) ICEBERG_DEFINE_FROM_JSON(FetchScanTasksRequest) +ICEBERG_DEFINE_FROM_JSON(FieldLabel) +ICEBERG_DEFINE_FROM_JSON(Labels) } // namespace iceberg::rest diff --git a/src/iceberg/catalog/rest/json_serde_internal.h b/src/iceberg/catalog/rest/json_serde_internal.h index 1bba932afe..9db6f78e58 100644 --- a/src/iceberg/catalog/rest/json_serde_internal.h +++ b/src/iceberg/catalog/rest/json_serde_internal.h @@ -60,6 +60,8 @@ ICEBERG_DECLARE_JSON_SERDE(ListTablesResponse) ICEBERG_DECLARE_JSON_SERDE(RegisterTableRequest) ICEBERG_DECLARE_JSON_SERDE(RenameTableRequest) ICEBERG_DECLARE_JSON_SERDE(OAuthTokenResponse) +ICEBERG_DECLARE_JSON_SERDE(FieldLabel) +ICEBERG_DECLARE_JSON_SERDE(Labels) #undef ICEBERG_DECLARE_JSON_SERDE diff --git a/src/iceberg/catalog/rest/rest_catalog.cc b/src/iceberg/catalog/rest/rest_catalog.cc index 767b6eef69..8e49338a3d 100644 --- a/src/iceberg/catalog/rest/rest_catalog.cc +++ b/src/iceberg/catalog/rest/rest_catalog.cc @@ -997,7 +997,7 @@ Result> RestCatalog::MakeTableFromLoadResult( return MakeTable(identifier, std::move(result.metadata), std::move(result.metadata_location), std::move(table_io), std::move(table_catalog), std::move(reporter), table_config, - std::move(table_session)); + std::move(table_session), std::move(result.labels)); } Result> RestCatalog::MakeTableFromCommitResponse( @@ -1020,7 +1020,7 @@ Result> RestCatalog::MakeTable( std::string metadata_location, std::shared_ptr table_io, std::shared_ptr table_catalog, std::shared_ptr reporter, const std::unordered_map& table_config, - std::shared_ptr table_session) { + std::shared_ptr table_session, Labels labels) { // Table config overrides client config. ICEBERG_ASSIGN_OR_RAISE(auto server_mode, RestCatalogProperties::ScanPlanningModeFrom(table_config)); @@ -1068,12 +1068,13 @@ Result> RestCatalog::MakeTable( return RestTable::Make(identifier, std::move(metadata), std::move(metadata_location), std::move(table_io), std::move(table_catalog), RestTableName(name_, identifier), std::move(reporter), - std::move(rest_ctx)); + std::move(rest_ctx), std::move(labels)); } return Table::Make(identifier, std::move(metadata), std::move(metadata_location), std::move(table_io), std::move(table_catalog), - RestTableName(name_, identifier), std::move(reporter)); + RestTableName(name_, identifier), std::move(reporter), + std::move(labels)); } } // namespace iceberg::rest diff --git a/src/iceberg/catalog/rest/rest_catalog.h b/src/iceberg/catalog/rest/rest_catalog.h index 967e7cc9f6..8efeb434b3 100644 --- a/src/iceberg/catalog/rest/rest_catalog.h +++ b/src/iceberg/catalog/rest/rest_catalog.h @@ -214,7 +214,7 @@ class ICEBERG_REST_EXPORT RestCatalog final std::string metadata_location, std::shared_ptr table_io, std::shared_ptr table_catalog, std::shared_ptr reporter, const std::unordered_map& table_config, - std::shared_ptr table_session); + std::shared_ptr table_session, Labels labels = {}); RestCatalogProperties config_; std::shared_ptr file_io_; diff --git a/src/iceberg/catalog/rest/rest_table.cc b/src/iceberg/catalog/rest/rest_table.cc index dda33316fc..0f685532a9 100644 --- a/src/iceberg/catalog/rest/rest_table.cc +++ b/src/iceberg/catalog/rest/rest_table.cc @@ -33,9 +33,10 @@ RestTable::RestTable(TableIdentifier identifier, std::shared_ptr std::string metadata_location, std::shared_ptr io, std::shared_ptr catalog, std::string full_name, std::shared_ptr reporter, - RestScanContext rest_context) + RestScanContext rest_context, Labels labels) : Table(std::move(identifier), std::move(metadata), std::move(metadata_location), - std::move(io), std::move(catalog), std::move(full_name), std::move(reporter)), + std::move(io), std::move(catalog), std::move(full_name), std::move(reporter), + std::move(labels)), rest_context_(std::make_shared(std::move(rest_context))) {} RestTable::~RestTable() = default; @@ -44,14 +45,15 @@ Result> RestTable::Make( TableIdentifier identifier, std::shared_ptr metadata, std::string metadata_location, std::shared_ptr io, std::shared_ptr catalog, std::string full_name, - std::shared_ptr reporter, RestScanContext rest_context) { + std::shared_ptr reporter, RestScanContext rest_context, + Labels labels) { if (metadata == nullptr) { return InvalidArgument("Metadata cannot be null"); } - return std::shared_ptr( - new RestTable(std::move(identifier), std::move(metadata), - std::move(metadata_location), std::move(io), std::move(catalog), - std::move(full_name), std::move(reporter), std::move(rest_context))); + return std::shared_ptr(new RestTable( + std::move(identifier), std::move(metadata), std::move(metadata_location), + std::move(io), std::move(catalog), std::move(full_name), std::move(reporter), + std::move(rest_context), std::move(labels))); } Result> RestTable::NewScan() const { diff --git a/src/iceberg/catalog/rest/rest_table.h b/src/iceberg/catalog/rest/rest_table.h index fdfa7d9526..ae0accd0e7 100644 --- a/src/iceberg/catalog/rest/rest_table.h +++ b/src/iceberg/catalog/rest/rest_table.h @@ -41,7 +41,8 @@ class ICEBERG_REST_EXPORT RestTable final : public Table { TableIdentifier identifier, std::shared_ptr metadata, std::string metadata_location, std::shared_ptr io, std::shared_ptr catalog, std::string full_name, - std::shared_ptr reporter, RestScanContext rest_context); + std::shared_ptr reporter, RestScanContext rest_context, + Labels labels = {}); ~RestTable() override; @@ -57,7 +58,8 @@ class ICEBERG_REST_EXPORT RestTable final : public Table { RestTable(TableIdentifier identifier, std::shared_ptr metadata, std::string metadata_location, std::shared_ptr io, std::shared_ptr catalog, std::string full_name, - std::shared_ptr reporter, RestScanContext rest_context); + std::shared_ptr reporter, RestScanContext rest_context, + Labels labels); std::shared_ptr rest_context_; }; diff --git a/src/iceberg/catalog/rest/types.cc b/src/iceberg/catalog/rest/types.cc index 27cd7b1705..156df17eb4 100644 --- a/src/iceberg/catalog/rest/types.cc +++ b/src/iceberg/catalog/rest/types.cc @@ -87,7 +87,7 @@ bool CreateTableRequest::operator==(const CreateTableRequest& other) const { bool LoadTableResult::operator==(const LoadTableResult& other) const { if (metadata_location != other.metadata_location || config != other.config || - storage_credentials != other.storage_credentials) { + storage_credentials != other.storage_credentials || labels != other.labels) { return false; } diff --git a/src/iceberg/catalog/rest/types.h b/src/iceberg/catalog/rest/types.h index f048d6e7d6..f6f8090223 100644 --- a/src/iceberg/catalog/rest/types.h +++ b/src/iceberg/catalog/rest/types.h @@ -29,6 +29,7 @@ #include "iceberg/catalog/rest/endpoint.h" #include "iceberg/catalog/rest/iceberg_rest_export.h" +#include "iceberg/labels.h" #include "iceberg/result.h" #include "iceberg/storage_credential.h" #include "iceberg/table_identifier.h" @@ -188,6 +189,8 @@ struct ICEBERG_REST_EXPORT LoadTableResult { std::unordered_map config; /// \brief Vended storage credentials, one per URI prefix; empty if none. std::vector storage_credentials; + /// \brief Catalog-provided labels; empty if none. + Labels labels; /// \brief Validates the LoadTableResult. Status Validate() const { diff --git a/src/iceberg/cpp20_compatibility_internal.h b/src/iceberg/cpp20_compatibility_internal.h index 3f9b5b5773..1b719f1b24 100644 --- a/src/iceberg/cpp20_compatibility_internal.h +++ b/src/iceberg/cpp20_compatibility_internal.h @@ -81,6 +81,7 @@ #include "iceberg/inspect/history_table.h" #include "iceberg/inspect/metadata_table.h" #include "iceberg/inspect/snapshots_table.h" +#include "iceberg/labels.h" #include "iceberg/location_provider.h" #include "iceberg/logging/cerr_logger.h" #include "iceberg/logging/log_level.h" diff --git a/src/iceberg/labels.h b/src/iceberg/labels.h new file mode 100644 index 0000000000..f97975b387 --- /dev/null +++ b/src/iceberg/labels.h @@ -0,0 +1,56 @@ +/* + * 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. + */ + +#pragma once + +/// \file iceberg/labels.h +/// \brief Catalog-provided labels for tables and their fields. + +#include +#include +#include +#include + +#include "iceberg/iceberg_export.h" + +namespace iceberg { + +/// \brief Catalog-provided labels for a single field, identified by its field ID. +struct ICEBERG_EXPORT FieldLabel { + int32_t field_id = 0; + std::unordered_map labels; + + bool operator==(const FieldLabel&) const = default; +}; + +/// \brief Optional catalog-provided labels returned when a table is loaded. +/// +/// Labels are advisory enrichment, not table state: they are not persisted with the +/// table and may be absent. +struct ICEBERG_EXPORT Labels { + std::unordered_map object_labels; + std::vector fields; + + /// \brief Returns true when there are neither object-level nor field-level labels. + bool empty() const { return object_labels.empty() && fields.empty(); } + + bool operator==(const Labels&) const = default; +}; + +} // namespace iceberg diff --git a/src/iceberg/table.cc b/src/iceberg/table.cc index ca02c62318..67791b801e 100644 --- a/src/iceberg/table.cc +++ b/src/iceberg/table.cc @@ -56,13 +56,11 @@ namespace iceberg { -Result> Table::Make(TableIdentifier identifier, - std::shared_ptr metadata, - std::string metadata_location, - std::shared_ptr io, - std::shared_ptr catalog, - std::string full_name, - std::shared_ptr reporter) { +Result> Table::Make( + TableIdentifier identifier, std::shared_ptr metadata, + std::string metadata_location, std::shared_ptr io, + std::shared_ptr catalog, std::string full_name, + std::shared_ptr reporter, Labels labels) { if (metadata == nullptr) [[unlikely]] { return InvalidArgument("Metadata cannot be null"); } @@ -75,9 +73,10 @@ Result> Table::Make(TableIdentifier identifier, if (catalog == nullptr) [[unlikely]] { return InvalidArgument("Catalog cannot be null"); } - return std::shared_ptr(new Table( - std::move(identifier), std::move(metadata), std::move(metadata_location), - std::move(io), std::move(catalog), std::move(full_name), std::move(reporter))); + return std::shared_ptr
(new Table(std::move(identifier), std::move(metadata), + std::move(metadata_location), std::move(io), + std::move(catalog), std::move(full_name), + std::move(reporter), std::move(labels))); } Table::~Table() = default; @@ -85,7 +84,7 @@ Table::~Table() = default; Table::Table(TableIdentifier identifier, std::shared_ptr metadata, std::string metadata_location, std::shared_ptr io, std::shared_ptr catalog, std::string full_name, - std::shared_ptr reporter) + std::shared_ptr reporter, Labels labels) : identifier_(std::move(identifier)), full_name_(full_name.empty() ? identifier_.ToString() : std::move(full_name)), metadata_(std::move(metadata)), @@ -93,7 +92,8 @@ Table::Table(TableIdentifier identifier, std::shared_ptr metadata io_(std::move(io)), catalog_(std::move(catalog)), reporter_(std::move(reporter)), - metadata_cache_(std::make_unique(metadata_.get())) {} + metadata_cache_(std::make_unique(metadata_.get())), + labels_(std::move(labels)) {} const std::string& Table::uuid() const { return metadata_->table_uuid; } @@ -167,6 +167,8 @@ const std::shared_ptr& Table::catalog() const { return catalog_; } const std::shared_ptr& Table::reporter() const { return reporter_; } +const Labels& Table::labels() const { return labels_; } + Result> Table::location_provider() const { return LocationProvider::Make(metadata_->location, metadata_->properties); } diff --git a/src/iceberg/table.h b/src/iceberg/table.h index 4c0470cb9c..b34ed80ad9 100644 --- a/src/iceberg/table.h +++ b/src/iceberg/table.h @@ -29,6 +29,7 @@ #include #include "iceberg/iceberg_export.h" +#include "iceberg/labels.h" #include "iceberg/snapshot.h" #include "iceberg/table_identifier.h" #include "iceberg/type_fwd.h" @@ -50,11 +51,12 @@ class ICEBERG_EXPORT Table : public std::enable_shared_from_this
{ /// 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> Make( TableIdentifier identifier, std::shared_ptr metadata, std::string metadata_location, std::shared_ptr io, std::shared_ptr catalog, std::string full_name = "", - std::shared_ptr reporter = nullptr); + std::shared_ptr reporter = nullptr, Labels labels = {}); virtual ~Table(); @@ -130,6 +132,13 @@ class ICEBERG_EXPORT Table : public std::enable_shared_from_this
{ /// \brief Returns the metrics reporter for this table. const std::shared_ptr& reporter() const; + /// \brief Returns the catalog-provided labels for this table. + /// + /// Labels come only from the catalog's load, create or register response and are fixed + /// for the life of this table: Refresh() does not update them, and a table returned by + /// Transaction::Commit() has none. Load the table again to get current labels. + const Labels& labels() const; + /// \brief Returns a LocationProvider for this table Result> location_provider() const; @@ -216,7 +225,7 @@ class ICEBERG_EXPORT Table : public std::enable_shared_from_this
{ Table(TableIdentifier identifier, std::shared_ptr metadata, std::string metadata_location, std::shared_ptr io, std::shared_ptr catalog, std::string full_name, - std::shared_ptr reporter = nullptr); + std::shared_ptr reporter = nullptr, Labels labels = {}); const TableIdentifier identifier_; const std::string full_name_; @@ -226,6 +235,7 @@ class ICEBERG_EXPORT Table : public std::enable_shared_from_this
{ std::shared_ptr catalog_; std::shared_ptr reporter_; std::unique_ptr metadata_cache_; + const Labels labels_; }; /// \brief A table created by stage-create and not yet committed. diff --git a/src/iceberg/test/rest_json_serde_test.cc b/src/iceberg/test/rest_json_serde_test.cc index 34ca4a45b3..db19860a5f 100644 --- a/src/iceberg/test/rest_json_serde_test.cc +++ b/src/iceberg/test/rest_json_serde_test.cc @@ -33,6 +33,7 @@ #include "iceberg/expression/expressions.h" #include "iceberg/file_format.h" #include "iceberg/json_serde_internal.h" +#include "iceberg/labels.h" #include "iceberg/manifest/manifest_entry.h" #include "iceberg/partition_spec.h" #include "iceberg/result.h" @@ -85,6 +86,11 @@ static std::shared_ptr MakeSimpleTableMetadata() { }); } +std::string LoadTableJsonWithLabels(std::string_view labels) { + return std::string(R"({"labels":)") + std::string(labels) + + R"(,"metadata":{"format-version":2,"table-uuid":"test-uuid-1234","location":"s3://bucket/test","last-sequence-number":0,"last-updated-ms":0,"last-column-id":1,"schemas":[{"type":"struct","schema-id":1,"fields":[{"id":1,"name":"id","type":"int","required":true}]}],"current-schema-id":1,"partition-specs":[{"spec-id":0,"fields":[]}],"default-spec-id":0,"last-partition-id":0,"sort-orders":[{"order-id":0,"fields":[]}],"default-sort-order-id":0,"properties":{}}})"; +} + std::string LoadTableJsonWithCredentials(std::string_view storage_credentials) { return std::string(R"({"storage-credentials":)") + std::string(storage_credentials) + R"(,"metadata":{"format-version":2,"table-uuid":"test","location":"s3://test","last-sequence-number":0,"last-column-id":1,"last-updated-ms":0,"schemas":[{"type":"struct","schema-id":1,"fields":[{"id":1,"name":"id","type":"int","required":true}]}],"current-schema-id":1,"partition-specs":[{"spec-id":0,"fields":[]}],"default-spec-id":0,"last-partition-id":0,"sort-orders":[{"order-id":0,"fields":[]}],"default-sort-order-id":0}})"; @@ -1136,7 +1142,15 @@ INSTANTIATE_TEST_SUITE_P( .config = {{"s3.access-key-id", "AKIAtest"}, {"s3.secret-access-key", "secret"}, {"s3.session-token", "token"}, - {"client.region", "us-east-1"}}}}}}), + {"client.region", "us-east-1"}}}}}}, + LoadTableResultParam{ + .test_name = "WithLabels", + .expected_json_str = + R"({"labels":{"fields":[{"field-id":1,"labels":{"classification":"pii"}}],"object-labels":{"owner":"team-a"}},"metadata":{"current-schema-id":1,"current-snapshot-id":null,"default-sort-order-id":0,"default-spec-id":0,"format-version":2,"last-column-id":1,"last-partition-id":0,"last-sequence-number":0,"last-updated-ms":0,"location":"s3://bucket/test","metadata-log":[],"partition-specs":[{"fields":[],"spec-id":0}],"partition-statistics":[],"properties":{},"refs":{},"schemas":[{"fields":[{"id":1,"name":"id","required":true,"type":"int"}],"schema-id":1,"type":"struct"}],"snapshot-log":[],"snapshots":[],"sort-orders":[{"fields":[],"order-id":0}],"statistics":[],"table-uuid":"test-uuid-1234"}})", + .model = {.metadata = MakeSimpleTableMetadata(), + .labels = {.object_labels = {{"owner", "team-a"}}, + .fields = {{.field_id = 1, + .labels = {{"classification", "pii"}}}}}}}), [](const ::testing::TestParamInfo& info) { return info.param.test_name; }); @@ -1165,7 +1179,33 @@ INSTANTIATE_TEST_SUITE_P( .json_str = R"({"metadata":{"format-version":2,"table-uuid":"test-uuid-1234","location":"s3://bucket/test","last-sequence-number":0,"last-updated-ms":0,"last-column-id":1,"schemas":[{"type":"struct","schema-id":1,"fields":[{"id":1,"name":"id","type":"int","required":true}]}],"current-schema-id":1,"partition-specs":[{"spec-id":0,"fields":[]}],"default-spec-id":0,"last-partition-id":0,"sort-orders":[{"order-id":0,"fields":[]}],"default-sort-order-id":0,"properties":{}},"config":{"warehouse":"s3://bucket/warehouse"}})", .expected_model = {.metadata = MakeSimpleTableMetadata(), - .config = {{"warehouse", "s3://bucket/warehouse"}}}}), + .config = {{"warehouse", "s3://bucket/warehouse"}}}}, + LoadTableResultDeserializeParam{ + .test_name = "WithLabels", + .json_str = LoadTableJsonWithLabels( + R"({"object-labels":{"owner":"team-a"},"fields":[{"field-id":1,"labels":{"classification":"pii"}}]})"), + .expected_model = {.metadata = MakeSimpleTableMetadata(), + .labels = {.object_labels = {{"owner", "team-a"}}, + .fields = {{.field_id = 1, + .labels = {{"classification", + "pii"}}}}}}}, + LoadTableResultDeserializeParam{ + .test_name = "NullLabels", + .json_str = LoadTableJsonWithLabels("null"), + .expected_model = {.metadata = MakeSimpleTableMetadata()}}, + LoadTableResultDeserializeParam{ + .test_name = "EmptyLabels", + .json_str = LoadTableJsonWithLabels("{}"), + .expected_model = {.metadata = MakeSimpleTableMetadata()}}, + // Non-object labels are ignored rather than rejected + LoadTableResultDeserializeParam{ + .test_name = "StringLabels", + .json_str = LoadTableJsonWithLabels(R"("oops")"), + .expected_model = {.metadata = MakeSimpleTableMetadata()}}, + LoadTableResultDeserializeParam{ + .test_name = "ArrayLabels", + .json_str = LoadTableJsonWithLabels(R"([{"object-labels":{"k":"v"}}])"), + .expected_model = {.metadata = MakeSimpleTableMetadata()}}), [](const ::testing::TestParamInfo& info) { return info.param.test_name; }); @@ -1205,6 +1245,10 @@ INSTANTIATE_TEST_SUITE_P( .test_name = "InvalidMetadataContent", .invalid_json_str = R"({"metadata":{"format-version":"invalid"}})", .expected_error_message = "type must be number, but is string"}, + LoadTableResultInvalidParam{ + .test_name = "LabelsFieldsNotArray", + .invalid_json_str = LoadTableJsonWithLabels(R"({"fields":"oops"})"), + .expected_error_message = "Cannot parse 'fields' from non-array"}, LoadTableResultInvalidParam{ .test_name = "CredentialsNotArray", .invalid_json_str = LoadTableJsonWithCredentials(R"("oops")"), @@ -2580,4 +2624,165 @@ TEST(FetchPlanningResultResponseRoundtripTest, FailedWithError) { EXPECT_EQ(*result, *result2); } +DECLARE_ROUNDTRIP_TEST(FieldLabel) + +INSTANTIATE_TEST_SUITE_P( + FieldLabelCases, FieldLabelTest, + ::testing::Values( + FieldLabelParam{ + .test_name = "WithLabels", + .expected_json_str = + R"({"field-id":1,"labels":{"classification":"pii","owner":"team-a"}})", + .model = {.field_id = 1, + .labels = {{"classification", "pii"}, {"owner", "team-a"}}}}, + // A field label always writes "labels", even when empty + FieldLabelParam{.test_name = "EmptyLabels", + .expected_json_str = R"({"field-id":1,"labels":{}})", + .model = {.field_id = 1}}, + FieldLabelParam{.test_name = "MaxInt32FieldId", + .expected_json_str = R"({"field-id":2147483647,"labels":{}})", + .model = {.field_id = 2147483647}}, + FieldLabelParam{.test_name = "NegativeFieldId", + .expected_json_str = R"({"field-id":-1,"labels":{"k":"v"}})", + .model = {.field_id = -1, .labels = {{"k", "v"}}}}), + [](const ::testing::TestParamInfo& info) { + return info.param.test_name; + }); + +DECLARE_INVALID_TEST(FieldLabel) + +INSTANTIATE_TEST_SUITE_P( + FieldLabelInvalidCases, FieldLabelInvalidTest, + ::testing::Values( + FieldLabelInvalidParam{ + .test_name = "NotObject", + .invalid_json_str = R"([])", + .expected_error_message = "Cannot parse field label from non-object"}, + FieldLabelInvalidParam{.test_name = "MissingFieldId", + .invalid_json_str = R"({"labels":{}})", + .expected_error_message = "Missing 'field-id'"}, + FieldLabelInvalidParam{.test_name = "NullFieldId", + .invalid_json_str = R"({"field-id":null,"labels":{}})", + .expected_error_message = "Missing 'field-id'"}, + FieldLabelInvalidParam{.test_name = "FloatFieldId", + .invalid_json_str = R"({"field-id":1.5,"labels":{}})", + .expected_error_message = "'field-id' must be an integer"}, + FieldLabelInvalidParam{.test_name = "BoolFieldId", + .invalid_json_str = R"({"field-id":true,"labels":{}})", + .expected_error_message = "'field-id' must be an integer"}, + FieldLabelInvalidParam{.test_name = "StringFieldId", + .invalid_json_str = R"({"field-id":"1","labels":{}})", + .expected_error_message = "'field-id' must be an integer"}, + FieldLabelInvalidParam{ + .test_name = "FieldIdAboveInt32", + .invalid_json_str = R"({"field-id":2147483648,"labels":{}})", + .expected_error_message = "'field-id' integer is out of range"}, + FieldLabelInvalidParam{ + .test_name = "FieldIdBelowInt32", + .invalid_json_str = R"({"field-id":-2147483649,"labels":{}})", + .expected_error_message = "'field-id' integer is out of range"}, + FieldLabelInvalidParam{.test_name = "MissingLabels", + .invalid_json_str = R"({"field-id":1})", + .expected_error_message = "Missing 'labels'"}, + FieldLabelInvalidParam{.test_name = "NullLabels", + .invalid_json_str = R"({"field-id":1,"labels":null})", + .expected_error_message = "Missing 'labels'"}, + FieldLabelInvalidParam{.test_name = "LabelsNotObject", + .invalid_json_str = R"({"field-id":1,"labels":"x"})", + .expected_error_message = "type must be object"}, + FieldLabelInvalidParam{.test_name = "NonStringLabelValue", + .invalid_json_str = R"({"field-id":1,"labels":{"k":1}})", + .expected_error_message = "type must be string"}), + [](const ::testing::TestParamInfo& info) { + return info.param.test_name; + }); + +DECLARE_ROUNDTRIP_TEST(Labels) + +INSTANTIATE_TEST_SUITE_P( + LabelsCases, LabelsTest, + ::testing::Values( + LabelsParam{.test_name = "Empty", .expected_json_str = R"({})", .model = {}}, + LabelsParam{.test_name = "ObjectLabelsOnly", + .expected_json_str = R"({"object-labels":{"owner":"team-a"}})", + .model = {.object_labels = {{"owner", "team-a"}}}}, + LabelsParam{ + .test_name = "FieldsOnly", + .expected_json_str = + R"({"fields":[{"field-id":1,"labels":{"classification":"pii"}},{"field-id":2,"labels":{"unit":"usd"}}]})", + .model = {.fields = {{.field_id = 1, .labels = {{"classification", "pii"}}}, + {.field_id = 2, .labels = {{"unit", "usd"}}}}}}, + LabelsParam{ + .test_name = "ObjectAndFields", + .expected_json_str = + R"({"object-labels":{"owner":"team-a"},"fields":[{"field-id":1,"labels":{"classification":"pii"}}]})", + .model = {.object_labels = {{"owner", "team-a"}}, + .fields = {{.field_id = 1, + .labels = {{"classification", "pii"}}}}}}, + // Duplicate field IDs are kept verbatim and in order; no resolution + LabelsParam{ + .test_name = "DuplicateFieldIds", + .expected_json_str = + R"({"fields":[{"field-id":1,"labels":{"a":"1"}},{"field-id":1,"labels":{"b":"2"}}]})", + .model = {.fields = {{.field_id = 1, .labels = {{"a", "1"}}}, + {.field_id = 1, .labels = {{"b", "2"}}}}}}), + [](const ::testing::TestParamInfo& info) { + return info.param.test_name; + }); + +DECLARE_DESERIALIZE_TEST(Labels) + +INSTANTIATE_TEST_SUITE_P( + LabelsDeserializeCases, LabelsDeserializeTest, + ::testing::Values( + LabelsDeserializeParam{.test_name = "NullObjectLabels", + .json_str = R"({"object-labels":null})", + .expected_model = {}}, + LabelsDeserializeParam{.test_name = "NullFields", + .json_str = R"({"fields":null})", + .expected_model = {}}, + LabelsDeserializeParam{ + .test_name = "UnknownKeysIgnored", + .json_str = + R"({"object-labels":{"a":"b"},"future":1,"fields":[{"field-id":2,"labels":{},"extra":true}]})", + .expected_model = {.object_labels = {{"a", "b"}}, + .fields = {{.field_id = 2}}}}), + [](const ::testing::TestParamInfo& info) { + return info.param.test_name; + }); + +DECLARE_INVALID_TEST(Labels) + +INSTANTIATE_TEST_SUITE_P( + LabelsInvalidCases, LabelsInvalidTest, + ::testing::Values( + LabelsInvalidParam{.test_name = "ObjectLabelsNotObject", + .invalid_json_str = R"({"object-labels":"x"})", + .expected_error_message = "type must be object"}, + LabelsInvalidParam{.test_name = "ObjectLabelsNonStringValue", + .invalid_json_str = R"({"object-labels":{"k":1}})", + .expected_error_message = "type must be string"}, + LabelsInvalidParam{.test_name = "ObjectLabelsNullValue", + .invalid_json_str = R"({"object-labels":{"k":null}})", + .expected_error_message = "type must be string"}, + LabelsInvalidParam{ + .test_name = "FieldsNotArray", + .invalid_json_str = R"({"fields":{}})", + .expected_error_message = "Cannot parse 'fields' from non-array"}, + LabelsInvalidParam{ + .test_name = "FieldEntryNotObject", + .invalid_json_str = R"({"fields":[1]})", + .expected_error_message = "Cannot parse field label from non-object"}, + LabelsInvalidParam{ + .test_name = "FieldEntryNull", + .invalid_json_str = R"({"fields":[null]})", + .expected_error_message = "Cannot parse field label from non-object"}, + LabelsInvalidParam{ + .test_name = "FieldEntryInvalidFieldId", + .invalid_json_str = R"({"fields":[{"field-id":1.5,"labels":{}}]})", + .expected_error_message = "'field-id' must be an integer"}), + [](const ::testing::TestParamInfo& info) { + return info.param.test_name; + }); + } // namespace iceberg::rest diff --git a/src/iceberg/test/table_test.cc b/src/iceberg/test/table_test.cc index a85dc30147..6121fc55b7 100644 --- a/src/iceberg/test/table_test.cc +++ b/src/iceberg/test/table_test.cc @@ -22,6 +22,7 @@ #include #include +#include "iceberg/labels.h" #include "iceberg/schema.h" #include "iceberg/schema_field.h" #include "iceberg/table_metadata.h" @@ -148,6 +149,55 @@ TYPED_TEST(TypedTableTest, NewTransaction) { } } +TYPED_TEST(TypedTableTest, LabelsDefaultToEmpty) { + ICEBERG_UNWRAP_OR_FAIL(auto table, this->MakeTable("test_table")); + + EXPECT_TRUE(table->labels().empty()); +} + +class TableLabelsTest : public ::testing::Test { + protected: + void SetUp() override { + io_ = std::make_shared(); + catalog_ = std::make_shared(); + auto schema = std::make_shared( + std::vector{SchemaField::MakeRequired(1, "id", int64())}, 1); + metadata_ = std::make_shared( + TableMetadata{.format_version = 2, .schemas = {schema}, .current_schema_id = 1}); + } + + const TableIdentifier ident_{.ns = Namespace{.levels = {"db"}}, .name = "t"}; + const Labels labels_{ + .object_labels = {{"owner", "team-a"}}, + .fields = {{.field_id = 1, .labels = {{"classification", "pii"}}}}}; + std::shared_ptr io_; + std::shared_ptr catalog_; + std::shared_ptr metadata_; +}; + +TEST_F(TableLabelsTest, ExposesProvidedLabels) { + ICEBERG_UNWRAP_OR_FAIL( + auto table, Table::Make(ident_, metadata_, "s3://bucket/meta.json", io_, catalog_, + /*full_name=*/"", /*reporter=*/nullptr, labels_)); + + EXPECT_EQ(table->labels(), labels_); +} + +TEST_F(TableLabelsTest, RefreshKeepsLabels) { + ICEBERG_UNWRAP_OR_FAIL( + auto table, Table::Make(ident_, metadata_, "s3://bucket/meta.json", io_, catalog_, + /*full_name=*/"", /*reporter=*/nullptr, labels_)); + ICEBERG_UNWRAP_OR_FAIL( + auto refreshed, + Table::Make(ident_, metadata_, "s3://bucket/meta2.json", io_, catalog_)); + EXPECT_CALL(*catalog_, LoadTable(::testing::_)).WillOnce(::testing::Return(refreshed)); + + ASSERT_THAT(table->Refresh(), IsOk()); + + EXPECT_EQ(table->metadata_file_location(), "s3://bucket/meta2.json"); + EXPECT_EQ(table->labels(), labels_); +} + TEST(StaticTableTest, NewMutatingOperationsAreNotSupported) { auto io = std::make_shared(); auto schema = std::make_shared( diff --git a/src/iceberg/type_fwd.h b/src/iceberg/type_fwd.h index 511b14eb05..477774a173 100644 --- a/src/iceberg/type_fwd.h +++ b/src/iceberg/type_fwd.h @@ -241,6 +241,8 @@ class CommitMetrics; /// \brief Table. class Table; class TableProperties; +struct FieldLabel; +struct Labels; /// \brief Table update. class TableMetadataBuilder; From b85641315fbf35adc8b782a791f591ce538d50cc Mon Sep 17 00:00:00 2001 From: Gang Wu Date: Wed, 14 Oct 2026 17:45:44 +0800 Subject: [PATCH 2/3] refactor: allocate table labels only when present Clarify label lifecycle and cover refresh, empty transactions, and REST load/create/register in both planning modes. --- src/iceberg/table.cc | 11 +++- src/iceberg/table.h | 7 +-- src/iceberg/test/rest_catalog_unit_test.cc | 72 ++++++++++++++++++++++ src/iceberg/test/table_test.cc | 72 ++++++++++++++++++---- 4 files changed, 143 insertions(+), 19 deletions(-) diff --git a/src/iceberg/table.cc b/src/iceberg/table.cc index 67791b801e..3b801af1f4 100644 --- a/src/iceberg/table.cc +++ b/src/iceberg/table.cc @@ -93,7 +93,8 @@ Table::Table(TableIdentifier identifier, std::shared_ptr metadata catalog_(std::move(catalog)), reporter_(std::move(reporter)), metadata_cache_(std::make_unique(metadata_.get())), - labels_(std::move(labels)) {} + labels_(labels.empty() ? nullptr + : std::make_unique(std::move(labels))) {} const std::string& Table::uuid() const { return metadata_->table_uuid; } @@ -167,7 +168,13 @@ const std::shared_ptr& Table::catalog() const { return catalog_; } const std::shared_ptr& Table::reporter() const { return reporter_; } -const Labels& Table::labels() const { return labels_; } +const Labels& Table::labels() const { + if (labels_) { + return *labels_; + } + static const Labels kEmpty; + return kEmpty; +} Result> Table::location_provider() const { return LocationProvider::Make(metadata_->location, metadata_->properties); diff --git a/src/iceberg/table.h b/src/iceberg/table.h index b34ed80ad9..f53cdc3fb1 100644 --- a/src/iceberg/table.h +++ b/src/iceberg/table.h @@ -134,9 +134,8 @@ class ICEBERG_EXPORT Table : public std::enable_shared_from_this
{ /// \brief Returns the catalog-provided labels for this table. /// - /// Labels come only from the catalog's load, create or register response and are fixed - /// for the life of this table: Refresh() does not update them, and a table returned by - /// Transaction::Commit() has none. Load the table again to get current labels. + /// Labels are supplied at construction and are not updated by Refresh(). + /// Reload the table to obtain current catalog labels. const Labels& labels() const; /// \brief Returns a LocationProvider for this table @@ -235,7 +234,7 @@ class ICEBERG_EXPORT Table : public std::enable_shared_from_this
{ std::shared_ptr catalog_; std::shared_ptr reporter_; std::unique_ptr metadata_cache_; - const Labels labels_; + const std::unique_ptr labels_; }; /// \brief A table created by stage-create and not yet committed. diff --git a/src/iceberg/test/rest_catalog_unit_test.cc b/src/iceberg/test/rest_catalog_unit_test.cc index bb76a59a4f..97c9c36504 100644 --- a/src/iceberg/test/rest_catalog_unit_test.cc +++ b/src/iceberg/test/rest_catalog_unit_test.cc @@ -39,6 +39,8 @@ #include "iceberg/catalog/rest/types.h" #include "iceberg/file_io.h" #include "iceberg/file_io_registry.h" +#include "iceberg/labels.h" +#include "iceberg/schema.h" #include "iceberg/storage_credential.h" #include "iceberg/table.h" #include "iceberg/table_identifier.h" @@ -214,6 +216,76 @@ class RestCatalogLoadTableTest : public ::testing::Test { std::unordered_set no_plan_endpoint_set_; }; +class RestCatalogLabelsTest : public RestCatalogLoadTableTest, + public ::testing::WithParamInterface { + protected: + std::string LabelsResponse() const { + auto response = nlohmann::json::parse(kLoadTableDefaultResponse); + response["config"]["scan-planning-mode"] = GetParam() ? "server" : "client"; + response["labels"] = nlohmann::json::parse( + R"({"object-labels":{"owner":"team-a"},"fields":[{"field-id":1,"labels":{"classification":"pii"}}]})"); + return response.dump(); + } + + Result> MakeLabelsCatalog() { + auto endpoints = all_plan_endpoints_; + endpoints.insert(Endpoint::CreateTable()); + endpoints.insert(Endpoint::RegisterTable()); + ICEBERG_ASSIGN_OR_RAISE(auto catalog, MakeCatalog(endpoints)); + return catalog->AsCatalog(); + } + + void CheckLabels(const std::shared_ptr
& table) const { + ASSERT_NE(table, nullptr); + EXPECT_EQ(table->labels(), labels_); + EXPECT_EQ(std::dynamic_pointer_cast(table) != nullptr, GetParam()); + } + + const Labels labels_{ + .object_labels = {{"owner", "team-a"}}, + .fields = {{.field_id = 1, .labels = {{"classification", "pii"}}}}}; +}; + +TEST_P(RestCatalogLabelsTest, LoadExposesLabels) { + EXPECT_CALL(*mock_client_, Get(::testing::EndsWith("/tables/my_table"), _, _, _, _)) + .WillOnce(Return(HttpResponse(200, LabelsResponse()))); + ICEBERG_UNWRAP_OR_FAIL(auto catalog, MakeLabelsCatalog()); + ICEBERG_UNWRAP_OR_FAIL(auto table, catalog->LoadTable(identifier_)); + + CheckLabels(table); +} + +TEST_P(RestCatalogLabelsTest, CreateExposesLabels) { + EXPECT_CALL(*mock_client_, + Post(::testing::EndsWith("/namespaces/default/tables"), _, _, _, _)) + .WillOnce(Return(HttpResponse(200, LabelsResponse()))); + ICEBERG_UNWRAP_OR_FAIL(auto catalog, MakeLabelsCatalog()); + auto schema = std::make_shared( + std::vector{SchemaField::MakeRequired(1, "id", int32())}, 1); + ICEBERG_UNWRAP_OR_FAIL(auto table, + catalog->CreateTable(identifier_, schema, /*spec=*/nullptr, + /*order=*/nullptr, /*location=*/"", + /*properties=*/{})); + + CheckLabels(table); +} + +TEST_P(RestCatalogLabelsTest, RegisterExposesLabels) { + EXPECT_CALL(*mock_client_, + Post(::testing::EndsWith("/namespaces/default/register"), _, _, _, _)) + .WillOnce(Return(HttpResponse(200, LabelsResponse()))); + ICEBERG_UNWRAP_OR_FAIL(auto catalog, MakeLabelsCatalog()); + ICEBERG_UNWRAP_OR_FAIL( + auto table, catalog->RegisterTable(identifier_, "s3://bucket/metadata/v1.json")); + + CheckLabels(table); +} + +INSTANTIATE_TEST_SUITE_P(PlanningModes, RestCatalogLabelsTest, ::testing::Bool(), + [](const ::testing::TestParamInfo& info) { + return info.param ? "Server" : "Client"; + }); + TEST_F(RestCatalogLoadTableTest, InitializationPropagatesClientFactoryErrors) { auto config = RestCatalogProperties::FromMap( {{"uri", "http://test-server"}, {"io-impl", "rest-catalog-unit-test-noop"}}); diff --git a/src/iceberg/test/table_test.cc b/src/iceberg/test/table_test.cc index 6121fc55b7..c269240875 100644 --- a/src/iceberg/test/table_test.cc +++ b/src/iceberg/test/table_test.cc @@ -29,6 +29,7 @@ #include "iceberg/test/matchers.h" #include "iceberg/test/mock_catalog.h" #include "iceberg/test/mock_io.h" +#include "iceberg/transaction.h" namespace iceberg { @@ -175,27 +176,72 @@ class TableLabelsTest : public ::testing::Test { std::shared_ptr metadata_; }; -TEST_F(TableLabelsTest, ExposesProvidedLabels) { +TEST_F(TableLabelsTest, EmptyLabelsShareFallback) { ICEBERG_UNWRAP_OR_FAIL( - auto table, Table::Make(ident_, metadata_, "s3://bucket/meta.json", io_, catalog_, - /*full_name=*/"", /*reporter=*/nullptr, labels_)); + auto first, Table::Make(ident_, metadata_, "s3://bucket/meta.json", io_, catalog_)); + ICEBERG_UNWRAP_OR_FAIL( + auto second, Table::Make(ident_, metadata_, "s3://bucket/meta.json", io_, catalog_, + /*full_name=*/"", /*reporter=*/nullptr, Labels{})); - EXPECT_EQ(table->labels(), labels_); + EXPECT_TRUE(first->labels().empty()); + EXPECT_TRUE(second->labels().empty()); + EXPECT_EQ(&first->labels(), &second->labels()); } -TEST_F(TableLabelsTest, RefreshKeepsLabels) { +TEST_F(TableLabelsTest, ExposesProvidedLabels) { + for (const auto& labels : {labels_, Labels{.object_labels = {{"owner", "team-a"}}}, + Labels{.fields = {{.field_id = 1}}}}) { + ICEBERG_UNWRAP_OR_FAIL( + auto table, Table::Make(ident_, metadata_, "s3://bucket/meta.json", io_, catalog_, + /*full_name=*/"", /*reporter=*/nullptr, labels)); + + EXPECT_EQ(table->labels(), labels); + } +} + +class TableLabelsRefreshTest : public TableLabelsTest, + public ::testing::WithParamInterface {}; + +TEST_P(TableLabelsRefreshTest, RefreshKeepsLabels) { + const std::string metadata_location = + GetParam() ? "s3://bucket/meta2.json" : "s3://bucket/meta.json"; + const Labels changed_labels{ + .object_labels = {{"owner", "team-b"}}, + .fields = {{.field_id = 1, .labels = {{"classification", "public"}}}}}; + for (const auto& refreshed_labels : {Labels{}, changed_labels}) { + SCOPED_TRACE(refreshed_labels.empty() ? "Absent labels" : "Changed labels"); + ICEBERG_UNWRAP_OR_FAIL( + auto table, Table::Make(ident_, metadata_, "s3://bucket/meta.json", io_, catalog_, + /*full_name=*/"", /*reporter=*/nullptr, labels_)); + ICEBERG_UNWRAP_OR_FAIL( + auto refreshed, + Table::Make(ident_, metadata_, metadata_location, io_, catalog_, + /*full_name=*/"", /*reporter=*/nullptr, refreshed_labels)); + EXPECT_CALL(*catalog_, LoadTable(::testing::_)) + .WillOnce(::testing::Return(refreshed)); + + ASSERT_THAT(table->Refresh(), IsOk()); + + EXPECT_EQ(table->metadata_file_location(), metadata_location); + EXPECT_EQ(table->labels(), labels_); + } +} + +INSTANTIATE_TEST_SUITE_P(MetadataLocation, TableLabelsRefreshTest, ::testing::Bool(), + [](const ::testing::TestParamInfo& info) { + return info.param ? "Changed" : "Unchanged"; + }); + +TEST_F(TableLabelsTest, EmptyTransactionKeepsLabels) { ICEBERG_UNWRAP_OR_FAIL( auto table, Table::Make(ident_, metadata_, "s3://bucket/meta.json", io_, catalog_, /*full_name=*/"", /*reporter=*/nullptr, labels_)); - ICEBERG_UNWRAP_OR_FAIL( - auto refreshed, - Table::Make(ident_, metadata_, "s3://bucket/meta2.json", io_, catalog_)); - EXPECT_CALL(*catalog_, LoadTable(::testing::_)).WillOnce(::testing::Return(refreshed)); - - ASSERT_THAT(table->Refresh(), IsOk()); + EXPECT_CALL(*catalog_, UpdateTable(::testing::_, ::testing::_, ::testing::_)).Times(0); + ICEBERG_UNWRAP_OR_FAIL(auto transaction, table->NewTransaction()); + ICEBERG_UNWRAP_OR_FAIL(auto committed, transaction->Commit()); - EXPECT_EQ(table->metadata_file_location(), "s3://bucket/meta2.json"); - EXPECT_EQ(table->labels(), labels_); + ASSERT_EQ(committed, table); + EXPECT_EQ(committed->labels(), labels_); } TEST(StaticTableTest, NewMutatingOperationsAreNotSupported) { From 4ef2834f556671a9d8a4dbaa5937b79d30d53015 Mon Sep 17 00:00:00 2001 From: Gang Wu Date: Wed, 14 Oct 2026 20:29:16 +0800 Subject: [PATCH 3/3] refactor: pass table labels through unique ownership Keep complete label definitions available for default unique_ptr arguments and remove redundant label getter tests. --- src/iceberg/catalog/rest/rest_catalog.cc | 12 +++++-- src/iceberg/catalog/rest/rest_catalog.h | 4 ++- src/iceberg/catalog/rest/rest_table.cc | 5 +-- src/iceberg/catalog/rest/rest_table.h | 4 +-- src/iceberg/table.cc | 22 ++++++------ src/iceberg/table.h | 8 +++-- src/iceberg/test/table_test.cc | 43 +++++------------------- 7 files changed, 43 insertions(+), 55 deletions(-) diff --git a/src/iceberg/catalog/rest/rest_catalog.cc b/src/iceberg/catalog/rest/rest_catalog.cc index 8e49338a3d..c450673105 100644 --- a/src/iceberg/catalog/rest/rest_catalog.cc +++ b/src/iceberg/catalog/rest/rest_catalog.cc @@ -47,6 +47,7 @@ #include "iceberg/catalog/rest/rest_util.h" #include "iceberg/catalog/rest/types.h" #include "iceberg/json_serde_internal.h" +#include "iceberg/labels.h" #include "iceberg/logging/log_level.h" #include "iceberg/logging/log_macros.h" #include "iceberg/logging/logger.h" @@ -984,6 +985,11 @@ Result> RestCatalog::MakeTableFromLoadResult( std::shared_ptr contextual_session) { auto table_config = std::move(result.config); auto storage_credentials = std::move(result.storage_credentials); + // Labels are usually absent; keep the table free of an empty Labels allocation. + std::unique_ptr labels; + if (!result.labels.empty()) { + labels = std::make_unique(std::move(result.labels)); + } // Before the FileIO: refreshing its credentials reuses the table session. ICEBERG_ASSIGN_OR_RAISE( auto table_session, @@ -997,7 +1003,7 @@ Result> RestCatalog::MakeTableFromLoadResult( return MakeTable(identifier, std::move(result.metadata), std::move(result.metadata_location), std::move(table_io), std::move(table_catalog), std::move(reporter), table_config, - std::move(table_session), std::move(result.labels)); + std::move(table_session), std::move(labels)); } Result> RestCatalog::MakeTableFromCommitResponse( @@ -1012,7 +1018,7 @@ Result> RestCatalog::MakeTableFromCommitResponse( return MakeTable(identifier, std::move(response.metadata), std::move(response.metadata_location), std::move(table_io), std::move(table_catalog), std::move(reporter), table_config, - std::move(table_session)); + std::move(table_session), /*labels=*/nullptr); } Result> RestCatalog::MakeTable( @@ -1020,7 +1026,7 @@ Result> RestCatalog::MakeTable( std::string metadata_location, std::shared_ptr table_io, std::shared_ptr table_catalog, std::shared_ptr reporter, const std::unordered_map& table_config, - std::shared_ptr table_session, Labels labels) { + std::shared_ptr table_session, std::unique_ptr labels) { // Table config overrides client config. ICEBERG_ASSIGN_OR_RAISE(auto server_mode, RestCatalogProperties::ScanPlanningModeFrom(table_config)); diff --git a/src/iceberg/catalog/rest/rest_catalog.h b/src/iceberg/catalog/rest/rest_catalog.h index 8efeb434b3..1e713a5040 100644 --- a/src/iceberg/catalog/rest/rest_catalog.h +++ b/src/iceberg/catalog/rest/rest_catalog.h @@ -32,6 +32,7 @@ #include "iceberg/catalog/rest/type_fwd.h" #include "iceberg/catalog/session_catalog.h" #include "iceberg/catalog/session_context.h" +#include "iceberg/labels.h" #include "iceberg/result.h" #include "iceberg/storage_credential.h" #include "iceberg/type_fwd.h" @@ -214,7 +215,8 @@ class ICEBERG_REST_EXPORT RestCatalog final std::string metadata_location, std::shared_ptr table_io, std::shared_ptr table_catalog, std::shared_ptr reporter, const std::unordered_map& table_config, - std::shared_ptr table_session, Labels labels = {}); + std::shared_ptr table_session, + std::unique_ptr labels = nullptr); RestCatalogProperties config_; std::shared_ptr file_io_; diff --git a/src/iceberg/catalog/rest/rest_table.cc b/src/iceberg/catalog/rest/rest_table.cc index 0f685532a9..f70bd9e926 100644 --- a/src/iceberg/catalog/rest/rest_table.cc +++ b/src/iceberg/catalog/rest/rest_table.cc @@ -23,6 +23,7 @@ #include #include "iceberg/catalog/rest/rest_table_scan.h" +#include "iceberg/labels.h" #include "iceberg/result.h" #include "iceberg/table_metadata.h" #include "iceberg/util/macros.h" @@ -33,7 +34,7 @@ RestTable::RestTable(TableIdentifier identifier, std::shared_ptr std::string metadata_location, std::shared_ptr io, std::shared_ptr catalog, std::string full_name, std::shared_ptr reporter, - RestScanContext rest_context, Labels labels) + RestScanContext rest_context, std::unique_ptr labels) : Table(std::move(identifier), std::move(metadata), std::move(metadata_location), std::move(io), std::move(catalog), std::move(full_name), std::move(reporter), std::move(labels)), @@ -46,7 +47,7 @@ Result> RestTable::Make( std::string metadata_location, std::shared_ptr io, std::shared_ptr catalog, std::string full_name, std::shared_ptr reporter, RestScanContext rest_context, - Labels labels) { + std::unique_ptr labels) { if (metadata == nullptr) { return InvalidArgument("Metadata cannot be null"); } diff --git a/src/iceberg/catalog/rest/rest_table.h b/src/iceberg/catalog/rest/rest_table.h index ae0accd0e7..57b06f7b19 100644 --- a/src/iceberg/catalog/rest/rest_table.h +++ b/src/iceberg/catalog/rest/rest_table.h @@ -42,7 +42,7 @@ class ICEBERG_REST_EXPORT RestTable final : public Table { std::string metadata_location, std::shared_ptr io, std::shared_ptr catalog, std::string full_name, std::shared_ptr reporter, RestScanContext rest_context, - Labels labels = {}); + std::unique_ptr labels = nullptr); ~RestTable() override; @@ -59,7 +59,7 @@ class ICEBERG_REST_EXPORT RestTable final : public Table { std::string metadata_location, std::shared_ptr io, std::shared_ptr catalog, std::string full_name, std::shared_ptr reporter, RestScanContext rest_context, - Labels labels); + std::unique_ptr labels); std::shared_ptr rest_context_; }; diff --git a/src/iceberg/table.cc b/src/iceberg/table.cc index 3b801af1f4..e56746e3b3 100644 --- a/src/iceberg/table.cc +++ b/src/iceberg/table.cc @@ -25,6 +25,7 @@ #include "iceberg/data_table_scan.h" #include "iceberg/incremental_append_scan.h" #include "iceberg/incremental_changelog_scan.h" +#include "iceberg/labels.h" #include "iceberg/location_provider.h" #include "iceberg/partition_spec.h" #include "iceberg/result.h" @@ -60,7 +61,7 @@ Result> Table::Make( TableIdentifier identifier, std::shared_ptr metadata, std::string metadata_location, std::shared_ptr io, std::shared_ptr catalog, std::string full_name, - std::shared_ptr reporter, Labels labels) { + std::shared_ptr reporter, std::unique_ptr labels) { if (metadata == nullptr) [[unlikely]] { return InvalidArgument("Metadata cannot be null"); } @@ -84,7 +85,7 @@ Table::~Table() = default; Table::Table(TableIdentifier identifier, std::shared_ptr metadata, std::string metadata_location, std::shared_ptr io, std::shared_ptr catalog, std::string full_name, - std::shared_ptr reporter, Labels labels) + std::shared_ptr reporter, std::unique_ptr labels) : identifier_(std::move(identifier)), full_name_(full_name.empty() ? identifier_.ToString() : std::move(full_name)), metadata_(std::move(metadata)), @@ -93,8 +94,7 @@ Table::Table(TableIdentifier identifier, std::shared_ptr metadata catalog_(std::move(catalog)), reporter_(std::move(reporter)), metadata_cache_(std::make_unique(metadata_.get())), - labels_(labels.empty() ? nullptr - : std::make_unique(std::move(labels))) {} + labels_(labels && !labels->empty() ? std::move(labels) : nullptr) {} const std::string& Table::uuid() const { return metadata_->table_uuid; } @@ -308,9 +308,10 @@ Result> StagedTable::Make( if (catalog == nullptr) [[unlikely]] { return InvalidArgument("Catalog cannot be null"); } - return std::shared_ptr(new StagedTable( - std::move(identifier), std::move(metadata), std::move(metadata_location), - std::move(io), std::move(catalog), std::move(full_name), std::move(reporter))); + return std::shared_ptr( + new StagedTable(std::move(identifier), std::move(metadata), + std::move(metadata_location), std::move(io), std::move(catalog), + std::move(full_name), std::move(reporter), /*labels=*/nullptr)); } StagedTable::~StagedTable() = default; @@ -328,9 +329,10 @@ Result> StaticTable::Make( if (io == nullptr) [[unlikely]] { return InvalidArgument("FileIO cannot be null"); } - return std::shared_ptr(new StaticTable( - std::move(identifier), std::move(metadata), std::move(metadata_location), - std::move(io), /*catalog=*/nullptr, std::move(full_name))); + return std::shared_ptr( + new StaticTable(std::move(identifier), std::move(metadata), + std::move(metadata_location), std::move(io), /*catalog=*/nullptr, + std::move(full_name), /*reporter=*/nullptr, /*labels=*/nullptr)); } StaticTable::~StaticTable() = default; diff --git a/src/iceberg/table.h b/src/iceberg/table.h index f53cdc3fb1..97b1a43a9b 100644 --- a/src/iceberg/table.h +++ b/src/iceberg/table.h @@ -51,12 +51,13 @@ class ICEBERG_EXPORT Table : public std::enable_shared_from_this
{ /// 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. + /// \param[in] labels Labels for this table, or nullptr when it has none. static Result> Make( TableIdentifier identifier, std::shared_ptr metadata, std::string metadata_location, std::shared_ptr io, std::shared_ptr catalog, std::string full_name = "", - std::shared_ptr reporter = nullptr, Labels labels = {}); + std::shared_ptr reporter = nullptr, + std::unique_ptr labels = nullptr); virtual ~Table(); @@ -224,7 +225,8 @@ class ICEBERG_EXPORT Table : public std::enable_shared_from_this
{ Table(TableIdentifier identifier, std::shared_ptr metadata, std::string metadata_location, std::shared_ptr io, std::shared_ptr catalog, std::string full_name, - std::shared_ptr reporter = nullptr, Labels labels = {}); + std::shared_ptr reporter = nullptr, + std::unique_ptr labels = nullptr); const TableIdentifier identifier_; const std::string full_name_; diff --git a/src/iceberg/test/table_test.cc b/src/iceberg/test/table_test.cc index c269240875..00c77371fb 100644 --- a/src/iceberg/test/table_test.cc +++ b/src/iceberg/test/table_test.cc @@ -150,12 +150,6 @@ TYPED_TEST(TypedTableTest, NewTransaction) { } } -TYPED_TEST(TypedTableTest, LabelsDefaultToEmpty) { - ICEBERG_UNWRAP_OR_FAIL(auto table, this->MakeTable("test_table")); - - EXPECT_TRUE(table->labels().empty()); -} - class TableLabelsTest : public ::testing::Test { protected: void SetUp() override { @@ -176,29 +170,6 @@ class TableLabelsTest : public ::testing::Test { std::shared_ptr metadata_; }; -TEST_F(TableLabelsTest, EmptyLabelsShareFallback) { - ICEBERG_UNWRAP_OR_FAIL( - auto first, Table::Make(ident_, metadata_, "s3://bucket/meta.json", io_, catalog_)); - ICEBERG_UNWRAP_OR_FAIL( - auto second, Table::Make(ident_, metadata_, "s3://bucket/meta.json", io_, catalog_, - /*full_name=*/"", /*reporter=*/nullptr, Labels{})); - - EXPECT_TRUE(first->labels().empty()); - EXPECT_TRUE(second->labels().empty()); - EXPECT_EQ(&first->labels(), &second->labels()); -} - -TEST_F(TableLabelsTest, ExposesProvidedLabels) { - for (const auto& labels : {labels_, Labels{.object_labels = {{"owner", "team-a"}}}, - Labels{.fields = {{.field_id = 1}}}}) { - ICEBERG_UNWRAP_OR_FAIL( - auto table, Table::Make(ident_, metadata_, "s3://bucket/meta.json", io_, catalog_, - /*full_name=*/"", /*reporter=*/nullptr, labels)); - - EXPECT_EQ(table->labels(), labels); - } -} - class TableLabelsRefreshTest : public TableLabelsTest, public ::testing::WithParamInterface {}; @@ -212,11 +183,14 @@ TEST_P(TableLabelsRefreshTest, RefreshKeepsLabels) { SCOPED_TRACE(refreshed_labels.empty() ? "Absent labels" : "Changed labels"); ICEBERG_UNWRAP_OR_FAIL( auto table, Table::Make(ident_, metadata_, "s3://bucket/meta.json", io_, catalog_, - /*full_name=*/"", /*reporter=*/nullptr, labels_)); + /*full_name=*/"", /*reporter=*/nullptr, + std::make_unique(labels_))); ICEBERG_UNWRAP_OR_FAIL( - auto refreshed, - Table::Make(ident_, metadata_, metadata_location, io_, catalog_, - /*full_name=*/"", /*reporter=*/nullptr, refreshed_labels)); + auto refreshed, Table::Make(ident_, metadata_, metadata_location, io_, catalog_, + /*full_name=*/"", /*reporter=*/nullptr, + refreshed_labels.empty() + ? nullptr + : std::make_unique(refreshed_labels))); EXPECT_CALL(*catalog_, LoadTable(::testing::_)) .WillOnce(::testing::Return(refreshed)); @@ -235,7 +209,8 @@ INSTANTIATE_TEST_SUITE_P(MetadataLocation, TableLabelsRefreshTest, ::testing::Bo TEST_F(TableLabelsTest, EmptyTransactionKeepsLabels) { ICEBERG_UNWRAP_OR_FAIL( auto table, Table::Make(ident_, metadata_, "s3://bucket/meta.json", io_, catalog_, - /*full_name=*/"", /*reporter=*/nullptr, labels_)); + /*full_name=*/"", /*reporter=*/nullptr, + std::make_unique(labels_))); EXPECT_CALL(*catalog_, UpdateTable(::testing::_, ::testing::_, ::testing::_)).Times(0); ICEBERG_UNWRAP_OR_FAIL(auto transaction, table->NewTransaction()); ICEBERG_UNWRAP_OR_FAIL(auto committed, transaction->Commit());