API: Add Schema.isOptional() by nastra · Pull Request #17513 · apache/iceberg

nastra · 2026-08-04T10:22:29Z

Determining whether a field may contain nulls requires checking the field itself as well as every field that contains it. Expose this as a reusable utility so callers don't have to reimplement the ancestor walk.

I've pulled this out of c59ed0d and the plan is to use this functionality later in the stats evaluation

pvary

Left some minor suggestions for the comments, but otherwise LGTM

dramaticlly · 2026-08-04T20:39:26Z

+
+    return field == null
+        || field.isOptional()
+        || ancestorFields(schema, fieldId).stream().anyMatch(Types.NestedField::isOptional);


ancestorFields calls TypeUtil.indexParents for each invocation and ends up traverse the schema tree. I am wondering if worth caching the indexParents in Schema. Currently it's only used for identifier field in constructor and discarded right after

iceberg/api/src/main/java/org/apache/iceberg/Schema.java

Line 153 in 1d8c715

Map<Integer, Integer> idToParent = TypeUtil.indexParents(struct);

Not caching is fine if we are doing it once per predicate. But if this is headed to the hot path in stats evaluation (which is O(files)), it is worth caching it.

we can defer caching to a separate PR. @dramaticlly would you be interested in adding that?

I'm fine with deferring but I do agree with @anoopj and @dramaticlly that caching in this case is important especially if we're invoking this per entry. And I think this is actually another argument for maybe why this API should actually be on Schema itself since that idToParent state can be lazily kept there.

given that multiple folks brought this up, I went ahead and moved this to Schema.isNullable() where we can cache the idToParent map

amogh-jahagirdar · 2026-08-06T17:41:36Z

+
+    return field == null
+        || field.isOptional()
+        || ancestorFields(schema, fieldId).stream().anyMatch(Types.NestedField::isOptional);


I'm fine with deferring but I do agree with @anoopj and @dramaticlly that caching in this case is important especially if we're invoking this per entry. And I think this is actually another argument for maybe why this API should actually be on Schema itself since that idToParent state can be lazily kept there.

Determining whether a field may contain nulls requires checking the field itself as well as every field that contains it. Expose this as a reusable utility so callers don't have to reimplement the ancestor walk.

singhpk234

LGTM thanks @nastra !

singhpk234 · 2026-08-07T22:50:14Z

+    if (idToParent == null) {
+      this.idToParent = TypeUtil.indexParents(struct);
+    }
+    return idToParent;


i wonder if we should synchronize this if this now part of a public which can be called concurrently ? wdyt

I was mainly keeping it in-sync with all the other methods. I think it we want to synchronize, then we should probably do this across the board?

Agree, we can do this in follow-up too / later, this looks great to me as is !

dramaticlly · 2026-08-12T21:23:22Z

+    assertThat(schema.isOptional(5)).isFalse();
+    assertThat(schema.isOptional(6)).isFalse();
+
+    // an optional field is nullable regardless of the fields that contain it


nit: I think we renamed from isNullable to isOptional but some of the UT comment is lagging behind. Might consider rename as well, or better to inline using as(...)

rdblue · 2026-08-12T22:16:22Z

+   * @return true if the field may be null, false if it cannot be null
+   * @throws IllegalArgumentException if the field is not present in this schema
+   */
+  public boolean isOptional(int id) {


I do not think this method should not be part of the public API. I pointed out a very similar problem on the other version of this method, here: https://github.com/apache/iceberg/pull/17413/changes#r3762320465

Whether a field is optional is clearly defined: there's a boolean flag that determines whether the field's value can be null. This method misuses the term "optional" to mean whether the field or any parent may be null. That difference is not clear and makes the term "optional" confusing. I also don't think that this gets any better by introducing a different "nullable" term.

My point on the other PR wasn't to move this method to the public API, it was that we should rename the utility method to be clear. To do that, I think we should switch to using required. If the field and all parents are required ("fully required" or "always present"?) that is the opposite of this method. Using a name like alwaysPresent combined with isRequired for the leaf makes the most sense to me, but we can come up with better ideas.

rdblue

I think that this PR should be closed in favor of a utility method in #17413.

This method does not need to be part of the public API and including it in this form makes the API harder to use because it is more confusing.

In addition, I don't think Schema is a good place for this. The calling code in #17413 converts a struct to a Schema every time because this uses an id to parent map. I think it's a bad practice to convert to a schema just to call a util method for a struct and would like to see a better implementation that can reuse an id to parent map.

Let's close this and continue the work on the other PR.

github-actions Bot added the API label Aug 4, 2026

nastra requested review from amogh-jahagirdar, pvary, singhpk234 and stevenzwu August 4, 2026 10:26

pvary reviewed Aug 4, 2026

View reviewed changes

Comment thread api/src/main/java/org/apache/iceberg/types/TypeUtil.java Outdated

pvary reviewed Aug 4, 2026

View reviewed changes

Comment thread api/src/test/java/org/apache/iceberg/types/TestTypeUtil.java Outdated

nastra force-pushed the typeutil-isnullable branch from 859280a to cb45906 Compare August 4, 2026 13:43

pvary approved these changes Aug 4, 2026

View reviewed changes

dramaticlly reviewed Aug 4, 2026

View reviewed changes

nastra force-pushed the typeutil-isnullable branch from cb45906 to e56dd25 Compare August 5, 2026 04:49

uros-b reviewed Aug 5, 2026

View reviewed changes

Comment thread api/src/main/java/org/apache/iceberg/types/TypeUtil.java Outdated

dramaticlly approved these changes Aug 5, 2026

View reviewed changes

amogh-jahagirdar reviewed Aug 6, 2026

View reviewed changes

API: Add TypeUtil.isNullable()

80eda16

Determining whether a field may contain nulls requires checking the field itself as well as every field that contains it. Expose this as a reusable utility so callers don't have to reimplement the ancestor walk.

nastra force-pushed the typeutil-isnullable branch from e56dd25 to afd57e4 Compare August 7, 2026 10:00

nastra changed the title ~~API: Add TypeUtil.isNullable()~~ API: Add Schema.isNullable() Aug 7, 2026

nastra requested a review from amogh-jahagirdar August 7, 2026 10:08

Move stuff to Schema.isNullable() + add caching

b0b7a9a

nastra force-pushed the typeutil-isnullable branch from afd57e4 to b0b7a9a Compare August 7, 2026 10:22

singhpk234 approved these changes Aug 7, 2026

View reviewed changes

nastra mentioned this pull request Aug 12, 2026

Core: Add content stats evaluator #17413

Open

rename to isOptional and throw when field can't be found

0b9e76b

nastra changed the title ~~API: Add Schema.isNullable()~~ API: Add Schema.isOptional() Aug 12, 2026

dramaticlly reviewed Aug 12, 2026

View reviewed changes

rdblue reviewed Aug 12, 2026

View reviewed changes

nastra closed this Aug 13, 2026

nastra deleted the typeutil-isnullable branch August 13, 2026 06:30

Uh oh!

Conversation

nastra commented Aug 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pvary left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

singhpk234 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rdblue left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

nastra commented Aug 4, 2026 •

edited

Loading