Skip to content

API: Add Schema.isOptional() #17513

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
nastra wants to merge 3 commits into from
+205 −12
Closed

API: Add Schema.isOptional() #17513

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
80eda16
API: Add TypeUtil.isNullable()
nastra Aug 4, 2026
b0b7a9a
Move stuff to Schema.isNullable() + add caching
nastra Aug 7, 2026
0b9e76b
rename to isOptional and throw when field can't be found
nastra Aug 12, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 43 additions & 2 deletions api/src/main/java/org/apache/iceberg/Schema.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,7 @@ public class Schema implements Serializable {
private transient Map<String, Integer> lowerCaseNameToId = null;
private transient Map<Integer, Accessor<StructLike>> idToAccessor = null;
private transient Map<Integer, String> idToName = null;
private transient Map<Integer, Integer> idToParent = null;
private transient Set<Integer> identifierFieldIdSet = null;
private final transient Map<Integer, Integer> idsToReassigned;
private final transient Map<Integer, Integer> idsToOriginal;
Expand Down Expand Up @@ -150,8 +151,8 @@ public Schema(

// validate IdentifierField
if (identifierFieldIds != null) {
Map<Integer, Integer> idToParent = TypeUtil.indexParents(struct);
identifierFieldIds.forEach(id -> validateIdentifierField(id, lazyIdToField(), idToParent));
identifierFieldIds.forEach(
id -> validateIdentifierField(id, lazyIdToField(), lazyIdToParent()));
}

this.identifierFieldIds =
Expand Down Expand Up @@ -233,6 +234,13 @@ private Map<Integer, String> lazyIdToName() {
return idToName;
}

private Map<Integer, Integer> lazyIdToParent() {
if (idToParent == null) {
this.idToParent = TypeUtil.indexParents(struct);
}
return idToParent;
Comment on lines +238 to +241

@singhpk234 singhpk234 Aug 7, 2026

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.

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

@nastra nastra Aug 10, 2026

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.

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?

@singhpk234 singhpk234 Aug 12, 2026

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.

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

}

private Map<String, Integer> lazyLowerCaseNameToId() {
if (lowerCaseNameToId == null) {
this.lowerCaseNameToId = ImmutableMap.copyOf(TypeUtil.indexByLowerCaseName(struct));
Expand Down Expand Up @@ -449,6 +457,39 @@ public String idToAlias(Integer fieldId) {
return null;
}

/**
* Returns whether the sub-field identified by the field id is effectively optional.
*
* <p>A field is effectively optional if it is declared optional, or if it is nested inside an
* optional field. For example, a required field inside an optional struct is null whenever that
* struct is null. Field defaults are not taken into account, so an optional field with a non-null
* default is still optional.
*
* @param id a field id
* @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) {

@rdblue rdblue Aug 12, 2026

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.

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.

NestedField field = findField(id);
Preconditions.checkArgument(field != null, "Cannot find field with id: %s", id);

if (field.isOptional()) {
return true;
}

Map<Integer, Integer> parents = lazyIdToParent();
Integer parentId = parents.get(id);
while (parentId != null) {
if (findField(parentId).isOptional()) {
return true;
}

parentId = parents.get(parentId);
}

return false;
}

/**
* Returns an accessor for retrieving the data from {@link StructLike}.
*
Expand Down
12 changes: 2 additions & 10 deletions api/src/main/java/org/apache/iceberg/expressions/UnboundPredicate.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@
import org.apache.iceberg.relocated.com.google.common.collect.Lists;
import org.apache.iceberg.relocated.com.google.common.collect.Sets;
import org.apache.iceberg.types.Type;
import org.apache.iceberg.types.TypeUtil;
import org.apache.iceberg.types.Types;
import org.apache.iceberg.types.Types.StructType;
import org.apache.iceberg.util.CharSequenceSet;
Expand Down Expand Up @@ -124,18 +123,16 @@
}

private Expression bindUnaryOperation(StructType struct, BoundTerm<T> boundTerm) {
switch (op()) {

Check warning on line 126 in api/src/main/java/org/apache/iceberg/expressions/UnboundPredicate.java

View workflow job for this annotation

GitHub Actions / build-checks (17, pull_request) 

[StatementSwitchToExpressionSwitch] This statement switch can be converted to a new-style arrow switch
case IS_NULL:
if (!boundTerm.producesNull()
&& allAncestorFieldsAreRequired(struct, boundTerm.ref().fieldId())) {
if (!boundTerm.producesNull() && !struct.asSchema().isOptional(boundTerm.ref().fieldId())) {
return Expressions.alwaysFalse();
} else if (boundTerm.type().equals(Types.UnknownType.get())) {
return Expressions.alwaysTrue();
}
return new BoundUnaryPredicate<>(Operation.IS_NULL, boundTerm);
case NOT_NULL:
if (!boundTerm.producesNull()
&& allAncestorFieldsAreRequired(struct, boundTerm.ref().fieldId())) {
if (!boundTerm.producesNull() && !struct.asSchema().isOptional(boundTerm.ref().fieldId())) {
return Expressions.alwaysTrue();
} else if (boundTerm.type().equals(Types.UnknownType.get())) {
return Expressions.alwaysFalse();
Expand All @@ -158,11 +155,6 @@
}
}

private boolean allAncestorFieldsAreRequired(StructType struct, int fieldId) {
return TypeUtil.ancestorFields(struct.asSchema(), fieldId).stream()
.allMatch(Types.NestedField::isRequired);
}

private boolean floatingType(Type.TypeID typeID) {
return Type.TypeID.DOUBLE.equals(typeID) || Type.TypeID.FLOAT.equals(typeID);
}
Expand All @@ -184,7 +176,7 @@
boundTerm.type(), literal().value(), literal().value().getClass().getName());

} else if (lit == Literals.aboveMax()) {
switch (op()) {

Check warning on line 179 in api/src/main/java/org/apache/iceberg/expressions/UnboundPredicate.java

View workflow job for this annotation

GitHub Actions / build-checks (17, pull_request) 

[StatementSwitchToExpressionSwitch] This statement switch can be converted to a new-style arrow switch
case LT:
case LT_EQ:
case NOT_EQ:
Expand Down
160 changes: 160 additions & 0 deletions api/src/test/java/org/apache/iceberg/TestSchema.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,8 @@
import static org.apache.iceberg.Schema.DEFAULT_VALUES_MIN_FORMAT_VERSION;
import static org.apache.iceberg.Schema.MIN_FORMAT_VERSIONS;
import static org.apache.iceberg.TestHelpers.MAX_FORMAT_VERSION;
import static org.apache.iceberg.types.Types.NestedField.optional;
import static org.apache.iceberg.types.Types.NestedField.required;
import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.assertThatCode;
import static org.assertj.core.api.Assertions.assertThatThrownBy;
Expand Down Expand Up @@ -312,4 +314,162 @@ public void testIndexFieldsNestedSchema() {
assertThat(fields.get(5).name()).isEqualTo("email");
assertThat(((Types.StructType) fields.get(2).type()).fields()).hasSize(3);
}

@Test
void isOptionalWithEmptySchemaOrUnknownFields() {
assertThatThrownBy(() -> new Schema().isOptional(1))
.isInstanceOf(IllegalArgumentException.class)
.hasMessage("Cannot find field with id: 1");

assertThatThrownBy(() -> new Schema(required(1, "id", Types.IntegerType.get())).isOptional(2))
.isInstanceOf(IllegalArgumentException.class)
.hasMessage("Cannot find field with id: 2");
}

@Test
void isOptionalWithTopLevelFields() {
Schema schema =
new Schema(
required(1, "id", Types.IntegerType.get()),
optional(2, "data", Types.StringType.get()));

assertThat(schema.isOptional(1)).isFalse();
assertThat(schema.isOptional(2)).isTrue();
}

@Test
void isOptionalWithNestedStructs() {
Schema schema =
new Schema(
required(
1,
"required_location",
Types.StructType.of(
required(3, "required_lat", Types.DoubleType.get()),
optional(4, "optional_lon", Types.DoubleType.get()),
required(
5,
"required_inner",
Types.StructType.of(
required(6, "required_zip", Types.IntegerType.get()))))),
optional(
2,
"optional_location",
Types.StructType.of(
required(7, "required_lat", Types.DoubleType.get()),
required(
8,
"required_inner",
Types.StructType.of(
required(9, "required_zip", Types.IntegerType.get()))))));

// a required field is not nullable when every field that contains it is required
assertThat(schema.isOptional(1)).isFalse();
assertThat(schema.isOptional(3)).isFalse();
assertThat(schema.isOptional(5)).isFalse();
assertThat(schema.isOptional(6)).isFalse();

// an optional field is nullable regardless of the fields that contain it

@dramaticlly dramaticlly Aug 12, 2026

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.

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(...)

assertThat(schema.isOptional(4)).isTrue();

// a required field nested in an optional struct is nullable
assertThat(schema.isOptional(2)).isTrue();
assertThat(schema.isOptional(7)).isTrue();
assertThat(schema.isOptional(8)).isTrue();
assertThat(schema.isOptional(9)).isTrue();
}

@Test
void isOptionalWithLists() {
Schema schema =
new Schema(
required(
1,
"required_points",
Types.ListType.ofRequired(
2, Types.StructType.of(required(3, "required_x", Types.LongType.get())))),
optional(
4,
"optional_points",
Types.ListType.ofOptional(
5, Types.StructType.of(required(6, "required_x", Types.LongType.get())))),
optional(
7,
"optional_lines",
Types.ListType.ofRequired(
8, Types.StructType.of(required(9, "required_x", Types.LongType.get())))),
required(
10,
"required_shapes",
Types.ListType.ofOptional(
11, Types.StructType.of(required(12, "required_x", Types.LongType.get())))));

// a required element of a required list is not nullable, nor is anything it contains
assertThat(schema.isOptional(1)).isFalse();
assertThat(schema.isOptional(2)).isFalse();
assertThat(schema.isOptional(3)).isFalse();

// an optional element is nullable, as is anything it contains
assertThat(schema.isOptional(4)).isTrue();
assertThat(schema.isOptional(5)).isTrue();
assertThat(schema.isOptional(6)).isTrue();

// a required element of an optional list is nullable
assertThat(schema.isOptional(7)).isTrue();
assertThat(schema.isOptional(8)).isTrue();
assertThat(schema.isOptional(9)).isTrue();

// an optional element of a required list is nullable, as is anything it contains
assertThat(schema.isOptional(10)).isFalse();
assertThat(schema.isOptional(11)).isTrue();
assertThat(schema.isOptional(12)).isTrue();
}

@Test
void isOptionalWithMaps() {
Schema schema =
new Schema(
required(
1,
"required_locations",
Types.MapType.ofRequired(
2,
3,
Types.StringType.get(),
Types.StructType.of(required(4, "required_lat", Types.DoubleType.get())))),
optional(
5,
"optional_locations",
Types.MapType.ofRequired(
6,
7,
Types.StringType.get(),
Types.StructType.of(required(8, "required_lat", Types.DoubleType.get())))),
required(
9,
"locations_with_optional_values",
Types.MapType.ofOptional(
10,
11,
Types.StringType.get(),
Types.StructType.of(required(12, "required_lat", Types.DoubleType.get())))));

// required map keys and values are not nullable
assertThat(schema.isOptional(1)).isFalse();
assertThat(schema.isOptional(2)).isFalse();
assertThat(schema.isOptional(3)).isFalse();
assertThat(schema.isOptional(4)).isFalse();

// required keys and values of an optional map are nullable
assertThat(schema.isOptional(5)).isTrue();
assertThat(schema.isOptional(6)).isTrue();
assertThat(schema.isOptional(7)).isTrue();
assertThat(schema.isOptional(8)).isTrue();

// an optional value of a required map is nullable, as is anything it contains, but keys are not
assertThat(schema.isOptional(9)).isFalse();
assertThat(schema.isOptional(10)).isFalse();
assertThat(schema.isOptional(11)).isTrue();
assertThat(schema.isOptional(12)).isTrue();
}
}
Loading