You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "laskoviymishka (via GitHub)" <gi...@apache.org> on 2026/09/10 11:33:20 UTC
[PR] Core: add labels to the load table/view read path [iceberg]
laskoviymishka opened a new pull request, #18045:
URL: https://github.com/apache/iceberg/pull/18045
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 `labels` field returned on the load-table and load-view responses:
- `Labels` and `FieldLabels` immutable value types
- `LabelsParser` / `FieldLabelsParser` (wire keys `object-labels`, `fields`, `field-id`)
- wires `labels` into `LoadTableResponse` / `LoadViewResponse` and their parsers, alongside the existing `remote-signing-config`
- unit and round-trip tests
### Stack
1. **This PR** — read-path serde
2. Expose labels on the loaded table (`SupportsLabels`)
3. REST server test fixture emitting labels
4. Labels metadata table
5. Spark 4.1 DESCRIBE integration
Each subsequent PR is based on the previous branch. This restructures the earlier cross-fork PRs (#17337 / #17410 / #17411) into a reviewable native stack; those remain open for now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: add labels to the load table/view read path [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #18045:
URL: https://github.com/apache/iceberg/pull/18045#discussion_r3989789498
##########
core/src/test/java/org/apache/iceberg/TestFieldLabelsParser.java:
##########
@@ -0,0 +1,97 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
+
+import com.fasterxml.jackson.databind.JsonNode;
+import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap;
+import org.junit.jupiter.api.Test;
+
+public class TestFieldLabelsParser {
+
+ @Test
+ public void nullCheck() {
+ assertThatThrownBy(() -> FieldLabelsParser.toJson(null))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessage("Invalid field labels: null");
+
+ assertThatThrownBy(() -> FieldLabelsParser.fromJson((JsonNode) null))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessage("Cannot parse field labels from null object");
+ }
+
+ @Test
+ public void roundTrip() {
+ FieldLabels fieldLabels =
+ ImmutableFieldLabels.builder().fieldId(3).labels(ImmutableMap.of("pii", "true")).build();
+
+ String expectedJson =
+ """
+ {
+ "field-id" : 3,
+ "labels" : {
+ "pii" : "true"
+ }
+ }""";
+
+ assertThat(FieldLabelsParser.toJson(fieldLabels, true)).isEqualTo(expectedJson);
+ assertThat(FieldLabelsParser.fromJson(expectedJson)).isEqualTo(fieldLabels);
+ }
+
+ @Test
+ public void invalidFieldId() {
+ assertThatThrownBy(
+ () ->
+ ImmutableFieldLabels.builder().fieldId(0).labels(ImmutableMap.of("k", "v")).build())
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessage("Invalid field id: must be >= 1");
+ }
+
+ @Test
+ public void invalidFieldIdFromJson() {
+ assertThatThrownBy(
+ () -> FieldLabelsParser.fromJson("{\"field-id\": 0, \"labels\": {\"k\": \"v\"}}"))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessage("Invalid field id: must be >= 1");
+ }
+
+ @Test
+ public void emptyLabels() {
+ FieldLabels fieldLabels = ImmutableFieldLabels.builder().fieldId(1).build();
Review Comment:
nit: can be inlined
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: add labels to the load table/view read path [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #18045:
URL: https://github.com/apache/iceberg/pull/18045#discussion_r3989835129
##########
core/src/main/java/org/apache/iceberg/FieldLabels.java:
##########
@@ -0,0 +1,36 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg;
+
+import java.util.Map;
+import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
+import org.immutables.value.Value;
+
+/** Labels attached to a single field, identified by its field id. See {@link Labels}. */
+@Value.Immutable
+public interface FieldLabels {
Review Comment:
the double plural form of FieldLabels and then labels() reads weird
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: add labels to the load table/view read path [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #18045:
URL: https://github.com/apache/iceberg/pull/18045#discussion_r3989815478
##########
core/src/test/java/org/apache/iceberg/TestFieldLabelsParser.java:
##########
@@ -0,0 +1,97 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
+
+import com.fasterxml.jackson.databind.JsonNode;
+import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap;
+import org.junit.jupiter.api.Test;
+
+public class TestFieldLabelsParser {
+
+ @Test
+ public void nullCheck() {
+ assertThatThrownBy(() -> FieldLabelsParser.toJson(null))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessage("Invalid field labels: null");
+
+ assertThatThrownBy(() -> FieldLabelsParser.fromJson((JsonNode) null))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessage("Cannot parse field labels from null object");
+ }
+
+ @Test
+ public void roundTrip() {
+ FieldLabels fieldLabels =
+ ImmutableFieldLabels.builder().fieldId(3).labels(ImmutableMap.of("pii", "true")).build();
+
+ String expectedJson =
+ """
+ {
+ "field-id" : 3,
+ "labels" : {
+ "pii" : "true"
+ }
+ }""";
+
+ assertThat(FieldLabelsParser.toJson(fieldLabels, true)).isEqualTo(expectedJson);
+ assertThat(FieldLabelsParser.fromJson(expectedJson)).isEqualTo(fieldLabels);
+ }
+
+ @Test
+ public void invalidFieldId() {
+ assertThatThrownBy(
+ () ->
+ ImmutableFieldLabels.builder().fieldId(0).labels(ImmutableMap.of("k", "v")).build())
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessage("Invalid field id: must be >= 1");
+ }
+
+ @Test
+ public void invalidFieldIdFromJson() {
+ assertThatThrownBy(
Review Comment:
this can be combined with the other test right above this
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: add labels to the load table/view read path [iceberg]
Posted by "danielcweeks (via GitHub)" <gi...@apache.org>.
danielcweeks commented on PR #18045:
URL: https://github.com/apache/iceberg/pull/18045#issuecomment-5694633921
> 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 `org.apache.iceberg.rest` package, meaning that those things become part of the public API immediately. @danielcweeks any thoughts on that?
@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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: add labels to the load table/view read path [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #18045:
URL: https://github.com/apache/iceberg/pull/18045#discussion_r4028114105
##########
core/src/main/java/org/apache/iceberg/rest/responses/LoadTableResponse.java:
##########
@@ -114,6 +123,7 @@ public static class Builder {
private final Map<String, String> config = Maps.newHashMap();
private final List<Credential> credentials = Lists.newArrayList();
private RemoteSigningConfig remoteSigningConfig = RemoteSigningConfig.EMPTY;
+ private Labels labels;
Review Comment:
Should this be EMTPY as well?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: add labels to the load table/view read path [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #18045:
URL: https://github.com/apache/iceberg/pull/18045#discussion_r3989824739
##########
core/src/test/java/org/apache/iceberg/TestFieldLabelsParser.java:
##########
@@ -0,0 +1,97 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
+
+import com.fasterxml.jackson.databind.JsonNode;
+import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap;
+import org.junit.jupiter.api.Test;
+
+public class TestFieldLabelsParser {
+
+ @Test
+ public void nullCheck() {
+ assertThatThrownBy(() -> FieldLabelsParser.toJson(null))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessage("Invalid field labels: null");
+
+ assertThatThrownBy(() -> FieldLabelsParser.fromJson((JsonNode) null))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessage("Cannot parse field labels from null object");
+ }
+
+ @Test
+ public void roundTrip() {
+ FieldLabels fieldLabels =
+ ImmutableFieldLabels.builder().fieldId(3).labels(ImmutableMap.of("pii", "true")).build();
+
+ String expectedJson =
+ """
+ {
+ "field-id" : 3,
+ "labels" : {
+ "pii" : "true"
+ }
+ }""";
+
+ assertThat(FieldLabelsParser.toJson(fieldLabels, true)).isEqualTo(expectedJson);
+ assertThat(FieldLabelsParser.fromJson(expectedJson)).isEqualTo(fieldLabels);
+ }
+
+ @Test
+ public void invalidFieldId() {
+ assertThatThrownBy(
+ () ->
+ ImmutableFieldLabels.builder().fieldId(0).labels(ImmutableMap.of("k", "v")).build())
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessage("Invalid field id: must be >= 1");
+ }
+
+ @Test
+ public void invalidFieldIdFromJson() {
+ assertThatThrownBy(
+ () -> FieldLabelsParser.fromJson("{\"field-id\": 0, \"labels\": {\"k\": \"v\"}}"))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessage("Invalid field id: must be >= 1");
+ }
+
+ @Test
+ public void emptyLabels() {
+ FieldLabels fieldLabels = ImmutableFieldLabels.builder().fieldId(1).build();
+
+ assertThat(fieldLabels.labels()).isEmpty();
+ }
+
+ @Test
+ public void emptyLabelsFromJson() {
+ FieldLabels fieldLabels = FieldLabelsParser.fromJson("{\"field-id\": 1, \"labels\": {}}");
+
+ assertThat(fieldLabels.fieldId()).isEqualTo(1);
+ assertThat(fieldLabels.labels()).isEmpty();
+ }
+
+ @Test
+ public void missingLabelsFromJson() {
+ assertThatThrownBy(() -> FieldLabelsParser.fromJson("{\"field-id\": 1}"))
Review Comment:
you might also want to test this with an empty json
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: add labels to the load table/view read path [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #18045:
URL: https://github.com/apache/iceberg/pull/18045#discussion_r3989696702
##########
core/src/main/java/org/apache/iceberg/Labels.java:
##########
@@ -0,0 +1,40 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg;
+
+import java.util.List;
+import java.util.Map;
+import org.immutables.value.Value;
+
+/** Optional catalog-provided labels returned on a load response. */
+@Value.Immutable
+public interface Labels {
+ Labels EMPTY = ImmutableLabels.builder().build();
+
+ /** Object-level labels. */
+ Map<String, String> objectLabels();
+
+ /** Field-level labels */
+ List<FieldLabels> fields();
Review Comment:
nit: fieldLabels to align the naming?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: add labels to the load table/view read path [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #18045:
URL: https://github.com/apache/iceberg/pull/18045#discussion_r4028257370
##########
core/src/main/java/org/apache/iceberg/FieldLabel.java:
##########
@@ -0,0 +1,30 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg;
Review Comment:
Sorry for double posting!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: add labels to the load table/view read path [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #18045:
URL: https://github.com/apache/iceberg/pull/18045#discussion_r3989696702
##########
core/src/main/java/org/apache/iceberg/Labels.java:
##########
@@ -0,0 +1,40 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg;
+
+import java.util.List;
+import java.util.Map;
+import org.immutables.value.Value;
+
+/** Optional catalog-provided labels returned on a load response. */
+@Value.Immutable
+public interface Labels {
+ Labels EMPTY = ImmutableLabels.builder().build();
+
+ /** Object-level labels. */
+ Map<String, String> objectLabels();
+
+ /** Field-level labels */
+ List<FieldLabels> fields();
Review Comment:
nit: fieldLabels to align the naming?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: add labels to the load table/view read path [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #18045:
URL: https://github.com/apache/iceberg/pull/18045#discussion_r3989763247
##########
core/src/main/java/org/apache/iceberg/FieldLabels.java:
##########
@@ -0,0 +1,36 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg;
+
+import java.util.Map;
+import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
+import org.immutables.value.Value;
+
+/** Labels attached to a single field, identified by its field id. See {@link Labels}. */
+@Value.Immutable
+public interface FieldLabels {
+ int fieldId();
+
+ Map<String, String> labels();
+
+ @Value.Check
+ default void validate() {
+ Preconditions.checkArgument(fieldId() >= 1, "Invalid field id: must be >= 1");
Review Comment:
where does this requirement come from?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: add labels to the load table/view read path [iceberg]
Posted by "laskoviymishka (via GitHub)" <gi...@apache.org>.
laskoviymishka merged PR #18045:
URL: https://github.com/apache/iceberg/pull/18045
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: add labels to the load table/view read path [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #18045:
URL: https://github.com/apache/iceberg/pull/18045#discussion_r4012475661
##########
core/src/main/java/org/apache/iceberg/FieldLabelParser.java:
##########
@@ -0,0 +1,64 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg;
+
+import com.fasterxml.jackson.core.JsonGenerator;
+import com.fasterxml.jackson.databind.JsonNode;
+import java.io.IOException;
+import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
+import org.apache.iceberg.util.JsonUtil;
+
+public class FieldLabelParser {
+ private static final String FIELD_ID = "field-id";
+ private static final String LABELS = "labels";
+
+ private FieldLabelParser() {}
+
+ public static String toJson(FieldLabel fieldLabels) {
+ return toJson(fieldLabels, false);
+ }
+
+ public static String toJson(FieldLabel fieldLabels, boolean pretty) {
+ return JsonUtil.generate(gen -> toJson(fieldLabels, gen), pretty);
+ }
+
+ public static void toJson(FieldLabel fieldLabels, JsonGenerator gen) throws IOException {
Review Comment:
nit: fieldLabels -> fieldLabel
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: add labels to the load table/view read path [iceberg]
Posted by "laskoviymishka (via GitHub)" <gi...@apache.org>.
laskoviymishka commented on code in PR #18045:
URL: https://github.com/apache/iceberg/pull/18045#discussion_r4015309829
##########
core/src/main/java/org/apache/iceberg/FieldLabelParser.java:
##########
@@ -0,0 +1,64 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg;
+
+import com.fasterxml.jackson.core.JsonGenerator;
+import com.fasterxml.jackson.databind.JsonNode;
+import java.io.IOException;
+import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
+import org.apache.iceberg.util.JsonUtil;
+
+public class FieldLabelParser {
+ private static final String FIELD_ID = "field-id";
+ private static final String LABELS = "labels";
+
+ private FieldLabelParser() {}
+
+ public static String toJson(FieldLabel fieldLabels) {
+ return toJson(fieldLabels, false);
+ }
+
+ public static String toJson(FieldLabel fieldLabels, boolean pretty) {
+ return JsonUtil.generate(gen -> toJson(fieldLabels, gen), pretty);
+ }
+
+ public static void toJson(FieldLabel fieldLabels, JsonGenerator gen) throws IOException {
Review Comment:
ашчув!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: add labels to the load table/view read path [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #18045:
URL: https://github.com/apache/iceberg/pull/18045#discussion_r4027525431
##########
core/src/test/java/org/apache/iceberg/TestFieldLabelParser.java:
##########
@@ -0,0 +1,85 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
+
+import com.fasterxml.jackson.databind.JsonNode;
+import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap;
+import org.junit.jupiter.api.Test;
+
+public class TestFieldLabelParser {
+
+ @Test
+ public void nullCheck() {
Review Comment:
Test methods also could be package private
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: add labels to the load table/view read path [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #18045:
URL: https://github.com/apache/iceberg/pull/18045#discussion_r4027523483
##########
core/src/test/java/org/apache/iceberg/TestFieldLabelParser.java:
##########
@@ -0,0 +1,85 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
+
+import com.fasterxml.jackson.databind.JsonNode;
+import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap;
+import org.junit.jupiter.api.Test;
+
+public class TestFieldLabelParser {
Review Comment:
Should we make the new classes package private?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: add labels to the load table/view read path [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #18045:
URL: https://github.com/apache/iceberg/pull/18045#discussion_r4027553259
##########
core/src/main/java/org/apache/iceberg/Labels.java:
##########
@@ -0,0 +1,40 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg;
+
+import java.util.List;
+import java.util.Map;
+import org.immutables.value.Value;
+
+/** Optional catalog-provided labels returned on a load response. */
+@Value.Immutable
+public interface Labels {
+ Labels EMPTY = ImmutableLabels.builder().build();
+
+ /** Object-level labels. */
+ Map<String, String> objectLabels();
+
+ /** Field-level labels */
+ List<FieldLabel> fields();
+
+ /** Returns true when there are neither object-level nor field-level labels. */
+ default boolean isEmpty() {
Review Comment:
Do we want to use `@Value.Derived` as we did in `RemoteSigningConfig`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: add labels to the load table/view read path [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on PR #18045:
URL: https://github.com/apache/iceberg/pull/18045#issuecomment-5693341848
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 `org.apache.iceberg.rest` package, meaning that those things become part of the public API immediately.
@danielcweeks any thoughts on that?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: add labels to the load table/view read path [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #18045:
URL: https://github.com/apache/iceberg/pull/18045#discussion_r4027516166
##########
core/src/main/java/org/apache/iceberg/FieldLabel.java:
##########
@@ -0,0 +1,30 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg;
Review Comment:
Was it a conscious decision to keep this in the root (`org.apache.iceberg`) package instead of the `org.apache.iceberg.rest` package? This is a REST only feature, so these classes might be better placed there.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: add labels to the load table/view read path [iceberg]
Posted by "danielcweeks (via GitHub)" <gi...@apache.org>.
danielcweeks commented on code in PR #18045:
URL: https://github.com/apache/iceberg/pull/18045#discussion_r4027793189
##########
core/src/main/java/org/apache/iceberg/FieldLabel.java:
##########
@@ -0,0 +1,30 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg;
Review Comment:
Please see: https://github.com/apache/iceberg/pull/18045#issuecomment-5694633921
This is not a REST only feature
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org