Skip to content

Core: add labels to the load table/view read path - #17337

Open
laskoviymishka wants to merge 1 commit into
apache:mainfrom
laskoviymishka:labels-client-impl
Open

Core: add labels to the load table/view read path#17337
laskoviymishka wants to merge 1 commit into
apache:mainfrom
laskoviymishka:labels-client-impl

Conversation

@laskoviymishka

Copy link
Copy Markdown
Contributor

Reference client implementation for the IRC labels read path, the Java-side counterpart to the spec change in #15750.

Why

The spec PR (#15750) adds an optional labels field to LoadTableResult and LoadViewResult. Without a corresponding client change, RESTObjectMapper (which sets FAIL_ON_UNKNOWN_PROPERTIES = false) silently drops the field on deserialization, so labels returned by a catalog are invisible to the Java client. This PR makes the reference client able to read and round-trip them.

What changed

New org.apache.iceberg.rest.labels package (laid out like rest.credentials):

  • Labelsobject (catalog-object-level, flat string map) and fields (per-field labels).
  • FieldLabels — a single field's labels, identified by field-id.
  • LabelsParser / FieldLabelsParser — JSON serialization, symmetric with the spec schemas.

Wired an optional labels field into LoadTableResponse and LoadViewResponse and their parsers. Labels are omitted from the wire when absent, so the change is backward compatible — existing responses serialize identically.

Tests

  • TestLabelsParser: round-trips (object-only, fields-only, both), empty labels, and field-id validation.
  • roundTripSerdeWithLabels and labelsAreOptional added to TestLoadTableResponseParser and TestLoadViewResponseParser.

Notes

Comment thread core/src/test/java/org/apache/iceberg/rest/labels/TestLabelsParser.java Outdated
Comment thread .palantir/revapi.yml Outdated
@laskoviymishka
laskoviymishka requested a review from pvary July 24, 2026 12:14
@laskoviymishka
laskoviymishka marked this pull request as ready for review July 24, 2026 12:14
Comment thread core/src/test/java/org/apache/iceberg/rest/responses/TestLoadTableResponse.java Outdated
@pvary

pvary commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

How will old REST clients react if there is a new labels in the response JSON?

@laskoviymishka

Copy link
Copy Markdown
Contributor Author

@pvary -- they will simply ignore it.

Comment thread core/src/test/java/org/apache/iceberg/rest/responses/TestLoadTableResponse.java Outdated
@pvary

pvary commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

It the spec change is accepted, this is good to go as well.

@Value.Immutable
public interface Labels {
/** Catalog-object-level labels, attached to the object (table, view, ...) as a whole. */
Map<String, String> object();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

have we decided on the naming in the Spec for this? I still feel like object() is really not telling me anything as a reader of the code

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We discuss this here - #15750 (comment)

I'm thinking we are close to alignment, i don't have strong preferences honestly, just want to ensure it works :)

Comment thread core/src/test/java/org/apache/iceberg/rest/labels/TestLabelsParser.java Outdated
Comment thread core/src/main/java/org/apache/iceberg/rest/labels/LabelsParser.java Outdated
Comment thread core/src/test/java/org/apache/iceberg/rest/labels/TestLabelsParser.java Outdated
Comment thread core/src/main/java/org/apache/iceberg/rest/labels/FieldLabels.java Outdated
Comment thread core/src/main/java/org/apache/iceberg/rest/labels/Labels.java Outdated
@laskoviymishka
laskoviymishka requested a review from nastra July 28, 2026 19:53
@laskoviymishka
laskoviymishka force-pushed the labels-client-impl branch 2 times, most recently from 1162f1a to 7b97096 Compare July 28, 2026 22:25
}

@Test
public void emptyLabels() {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what about JSON that has empty or no labels defined?

Comment thread core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java Outdated
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants