Repository navigation
Core: expose catalog labels on the loaded table via SupportsLabels - #18046
Conversation
1f29eb0 to
38a506b
Compare
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.
38a506b to
5a0ab8c
Compare
- Rename TestBaseTableLabels to TestLabelsOnBaseTable, drop a redundant comment, and assert the field-label contents (field id + labels), not just the count. - Implement SupportsLabels on SerializableTable, returning Labels.EMPTY: labels are driver-side catalog enrichment and are not carried across serialization. Co-authored-by: Isaac <no-reply@databricks.com>
| * storage. | ||
| */ | ||
| public class SerializableTable implements Table, HasTableOperations, Serializable { | ||
| public class SerializableTable implements Table, HasTableOperations, Serializable, SupportsLabels { |
There was a problem hiding this comment.
Why do we implement SupportsLabels if we don't support it?
There was a problem hiding this comment.
Ot we can just copy and serialize the labels - it is not that complicated
| import org.junit.jupiter.api.Test; | ||
| import org.junit.jupiter.api.io.TempDir; | ||
|
|
||
| class TestLabelsOnBaseTable { |
There was a problem hiding this comment.
do we want to test that it is working with RESTCatalog?
|
Do we want to do the View labels in a different PR? |
|
|
||
| @Override | ||
| public Labels labels() { | ||
| // labels is null only when this instance was deserialized bypassing writeReplace (e.g. Kryo). |
There was a problem hiding this comment.
not just Kryo - normal Java deserialization from other package could cause this too.
| Object hadoopConf) { | ||
| super(ops, name, reporter); | ||
| Object hadoopConf, | ||
| Labels labels) { |
There was a problem hiding this comment.
This is getting super ugly - 11 parameters 🤮
Maybe in another PR?
There was a problem hiding this comment.
yeap, not the tiny thing, will file a separate PR to cleanup this.
| BaseTable table = | ||
| new BaseTable(new TestTables.TestTableOperations(TABLE_NAME, temp), TABLE_NAME); | ||
|
|
||
| assertThat(table).isInstanceOf(SupportsLabels.class); |
There was a problem hiding this comment.
Are we checking that the BaseTable class implements SupportsLabels?
|
Do we have a clear plan how caching/refs fetching should work with labels and REST catalog? Maybe we want to spec that we need to have different ETags for different labels if the labels carry mission critical info like PII settings, or we can ignore them, and highlight this in spec? |
|
this is all server-side, so i don't think labels need a spec change here. etag/caching is only defined once in the spec, on the etag parameter: it's "a unique version of the table metadata", and servers "should produce unique tags for responses that return different metadata content." the refs vs all example is already there. we don't repeat caching rules for config, storage-credentials, remote-signing-config, etc., so adding something labels-specific would be odd. on the client side, the etag is opaque. the client stores it and sends it back in whether a label change changes the etag is up to the server. etag versions the metadata response, and labels aren't table metadata. the Labels text already says they may be cached following the response etag, which seems enough to me. |
yes, i'll do it separately |
- Drop SupportsLabels from SerializableTable. Labels are transient catalog enrichment that is intentionally not shipped to executors, and the only consumer already guards with instanceof, so behavior is unchanged; a real label-carrying implementation stays additive if ever needed. - Broaden the BaseTable#labels() null comment: the transient field is null after any deserialization that bypasses writeReplace(), not only Kryo. - Make the TestLabelsOnBaseTable test methods package-private. Co-authored-by: Isaac <no-reply@databricks.com>
Loads a table through RESTCatalog backed by an adapter that returns object- and field-level labels on the load-table response, and asserts they are surfaced through SupportsLabels#labels(). Co-authored-by: Isaac <no-reply@databricks.com>
pvary
left a comment
pvary
left a comment
There was a problem hiding this comment.
LGTM. Small question on javadoc.
Thanks!
Co-authored-by: pvary <peter.vary.apache@gmail.com>
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.
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).
Part 2 of the catalog-labels stack (spec: #15750). Based on #18045 — review that first; this PR's diff is only the table-exposure layer.
Exposes catalog-provided labels on the loaded table:
SupportsLabelsmixin interface andBaseTable.labels()RESTSessionCatalog/RESTTablethread labels from the load response onto the tableTestBaseTableLabelsStacked on #18045.