Repository navigation
Conversation
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
The staged create-table path must preserve catalog-provided labels.
Review effort: Lite
Findings: None
What changed in this PR
Adds REST catalog label parsing and exposes catalog-provided labels through Table::labels().
Changes:
- Adds label models, serde support, and strict field-ID parsing.
- Wires labels into REST table responses and table instances.
- Adds serde and table behavior tests.
| File | Summary |
|---|---|
src/iceberg/util/json_util_internal.h |
Adds strict int32 parsing. |
src/iceberg/type_fwd.h |
Adds label forward declarations. |
src/iceberg/test/table_test.cc |
Tests table label behavior. |
src/iceberg/test/rest_json_serde_test.cc |
Tests label serialization and parsing. |
src/iceberg/table.h |
Adds the labels API. |
src/iceberg/table.cc |
Stores and exposes labels. |
src/iceberg/labels.h |
Defines label models. |
src/iceberg/catalog/rest/types.h |
Adds labels to load results; staged table construction still drops them (moderate issue). |
src/iceberg/catalog/rest/types.cc |
Includes labels in equality checks. |
src/iceberg/catalog/rest/rest_catalog.cc |
Wires labels into REST table handling. |
src/iceberg/catalog/rest/json_serde.cc |
Implements label JSON serde. |
src/iceberg/catalog/rest/json_serde_internal.h |
Declares label serde functions. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
Thanks for adding this! Could you please rebase and resolve merge conflicts? |
|
Sure let me do that. Will update by EOD! |
b0b1a63 to
37376c2
Compare
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 apache#938.
37376c2 to
dd1aee6
Compare
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Copilot review overview
1 open finding
🧠 Review effort: Lite
| // 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)); | ||
| } |
|
Rebased onto |
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Copilot review overview
3 open findings
🧠 Review effort: Lite
| // 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; | ||
| } |
| 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 = {}); |
Parse the optional
labelsfield 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.
Unit tests cover the serde (all label shapes, null/absent handling, invalid input, strict int32 field-id) and the Table side (default empty, provided labels exposed, unchanged by Refresh()). The REST-response-to-Table wiring isn't reachable from unit tests, so I also checked it locally against the REST fixture behind a proxy that adds labels, covering create, load and register. Test code posted at https://gist.github.com/NihalJain/847d9137f090db63978a56c9fa84cd0b
Proper integration tests come in C3 once apache/iceberg#18047 ships in the fixture image.
AI Disclosure