Conversation
Parse the optional `labels` field of the REST load-table response and expose it through Table::labels(), mirroring apache/iceberg#18045 and #18046. Labels are catalog-provided rather than table state, so they live on the Table instance instead of TableMetadata. Part of apache#938.
4 tasks
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.
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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