Repository navigation
Core: add labels to the load table/view read path - #18045
Conversation
Reference client implementation for the IRC labels read-path spec change (#15750). Without it, RESTObjectMapper (which sets FAIL_ON_UNKNOWN_PROPERTIES = false) silently drops the labels field on deserialization, so labels returned by a catalog are invisible to the Java client. New rest.labels package (mirroring rest.credentials): Labels (object + fields sub-scopes, with a shared empty instance) and FieldLabels (per-field, keyed by field-id) value types as immutables interfaces, each with a JSON parser. FieldLabels validates field-id >= 1 and a non-empty labels map, matching Credential. Wire an optional labels field into LoadTableResponse / LoadViewResponse and their parsers. labels() never returns null (empty instance when absent), on LoadViewResponse via a @Value.Default default method so no interface API break is introduced. Labels are omitted from the wire when absent (or empty), so the change is additive and backward compatible.
98d20c7 to
4cf923b
Compare
Address review nit: a single FieldLabel value was named `fieldLabels` (plural). Rename the parameter in FieldLabelParser, the loop variable in LabelsParser, and the test locals, and singularize the related precondition messages. Co-authored-by: Isaac <[email protected]>
The previous commit singularized the precondition messages in
FieldLabelParser, which broke TestFieldLabelParser.nullCheck (it asserts
the exact text). Keep the identifier rename fieldLabels -> fieldLabel but
restore the original messages ("Invalid field labels: null",
"Cannot parse field labels from null object").
Co-authored-by: Isaac <[email protected]>
|
the changes themselves LGTM but the one concern I have is that we're now adding a REST-specific concept (Labels, FieldLabel) outside of the |
@nastra this actually isn't REST-specific. For example, you can implement catalog labels with any catalog implementation (e.g. BigLake Catalog). While we expose this with REST, pretty much any implementation can support labels, so I think the packaging is correct. |
- Make TestFieldLabelParser and TestLabelsParser (and their test methods) package private. - Annotate Labels.isEmpty() with @Value.Derived, matching the pattern in RemoteSigningConfig. Co-authored-by: Isaac <[email protected]>
Address review: default the builder's labels field to Labels.EMPTY, matching remoteSigningConfig, so build() never passes null. Co-authored-by: Isaac <[email protected]>
pvary
left a comment
There was a problem hiding this comment.
LGTM
Thanks @laskoviymishka!
Parse the optional `labels` field of the REST load-table response and expose it through Table::labels(), mirroring apache/iceberg#18045 and live on the Table instance instead of TableMetadata. Part of apache#938.
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.
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.
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.
…he#4320) * Parse catalog labels in REST load table and load view responses The REST spec adds an optional `labels` field to LoadTableResult and LoadViewResult for catalog-provided enrichment such as ownership or classification. PyIceberg dropped the field. Add `Labels` and `FieldLabel` models and parse `labels` on `TableResponse` and `ViewResponse`, matching the Java read path (apache/iceberg#18045). * Read non-object labels as empty A string, list, number, or bool `labels` value failed the whole load. Java `LabelsParser` reads keys only under `hasNonNull`, so it returns empty labels for any non-object value. Do the same.
* Parse catalog labels in REST load table and load view responses The REST spec adds an optional `labels` field to LoadTableResult and LoadViewResult for catalog-provided enrichment such as ownership or classification. PyIceberg dropped the field. Add `Labels` and `FieldLabel` models and parse `labels` on `TableResponse` and `ViewResponse`, matching the Java read path (apache/iceberg#18045). * Read non-object labels as empty A string, list, number, or bool `labels` value failed the whole load. Java `LabelsParser` reads keys only under `hasNonNull`, so it returns empty labels for any non-object value. Do the same. * Expose catalog labels on Table Labels parsed from the load response were not reachable from the loaded table. Add `Table.labels`. The REST catalog sets it on load, create, and register. Other catalogs and staged tables (`create_table_transaction`) get empty labels. Labels are not table state: they are not part of equality, and refresh and commits keep them. This matches the Java `SupportsLabels` behavior (apache/iceberg#18046). * Test labels on create, register, and pickle Create and register hand labels to the same `_response_to_table` as load, but only load was tested. Check all three. Also pin that a pickled table keeps its labels, the one intended difference from Java, where `BaseTable.labels` is transient. --------- Co-authored-by: Fokko Driesprong <[email protected]>
Part 1 of a 5-PR stack implementing the client/engine side of catalog labels. The REST spec was merged in #15750.
This PR adds the Java model and REST serde for the
labelsfield returned on the load-table and load-view responses:LabelsandFieldLabelimmutable value typesLabelsParser/FieldLabelParser(wire keysobject-labels,fields,field-id)labelsintoLoadTableResponse/LoadViewResponseand their parsers, alongside the existingremote-signing-configNote: the Java type is
FieldLabel(singular) while the OpenAPI schema staysFieldLabels— an intentional Java-vs-spec divergence sofieldLabel.labels()reads cleanly. The wire contract (field-id,fields,labels) is unchanged.Stack (linear; each based on the previous)
SupportsLabels)Restructures the earlier cross-fork PRs (#17337 / #17410 / #17411) into a reviewable native stack; those remain open for now.