Repository navigation
Core: expose labels on the loaded table via SupportsLabels - #17410
laskoviymishka wants to merge 4 commits into
Conversation
79e2747 to
77ddd7c
Compare
77ddd7c to
3376335
Compare
| // Ephemeral catalog enrichment, not table state. Transient because it is not preserved across | ||
| // serialization: writeReplace() swaps in a SerializableTable for Java serialization, and any | ||
| // path that bypasses writeReplace (e.g. Kryo) leaves this null, handled by labels(). |
There was a problem hiding this comment.
This is a little verbose. I think the first sentence is enough.
There was a problem hiding this comment.
Actually, I'm not sure why we're not preserving these. I'm a little concerned because it creates some weird behaviors through serialization. This table will say it supports labels, but once serialized, it doesn't (since SerializedTable doesn't support the interface). Maybe this isn't an issue but I'm also not clear on why we would not maintain it through serialization for anything wanting this info on a worker/executor side.
| import org.apache.iceberg.rest.labels.Labels; | ||
|
|
||
| /** | ||
| * Implemented by tables that can expose catalog-provided labels obtained from the load response. |
There was a problem hiding this comment.
Remove the obtained from the load response from the end since you can implement this for other non-REST catalogs (e.g. BigLake)
| */ | ||
| package org.apache.iceberg; | ||
|
|
||
| import org.apache.iceberg.rest.labels.Labels; |
There was a problem hiding this comment.
See comment here: https://github.com/apache/iceberg/pull/17337/changes#r3739125412
I think the Labels class itself is general enough to put in the top level package.
3376335 to
1f80f84
Compare
Reference client implementation for the IRC labels read-path spec change (apache#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.
3a8d980 to
5056726
Compare
Adds an `include-labels` flag to the REST server test fixture that attaches labels to load table and load view responses, mirroring how `include-credentials` vends credentials. The injected labels are derived from the catalog name and the server hostname, and the first column of the schema is labeled so that both object-level and field-level labels are covered. This gives other client implementations a reference server that actually returns labels to test against.
5056726 to
56e34fa
Compare
The raw HTTP client used to read the load responses requires an auth session, so requests failed with "Invalid auth session: null". The local fixture does not authenticate requests, so the empty session is used.
Builds on the labels read-path serde: make the catalog-provided labels from the load response reachable by consumers. - New SupportsLabels mixin interface (mirroring SupportsDistributedScanPlanning) with a single labels() accessor. - BaseTable implements SupportsLabels via a new constructor that carries an optional Labels; existing constructors default to an empty instance. The field is transient and not copied into SerializableTable, so labels are ephemeral catalog enrichment and are not preserved across table serialization. - RESTSessionCatalog populates labels from the load response for both the plain BaseTable and the server-side scan-planning RESTTable paths, across loadTable, registerTable, and createTable.
56e34fa to
5fbd24f
Compare
|
This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the [email protected] list. Thank you for your contributions. |
|
Close in favor for #18045 |
Exposes the catalog-provided labels obtained from the load response on the loaded
Table, so engines and tooling can read them.Stacked on #17337 (the labels read-path serde). Review the top commit — the serde layer belongs to #17337.
What changed
SupportsLabelsmixin interface (mirroringSupportsDistributedScanPlanning) with a singlelabels()accessor.BaseTableimplementsSupportsLabelsvia a new constructor carrying an optionalLabels; existing constructors default to an empty instance. The field is transient and is not copied intoSerializableTable, so labels are ephemeral catalog enrichment and are not preserved across table serialization (driver-side only).RESTSessionCatalogpopulates labels from the load response for both the plainBaseTableand the server-side scan-planningRESTTablepaths, acrossloadTable,registerTable, andcreateTable.Read-only; the write path is out of scope.