You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "anoopj (via GitHub)" <gi...@apache.org> on 2026/10/01 17:07:01 UTC
[PR] API, Core: Add toString to partition struct implementations [iceberg]
anoopj opened a new pull request, #18342:
URL: https://github.com/apache/iceberg/pull/18342
StructProjection and the struct returned by StructLikeUtil.copy represent partition tuples but relied on the default Object.toString, which prints an unreadable identity hash when debugging. Render field names and values instead.
Followup from https://github.com/apache/iceberg/pull/18108
--
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] API, Core: Add toString to StructProject and StructLikeUtil [iceberg]
Posted by "anoopj (via GitHub)" <gi...@apache.org>.
anoopj commented on code in PR #18342:
URL: https://github.com/apache/iceberg/pull/18342#discussion_r4159877842
##########
api/src/main/java/org/apache/iceberg/util/StructProjection.java:
##########
@@ -223,4 +223,19 @@ public <T> T get(int pos, Class<T> javaClass) {
public <T> void set(int pos, T value) {
throw new UnsupportedOperationException("Cannot set fields in a TypeProjection");
}
+
+ @Override
+ public String toString() {
+ StringBuilder sb = new StringBuilder();
+ sb.append("StructProjection{");
+ List<Types.NestedField> fields = type.fields();
+ for (int i = 0; i < fields.size(); i += 1) {
+ if (i > 0) {
+ sb.append(", ");
+ }
+ sb.append(fields.get(i).name()).append("=").append(get(i, Object.class));
+ }
+ sb.append("}");
+ return sb.toString();
Review Comment:
Thanks for the feedback. I changed it such that we are reading it from the original struct.
--
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] API, Core: Add toString to StructProject and StructLikeUtil [iceberg]
Posted by "dramaticlly (via GitHub)" <gi...@apache.org>.
dramaticlly commented on code in PR #18342:
URL: https://github.com/apache/iceberg/pull/18342#discussion_r4161532198
##########
api/src/test/java/org/apache/iceberg/util/TestStructProjection.java:
##########
@@ -69,4 +69,22 @@ void createStillThrowsForMissingOptionalFieldInNestedStruct() {
.isInstanceOf(IllegalArgumentException.class)
.hasMessageStartingWith("Cannot find field");
}
+
+ @Test
+ void toStringRendersNestedProjectionRecursively() {
+ StructProjection projection = StructProjection.create(PROJECTED_STRUCT, PROJECTED_STRUCT);
+ projection.wrap(Row.of(1L, Row.of("John", "Q", "Doe")));
+
+ assertThat(projection)
+ .hasToString(
+ "StructProjection{id=1, person=StructProjection{first=John, middle=Q, last=Doe}}");
+ }
+
+ @Test
+ void toStringRendersNullNestedStructAsNull() {
Review Comment:
nit: would be great to have coverage on `StructProjection.createAllowMissing(DATA_STRUCT_MISSING_NESTED_FIELD, PROJECTED_STRUCT)` 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] API, Core: Add toString to StructProject and StructLikeUtil [iceberg]
Posted by "dramaticlly (via GitHub)" <gi...@apache.org>.
dramaticlly commented on code in PR #18342:
URL: https://github.com/apache/iceberg/pull/18342#discussion_r4160217975
##########
api/src/main/java/org/apache/iceberg/util/StructProjection.java:
##########
@@ -223,4 +223,19 @@ public <T> T get(int pos, Class<T> javaClass) {
public <T> void set(int pos, T value) {
throw new UnsupportedOperationException("Cannot set fields in a TypeProjection");
}
+
+ @Override
+ public String toString() {
+ StringBuilder sb = new StringBuilder();
+ sb.append("StructProjection{");
+ List<Types.NestedField> fields = type.fields();
+ for (int i = 0; i < fields.size(); i += 1) {
+ if (i > 0) {
+ sb.append(", ");
+ }
+ sb.append(fields.get(i).name()).append("=").append(get(i, Object.class));
+ }
+ sb.append("}");
+ return sb.toString();
Review Comment:
I am wondering if helps to add a few tests in https://github.com/apache/iceberg/blob/main/api/src/test/java/org/apache/iceberg/util/TestStructProjection.java
Given the current form, the structProjection toString only show the top level field, which might not help much for the human-readable debug-ability. I do feel that the toString is effectively a nested materialization for struct projection, so probing is not free.
``` java
StructProjection.create(PROJECTED_STRUCT, PROJECTED_STRUCT).wrap(Row.of(1L, Row.of("John", "Q", "Doe"))).toString()
//return "StructProjection{id=1, person=org.apache.iceberg.TestHelpers$Row@447e510}"
// instead of desired
// "StructProjection{id=1, person=StructProjection{first=John, middle=Q, last=Doe}}"
```
--
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] API, Core: Add toString to StructProjection and StructLikeUtil [iceberg]
Posted by "anoopj (via GitHub)" <gi...@apache.org>.
anoopj commented on code in PR #18342:
URL: https://github.com/apache/iceberg/pull/18342#discussion_r4162940515
##########
api/src/test/java/org/apache/iceberg/util/TestStructProjection.java:
##########
@@ -69,4 +69,22 @@ void createStillThrowsForMissingOptionalFieldInNestedStruct() {
.isInstanceOf(IllegalArgumentException.class)
.hasMessageStartingWith("Cannot find field");
}
+
+ @Test
+ void toStringRendersNestedProjectionRecursively() {
+ StructProjection projection = StructProjection.create(PROJECTED_STRUCT, PROJECTED_STRUCT);
+ projection.wrap(Row.of(1L, Row.of("John", "Q", "Doe")));
+
+ assertThat(projection)
+ .hasToString(
+ "StructProjection{id=1, person=StructProjection{first=John, middle=Q, last=Doe}}");
+ }
+
+ @Test
+ void toStringRendersNullNestedStructAsNull() {
Review Comment:
Added `toStringRendersMissingNestedFieldAsNull`
--
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] API, Core: Add toString to StructProject and StructLikeUtil [iceberg]
Posted by "anoopj (via GitHub)" <gi...@apache.org>.
anoopj commented on code in PR #18342:
URL: https://github.com/apache/iceberg/pull/18342#discussion_r4161352517
##########
api/src/main/java/org/apache/iceberg/util/StructProjection.java:
##########
@@ -223,4 +223,19 @@ public <T> T get(int pos, Class<T> javaClass) {
public <T> void set(int pos, T value) {
throw new UnsupportedOperationException("Cannot set fields in a TypeProjection");
}
+
+ @Override
+ public String toString() {
+ StringBuilder sb = new StringBuilder();
+ sb.append("StructProjection{");
+ List<Types.NestedField> fields = type.fields();
+ for (int i = 0; i < fields.size(); i += 1) {
+ if (i > 0) {
+ sb.append(", ");
+ }
+ sb.append(fields.get(i).name()).append("=").append(get(i, Object.class));
+ }
+ sb.append("}");
+ return sb.toString();
Review Comment:
I think both are good points. I initially hesitated to add tests because currently we don't have many `toString` tests in the project.
I made toString descend into nested projections. To avoid the wrap() side effect we discussed above, the nested descent uses `copyFor`, which wraps a fresh projection instead of mutating the shared nested one. This is OK, because we don't expect `toString()` to be used in the inner loop.
--
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] API, Core: Add toString to StructProjection and StructLikeUtil [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar merged PR #18342:
URL: https://github.com/apache/iceberg/pull/18342
--
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] API, Core: Add toString to StructProject and StructLikeUtil [iceberg]
Posted by "anoopj (via GitHub)" <gi...@apache.org>.
anoopj commented on code in PR #18342:
URL: https://github.com/apache/iceberg/pull/18342#discussion_r4159874141
##########
core/src/main/java/org/apache/iceberg/util/StructLikeUtil.java:
##########
@@ -64,5 +64,19 @@ public <T> T get(int pos, Class<T> javaClass) {
public <T> void set(int pos, T value) {
throw new UnsupportedOperationException("Struct copy cannot be modified");
}
+
+ @Override
+ public String toString() {
+ StringBuilder sb = new StringBuilder();
+ sb.append("[");
+ for (int i = 0; i < values.length; i += 1) {
+ if (i > 0) {
+ sb.append(", ");
+ }
+ sb.append(values[i]);
+ }
+ sb.append("]");
+ return sb.toString();
Review Comment:
Done. Thanks
--
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] API, Core: Add toString to StructProject and StructLikeUtil [iceberg]
Posted by "dramaticlly (via GitHub)" <gi...@apache.org>.
dramaticlly commented on code in PR #18342:
URL: https://github.com/apache/iceberg/pull/18342#discussion_r4159187846
##########
api/src/main/java/org/apache/iceberg/util/StructProjection.java:
##########
@@ -223,4 +223,19 @@ public <T> T get(int pos, Class<T> javaClass) {
public <T> void set(int pos, T value) {
throw new UnsupportedOperationException("Cannot set fields in a TypeProjection");
}
+
+ @Override
+ public String toString() {
+ StringBuilder sb = new StringBuilder();
+ sb.append("StructProjection{");
+ List<Types.NestedField> fields = type.fields();
+ for (int i = 0; i < fields.size(); i += 1) {
+ if (i > 0) {
+ sb.append(", ");
+ }
+ sb.append(fields.get(i).name()).append("=").append(get(i, Object.class));
+ }
+ sb.append("}");
+ return sb.toString();
Review Comment:
Looks like we already used StringJoiner before in PartitionSet https://github.com/apache/iceberg/blob/574e44670b8502479b5c74d0edf0b9190d235af4/core/src/main/java/org/apache/iceberg/util/PartitionSet.java#L194, so it shall save some manual stringBuilder logic. Might worth considering something like below
```suggestion
List<Types.NestedField> fields = type.fields();
StringJoiner joiner = new StringJoiner(", ", "StructProjection{", "}");
for (int pos = 0; pos < fields.size(); pos += 1) {
joiner.add(fields.get(pos).name() + "=" + get(pos, Object.class));
}
return joiner.toString();
```
On a separate note, I noticed that if we use get(pos, Object.class) on deeply nested struct, it might have side effect of invoking `javaClass.cast(nestedProjections[pos].wrap(nestedStruct))`, so worth double checking whether we want the behavior as part of toString call.
##########
core/src/main/java/org/apache/iceberg/util/StructLikeUtil.java:
##########
@@ -64,5 +64,19 @@ public <T> T get(int pos, Class<T> javaClass) {
public <T> void set(int pos, T value) {
throw new UnsupportedOperationException("Struct copy cannot be modified");
}
+
+ @Override
+ public String toString() {
+ StringBuilder sb = new StringBuilder();
+ sb.append("[");
+ for (int i = 0; i < values.length; i += 1) {
+ if (i > 0) {
+ sb.append(", ");
+ }
+ sb.append(values[i]);
+ }
+ sb.append("]");
+ return sb.toString();
Review Comment:
This is similar to [`StructTransform::toString`](https://github.com/apache/iceberg/blob/574e44670b8502479b5c74d0edf0b9190d235af4/api/src/main/java/org/apache/iceberg/StructTransform.java#L98-L109) with values is already object array, so can probably simplify to
```suggestion
return Arrays.stream(values)
.map(String::valueOf)
.collect(Collectors.joining(", ", "[", "]"));
```
--
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] API, Core: Add toString to StructProject and StructLikeUtil [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #18342:
URL: https://github.com/apache/iceberg/pull/18342#discussion_r4159475617
##########
api/src/main/java/org/apache/iceberg/util/StructProjection.java:
##########
@@ -223,4 +223,19 @@ public <T> T get(int pos, Class<T> javaClass) {
public <T> void set(int pos, T value) {
throw new UnsupportedOperationException("Cannot set fields in a TypeProjection");
}
+
+ @Override
+ public String toString() {
+ StringBuilder sb = new StringBuilder();
+ sb.append("StructProjection{");
+ List<Types.NestedField> fields = type.fields();
+ for (int i = 0; i < fields.size(); i += 1) {
+ if (i > 0) {
+ sb.append(", ");
+ }
+ sb.append(fields.get(i).name()).append("=").append(get(i, Object.class));
+ }
+ sb.append("}");
+ return sb.toString();
Review Comment:
>it might have side effect of invoking javaClass.cast(nestedProjections[pos].wrap(nestedStruct)),
This is a great callout, I missed this originally but any reason we can't just read from the actual struct? Otherwise logging one of these may lead to really unexpected results after the logging as a result of the side effect.
##########
api/src/main/java/org/apache/iceberg/util/StructProjection.java:
##########
@@ -223,4 +223,19 @@ public <T> T get(int pos, Class<T> javaClass) {
public <T> void set(int pos, T value) {
throw new UnsupportedOperationException("Cannot set fields in a TypeProjection");
}
+
+ @Override
+ public String toString() {
+ StringBuilder sb = new StringBuilder();
+ sb.append("StructProjection{");
+ List<Types.NestedField> fields = type.fields();
+ for (int i = 0; i < fields.size(); i += 1) {
+ if (i > 0) {
+ sb.append(", ");
+ }
+ sb.append(fields.get(i).name()).append("=").append(get(i, Object.class));
+ }
+ sb.append("}");
+ return sb.toString();
Review Comment:
>it might have side effect of invoking javaClass.cast(nestedProjections[pos].wrap(nestedStruct)),
This is a great callout, I missed this originally but any reason we can't just read from the actual struct? Otherwise logging one of these may lead to really unexpected results after the logging as a result of the side effect.
--
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